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

[Mats Petersson <mats.petersson@citrix.com>]

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@eu.citrix.com> 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.

--
Mats

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