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

[Thomas Zimmermann <tzimmermann@suse.de>]

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

Hi Javier

Am 29.04.22 um 11:23 schrieb Javier Martinez Canillas:
> Hello Thomas,
> 
> On 4/29/22 11:14, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 29.04.22 um 10:42 schrieb Javier Martinez Canillas:
>>> The DRIVER_FIRMWARE flag denotes that a DRM driver uses a framebuffer
>>> that was initialized and provided by the system firmware for scanout.
>>>
>>> Indicate to the fbdev subsystem that the registered framebuffer is a
>>> FBINFO_MISC_FIRMWARE, so that it can handle accordingly. For example,
>>> wold hot-unplug the associated device if asked to remove conflicting
>>> framebuffers.
>>>
>>> Suggested-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
>>> ---
>>>
>>> (no changes since v1)
>>>
>>>    drivers/gpu/drm/drm_fb_helper.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>>> index d265a73313c9..76dd11888621 100644
>>> --- a/drivers/gpu/drm/drm_fb_helper.c
>>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>>> @@ -1891,6 +1891,10 @@ __drm_fb_helper_initial_config_and_unlock(struct drm_fb_helper *fb_helper,
>>>    		/* don't leak any physical addresses to userspace */
>>>    		info->flags |= FBINFO_HIDE_SMEM_START;
>>>    
>>> +	/* Indicate that the framebuffer is provided by the firmware */
>>> +	if (drm_core_check_feature(dev, DRIVER_FIRMWARE))
>>> +		info->flags |= FBINFO_MISC_FIRMWARE;
>>> +
>>
>> Patches 1 to 3 should be squashed into one before landing.
>>
> 
> I actually considered this but then decided to go with the each change
> goes into its own patch approach. But I'll squash it in next revisions.
>   
>> 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.

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.

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.

Best regards
Thomas

> 
> Or do you envision a case where a driver would be DRIVER_FIRMWARE but we
> wouldn't want the emulated fbdev to also be FBINFO_MISC_FIRMWARE ?
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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