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

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

[Ruben Wauters]

gud_prove() is currently very large and does many things, including
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 <rubenru09@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);
+	if (ret) {
+		dev_err(dev, "Failed to get properties (error=%d)\n", ret);
+		return ret;
+	}
 
 	usb_set_intfdata(intf, gdrm);
 
@@ -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);
 
-- 
2.51.0

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

[Thomas Zimmermann]

Hi

Am 19.10.25 um 20:53 schrieb Ruben Wauters:
> gud_prove() is currently very large and does many things, including

'gud_probe'

> 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 <rubenru09@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.

[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

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)

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

[Ruben Wauters]

[-- Attachment #1: Type: text/plain, Size: 5325 bytes --]

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 <rubenru09@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.
> 
> [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);
> >   

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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 <rubenru09@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)