[PATCH] drm: Don't return unsupported formats in drm_mode_legacy_fb_format

[Thomas Zimmermann]

Hi

Am 11.03.24 um 20:34 schrieb Frej Drejhammar:
> Hi Thomas,
>
> Thanks for the review and suggestions. My experience with the drm parts
> of the kernel is limited to some weekends trying to fix the regression,
> so I'm afraid I have some questions to check my understanding before
> making a v2 of the patch.
>
> Thomas Zimmermann <tzimmermann at suse.de> writes:
>
>> I suggest to switch all fbdev code over to drm_driver_legacy_fb_format
>> <https://elixir.bootlin.com/linux/latest/C/ident/drm_driver_legacy_fb_format>()
>> first and then modify the format indrm_driver_legacy_fb_format
>> <https://elixir.bootlin.com/linux/latest/C/ident/drm_driver_legacy_fb_format>()
>> after reading it from drm_fb_legacy_fb_format().
> I see how doing the format massaging in drm_driver_legacy_fb_format()
> would fix the original regression (starting with the format returned by
> drm_mode_legacy_fb_format(), drm_fb_legacy_fb_format() is a typo,
> right?).

Yeah, a typo.

>   As drm_driver_legacy_fb_format() has only two callers,
> drm_mode_addfb() and __drm_fb_helper_find_sizes() that change is
> probably less likely to do something unintended. As far as I can tell,
> drm_driver_legacy_fb_format() is only used when userland hasn't
> specified a format or the kernel is initializing and have no format
> information. For these code paths it's clear that only formats which are
> actually supported by the hardware are meaningful.

Right.

>
> What I can't really see is what "switch all fbdev code over to
> drm_driver_legacy_fb_format" would entail and what the benefit would
> be. How do I determine when drm_mode_legacy_fb_format() should be
> replaced with drm_driver_legacy_fb_format()? I have already mistakenly
> considered the change to drm_mode_legacy_fb_format() as harmless and
> broken ofdrm... Shouldn't it be enough to make
> drm_driver_legacy_fb_format() select a format which is supported by the
> driver?

Your patch modifies drm_mode_legacy_fb_format() in a number of 
*_fbdev_*.c files. In those instances, the code could certainly use 
drm_driver_legacy_fb_format() instead.

The fbdev files provide the base for the kernel framebuffer console and 
should behave like DRM clients in userspace; just that they are 
implemented in the kernel. As userspace ioctls use 
drm_driver_legacy_fb_format(), converting the in-kernel clients makes 
sense. And that's it. We keep drm_mode_legacy_fb_format(), but call 
drm_driver_legacy_fb_format() where necessary.

About tilcdc: it uses fbdev-dma, which sets up an XRGB format [1]. 
Shouldn't this already fail? Do you see a framebuffer console?

[1] 
https://elixir.bootlin.com/linux/v6.8/source/drivers/gpu/drm/drm_fbdev_dma.c#L93



>
> Best regards,
>
> Frej
>
>

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