Message-ID: <02dc1df6-bdd7-4b6d-9668-9f700b33d97a@suse.de>
Date: Tue, 7 Oct 2025 12:56:51 +0200
From: Thomas Zimmermann <tzimmermann@...e.de>
To: Ruben Wauters <rubenru09@....com>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/gud: move plane init to gud_pipe.c

Hi

Am 07.10.25 um 11:52 schrieb Ruben Wauters:
> On Tue, 2025-10-07 at 11:17 +0200, Thomas Zimmermann wrote:
>> Hi Ruben,
>>
>> please see my comments below.
>>
>> Am 04.10.25 um 19:49 schrieb Ruben Wauters:
>>> gud_probe() currently is a quite large function that does a lot of
>>> different things, including USB detection, plane init, and several other
>>> things.
>>>
>>> This patch moves the plane and crtc init into gud_plane_init() in
>>> gud_pipe.c, which is a more appropriate file for this. Associated
>>> variables and structs have also been moved to gud_pipe.c
>>>
>>> Signed-off-by: Ruben Wauters <rubenru09@....com>
>>> ---
>>> It was somewhat difficult to determine what exactly should be moved
>>> over, gud_probe() as a function quite a mess, so I need to figure out
>>> exactly how to split this one up.
>> Agreed. The probe function looks really chaotic.
>>
>> I think that just moving CRTC and plane is a not enough. In ast and udl,
>> we have functions that init the whole display pipeline from
>> drmm_mode_config_init() to _reset(). See [1] and [2] for examples. That
>> would likely be a good model for gud as well, but gud's probe function
>> mixes up pipeline init with other code.
>>
>> [1]
>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/gpu/drm/ast/ast_mode.c#L1005
>> [2]
>> https://elixir.bootlin.com/linux/v6.17.1/source/drivers/gpu/drm/udl/udl_modeset.c#L482
>>
>>
>> Looking over gud_probe, the following blocks are related to pipeline init:
>>
>> - lines 466-469 [3]
>> - lines 486-489
>> - lines 558-565
>> - lines 590-599
>> - lines 610-623
>> - line 641
>>
>> [3]
>> https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-next/drivers/gpu/drm/gud/gud_drv.c#L466
>>
>> I'd try to move these lines into a new helper that initializes the full
>> modesetting pipeline.
>>
>> The other code that happens in between is either preparation or clean up
>> and should be done before or after creating the pipeline.
> These changes will probably required another patch/possibly even a
> patch series, so will be more extensive, as such they make take me
> longer to do as I consider the best way to go about it.

It's really just about moving code around and what you currently do 
(moving CRTC init into plane-init code) is generally not advised.

Another step in the right direction would be to reorganize gud_probe() 
first. I mentioned the pipeline init, but anything that is between could 
either go before or after pipeline init. That could be done in a patch 
series or even individual patches at your preferred pace. In the end, 
you'd have a block of pipeline-init code on the middle of gud_probe, 
from where it can be moved into a helper easily. Would that work for you?

Best regards
Thomas


>
> Ruben
>>
>>> As an aside, I noticed that the driver doesn't have a version macro in
>>> gud_drv.c, and therefore is shown as 1.0.0. I was thinking of
>>> introducing a version, but I wanted to know how others generally deal
>>> with driver versions. I'm not 100% sure if it's *necessary* for GUD but
>>> it might be a good idea.
>> I wouldn't bother at all about module versions. AFAIK no one cares about
>> it anyway.
>>
>> Best regards
>> Thomas
>>
>>> ---
>>>    drivers/gpu/drm/gud/gud_drv.c      | 48 +-----------------------
>>>    drivers/gpu/drm/gud/gud_internal.h |  1 +
>>>    drivers/gpu/drm/gud/gud_pipe.c     | 60 ++++++++++++++++++++++++++++++
>>>    3 files changed, 62 insertions(+), 47 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
>>> index b7345c8d823d..967c16479b5c 100644
>>> --- a/drivers/gpu/drm/gud/gud_drv.c
>>> +++ b/drivers/gpu/drm/gud/gud_drv.c
>>> @@ -16,7 +16,6 @@
>>>    #include <drm/clients/drm_client_setup.h>
>>>    #include <drm/drm_atomic_helper.h>
>>>    #include <drm/drm_blend.h>
>>> -#include <drm/drm_crtc_helper.h>
>>>    #include <drm/drm_damage_helper.h>
>>>    #include <drm/drm_debugfs.h>
>>>    #include <drm/drm_drv.h>
>>> @@ -338,43 +337,12 @@ static int gud_stats_debugfs(struct seq_file *m, void *data)
>>>    	return 0;
>>>    }
>>>    
>>> -static const struct drm_crtc_helper_funcs gud_crtc_helper_funcs = {
>>> -	.atomic_check = drm_crtc_helper_atomic_check
>>> -};
>>> -
>>> -static const struct drm_crtc_funcs gud_crtc_funcs = {
>>> -	.reset = drm_atomic_helper_crtc_reset,
>>> -	.destroy = drm_crtc_cleanup,
>>> -	.set_config = drm_atomic_helper_set_config,
>>> -	.page_flip = drm_atomic_helper_page_flip,
>>> -	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>>> -	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>> -};
>>> -
>>> -static const struct drm_plane_helper_funcs gud_plane_helper_funcs = {
>>> -	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>>> -	.atomic_check = gud_plane_atomic_check,
>>> -	.atomic_update = gud_plane_atomic_update,
>>> -};
>>> -
>>> -static const struct drm_plane_funcs gud_plane_funcs = {
>>> -	.update_plane = drm_atomic_helper_update_plane,
>>> -	.disable_plane = drm_atomic_helper_disable_plane,
>>> -	.destroy = drm_plane_cleanup,
>>> -	DRM_GEM_SHADOW_PLANE_FUNCS,
>>> -};
>>> -
>>>    static const struct drm_mode_config_funcs gud_mode_config_funcs = {
>>>    	.fb_create = drm_gem_fb_create_with_dirty,
>>>    	.atomic_check = drm_atomic_helper_check,
>>>    	.atomic_commit = drm_atomic_helper_commit,
>>>    };
>>>    
>>> -static const u64 gud_plane_modifiers[] = {
>>> -	DRM_FORMAT_MOD_LINEAR,
>>> -	DRM_FORMAT_MOD_INVALID
>>> -};
>>> -
>>>    DEFINE_DRM_GEM_FOPS(gud_fops);
>>>    
>>>    static const struct drm_driver gud_drm_driver = {
>>> @@ -587,17 +555,10 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>>>    			return -ENOMEM;
>>>    	}
>>>    
>>> -	ret = drm_universal_plane_init(drm, &gdrm->plane, 0,
>>> -				       &gud_plane_funcs,
>>> -				       formats, num_formats,
>>> -				       gud_plane_modifiers,
>>> -				       DRM_PLANE_TYPE_PRIMARY, NULL);
>>> +	ret = gud_plane_init(gdrm, formats, num_formats);
>>>    	if (ret)
>>>    		return ret;
>>>    
>>> -	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);
>>>    
>>> @@ -607,13 +568,6 @@ static int gud_probe(struct usb_interface *intf, const struct usb_device_id *id)
>>>    		return ret;
>>>    	}
>>>    
>>> -	ret = drm_crtc_init_with_planes(drm, &gdrm->crtc, &gdrm->plane, NULL,
>>> -					&gud_crtc_funcs, NULL);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>> -	drm_crtc_helper_add(&gdrm->crtc, &gud_crtc_helper_funcs);
>>> -
>>>    	ret = gud_get_connectors(gdrm);
>>>    	if (ret) {
>>>    		dev_err(dev, "Failed to get connectors (error=%d)\n", ret);
>>> diff --git a/drivers/gpu/drm/gud/gud_internal.h b/drivers/gpu/drm/gud/gud_internal.h
>>> index d27c31648341..4a91aae61e50 100644
>>> --- a/drivers/gpu/drm/gud/gud_internal.h
>>> +++ b/drivers/gpu/drm/gud/gud_internal.h
>>> @@ -69,6 +69,7 @@ void gud_plane_atomic_update(struct drm_plane *plane,
>>>    int gud_connector_fill_properties(struct drm_connector_state *connector_state,
>>>    				  struct gud_property_req *properties);
>>>    int gud_get_connectors(struct gud_device *gdrm);
>>> +int gud_plane_init(struct gud_device *gdrm, u32 *formats, unsigned int num_formats);
>>>    
>>>    /* Driver internal fourcc transfer formats */
>>>    #define GUD_DRM_FORMAT_R1		0x00000122
>>> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
>>> index 3a208e956dff..1f7af86b28fd 100644
>>> --- a/drivers/gpu/drm/gud/gud_pipe.c
>>> +++ b/drivers/gpu/drm/gud/gud_pipe.c
>>> @@ -10,6 +10,7 @@
>>>    
>>>    #include <drm/drm_atomic.h>
>>>    #include <drm/drm_connector.h>
>>> +#include <drm/drm_crtc_helper.h>
>>>    #include <drm/drm_damage_helper.h>
>>>    #include <drm/drm_drv.h>
>>>    #include <drm/drm_format_helper.h>
>>> @@ -450,6 +451,65 @@ static void gud_fb_handle_damage(struct gud_device *gdrm, struct drm_framebuffer
>>>    	gud_flush_damage(gdrm, fb, src, !fb->obj[0]->import_attach, damage);
>>>    }
>>>    
>>> +static const struct drm_plane_funcs gud_plane_funcs = {
>>> +	.update_plane = drm_atomic_helper_update_plane,
>>> +	.disable_plane = drm_atomic_helper_disable_plane,
>>> +	.destroy = drm_plane_cleanup,
>>> +	DRM_GEM_SHADOW_PLANE_FUNCS,
>>> +};
>>> +
>>> +static const struct drm_plane_helper_funcs gud_plane_helper_funcs = {
>>> +	DRM_GEM_SHADOW_PLANE_HELPER_FUNCS,
>>> +	.atomic_check = gud_plane_atomic_check,
>>> +	.atomic_update = gud_plane_atomic_update,
>>> +};
>>> +
>>> +static const struct drm_crtc_helper_funcs gud_crtc_helper_funcs = {
>>> +	.atomic_check = drm_crtc_helper_atomic_check
>>> +};
>>> +
>>> +static const struct drm_crtc_funcs gud_crtc_funcs = {
>>> +	.reset = drm_atomic_helper_crtc_reset,
>>> +	.destroy = drm_crtc_cleanup,
>>> +	.set_config = drm_atomic_helper_set_config,
>>> +	.page_flip = drm_atomic_helper_page_flip,
>>> +	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>>> +	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>>> +};
>>> +
>>> +static const u64 gud_plane_modifiers[] = {
>>> +	DRM_FORMAT_MOD_LINEAR,
>>> +	DRM_FORMAT_MOD_INVALID
>>> +};
>>> +
>>> +int gud_plane_init(struct gud_device *gdrm, u32 *formats, unsigned int num_formats)
>>> +{
>>> +	struct drm_device *drm = &gdrm->drm;
>>> +	struct drm_plane *plane = &gdrm->plane;
>>> +	struct drm_crtc *crtc = &gdrm->crtc;
>>> +	int ret;
>>> +
>>> +	ret = drm_universal_plane_init(drm, plane, 0,
>>> +				       &gud_plane_funcs,
>>> +				       formats, num_formats,
>>> +				       gud_plane_modifiers,
>>> +				       DRM_PLANE_TYPE_PRIMARY, NULL);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	drm_plane_helper_add(plane, &gud_plane_helper_funcs);
>>> +	drm_plane_enable_fb_damage_clips(plane);
>>> +
>>> +	ret = drm_crtc_init_with_planes(drm, crtc, plane, NULL,
>>> +					&gud_crtc_funcs, NULL);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	drm_crtc_helper_add(crtc, &gud_crtc_helper_funcs);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>    int gud_plane_atomic_check(struct drm_plane *plane,
>>>    			   struct drm_atomic_state *state)
>>>    {

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

Powered by blists - more mailing lists