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

[Thomas Zimmermann <tzimmermann@suse.de>]

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)