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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4339 bytes --]

Hi

Am 15.05.23 um 10:16 schrieb Daniel Vetter:
> 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 ...

That was the intention, but it never really worked at all. IIRC the 
color-format selection mixed up depth and bpp values freely. Factor in 
the command-line override (video=@bpp) and some odd case has always been 
broken.

So fbdev emulation now mostly uses the color-mode value that works as on 
the command line. The current semantics is:

  * select the user-given color mode, if non-zero
  * select the driver-given color mode, if non-zero
  * otherwise select a color mode of 32 by default (that's XRGB8888)

And later during fb_probe:

  * try the selected color mode
  * otherwise try to auto-detect if the selected color mode doesn't work,
  * otherwise use XRGB8888 as a last resort

That nicely splits the code into color-mode selection and color-format 
selection. And all the public interfaces (command line, 
drm_fbdev_{}_setup(), etc) use the same semantics, which is the color mode.

Best regards
Thomas

> -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)
> 
> 
> 
> 

-- 
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)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]