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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Javier

Am 17.03.23 um 13:24 schrieb Javier Martinez Canillas:
Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxxx>


--
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: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux