Re: [RFC PATCH v4 02/11] drm/fb-helper: Set FBINFO_MISC_FIRMWARE flag for DRIVER_FIRMWARE fb

[Javier Martinez Canillas <javierm@redhat.com>]

On 4/29/22 12:20, Thomas Zimmermann wrote:
> Hi Javier

[snip]

>>   
>>> We can do this with DRIVER_FIRMWARE. Alternatively, I'd suggest to we
>>> could also used the existing final parameter of
>>> drm_fbdev_generic_setup() to pass a flag that designates a firmware device.
>>>
>>
>> By existing final parameter you mean @preferred_bpp ? That doesn't seem
>> correct. I also like that by using DRIVER_FIRMWARE it is completely data
>> driven and transparent to the DRM driver.
> 
> DRIVER_FIRMWARE is an indirection and only used here. (Just like 
> FBINFO_MISC_FIRMWARE is a bad interface for marking framebuffers that 
> can be unplugged.) If a driver supports hot-unplugging, it should simply 
> register itself with aperture helpers, regardless of whether it's a 
> firmware framebuffer or not.
>

That's fair, and if in practice will only be used by one driver (simpledrm)
then that would also allow us to drop patches 1 and 2 from this series.

IOW, we wouldn't really need a DRIVER_FIRMWARE capability flag.
 
> Of preferred_bpp, we really only use the lowest byte. All other bits are 
> up for grabbing.  The argument is a workaround for handling 
> mode_config.prefered_depth correctly.
>

Yeah, but I didn't want to abuse that argument or package data in an int.

 
> Eventually, preferred_depth should be replaced by something like 
> 'preferred_format', which will hold the driver's preferred format in 
> 4CC.  We won't need preferred_bpp then. So we could turn preferred_bpp 
> into a flags argument.
>

That's a good point, maybe we could start from there and do this cleanup
as a preparatory change of this series ? Then the patches would only be
1) renaming preferred_bpp (that would be unused at this point) to flags
and 2) make simpledrm to set FBDEV_FIRMWARE flag or something like that.

Another option is to add a third flags param to drm_fbdev_generic_setup()
and make all drivers to set 0 besides simpledrm. That way marking the fb
as FBINFO_MISC_FIRMWARE won't be blocked by the preferred_depth cleanup.

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat