[fbdev_backend]: support of DIRECTCOLOR Frame buffer

[Pekka Paalanen]

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;