[fbdev_backend]: support of DIRECTCOLOR Frame buffer

[Pekka Paalanen]

On Fri, 7 Jun 2013 17:13:59 +0200
Marc Chalain <marc.chalain at gmail.com> wrote:

> From e30486e0288e7be526fc358a5318b344522e407b Mon Sep 17 00:00:00 2001
> From: mchalain <marc.chalain at gmail.com>
> Date: Fri, 7 Jun 2013 17:12:31 +0200
> Subject: [PATCH] [fbdev_backend]: support of DIRECTCOLOR Frame buffer
> 

A properly formatted title:
	compositor-fbdev: add direct color support

Commit message is missing.

> ---
>  src/compositor-fbdev.c |   49
> +++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 46 insertions(+), 3 deletions(-)
> 
> diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> index 9c3d17e..d6d911a 100644
> --- a/src/compositor-fbdev.c
> +++ b/src/compositor-fbdev.c
> @@ -203,9 +203,42 @@ finish_frame_handler(void *data)
>      return 1;
>  }
> 
> +static int
> +build_cmap(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;
> +
> +    /* 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;
> +
> +    cmap->start = 0;
> +    cmap->len = cols;
> +    cmap->red = colors;
> +    cmap->blue = colors;
> +    cmap->green = colors;

If there actually is a difference between rcols, gcols, and bcols,
should you not compute a different a 'colors' array for each different
_cols?

> +    if (vinfo->transp.length)
> +        cmap->transp = colors;
> +    else
> +        cmap->transp = NULL;

Now, 'colors' is malloc'd here. How does the caller of this function
know what to free? Freeing each of cmap->{red,blue,green,transp} would
lead to double-free errors.

Right, you free only 'red'. But that's rather surprising.

> +    return 0;
> +}
> +
>  static pixman_format_code_t
>  calculate_pixman_format(struct fb_var_screeninfo *vinfo,
> -                        struct fb_fix_screeninfo *finfo)
> +                        struct fb_fix_screeninfo *finfo,
> +                        struct fb_cmap *cmap)
>  {
>      /* Calculate the pixman format supported by the frame buffer from the
>       * buffer's metadata. Return 0 if no known pixman format is supported
> @@ -241,7 +274,10 @@ calculate_pixman_format(struct fb_var_screeninfo
> *vinfo,
>      if (finfo->type != FB_TYPE_PACKED_PIXELS)
>          return 0;
> 
> -    /* We only handle true-colour frame buffers at the moment. */
> +    if (finfo->visual == FB_VISUAL_DIRECTCOLOR)
> +        build_cmap(vinfo, cmap);

This is a strange place to call build_cmap(). The purpose of this
function is to return a pixman format or an error, not to fill a
fb_cmap struct.

Yes, you probably need to test for FB_VISUAL_DIRECTCOLOR in two places.

> +    else
> +     /* We only handle true-colour frame buffers at the moment. */
>      if (finfo->visual != FB_VISUAL_TRUECOLOR || vinfo->grayscale != 0)
>          return 0;
> 
> @@ -304,6 +340,9 @@ fbdev_query_screen_info(struct fbdev_output *output,
> int fd,
>  {
>      struct fb_var_screeninfo varinfo;
>      struct fb_fix_screeninfo fixinfo;
> +    struct fb_cmap cmap;
> +
> +    memset(&cmap, 0, sizeof(cmap));
> 
>      /* Probe the device for screen information. */
>      if (ioctl(fd, FBIOGET_FSCREENINFO, &fixinfo) < 0 ||
> @@ -322,9 +361,13 @@ fbdev_query_screen_info(struct fbdev_output *output,
> int fd,
>      info->line_length = fixinfo.line_length;
>      strncpy(info->id, fixinfo.id, sizeof(info->id) / sizeof(*info->id));
> 
> -    info->pixel_format = calculate_pixman_format(&varinfo, &fixinfo);
> +    info->pixel_format = calculate_pixman_format(&varinfo, &fixinfo,
> &cmap);
>      info->refresh_rate = calculate_refresh_rate(&varinfo);
> 
> +    if (cmap.red != NULL) {
> +        ioctl(fd, FBIOPUTCMAP, &cmap);
> +        free(cmap.red);
> +    }
>      if (info->pixel_format == 0) {
>          weston_log("Frame buffer uses an unsupported format.\n");
>          return -1;

This patch is indented with 4 spaces, while weston codebase uses hard
tabs of width 8. Maybe your email client messed up the whitespace?
I also see a few lines cut unintendedly.


Thanks
pq