Re: [PATCH] drm/gud: rearrange gud_probe() to prepare for function splitting

[Thomas Zimmermann]

Hi

Am 20.10.25 um 15:45 schrieb Ruben Wauters:
On Mon, 2025-10-20 at 09:00 +0200, Thomas Zimmermann wrote:
Hi
Am 19.10.25 um 20:53 schrieb Ruben Wauters:
gud_prove() is currently very large and does many things, including
'gud_probe'
Unfortunate typo that I only realised after sending
pipeline setup and feature detection, as well as having USB functions.

This patch re-orders the code in gud_probe() to make it more organised
and easier to split apart in the future.

Signed-off-by: Ruben Wauters <rubenr...@aol.com>
---
I wanted to move mode config to just before pipeline init, however mode
config is edited in feature detection so I was unsure how to go about it
exactly.
Further untangling of this may be required before splitting it out
---
   drivers/gpu/drm/gud/gud_drv.c | 31 +++++++++++++++++--------------
   1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
index b7345c8d823d..583f7f8f4c00 100644
--- a/drivers/gpu/drm/gud/gud_drv.c
+++ b/drivers/gpu/drm/gud/gud_drv.c
@@ -463,10 +463,6 @@ static int gud_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
                return PTR_ERR(gdrm);
   
   	drm = &gdrm->drm;
-       drm->mode_config.funcs = &gud_mode_config_funcs;
-       ret = drmm_mode_config_init(drm);
-       if (ret)
-               return ret;
   
   	gdrm->flags = le32_to_cpu(desc.flags);
        gdrm->compression = desc.compression & GUD_COMPRESSION_LZ4;
@@ -483,11 +479,18 @@ static int gud_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
        if (ret)
                return ret;
   
+	/* Mode config init*/
+       ret = drmm_mode_config_init(drm);
+       if (ret)
+               return ret;
+
        drm->mode_config.min_width = le32_to_cpu(desc.min_width);
        drm->mode_config.max_width = le32_to_cpu(desc.max_width);
        drm->mode_config.min_height = le32_to_cpu(desc.min_height);
        drm->mode_config.max_height = le32_to_cpu(desc.max_height);
+       drm->mode_config.funcs = &gud_mode_config_funcs;
   
+	/*Format init*/
        formats_dev = devm_kmalloc(dev, GUD_FORMATS_MAX_NUM, GFP_KERNEL);
        /* Add room for emulated XRGB8888 */
        formats = devm_kmalloc_array(dev, GUD_FORMATS_MAX_NUM + 1, 
sizeof(*formats), GFP_KERNEL);
@@ -587,6 +590,7 @@ static int gud_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
                        return -ENOMEM;
        }
   
+	/*Pipeline init*/
        ret = drm_universal_plane_init(drm, &gdrm->plane, 0,
                                       &gud_plane_funcs,
                                       formats, num_formats,
@@ -598,15 +602,6 @@ static int gud_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
        drm_plane_helper_add(&gdrm->plane, &gud_plane_helper_funcs);
        drm_plane_enable_fb_damage_clips(&gdrm->plane);
   
-	devm_kfree(dev, formats);
-       devm_kfree(dev, formats_dev);
-
-       ret = gud_get_properties(gdrm);
-       if (ret) {
-               dev_err(dev, "Failed to get properties (error=%d)\n", ret);
-               return ret;
-       }
-
        ret = drm_crtc_init_with_planes(drm, &gdrm->crtc, &gdrm->plane, NULL,
                                        &gud_crtc_funcs, NULL);
        if (ret)
@@ -621,6 +616,13 @@ static int gud_probe(struct usb_interface *intf, const 
struct usb_device_id *id)
        }
   
   	drm_mode_config_reset(drm);
+       drm_kms_helper_poll_init(drm);
+
+       ret = gud_get_properties(gdrm);
This function installs plane properties. So it need to be in its old
location after drm_universal_plane_init() and before
drm_mode_config_reset(). It should be renamed to
gud_plane_add_properties(). See [1] for the connector equivalent
This does make sense, though I do wonder why it worked on testing,
either way, will change it.

It could be that you can add it afterwards, but that certainly creates 
timing issues with user space. There's a related note at [1].

[1] 
https://elixir.bootlin.com/linux/v6.17.3/source/drivers/gpu/drm/drm_mode_object.c#L231

Best regards
Thomas

[1]
https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_connector.c#L474

+       if (ret) {
+               dev_err(dev, "Failed to get properties (error=%d)\n", ret);
+               return ret;
+       }
   
   	usb_set_intfdata(intf, gdrm);
After you've called drm_kms_helper_poll_init(), DRM could query the
connector for attached displays at any time. This might require the usb
interface to point to the gud device. And the DRM driver also needs to
know about the DMA device.  I quick look through the driver code suggest
that it works without, but better not count on it.

Best would be to move this line and the block with the DMA setup [2]
just after where the gud device got allocated with devmdrm_dev_alloc(). [3]

[2]
https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_drv.c#L627
[3]
https://elixir.bootlin.com/linux/v6.18-rc1/source/drivers/gpu/drm/gud/gud_drv.c#L464
Will do, I'll send a v2 patch with the requested changes.
Best regards
Thomas

   
@@ -638,7 +640,8 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
        if (ret)
                return ret;
   
-	drm_kms_helper_poll_init(drm);
+       devm_kfree(dev, formats);
+       devm_kfree(dev, formats_dev);
   
   	drm_client_setup(drm, NULL);
   

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)