[PATCH 4/6] drm/fbdev-generic: Clean up after failed probing

[Thomas Zimmermann]

Hi Javier

Am 17.03.23 um 13:24 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann at suse.de> writes:
> 
> [...]
> 
>> @@ -91,36 +93,52 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
>>   
>>   	fb_helper->buffer = buffer;
>>   	fb_helper->fb = buffer->fb;
>> -	fb = buffer->fb;
>> +
>> +	screen_size = buffer->gem->size;
> 
> [...]
> 
>> -	info->screen_size = sizes->surface_height * fb->pitches[0];
> 
> [...]
> 
> I wonder if this change should be a separate patch? I know that it should
> be the same size but still feels like an unrelated change that deserves a
> different patch and description.

This comment made me look up the exact meaning if screen_size, which is 
"Amount of ioremapped VRAM or 0". [1] Other drivers with shadow buffers 
(udlfb, metronomefb) apparently don't set this field. So the generic 
fbdev probably shouldn't either. The size of the video memory (physical 
or shadowed) in in fix.smem_len. [2] From grep'ing through the source 
code, it's not clear to me why screen_size exists in the first place.

Best regards
Thomas

[1] https://elixir.bootlin.com/linux/latest/source/include/linux/fb.h#L494
[2] 
https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/fb.h#L161

> 
> [...]
>    
>> -	/* Set a default deferred I/O handler */
>> +	/* deferred I/O */
> 
> I would either have it as /* Generic fbdev deferred I/O handler */ or just
> remove the comment. I understand why you are removing the "default", since
> implies that drivers can change the handler and that's not the case here.
> 
> But I think that just leaving the "deferred I/O" comment there doesn't say
> that much.
> 
> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
> 

-- 
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230320/383381c9/attachment.sig>