Re: [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  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 



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


OpenPGP_signature
Description: OpenPGP digital signature
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

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

[Thomas Zimmermann]

Clean up fbdev and client state if the probe function fails. It
used to leak allocated resources. Also reorder the individual steps
to simplify cleanup.

Signed-off-by: Thomas Zimmermann 
---
 drivers/gpu/drm/drm_fbdev_generic.c | 46 -
 1 file changed, 32 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/drm_fbdev_generic.c 
b/drivers/gpu/drm/drm_fbdev_generic.c
index e48a8e82378d..0d08ddbe9608 100644
--- a/drivers/gpu/drm/drm_fbdev_generic.c
+++ b/drivers/gpu/drm/drm_fbdev_generic.c
@@ -7,6 +7,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #include 
@@ -74,8 +75,9 @@ static int drm_fbdev_fb_probe(struct drm_fb_helper *fb_helper,
struct drm_client_dev *client = &fb_helper->client;
struct drm_device *dev = fb_helper->dev;
struct drm_client_buffer *buffer;
-   struct drm_framebuffer *fb;
struct fb_info *info;
+   size_t screen_size;
+   void *screen_buffer;
u32 format;
int ret;
 
@@ -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;
+   screen_buffer = vzalloc(screen_size);
+   if (!screen_buffer) {
+   ret = -ENOMEM;
+   goto err_drm_client_framebuffer_delete;
+   }
 
info = drm_fb_helper_alloc_info(fb_helper);
-   if (IS_ERR(info))
-   return PTR_ERR(info);
+   if (IS_ERR(info)) {
+   ret = PTR_ERR(info);
+   goto err_vfree;
+   }
+
+   drm_fb_helper_fill_info(info, fb_helper, sizes);
 
info->fbops = &drm_fbdev_fb_ops;
-   info->screen_size = sizes->surface_height * fb->pitches[0];
-   info->fix.smem_len = info->screen_size;
info->flags = FBINFO_DEFAULT;
 
-   drm_fb_helper_fill_info(info, fb_helper, sizes);
-
-   info->screen_buffer = vzalloc(info->screen_size);
-   if (!info->screen_buffer)
-   return -ENOMEM;
+   /* screen */
info->flags |= FBINFO_VIRTFB | FBINFO_READS_FAST;
-
+   info->screen_size = screen_size;
+   info->screen_buffer = screen_buffer;
info->fix.smem_start = 
page_to_phys(vmalloc_to_page(info->screen_buffer));
+   info->fix.smem_len = info->screen_size;
 
-   /* Set a default deferred I/O handler */
+   /* deferred I/O */
fb_helper->fbdefio.delay = HZ / 20;
fb_helper->fbdefio.deferred_io = drm_fb_helper_deferred_io;
 
info->fbdefio = &fb_helper->fbdefio;
ret = fb_deferred_io_init(info);
if (ret)
-   return ret;
+   goto err_drm_fb_helper_release_info;
 
return 0;
+
+err_drm_fb_helper_release_info:
+   drm_fb_helper_release_info(fb_helper);
+err_vfree:
+   vfree(screen_buffer);
+err_drm_client_framebuffer_delete:
+   fb_helper->fb = NULL;
+   fb_helper->buffer = NULL;
+   drm_client_framebuffer_delete(buffer);
+   return ret;
 }
 
 static void drm_fbdev_damage_blit_real(struct drm_fb_helper *fb_helper,
-- 
2.39.2

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization