Re: [Xen-devel] [PATCH v3 3/8] xen: introduce a generic framebuffer driver

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 18.12.12 at 19:46, Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> 
>>> wrote:
> +int __init fb_init(struct fb_prop fbp)

Any reason to pass a structure by value here?

> +{
> +    if ( fbp.width > MAX_XRES || fbp.height > MAX_YRES )
> +    {
> +        printk("Couldn't initialize a %xx%x framebuffer early.\n",

%x to me seems to produce rather meaningless messages here;
customarily screen dimensions are expected to be decimal. Also,
in new code you should specify a log level.

> +                        fbp.width, fbp.height);
> +        return -EINVAL;
> +    }
> +
> +    fb.fbp = fbp;
> +    fb.lbuf = lbuf;
> +    fb.text_buf = text_buf;
> +    fb.line_len = line_len;
> +    return 0;
> +}
> +
> +int __init fb_alloc(void)
> +{
> +    fb.lbuf = NULL;
> +    fb.text_buf = NULL;
> +    fb.line_len = NULL;
> +
> +    fb.lbuf = xmalloc_bytes(fb.fbp.bytes_per_line);
> +    if ( !fb.lbuf )
> +        goto fail;
> +
> +    fb.text_buf = xzalloc_bytes(fb.fbp.text_columns * fb.fbp.text_rows);
> +    if ( !fb.text_buf )
> +        goto fail;
> +
> +    fb.line_len = xzalloc_array(unsigned int, fb.fbp.text_columns);
> +    if ( !fb.line_len )
> +        goto fail;
> +
> +    memcpy(fb.lbuf, lbuf, fb.fbp.bytes_per_line);
> +    memcpy(fb.text_buf, text_buf, fb.fbp.text_columns * fb.fbp.text_rows);
> +    memcpy(fb.line_len, line_len, fb.fbp.text_columns);
> +
> +    return 0;
> +
> +fail:
> +    printk(XENLOG_ERR "Couldn't allocate enough memory to drive "
> +                    "the framebuffer\n");

I think it was generally agreed that breaking up messages to fit 80
columns is undesirable.

> +    xfree(fb.lbuf);
> +    xfree(fb.text_buf);
> +    xfree(fb.line_len);

fb_free()?

> +
> +    return -ENOMEM;
> +}
> +
> +void fb_free(void)
> +{
> +    xfree(fb.lbuf);
> +    xfree(fb.text_buf);
> +    xfree(fb.line_len);
> +}
> --- /dev/null
> +++ b/xen/drivers/video/fb.h
> @@ -0,0 +1,49 @@
> +/*
> + * xen/drivers/video/fb.h
> + *
> + * Cross-platform framebuffer library
> + *
> + * Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> + * Copyright (c) 2012 Citrix Systems.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _XEN_FB_H
> +#define _XEN_FB_H
> +
> +#include <xen/init.h>
> +
> +struct fb_prop {
> +    const struct font_desc *font;
> +    unsigned char *lfb;
> +    unsigned int pixel_on;
> +    uint16_t width, height;
> +    uint16_t bytes_per_line;
> +    uint16_t bits_per_pixel;
> +    void (*flush)(void);
> +
> +    unsigned int text_columns;
> +    unsigned int text_rows;
> +};
> +
> +void fb_redraw_puts(const char *s);
> +void fb_scroll_puts(const char *s);
> +void fb_carriage_return(void);
> +void fb_free(void);
> +
> +/* initialize the framebuffer, can be called early (before xmalloc is
> + * available) */
> +int __init fb_init(struct fb_prop fbp);
> +/* fb_alloc allocates internal structures using xmalloc */
> +int __init fb_alloc(void);

No __init annotations on declarations, please.

Jan

> +
> +#endif



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel