[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Wed, 9 Jan 2013, Mats Petersson wrote:
> On 09/01/13 14:45, Mats Petersson wrote:
> > On 09/01/13 14:05, Stefano Stabellini wrote:
> >> On Wed, 9 Jan 2013, Jan Beulich wrote:
> >>>>>> On 08.01.13 at 21:03, Stefano Stabellini 
> >>>>>> <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> >>>> --- /dev/null
> >>>> +++ b/xen/drivers/video/lfb.c
> >>>> @@ -0,0 +1,206 @@
> >>>> +/******************************************************************************
> >>>> + * lfb.c
> >>>> + *
> >>>> + * linear frame buffer handling.
> >>>> + */
> >>>> +
> >>>> +#include <xen/config.h>
> >>>> +#include <xen/kernel.h>
> >>>> +#include <xen/lib.h>
> >>>> +#include <xen/errno.h>
> >>>> +#include "lfb.h"
> >>>> +#include "font.h"
> >>>> +
> >>>> +#define MAX_XRES 1900
> >>>> +#define MAX_YRES 1200
> >>>> +#define MAX_BPP 4
> >>>> +#define MAX_FONT_W 8
> >>>> +#define MAX_FONT_H 16
> >>>> +static __initdata unsigned int line_len[MAX_XRES / MAX_FONT_W];
> >>>> +static __initdata unsigned char lbuf[MAX_XRES * MAX_BPP];
> >>>> +static __initdata unsigned char text_buf[(MAX_XRES / MAX_FONT_W) * \
> >>>> +                          (MAX_YRES / MAX_FONT_H)];
> >>> Imo it would be better to move all these __initdata definitions
> >>> do to where the using __init functions actually live, to make
> >>> sure unintended use of them in non-__init ones is noticed.
> >> OK
> >>
> >>
> >>>> +
> >>>> +struct lfb_status {
> >>>> +    struct lfb_prop lfbp;
> >>>> +
> >>>> +    unsigned char *lbuf, *text_buf;
> >>>> +    unsigned int *line_len;
> >>>> +    unsigned int xpos, ypos;
> >>>> +};
> >>>> +static struct lfb_status lfb;
> >>>> +
> >>>> +static void lfb_show_line(
> >>>> +    const unsigned char *text_line,
> >>>> +    unsigned char *video_line,
> >>>> +    unsigned int nr_chars,
> >>>> +    unsigned int nr_cells)
> >>>> +{
> >>>> +    unsigned int i, j, b, bpp, pixel;
> >>>> +
> >>>> +    bpp = (lfb.lfbp.bits_per_pixel + 7) >> 3;
> >>>> +
> >>>> +    for ( i = 0; i < lfb.lfbp.font->height; i++ )
> >>>> +    {
> >>>> +        unsigned char *ptr = lfb.lbuf;
> >>>> +
> >>>> +        for ( j = 0; j < nr_chars; j++ )
> >>>> +        {
> >>>> +            const unsigned char *bits = lfb.lfbp.font->data;
> >>>> +            bits += ((text_line[j] * lfb.lfbp.font->height + i) *
> >>>> +                     ((lfb.lfbp.font->width + 7) >> 3));
> >>>> +            for ( b = lfb.lfbp.font->width; b--; )
> >>>> +            {
> >>>> +                pixel = (*bits & (1u<<b)) ? lfb.lfbp.pixel_on : 0;
> >>>> +                memcpy(ptr, &pixel, bpp);
> >>>> +                ptr += bpp;
> >>>> +            }
> >>>> +        }
> >>>> +
> >>>> +        memset(ptr, 0, (lfb.lfbp.width - nr_chars * 
> >>>> lfb.lfbp.font->width) * bpp);
> >>>> +        memcpy(video_line, lfb.lbuf, nr_cells * lfb.lfbp.font->width * 
> >>>> bpp);
> >>>> +        video_line += lfb.lfbp.bytes_per_line;
> >>>> +    }
> >>>> +}
> >>>> +
> >>>> +/* Fast mode which redraws all modified parts of a 2D text buffer. */
> >>>> +void lfb_redraw_puts(const char *s)
> >>>> +{
> >>>> +    unsigned int i, min_redraw_y = lfb.ypos;
> >>>> +    char c;
> >>>> +
> >>>> +    /* Paste characters into text buffer. */
> >>>> +    while ( (c = *s++) != '\0' )
> >>>> +    {
> >>>> +        if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
> >>>> +        {
> >>>> +            if ( ++lfb.ypos >= lfb.lfbp.text_rows )
> >>>> +            {
> >>>> +                min_redraw_y = 0;
> >>>> +                lfb.ypos = lfb.lfbp.text_rows - 1;
> >>>> +                memmove(lfb.text_buf, lfb.text_buf + 
> >>>> lfb.lfbp.text_columns,
> >>>> +                        lfb.ypos * lfb.lfbp.text_columns);
> >>>> +                memset(lfb.text_buf + lfb.ypos * lfb.lfbp.text_columns, 
> >>>> 0, lfb.xpos);
> >>>> +            }
> >>>> +            lfb.xpos = 0;
> >>>> +        }
> >>>> +
> >>>> +        if ( c != '\n' )
> >>>> +            lfb.text_buf[lfb.xpos++ + lfb.ypos * lfb.lfbp.text_columns] 
> >>>> = c;
> >>>> +    }
> >>>> +
> >>>> +    /* Render modified section of text buffer to VESA linear 
> >>>> framebuffer. */
> >>>> +    for ( i = min_redraw_y; i <= lfb.ypos; i++ )
> >>>> +    {
> >>>> +        const unsigned char *line = lfb.text_buf + i * 
> >>>> lfb.lfbp.text_columns;
> >>>> +        unsigned int width;
> >>>> +
> >>>> +        for ( width = lfb.lfbp.text_columns; width; --width )
> >>>> +            if ( line[width - 1] )
> >>>> +                 break;
> >>>> +        lfb_show_line(line,
> >>>> +                       lfb.lfbp.lfb + i * lfb.lfbp.font->height * 
> >>>> lfb.lfbp.bytes_per_line,
> >>>> +                       width, max(lfb.line_len[i], width));
> >>>> +        lfb.line_len[i] = width;
> >>>> +    }
> >>>> +
> >>>> +    lfb.lfbp.flush();
> >>>> +}
> >>>> +
> >>>> +/* Slower line-based scroll mode which interacts better with dom0. */
> >>>> +void lfb_scroll_puts(const char *s)
> >>>> +{
> >>>> +    unsigned int i;
> >>>> +    char c;
> >>>> +
> >>>> +    while ( (c = *s++) != '\0' )
> >>>> +    {
> >>>> +        if ( (c == '\n') || (lfb.xpos >= lfb.lfbp.text_columns) )
> >>>> +        {
> >>>> +            unsigned int bytes = (lfb.lfbp.width *
> >>>> +                                  ((lfb.lfbp.bits_per_pixel + 7) >> 3));
> >>>> +            unsigned char *src = lfb.lfbp.lfb + lfb.lfbp.font->height * 
> >>>> lfb.lfbp.bytes_per_line;
> >>>> +            unsigned char *dst = lfb.lfbp.lfb;
> >>>> +
> >>>> +            /* New line: scroll all previous rows up one line. */
> >>>> +            for ( i = lfb.lfbp.font->height; i < lfb.lfbp.height; i++ )
> >>>> +            {
> >>>> +                memcpy(dst, src, bytes);
> >>>> +                src += lfb.lfbp.bytes_per_line;
> >>>> +                dst += lfb.lfbp.bytes_per_line;
> >>>> +            }
> >>>> +
> >>>> +            /* Render new line. */
> >>>> +            lfb_show_line(
> >>>> +                lfb.text_buf,
> >>>> +                lfb.lfbp.lfb + (lfb.lfbp.text_rows-1) * 
> >>>> lfb.lfbp.font->height * lfb.lfbp.bytes_per_line,
> >>> Long line?
> >> Right
> >>
> > The calculation of
> >
> > lfb.lfbp.lfb + lfb.lfbp.font->height * lfb.lfbp.bytes_per_line;
> >
> > is done twice in the above section - surely that's constant over time, so 
> > could be stored in another local variable - thus making the line shorter 
> > and at the same time more obvious that "it's the same thing".
> No, it isn't! I started out with
> 
> lfb.lfbp.font->height * lfb.lfbp.bytes_per_line
> 
> being calculated twice. Then mistakenly thought that it was MORE of the same, 
> missing out that the second calculation is different. Sorry for that. But the 
> font->hight * bytes_per_line calculation is definitely the same in both 
> places.

Thanks for the review!

lfb_scroll_puts and lfb_show_line are just vesa_scroll_puts and
vesa_show_line renamed and with some basic variable name changes.
I would be reticent to change them in this patch unless necessary.

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.