diff mbox series

[2/8] drm/fb-helper: Add support for DRM_FORMAT_C[124]

Message ID 20220215165226.2738568-3-geert@linux-m68k.org (mailing list archive)
State Handled Elsewhere
Headers show
Series drm: Add support for low-color frame buffer formats | expand

Commit Message

Geert Uytterhoeven Feb. 15, 2022, 4:52 p.m. UTC
Add support for color-indexed frame buffer formats with two, four, and
sixteen colors to the DRM framebuffer helper functions:
  1. Add support for depths 1/2/4 to the damage helper,
  2. For color-indexed modes, the length of the color bitfields must be
     set to the color depth, else the logo code may pick a logo with too
     many colors.  Drop the incorrect DAC width comment, which
     originates from the i915 driver.
  3. Accept C[124] modes when validating or filling in struct
     fb_var_screeninfo, and  use the correct number of bits per pixel.
  4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all supported
     color-indexed modes.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/gpu/drm/drm_fb_helper.c | 120 +++++++++++++++++++++++++-------
 1 file changed, 93 insertions(+), 27 deletions(-)

Comments

Thomas Zimmermann Feb. 17, 2022, 2:57 p.m. UTC | #1
Hi Geert

Am 15.02.22 um 17:52 schrieb Geert Uytterhoeven:
> Add support for color-indexed frame buffer formats with two, four, and
> sixteen colors to the DRM framebuffer helper functions:
>    1. Add support for depths 1/2/4 to the damage helper,
>    2. For color-indexed modes, the length of the color bitfields must be
>       set to the color depth, else the logo code may pick a logo with too
>       many colors.  Drop the incorrect DAC width comment, which
>       originates from the i915 driver.
>    3. Accept C[124] modes when validating or filling in struct
>       fb_var_screeninfo, and  use the correct number of bits per pixel.
>    4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all supported
>       color-indexed modes.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 120 +++++++++++++++++++++++++-------
>   1 file changed, 93 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index ed43b987d306afce..a4afed0de1570841 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -376,12 +376,34 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
>   					   struct iosys_map *dst)
>   {
>   	struct drm_framebuffer *fb = fb_helper->fb;
> -	unsigned int cpp = fb->format->cpp[0];
> -	size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> -	void *src = fb_helper->fbdev->screen_buffer + offset;
> -	size_t len = (clip->x2 - clip->x1) * cpp;
> +	size_t offset = clip->y1 * fb->pitches[0];
> +	size_t len = clip->x2 - clip->x1;
>   	unsigned int y;
> +	void *src;
>   
> +	switch (fb->format->depth) {

The depth field is deprecated. It's probably better to use 
fb->format->format and test against 4CC codes.

> +	case 1:
> +		offset += clip->x1 / 8;
> +		len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
> +		break;
> +

Style: no empty lines here.

> +	case 2:
> +		offset += clip->x1 / 4;
> +		len = DIV_ROUND_UP(len + clip->x1 % 4, 4);
> +		break;
> +
> +	case 4:
> +		offset += clip->x1 / 2;
> +		len = DIV_ROUND_UP(len + clip->x1 % 2, 2);
> +		break;
> +

Can we handle case C8 like C[124]? Seems cleaner to me.

> +	default:
> +		offset += clip->x1 * fb->format->cpp[0];
> +		len *= fb->format->cpp[0];
> +		break;
> +	}
> +
> +	src = fb_helper->fbdev->screen_buffer + offset;
>   	iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
>   
>   	for (y = clip->y1; y < clip->y2; y++) {
> @@ -1231,19 +1253,30 @@ static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1,
>   }
>   
>   static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
> -					 u8 depth)
> -{
> -	switch (depth) {
> -	case 8:
> +					 const struct drm_format_info *format)
> +{
> +	u8 depth = format->depth;
> +
> +	switch (format->format) {
> +	// FIXME Perhaps
> +	// #define DRM_FORMAT_C0 fourcc_code('C', '0', ' ', ' ')

What is C0?

> +	// if ((format & fourcc_code(0xff, 0xf8, 0xff, 0xff) == DRM_FORMAT_C0) ...
> +	case DRM_FORMAT_C1:
> +	case DRM_FORMAT_C2:
> +	case DRM_FORMAT_C4:
> +	case DRM_FORMAT_C8:
>   		var->red.offset = 0;
>   		var->green.offset = 0;
>   		var->blue.offset = 0;
> -		var->red.length = 8; /* 8bit DAC */
> -		var->green.length = 8;
> -		var->blue.length = 8;
> +		var->red.length = depth;
> +		var->green.length = depth;
> +		var->blue.length = depth;
>   		var->transp.offset = 0;
>   		var->transp.length = 0;
> -		break;
> +		return;
> +	}
> +
> +	switch (depth) {
>   	case 15:
>   		var->red.offset = 10;
>   		var->green.offset = 5;
> @@ -1298,7 +1331,9 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>   {
>   	struct drm_fb_helper *fb_helper = info->par;
>   	struct drm_framebuffer *fb = fb_helper->fb;
> +	const struct drm_format_info *format = fb->format;
>   	struct drm_device *dev = fb_helper->dev;
> +	unsigned int bpp;
>   
>   	if (in_dbg_master())
>   		return -EINVAL;
> @@ -1308,22 +1343,34 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>   		var->pixclock = 0;
>   	}
>   
> -	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
> -	    (drm_format_info_block_height(fb->format, 0) > 1))
> -		return -EINVAL;
> +	switch (format->format) {
> +	case DRM_FORMAT_C1:
> +	case DRM_FORMAT_C2:
> +	case DRM_FORMAT_C4:
> +		bpp = format->depth;
> +		break;

Added C8 here would be more consistent.

> +
> +	default:
> +		if ((drm_format_info_block_width(format, 0) > 1) ||
> +		    (drm_format_info_block_height(format, 0) > 1))
> +			return -EINVAL;
> +
> +		bpp = format->cpp[0] * 8;
> +		break;
> +	}
>   
>   	/*
>   	 * Changes struct fb_var_screeninfo are currently not pushed back
>   	 * to KMS, hence fail if different settings are requested.
>   	 */
> -	if (var->bits_per_pixel > fb->format->cpp[0] * 8 ||
> +	if (var->bits_per_pixel > bpp ||
>   	    var->xres > fb->width || var->yres > fb->height ||
>   	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
>   		drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in current fb "
>   			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
>   			  var->xres, var->yres, var->bits_per_pixel,
>   			  var->xres_virtual, var->yres_virtual,
> -			  fb->width, fb->height, fb->format->cpp[0] * 8);
> +			  fb->width, fb->height, bpp);
>   		return -EINVAL;
>   	}
>   
> @@ -1338,13 +1385,13 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>   	    !var->blue.length    && !var->transp.length   &&
>   	    !var->red.msb_right  && !var->green.msb_right &&
>   	    !var->blue.msb_right && !var->transp.msb_right) {
> -		drm_fb_helper_fill_pixel_fmt(var, fb->format->depth);
> +		drm_fb_helper_fill_pixel_fmt(var, format);
>   	}
>   
>   	/*
>   	 * Likewise, bits_per_pixel should be rounded up to a supported value.
>   	 */
> -	var->bits_per_pixel = fb->format->cpp[0] * 8;
> +	var->bits_per_pixel = bpp;
>   
>   	/*
>   	 * drm fbdev emulation doesn't support changing the pixel format at all,
> @@ -1680,11 +1727,20 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>   }
>   
>   static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> -				   uint32_t depth)
> +				   uint32_t format)
>   {
>   	info->fix.type = FB_TYPE_PACKED_PIXELS;
> -	info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> -		FB_VISUAL_TRUECOLOR;
> +	switch (format) {
> +	case DRM_FORMAT_C1:
> +	case DRM_FORMAT_C2:
> +	case DRM_FORMAT_C4:
> +	case DRM_FORMAT_C8:
> +		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
> +		break;
> +	default:
> +		info->fix.visual = FB_VISUAL_TRUECOLOR;
> +		break;
> +	}
>   	info->fix.mmio_start = 0;
>   	info->fix.mmio_len = 0;
>   	info->fix.type_aux = 0;
> @@ -1701,19 +1757,29 @@ static void drm_fb_helper_fill_var(struct fb_info *info,
>   				   uint32_t fb_width, uint32_t fb_height)
>   {
>   	struct drm_framebuffer *fb = fb_helper->fb;
> +	const struct drm_format_info *format = fb->format;
>   
> -	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
> -		(drm_format_info_block_height(fb->format, 0) > 1));
>   	info->pseudo_palette = fb_helper->pseudo_palette;
>   	info->var.xres_virtual = fb->width;
>   	info->var.yres_virtual = fb->height;
> -	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
> +	switch (format->format) {
> +	case DRM_FORMAT_C1:
> +	case DRM_FORMAT_C2:
> +	case DRM_FORMAT_C4:
> +		info->var.bits_per_pixel = format->depth;
> +		break;

C8.


The fbdev helpers look correct to me.  I'm not so sure about the usage 
of the format info; especially the depth field.  The docs say that the 
field is deprecated and should be 0. Maybe depth can be handled within 
fbdev?

Best regards
Thomas

> +
> +	default:
> +		WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
> +			(drm_format_info_block_height(format, 0) > 1));
> +		info->var.bits_per_pixel = format->cpp[0] * 8;
> +	}
>   	info->var.accel_flags = FB_ACCELF_TEXT;
>   	info->var.xoffset = 0;
>   	info->var.yoffset = 0;
>   	info->var.activate = FB_ACTIVATE_NOW;
>   
> -	drm_fb_helper_fill_pixel_fmt(&info->var, fb->format->depth);
> +	drm_fb_helper_fill_pixel_fmt(&info->var, format);
>   
>   	info->var.xres = fb_width;
>   	info->var.yres = fb_height;
> @@ -1738,7 +1804,7 @@ void drm_fb_helper_fill_info(struct fb_info *info,
>   {
>   	struct drm_framebuffer *fb = fb_helper->fb;
>   
> -	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->depth);
> +	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->format);
>   	drm_fb_helper_fill_var(info, fb_helper,
>   			       sizes->fb_width, sizes->fb_height);
>
Geert Uytterhoeven Feb. 17, 2022, 4:12 p.m. UTC | #2
Hi Thomas,

Thanks for your review!

On Thu, Feb 17, 2022 at 3:57 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 15.02.22 um 17:52 schrieb Geert Uytterhoeven:
> > Add support for color-indexed frame buffer formats with two, four, and
> > sixteen colors to the DRM framebuffer helper functions:
> >    1. Add support for depths 1/2/4 to the damage helper,
> >    2. For color-indexed modes, the length of the color bitfields must be
> >       set to the color depth, else the logo code may pick a logo with too
> >       many colors.  Drop the incorrect DAC width comment, which
> >       originates from the i915 driver.
> >    3. Accept C[124] modes when validating or filling in struct
> >       fb_var_screeninfo, and  use the correct number of bits per pixel.
> >    4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all supported
> >       color-indexed modes.
> >
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>

> > --- a/drivers/gpu/drm/drm_fb_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_helper.c
> > @@ -376,12 +376,34 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
> >                                          struct iosys_map *dst)
> >   {
> >       struct drm_framebuffer *fb = fb_helper->fb;
> > -     unsigned int cpp = fb->format->cpp[0];
> > -     size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> > -     void *src = fb_helper->fbdev->screen_buffer + offset;
> > -     size_t len = (clip->x2 - clip->x1) * cpp;
> > +     size_t offset = clip->y1 * fb->pitches[0];
> > +     size_t len = clip->x2 - clip->x1;
> >       unsigned int y;
> > +     void *src;
> >
> > +     switch (fb->format->depth) {
>
> The depth field is deprecated. It's probably better to use
> fb->format->format and test against 4CC codes.

The reason I checked for depth instead of a 4CC code is that the only
thing that matters here is the number of bits per pixel.  Hence this
function won't need any changes to support R1, R2, R4, and D1 later.
When we get here, we already know that we are using a format that
is supported by the fbdev helper code, and thus passed the 4CC
checks elsewhere.

Alternatively, we could introduce drm_format_info_bpp() earlier in
the series, and use that?

>
> > +     case 1:
> > +             offset += clip->x1 / 8;
> > +             len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
> > +             break;
> > +
>
> Style: no empty lines here.

OK.

> > +     case 2:
> > +             offset += clip->x1 / 4;
> > +             len = DIV_ROUND_UP(len + clip->x1 % 4, 4);
> > +             break;
> > +
> > +     case 4:
> > +             offset += clip->x1 / 2;
> > +             len = DIV_ROUND_UP(len + clip->x1 % 2, 2);
> > +             break;
> > +
>
> Can we handle case C8 like C[124]? Seems cleaner to me.

The cases above are purely to handle bpp < 8; they are not
about color-indexed vs. truecolor modes.
XRGB1111 mode would need to be handled above, too.

> > @@ -1231,19 +1253,30 @@ static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1,
> >   }
> >
> >   static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
> > -                                      u8 depth)
> > -{
> > -     switch (depth) {
> > -     case 8:
> > +                                      const struct drm_format_info *format)
> > +{
> > +     u8 depth = format->depth;
> > +
> > +     switch (format->format) {
> > +     // FIXME Perhaps
> > +     // #define DRM_FORMAT_C0 fourcc_code('C', '0', ' ', ' ')
>
> What is C0?

A non-existing color-indexed mode with zero colors ;-)
Introduced purely to make a check like in the comment below work.
What we really want to check here is if the mode is color-indexed
or not...

> > +     // if ((format & fourcc_code(0xff, 0xf8, 0xff, 0xff) == DRM_FORMAT_C0) ...
> > +     case DRM_FORMAT_C1:
> > +     case DRM_FORMAT_C2:
> > +     case DRM_FORMAT_C4:
> > +     case DRM_FORMAT_C8:
> >               var->red.offset = 0;
> >               var->green.offset = 0;
> >               var->blue.offset = 0;
> > -             var->red.length = 8; /* 8bit DAC */
> > -             var->green.length = 8;
> > -             var->blue.length = 8;
> > +             var->red.length = depth;
> > +             var->green.length = depth;
> > +             var->blue.length = depth;
> >               var->transp.offset = 0;
> >               var->transp.length = 0;
> > -             break;
> > +             return;
> > +     }
> > +
> > +     switch (depth) {
> >       case 15:
> >               var->red.offset = 10;
> >               var->green.offset = 5;
> > @@ -1298,7 +1331,9 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >   {
> >       struct drm_fb_helper *fb_helper = info->par;
> >       struct drm_framebuffer *fb = fb_helper->fb;
> > +     const struct drm_format_info *format = fb->format;
> >       struct drm_device *dev = fb_helper->dev;
> > +     unsigned int bpp;
> >
> >       if (in_dbg_master())
> >               return -EINVAL;
> > @@ -1308,22 +1343,34 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> >               var->pixclock = 0;
> >       }
> >
> > -     if ((drm_format_info_block_width(fb->format, 0) > 1) ||
> > -         (drm_format_info_block_height(fb->format, 0) > 1))
> > -             return -EINVAL;
> > +     switch (format->format) {
> > +     case DRM_FORMAT_C1:
> > +     case DRM_FORMAT_C2:
> > +     case DRM_FORMAT_C4:
> > +             bpp = format->depth;
> > +             break;
>
> Added C8 here would be more consistent.

Again, this is not about color-indexed vs. truecolor, but about bpp.
drm_format_info_bpp()?

 > +
> > +     default:
> > +             if ((drm_format_info_block_width(format, 0) > 1) ||
> > +                 (drm_format_info_block_height(format, 0) > 1))
> > +                     return -EINVAL;
> > +
> > +             bpp = format->cpp[0] * 8;
> > +             break;
> > +     }

> > @@ -1680,11 +1727,20 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> >   }
> >
> >   static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> > -                                uint32_t depth)
> > +                                uint32_t format)
> >   {
> >       info->fix.type = FB_TYPE_PACKED_PIXELS;
> > -     info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> > -             FB_VISUAL_TRUECOLOR;
> > +     switch (format) {

This one is about color-indexed vs. truecolor.

> > +     case DRM_FORMAT_C1:
> > +     case DRM_FORMAT_C2:
> > +     case DRM_FORMAT_C4:
> > +     case DRM_FORMAT_C8:
> > +             info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
> > +             break;
> > +     default:
> > +             info->fix.visual = FB_VISUAL_TRUECOLOR;
> > +             break;
> > +     }
> >       info->fix.mmio_start = 0;
> >       info->fix.mmio_len = 0;
> >       info->fix.type_aux = 0;
> > @@ -1701,19 +1757,29 @@ static void drm_fb_helper_fill_var(struct fb_info *info,
> >                                  uint32_t fb_width, uint32_t fb_height)
> >   {
> >       struct drm_framebuffer *fb = fb_helper->fb;
> > +     const struct drm_format_info *format = fb->format;
> >
> > -     WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
> > -             (drm_format_info_block_height(fb->format, 0) > 1));
> >       info->pseudo_palette = fb_helper->pseudo_palette;
> >       info->var.xres_virtual = fb->width;
> >       info->var.yres_virtual = fb->height;
> > -     info->var.bits_per_pixel = fb->format->cpp[0] * 8;
> > +     switch (format->format) {
> > +     case DRM_FORMAT_C1:
> > +     case DRM_FORMAT_C2:
> > +     case DRM_FORMAT_C4:
> > +             info->var.bits_per_pixel = format->depth;
> > +             break;
>
> C8.

Again, this is not about color-indexed vs. truecolor, but about bpp.
Here I do check the 4CC codes, as this controls which modes can be
handled by the fbdev emulation, and we do not want to let random
modes with depth or bpp < 8 pass.

> The fbdev helpers look correct to me.  I'm not so sure about the usage
> of the format info; especially the depth field.  The docs say that the
> field is deprecated and should be 0. Maybe depth can be handled within
> fbdev?

Perhaps. I don't know enough about DRM to know what the depth field
is used for.

Note that true fbdev supports all values of depth < bpp (e.g. a
32-color mode (depth = 5) where each pixel is stored in one byte).
I do not suggest adding support for that, though ;-)

> > +
> > +     default:
> > +             WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
> > +                     (drm_format_info_block_height(format, 0) > 1));

BTW, probably this WARN_ON() (which existed before, but got moved)
should be converted into returning an error instead.

> > +             info->var.bits_per_pixel = format->cpp[0] * 8;
> > +     }
> >       info->var.accel_flags = FB_ACCELF_TEXT;
> >       info->var.xoffset = 0;
> >       info->var.yoffset = 0;
> >       info->var.activate = FB_ACTIVATE_NOW;
> >
> > -     drm_fb_helper_fill_pixel_fmt(&info->var, fb->format->depth);
> > +     drm_fb_helper_fill_pixel_fmt(&info->var, format);
> >
> >       info->var.xres = fb_width;
> >       info->var.yres = fb_height;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Simon Ser Feb. 17, 2022, 4:18 p.m. UTC | #3
On Thursday, February 17th, 2022 at 17:12, Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > What is C0?
>
> A non-existing color-indexed mode with zero colors ;-)
> Introduced purely to make a check like in the comment below work.
> What we really want to check here is if the mode is color-indexed
> or not...

Maybe it would be worth introducing a drm_format_info_is_color_indexed
function? Would be self-describing when used, and would avoid to miss
some places to update when adding new color-indexed formats.
Geert Uytterhoeven Feb. 17, 2022, 5:21 p.m. UTC | #4
Hi Simon,

On Thu, Feb 17, 2022 at 5:18 PM Simon Ser <contact@emersion.fr> wrote:
> On Thursday, February 17th, 2022 at 17:12, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > What is C0?
> >
> > A non-existing color-indexed mode with zero colors ;-)
> > Introduced purely to make a check like in the comment below work.
> > What we really want to check here is if the mode is color-indexed
> > or not...
>
> Maybe it would be worth introducing a drm_format_info_is_color_indexed
> function? Would be self-describing when used, and would avoid to miss
> some places to update when adding new color-indexed formats.

Yep, and a .is_color_indexed flag, cfr. the existing .is_yuv flag.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Sam Ravnborg Feb. 17, 2022, 8:34 p.m. UTC | #5
Hi Geert,

> > > +     switch (fb->format->depth) {
> >
> > The depth field is deprecated. It's probably better to use
> > fb->format->format and test against 4CC codes.
> 
> The reason I checked for depth instead of a 4CC code is that the only
> thing that matters here is the number of bits per pixel.  Hence this
> function won't need any changes to support R1, R2, R4, and D1 later.
> When we get here, we already know that we are using a format that
> is supported by the fbdev helper code, and thus passed the 4CC
> checks elsewhere.
> 
> Alternatively, we could introduce drm_format_info_bpp() earlier in
> the series, and use that?

The drm_format_info_bpp() is very descriptive, so yes it would be good to use
it here also.

	Sam
Thomas Zimmermann Feb. 18, 2022, 8:14 a.m. UTC | #6
Hi

Am 17.02.22 um 17:12 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> Thanks for your review!
> 
> On Thu, Feb 17, 2022 at 3:57 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 15.02.22 um 17:52 schrieb Geert Uytterhoeven:
>>> Add support for color-indexed frame buffer formats with two, four, and
>>> sixteen colors to the DRM framebuffer helper functions:
>>>     1. Add support for depths 1/2/4 to the damage helper,
>>>     2. For color-indexed modes, the length of the color bitfields must be
>>>        set to the color depth, else the logo code may pick a logo with too
>>>        many colors.  Drop the incorrect DAC width comment, which
>>>        originates from the i915 driver.
>>>     3. Accept C[124] modes when validating or filling in struct
>>>        fb_var_screeninfo, and  use the correct number of bits per pixel.
>>>     4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all supported
>>>        color-indexed modes.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> 
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -376,12 +376,34 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
>>>                                           struct iosys_map *dst)
>>>    {
>>>        struct drm_framebuffer *fb = fb_helper->fb;
>>> -     unsigned int cpp = fb->format->cpp[0];
>>> -     size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
>>> -     void *src = fb_helper->fbdev->screen_buffer + offset;
>>> -     size_t len = (clip->x2 - clip->x1) * cpp;
>>> +     size_t offset = clip->y1 * fb->pitches[0];
>>> +     size_t len = clip->x2 - clip->x1;
>>>        unsigned int y;
>>> +     void *src;
>>>
>>> +     switch (fb->format->depth) {
>>
>> The depth field is deprecated. It's probably better to use
>> fb->format->format and test against 4CC codes.
> 
> The reason I checked for depth instead of a 4CC code is that the only
> thing that matters here is the number of bits per pixel.  Hence this
> function won't need any changes to support R1, R2, R4, and D1 later.
> When we get here, we already know that we are using a format that
> is supported by the fbdev helper code, and thus passed the 4CC
> checks elsewhere.

At some point, we will probably have to change several of these tests to 
4cc. C8 and RGB332 both have 8-bit depth/bpp; same for C4 and RGB121; or 
whatever low-color formats we also want to add.

It's not a blocker now, but maybe something to keep in mind.

> 
> Alternatively, we could introduce drm_format_info_bpp() earlier in
> the series, and use that?

Having a helper for this might indeed be useful. We use depth for the 
number of color bits and bpp for the number of bits in he pixel.  That's 
important for XRGB8888, where depth is 24, or XRGB555 where depth is 15.

If that makes sense, maybe have a helper for depth and one for bpp, even 
if they return the same value in most of the cases.

> 
>>
>>> +     case 1:
>>> +             offset += clip->x1 / 8;
>>> +             len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
>>> +             break;
>>> +
>>
>> Style: no empty lines here.
> 
> OK.
> 
>>> +     case 2:
>>> +             offset += clip->x1 / 4;
>>> +             len = DIV_ROUND_UP(len + clip->x1 % 4, 4);
>>> +             break;
>>> +
>>> +     case 4:
>>> +             offset += clip->x1 / 2;
>>> +             len = DIV_ROUND_UP(len + clip->x1 % 2, 2);
>>> +             break;
>>> +
>>
>> Can we handle case C8 like C[124]? Seems cleaner to me.
> 
> The cases above are purely to handle bpp < 8; they are not
> about color-indexed vs. truecolor modes.
> XRGB1111 mode would need to be handled above, too.

I see.

> 
>>> @@ -1231,19 +1253,30 @@ static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1,
>>>    }
>>>
>>>    static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
>>> -                                      u8 depth)
>>> -{
>>> -     switch (depth) {
>>> -     case 8:
>>> +                                      const struct drm_format_info *format)
>>> +{
>>> +     u8 depth = format->depth;
>>> +
>>> +     switch (format->format) {
>>> +     // FIXME Perhaps
>>> +     // #define DRM_FORMAT_C0 fourcc_code('C', '0', ' ', ' ')
>>
>> What is C0?
> 
> A non-existing color-indexed mode with zero colors ;-)
> Introduced purely to make a check like in the comment below work.
> What we really want to check here is if the mode is color-indexed
> or not...

I think I'd rather keep that switch.

Best regards
Thomas

> 
>>> +     // if ((format & fourcc_code(0xff, 0xf8, 0xff, 0xff) == DRM_FORMAT_C0) ...
>>> +     case DRM_FORMAT_C1:
>>> +     case DRM_FORMAT_C2:
>>> +     case DRM_FORMAT_C4:
>>> +     case DRM_FORMAT_C8:
>>>                var->red.offset = 0;
>>>                var->green.offset = 0;
>>>                var->blue.offset = 0;
>>> -             var->red.length = 8; /* 8bit DAC */
>>> -             var->green.length = 8;
>>> -             var->blue.length = 8;
>>> +             var->red.length = depth;
>>> +             var->green.length = depth;
>>> +             var->blue.length = depth;
>>>                var->transp.offset = 0;
>>>                var->transp.length = 0;
>>> -             break;
>>> +             return;
>>> +     }
>>> +
>>> +     switch (depth) {
>>>        case 15:
>>>                var->red.offset = 10;
>>>                var->green.offset = 5;
>>> @@ -1298,7 +1331,9 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>>>    {
>>>        struct drm_fb_helper *fb_helper = info->par;
>>>        struct drm_framebuffer *fb = fb_helper->fb;
>>> +     const struct drm_format_info *format = fb->format;
>>>        struct drm_device *dev = fb_helper->dev;
>>> +     unsigned int bpp;
>>>
>>>        if (in_dbg_master())
>>>                return -EINVAL;
>>> @@ -1308,22 +1343,34 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
>>>                var->pixclock = 0;
>>>        }
>>>
>>> -     if ((drm_format_info_block_width(fb->format, 0) > 1) ||
>>> -         (drm_format_info_block_height(fb->format, 0) > 1))
>>> -             return -EINVAL;
>>> +     switch (format->format) {
>>> +     case DRM_FORMAT_C1:
>>> +     case DRM_FORMAT_C2:
>>> +     case DRM_FORMAT_C4:
>>> +             bpp = format->depth;
>>> +             break;
>>
>> Added C8 here would be more consistent.
> 
> Again, this is not about color-indexed vs. truecolor, but about bpp.
> drm_format_info_bpp()?
> 
>   > +
>>> +     default:
>>> +             if ((drm_format_info_block_width(format, 0) > 1) ||
>>> +                 (drm_format_info_block_height(format, 0) > 1))
>>> +                     return -EINVAL;
>>> +
>>> +             bpp = format->cpp[0] * 8;
>>> +             break;
>>> +     }
> 
>>> @@ -1680,11 +1727,20 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
>>>    }
>>>
>>>    static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
>>> -                                uint32_t depth)
>>> +                                uint32_t format)
>>>    {
>>>        info->fix.type = FB_TYPE_PACKED_PIXELS;
>>> -     info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
>>> -             FB_VISUAL_TRUECOLOR;
>>> +     switch (format) {
> 
> This one is about color-indexed vs. truecolor.
> 
>>> +     case DRM_FORMAT_C1:
>>> +     case DRM_FORMAT_C2:
>>> +     case DRM_FORMAT_C4:
>>> +     case DRM_FORMAT_C8:
>>> +             info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
>>> +             break;
>>> +     default:
>>> +             info->fix.visual = FB_VISUAL_TRUECOLOR;
>>> +             break;
>>> +     }
>>>        info->fix.mmio_start = 0;
>>>        info->fix.mmio_len = 0;
>>>        info->fix.type_aux = 0;
>>> @@ -1701,19 +1757,29 @@ static void drm_fb_helper_fill_var(struct fb_info *info,
>>>                                   uint32_t fb_width, uint32_t fb_height)
>>>    {
>>>        struct drm_framebuffer *fb = fb_helper->fb;
>>> +     const struct drm_format_info *format = fb->format;
>>>
>>> -     WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
>>> -             (drm_format_info_block_height(fb->format, 0) > 1));
>>>        info->pseudo_palette = fb_helper->pseudo_palette;
>>>        info->var.xres_virtual = fb->width;
>>>        info->var.yres_virtual = fb->height;
>>> -     info->var.bits_per_pixel = fb->format->cpp[0] * 8;
>>> +     switch (format->format) {
>>> +     case DRM_FORMAT_C1:
>>> +     case DRM_FORMAT_C2:
>>> +     case DRM_FORMAT_C4:
>>> +             info->var.bits_per_pixel = format->depth;
>>> +             break;
>>
>> C8.
> 
> Again, this is not about color-indexed vs. truecolor, but about bpp.
> Here I do check the 4CC codes, as this controls which modes can be
> handled by the fbdev emulation, and we do not want to let random
> modes with depth or bpp < 8 pass.
> 
>> The fbdev helpers look correct to me.  I'm not so sure about the usage
>> of the format info; especially the depth field.  The docs say that the
>> field is deprecated and should be 0. Maybe depth can be handled within
>> fbdev?
> 
> Perhaps. I don't know enough about DRM to know what the depth field
> is used for.
> 
> Note that true fbdev supports all values of depth < bpp (e.g. a
> 32-color mode (depth = 5) where each pixel is stored in one byte).
> I do not suggest adding support for that, though ;-)
> 
>>> +
>>> +     default:
>>> +             WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
>>> +                     (drm_format_info_block_height(format, 0) > 1));
> 
> BTW, probably this WARN_ON() (which existed before, but got moved)
> should be converted into returning an error instead.
> 
>>> +             info->var.bits_per_pixel = format->cpp[0] * 8;
>>> +     }
>>>        info->var.accel_flags = FB_ACCELF_TEXT;
>>>        info->var.xoffset = 0;
>>>        info->var.yoffset = 0;
>>>        info->var.activate = FB_ACTIVATE_NOW;
>>>
>>> -     drm_fb_helper_fill_pixel_fmt(&info->var, fb->format->depth);
>>> +     drm_fb_helper_fill_pixel_fmt(&info->var, format);
>>>
>>>        info->var.xres = fb_width;
>>>        info->var.yres = fb_height;
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                  -- Linus Torvalds
Geert Uytterhoeven Feb. 18, 2022, 8:53 a.m. UTC | #7
Hi Thomas,

On Fri, Feb 18, 2022 at 9:14 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 17.02.22 um 17:12 schrieb Geert Uytterhoeven:
> > On Thu, Feb 17, 2022 at 3:57 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Am 15.02.22 um 17:52 schrieb Geert Uytterhoeven:
> >>> Add support for color-indexed frame buffer formats with two, four, and
> >>> sixteen colors to the DRM framebuffer helper functions:
> >>>     1. Add support for depths 1/2/4 to the damage helper,
> >>>     2. For color-indexed modes, the length of the color bitfields must be
> >>>        set to the color depth, else the logo code may pick a logo with too
> >>>        many colors.  Drop the incorrect DAC width comment, which
> >>>        originates from the i915 driver.
> >>>     3. Accept C[124] modes when validating or filling in struct
> >>>        fb_var_screeninfo, and  use the correct number of bits per pixel.
> >>>     4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all supported
> >>>        color-indexed modes.
> >>>
> >>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> >
> >>> --- a/drivers/gpu/drm/drm_fb_helper.c
> >>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> >>> @@ -376,12 +376,34 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
> >>>                                           struct iosys_map *dst)
> >>>    {
> >>>        struct drm_framebuffer *fb = fb_helper->fb;
> >>> -     unsigned int cpp = fb->format->cpp[0];
> >>> -     size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> >>> -     void *src = fb_helper->fbdev->screen_buffer + offset;
> >>> -     size_t len = (clip->x2 - clip->x1) * cpp;
> >>> +     size_t offset = clip->y1 * fb->pitches[0];
> >>> +     size_t len = clip->x2 - clip->x1;
> >>>        unsigned int y;
> >>> +     void *src;
> >>>
> >>> +     switch (fb->format->depth) {
> >>
> >> The depth field is deprecated. It's probably better to use
> >> fb->format->format and test against 4CC codes.
> >
> > The reason I checked for depth instead of a 4CC code is that the only
> > thing that matters here is the number of bits per pixel.  Hence this
> > function won't need any changes to support R1, R2, R4, and D1 later.
> > When we get here, we already know that we are using a format that
> > is supported by the fbdev helper code, and thus passed the 4CC
> > checks elsewhere.
>
> At some point, we will probably have to change several of these tests to
> 4cc. C8 and RGB332 both have 8-bit depth/bpp; same for C4 and RGB121; or
> whatever low-color formats we also want to add.
>
> It's not a blocker now, but maybe something to keep in mind.
>
> >
> > Alternatively, we could introduce drm_format_info_bpp() earlier in
> > the series, and use that?
>
> Having a helper for this might indeed be useful. We use depth for the
> number of color bits and bpp for the number of bits in he pixel.  That's
> important for XRGB8888, where depth is 24, or XRGB555 where depth is 15.
>
> If that makes sense, maybe have a helper for depth and one for bpp, even
> if they return the same value in most of the cases.

The helper for bpp is introduced in "[PATCH 3/8] drm/fourcc: Add
drm_format_info_bpp() helper".
I don't think we need a helper for depth, there's already the .depth
field.  It might be deprecated, but it's still used?
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index ed43b987d306afce..a4afed0de1570841 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -376,12 +376,34 @@  static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
 					   struct iosys_map *dst)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
-	unsigned int cpp = fb->format->cpp[0];
-	size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
-	void *src = fb_helper->fbdev->screen_buffer + offset;
-	size_t len = (clip->x2 - clip->x1) * cpp;
+	size_t offset = clip->y1 * fb->pitches[0];
+	size_t len = clip->x2 - clip->x1;
 	unsigned int y;
+	void *src;
 
+	switch (fb->format->depth) {
+	case 1:
+		offset += clip->x1 / 8;
+		len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
+		break;
+
+	case 2:
+		offset += clip->x1 / 4;
+		len = DIV_ROUND_UP(len + clip->x1 % 4, 4);
+		break;
+
+	case 4:
+		offset += clip->x1 / 2;
+		len = DIV_ROUND_UP(len + clip->x1 % 2, 2);
+		break;
+
+	default:
+		offset += clip->x1 * fb->format->cpp[0];
+		len *= fb->format->cpp[0];
+		break;
+	}
+
+	src = fb_helper->fbdev->screen_buffer + offset;
 	iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
 
 	for (y = clip->y1; y < clip->y2; y++) {
@@ -1231,19 +1253,30 @@  static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1,
 }
 
 static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
-					 u8 depth)
-{
-	switch (depth) {
-	case 8:
+					 const struct drm_format_info *format)
+{
+	u8 depth = format->depth;
+
+	switch (format->format) {
+	// FIXME Perhaps
+	// #define DRM_FORMAT_C0 fourcc_code('C', '0', ' ', ' ')
+	// if ((format & fourcc_code(0xff, 0xf8, 0xff, 0xff) == DRM_FORMAT_C0) ...
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+	case DRM_FORMAT_C8:
 		var->red.offset = 0;
 		var->green.offset = 0;
 		var->blue.offset = 0;
-		var->red.length = 8; /* 8bit DAC */
-		var->green.length = 8;
-		var->blue.length = 8;
+		var->red.length = depth;
+		var->green.length = depth;
+		var->blue.length = depth;
 		var->transp.offset = 0;
 		var->transp.length = 0;
-		break;
+		return;
+	}
+
+	switch (depth) {
 	case 15:
 		var->red.offset = 10;
 		var->green.offset = 5;
@@ -1298,7 +1331,9 @@  int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 {
 	struct drm_fb_helper *fb_helper = info->par;
 	struct drm_framebuffer *fb = fb_helper->fb;
+	const struct drm_format_info *format = fb->format;
 	struct drm_device *dev = fb_helper->dev;
+	unsigned int bpp;
 
 	if (in_dbg_master())
 		return -EINVAL;
@@ -1308,22 +1343,34 @@  int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 		var->pixclock = 0;
 	}
 
-	if ((drm_format_info_block_width(fb->format, 0) > 1) ||
-	    (drm_format_info_block_height(fb->format, 0) > 1))
-		return -EINVAL;
+	switch (format->format) {
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+		bpp = format->depth;
+		break;
+
+	default:
+		if ((drm_format_info_block_width(format, 0) > 1) ||
+		    (drm_format_info_block_height(format, 0) > 1))
+			return -EINVAL;
+
+		bpp = format->cpp[0] * 8;
+		break;
+	}
 
 	/*
 	 * Changes struct fb_var_screeninfo are currently not pushed back
 	 * to KMS, hence fail if different settings are requested.
 	 */
-	if (var->bits_per_pixel > fb->format->cpp[0] * 8 ||
+	if (var->bits_per_pixel > bpp ||
 	    var->xres > fb->width || var->yres > fb->height ||
 	    var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
 		drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in current fb "
 			  "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
 			  var->xres, var->yres, var->bits_per_pixel,
 			  var->xres_virtual, var->yres_virtual,
-			  fb->width, fb->height, fb->format->cpp[0] * 8);
+			  fb->width, fb->height, bpp);
 		return -EINVAL;
 	}
 
@@ -1338,13 +1385,13 @@  int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
 	    !var->blue.length    && !var->transp.length   &&
 	    !var->red.msb_right  && !var->green.msb_right &&
 	    !var->blue.msb_right && !var->transp.msb_right) {
-		drm_fb_helper_fill_pixel_fmt(var, fb->format->depth);
+		drm_fb_helper_fill_pixel_fmt(var, format);
 	}
 
 	/*
 	 * Likewise, bits_per_pixel should be rounded up to a supported value.
 	 */
-	var->bits_per_pixel = fb->format->cpp[0] * 8;
+	var->bits_per_pixel = bpp;
 
 	/*
 	 * drm fbdev emulation doesn't support changing the pixel format at all,
@@ -1680,11 +1727,20 @@  static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 }
 
 static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
-				   uint32_t depth)
+				   uint32_t format)
 {
 	info->fix.type = FB_TYPE_PACKED_PIXELS;
-	info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
-		FB_VISUAL_TRUECOLOR;
+	switch (format) {
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+	case DRM_FORMAT_C8:
+		info->fix.visual = FB_VISUAL_PSEUDOCOLOR;
+		break;
+	default:
+		info->fix.visual = FB_VISUAL_TRUECOLOR;
+		break;
+	}
 	info->fix.mmio_start = 0;
 	info->fix.mmio_len = 0;
 	info->fix.type_aux = 0;
@@ -1701,19 +1757,29 @@  static void drm_fb_helper_fill_var(struct fb_info *info,
 				   uint32_t fb_width, uint32_t fb_height)
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
+	const struct drm_format_info *format = fb->format;
 
-	WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
-		(drm_format_info_block_height(fb->format, 0) > 1));
 	info->pseudo_palette = fb_helper->pseudo_palette;
 	info->var.xres_virtual = fb->width;
 	info->var.yres_virtual = fb->height;
-	info->var.bits_per_pixel = fb->format->cpp[0] * 8;
+	switch (format->format) {
+	case DRM_FORMAT_C1:
+	case DRM_FORMAT_C2:
+	case DRM_FORMAT_C4:
+		info->var.bits_per_pixel = format->depth;
+		break;
+
+	default:
+		WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
+			(drm_format_info_block_height(format, 0) > 1));
+		info->var.bits_per_pixel = format->cpp[0] * 8;
+	}
 	info->var.accel_flags = FB_ACCELF_TEXT;
 	info->var.xoffset = 0;
 	info->var.yoffset = 0;
 	info->var.activate = FB_ACTIVATE_NOW;
 
-	drm_fb_helper_fill_pixel_fmt(&info->var, fb->format->depth);
+	drm_fb_helper_fill_pixel_fmt(&info->var, format);
 
 	info->var.xres = fb_width;
 	info->var.yres = fb_height;
@@ -1738,7 +1804,7 @@  void drm_fb_helper_fill_info(struct fb_info *info,
 {
 	struct drm_framebuffer *fb = fb_helper->fb;
 
-	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->depth);
+	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->format);
 	drm_fb_helper_fill_var(info, fb_helper,
 			       sizes->fb_width, sizes->fb_height);