[fbdev_backend]: support of DIRECTCOLOR Frame buffer

[Marc Chalain]

It's not broken, it's just 1 value per 65535 unused for red and blue. The
difference between the red 65534 and the red 65535 is insignificant. The
real fix is more significant when we run on target with 126Mb of RAM or
less. Don't forget that on PC Desktop you find very big video card which
runs with A8R8G8B8 pixel format in lot of cases.



2013/6/11 Pekka Paalanen <ppaalanen at gmail.com>

> On Tue, 11 Jun 2013 16:31:17 +0200
> Marc Chalain <marc.chalain at gmail.com> wrote:
>
> > Theoretically you have right.
> > But the pixel formats are defined to contains the same amounts of values
> > for each colors except for 16bpp format (r5g6b5 and b5g6r5 one value more
> > for green) and the alpha. My code is optimized for a lot of cases.
> > For the use of rcols instead cols, you have right. It's a mistake.
>
> So, you think it's ok to be presumably broken on 565 formats?
> The fix really is not that hard, and the "optimizations" here are
> totally insignificant.
>
> > Sorry "git send-email" fails on my computer (I think it's come from the
> > proxy).
>
> There are other ways to send email without messing up whitespace,
> any of them will do. Take a look at other people's patch series on
> the mailing list, how they look, and try to replicate that.
>
> Until then, I think I've reviewed enough here for now.
>
> Btw. the idle-time man page patch should be part of the same commit
> as the idle-time config implementation.
>
>
> Thanks,
> pq
>
> > [PATCH] fbdev: directcolor support
> > The backend accepts to use TRUECOLOR and DIRECTCOLOR which are similares.
> > DIRECTCOLOR has better results if the application applies a Colors
> > map before to use it.
> > The colors map is just a list of gradients for each colors and alpha.
> >
> > ---
> >  src/compositor-fbdev.c |   43
> ++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 42 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> > index 9c3d17e..7a3374e 100644
> > --- a/src/compositor-fbdev.c
> > +++ b/src/compositor-fbdev.c
> > @@ -203,6 +203,43 @@ finish_frame_handler(void *data)
> >      return 1;
> >  }
> >
> > +static int
> > +set_cmap(int fd, struct fb_var_screeninfo *vinfo)
> > +{
> > +    struct fb_cmap cmap;
> > +    uint16_t *colors;
> > +    int i;
> > +    int rcols = 1 << vinfo->red.length;
> > +    int gcols = 1 << vinfo->green.length;
> > +    int bcols = 1 << vinfo->blue.length;
> > +
> > +    memset(&cmap, 0, sizeof(cmap));
> > +
> > +    /* Make our palette the length of the deepest color */
> > +    int cols = (rcols > gcols ? rcols : gcols);
> > +    cols = (cols > bcols ? cols : bcols);
> > +
> > +    colors = malloc(sizeof(uint16_t) * cols);
> > +    if (!colors) return -1;
> > +
> > +    for (i = 0; i < cols; i++)
> > +        colors[i] = (UINT16_MAX / (cols - 1)) * i;
> > +
> > +    cmap.start = 0;
> > +    cmap.len = cols;
> > +    cmap.red = colors;
> > +    cmap.blue = colors;
> > +    cmap.green = colors;
> > +    if (vinfo->transp.length)
> > +        cmap.transp = colors;
> > +    else
> > +        cmap.transp = NULL;
> > +    ioctl(fd, FBIOPUTCMAP, &cmap);
> > +
> > +    free(colors);
> > +    return 0;
> > +}
> > +
> >  static pixman_format_code_t
> >  calculate_pixman_format(struct fb_var_screeninfo *vinfo,
> >                          struct fb_fix_screeninfo *finfo)
> > @@ -242,7 +279,8 @@ calculate_pixman_format(struct fb_var_screeninfo
> *vinfo,
> >          return 0;
> >
> >      /* We only handle true-colour frame buffers at the moment. */
> > -    if (finfo->visual != FB_VISUAL_TRUECOLOR || vinfo->grayscale != 0)
> > +    if (!(finfo->visual == FB_VISUAL_TRUECOLOR || finfo->visual ==
> > FB_VISUAL_DIRECTCOLOR)
> > +        || vinfo->grayscale != 0)
> >          return 0;
> >
> >      /* We only support formats with MSBs on the left. */
> > @@ -325,6 +363,9 @@ fbdev_query_screen_info(struct fbdev_output *output,
> > int fd,
> >      info->pixel_format = calculate_pixman_format(&varinfo, &fixinfo);
> >      info->refresh_rate = calculate_refresh_rate(&varinfo);
> >
> > +    if (fixinfo.visual == FB_VISUAL_DIRECTCOLOR)
> > +        set_cmap(fd, &varinfo);
> > +
> >      if (info->pixel_format == 0) {
> >          weston_log("Frame buffer uses an unsupported format.\n");
> >          return -1;
> > --
> > 1.7.9.5
> >
> >
> >
> >
> > 2013/6/11 Pekka Paalanen <ppaalanen at gmail.com>
> >
> > > On Tue, 11 Jun 2013 15:15:29 +0200
> > > Marc Chalain <marc.chalain at gmail.com> wrote:
> > >
> > > > Hello,
> > > > OK this patch now checks FB_VISUAL_DIRECTCOLOR twice but set the cmap
> > > > inside the fbdev_query_screen_info function.
> > > > I use "malloc" instead "table" declaration because it takes less
> memory
> > > and
> > > > it's used only once, it's useless to be faster.
> > > > After I set the same values for each colors, because it's a default
> map.
> > > To
> > > > set more finely we have to know the screen and it's capabilities.
> > >
> > > What I meant is, if rcols = 32 and gcols = 64, you allocate a 64-long
> > > array, and fill it with 64 step values. Does that not mean, that the
> > > maximum red color will be only 50% intensity, because it goes only up
> to
> > > 32 steps, and the remaining 32 steps will never be used?
> > >
> > > Or am I misunderstanding how fbdev colormap works?
> > >
> > > > I don't set the alpha when it's required (no alpha => no alpha map)
> > > >
> > > > [PATCH] [compositor-fbdev]: DirectColor support
> > > > The backend accepts to use TRUECOLOR and DIRECTCOLOR which are
> similares.
> > > > DIRECTCOLOR has better results if the application applies a Colors
> map
> > > > before to use it.
> > > > The colors map is just a list of gradients for each colors and alpha.
> > > >
> > > > ---
> > > >  src/compositor-fbdev.c |   43
> > > ++++++++++++++++++++++++++++++++++++++++++-
> > > >  1 file changed, 42 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> > > > index 9c3d17e..c966d5a 100644
> > > > --- a/src/compositor-fbdev.c
> > > > +++ b/src/compositor-fbdev.c
> > > > @@ -203,6 +203,43 @@ finish_frame_handler(void *data)
> > > >      return 1;
> > > >  }
> > > >
> > > > +static int
> > > > +set_cmap(int fd, struct fb_var_screeninfo *vinfo)
> > > > +{
> > > > +    struct fb_cmap cmap;
> > > > +    uint16_t *colors;
> > > > +    int i;
> > > > +    int rcols = 1 << vinfo->red.length;
> > > > +    int gcols = 1 << vinfo->green.length;
> > > > +    int bcols = 1 << vinfo->blue.length;
> > > > +
> > > > +    memset(&cmap, 0, sizeof(cmap));
> > > > +
> > > > +    /* Make our palette the length of the deepest color */
> > > > +    int cols = (rcols > gcols ? rcols : gcols);
> > > > +    cols = (cols > bcols ? cols : bcols);
> > > > +
> > > > +    colors = malloc(sizeof(uint16_t) * cols);
> > > > +    if (!colors) return -1;
> > > > +
> > > > +    for (i = 0; i < rcols; i++)
> > > > +        colors[i] = (UINT16_MAX / (rcols - 1)) * i;
> > >
> > > IOW, should this divisor not be per channel, instead of using the same
> > > for all channels?
> > >
> > > In fact, you are using rcols in the divisor here, so if cols > rcols...
> > > no wait...
> > >
> > > > +
> > > > +    cmap.start = 0;
> > > > +    cmap.len = cols;
> > >
> > > Here you use cols, but the loop uses rcols. How does this make sense?
> > >
> > > > +    cmap.red = colors;
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20130612/df7f93e2/attachment.html>