Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, May 12, 2023 at 04:11:48PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.05.23 um 15:20 schrieb Linus Walleij:
> > Sorry for late regression detection but this patch regresses
> > the Integrator AB IMPD-1 graphics, I bisected down to this
> > patch.
> [...]
> > This is the driver:
> > drivers/gpu/drm/pl111/pl111_versatile.c
> > with the pl110_impd1 variant, so these are the supported modes:
> > 
> > /* PL110 pixel formats for Integrator, vanilla PL110 */
> > static const u32 pl110_integrator_pixel_formats[] = {
> >          DRM_FORMAT_ABGR8888,
> >          DRM_FORMAT_XBGR8888,
> >          DRM_FORMAT_ARGB8888,
> >          DRM_FORMAT_XRGB8888,
> >          DRM_FORMAT_ABGR1555,
> >          DRM_FORMAT_XBGR1555,
> >          DRM_FORMAT_ARGB1555,
> >          DRM_FORMAT_XRGB1555,
> > };
> > (...)
> > /*
> >   * The IM-PD1 variant is a PL110 with a bunch of broken, or not
> >   * yet implemented features
> >   */
> > static const struct pl111_variant_data pl110_impd1 = {
> >          .name = "PL110 IM-PD1",
> >          .is_pl110 = true,
> >          .broken_clockdivider = true,
> >          .broken_vblank = true,
> >          .formats = pl110_integrator_pixel_formats,
> >          .nformats = ARRAY_SIZE(pl110_integrator_pixel_formats),
> >          .fb_bpp = 16,
> > };
> > 
> > Notice the absence of RGB565!
> > Then we initialized the frambuffer like this:
> > 
> >          drm_fbdev_dma_setup(drm, priv->variant->fb_bpp);
> > 
> > And as you see priv->variant->fb_bpp will be 16, so we want some
> > 16bpp mode however the only supported depth is 15 (the 1555 modes)
> > so it would use that by scaling back depth to 15.
> > 
> > However after this patch that doesn't work anymore.
> > 
> > Any hints on how we can fix this?
> 
> According to a quick grep for fb_bpp, it's only used for the call to
> drm_fbdev_dma_setup(), right? In this case, you should set it to 15 for the
> models without rgb565. The switch at [1] should then pick the correct
> values.
> 
> The preferred_bpp parameter had a change in semantics. It used to be the
> number of bits per pixel. But that makes it hard to select between RGB1555
> and RGB565.  So it's now a special color-mode value that works like the
> kernel's video= parameter. Values of 15 and 32 are different from the rest.
> That switch at [1] explains it. Maybe you should rename fb_bpp to color_mode
> for clarity.
> 
> Let me know if this helps.

Shouldn't the helpers try to do this automatically? I think they kinda did
that in the past in some limited circumstances like this ...
-Daniel

> 
> Best regards
> Thomas
> 
> [1] https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/gpu/drm/drm_fb_helper.c#L1827
> 
> > 
> > Yours,
> > Linus Walleij
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch