Message-ID: <61317a67-e73e-43d3-85be-8d4e86c1aae2@suse.de>
Date: Mon, 18 Aug 2025 15:29:13 +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>, Ingo Molnar <mingo@...nel.org>,
 Thomas Gleixner <tglx@...utronix.de>, Jani Nikula <jani.nikula@...el.com>,
 Jocelyn Falempe <jfalempe@...hat.com>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm/gud: Replace simple display pipe with DRM atomic
 helpers

Hi

Am 18.08.25 um 15:07 schrieb Ruben Wauters:
[...]

>> Thanks for the update. Great to hear that it works. What's that other
>> problem? Maybe it's a known issue with a known workaround.
> It's mainly latency issues, and problems that I had with the gadget
> driver, I initially tried to get it to work on a different raspberry pi
> but couldn't figure that out, but it works well on the zero.

Ok, got it.

>> I left additional comments below.
>>
>>
>>> v2 changes:
>>> - address review comments by reorganising gud_probe()
>>> ---
>>>    drivers/gpu/drm/gud/gud_connector.c | 24 +++++-----
>>>    drivers/gpu/drm/gud/gud_drv.c       | 52 +++++++++++++++++-----
>>>    drivers/gpu/drm/gud/gud_internal.h  | 13 +++---
>>>    drivers/gpu/drm/gud/gud_pipe.c      | 69 ++++++++++++++++++++----
>>> -----
>>>    4 files changed, 108 insertions(+), 50 deletions(-)
>> AFAICT you should be able to un-include <drm/drm_simple_kms_helper.h>
>> from the various files within the driver.
>>
>>
>>> diff --git a/drivers/gpu/drm/gud/gud_connector.c
>>> b/drivers/gpu/drm/gud/gud_connector.c
>>> index 0f07d77c5d52..75f404ec08b4 100644
>>> --- a/drivers/gpu/drm/gud/gud_connector.c
>>> +++ b/drivers/gpu/drm/gud/gud_connector.c
>>> @@ -607,13 +607,16 @@ int gud_connector_fill_properties(struct
>>> drm_connector_state *connector_state,
>>>    	return gconn->num_properties;
>>>    }
>>>    
>>> +static const struct drm_encoder_funcs
>>> gud_drm_simple_encoder_funcs_cleanup = {
>>> +	.destroy = drm_encoder_cleanup,
>>> +};
>>> +
>>>    static int gud_connector_create(struct gud_device *gdrm, unsigned
>>> int index,
>>>    				struct
>>> gud_connector_descriptor_req *desc)
>>>    {
>>>    	struct drm_device *drm = &gdrm->drm;
>>>    	struct gud_connector *gconn;
>>>    	struct drm_connector *connector;
>>> -	struct drm_encoder *encoder;
>>>    	int ret, connector_type;
>>>    	u32 flags;
>>>    
>>> @@ -681,20 +684,13 @@ static int gud_connector_create(struct
>>> gud_device *gdrm, unsigned int index,
>>>    		return ret;
>>>    	}
>>>    
>>> -	/* The first connector is attached to the existing simple
>>> pipe encoder */
>>> -	if (!connector->index) {
>>> -		encoder = &gdrm->pipe.encoder;
>>> -	} else {
>>> -		encoder = &gconn->encoder;
>>> -
>>> -		ret = drm_simple_encoder_init(drm, encoder,
>>> DRM_MODE_ENCODER_NONE);
>>> -		if (ret)
>>> -			return ret;
>>> -
>>> -		encoder->possible_crtcs = 1;
>>> -	}
>>> +	gdrm->encoder.possible_crtcs = drm_crtc_mask(&gdrm->crtc);
>>> +	ret = drm_encoder_init(drm, &gdrm->encoder,
>>> &gud_drm_simple_encoder_funcs_cleanup,
>>> +			       DRM_MODE_ENCODER_NONE, NULL);
>> The display pipeline sends pixels from plane to CRTC to a number of
>> encoder/connector pairs. Hence you have to initialize the encoder at
>> gconn->encoder. The encoder in gud_device should  be removed.
>>
>>
>>> +	if (ret)
>>> +		return ret;
>>>    
>>> -	return drm_connector_attach_encoder(connector, encoder);
>>> +	return drm_connector_attach_encoder(connector, &gdrm-
>>>> encoder);
>>>    }
>>>    
>>>    int gud_get_connectors(struct gud_device *gdrm)
>>> diff --git a/drivers/gpu/drm/gud/gud_drv.c
>>> b/drivers/gpu/drm/gud/gud_drv.c
>>> index 5385a2126e45..65c3a83c4037 100644
>>> --- a/drivers/gpu/drm/gud/gud_drv.c
>>> +++ b/drivers/gpu/drm/gud/gud_drv.c
>>> @@ -16,6 +16,7 @@
>>>    #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>
>>> @@ -289,7 +290,7 @@ static int gud_get_properties(struct gud_device
>>> *gdrm)
>>>    			 * but mask out any additions on future
>>> devices.
>>>    			 */
>>>    			val &= GUD_ROTATION_MASK;
>>> -			ret =
>>> drm_plane_create_rotation_property(&gdrm->pipe.plane,
>>> +			ret =
>>> drm_plane_create_rotation_property(&gdrm->plane,
>>>    								
>>> DRM_MODE_ROTATE_0, val);
>>>    			break;
>>>    		default:
>>> @@ -338,10 +339,30 @@ static int gud_stats_debugfs(struct seq_file
>>> *m, void *data)
>>>    	return 0;
>>>    }
>>>    
>>> -static const struct drm_simple_display_pipe_funcs gud_pipe_funcs =
>>> {
>>> -	.check      = gud_pipe_check,
>>> -	.update	    = gud_pipe_update,
>>> -	DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS
>>> +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 =
>>> {
>>> @@ -350,7 +371,7 @@ static const struct drm_mode_config_funcs
>>> gud_mode_config_funcs = {
>>>    	.atomic_commit = drm_atomic_helper_commit,
>>>    };
>>>    
>>> -static const u64 gud_pipe_modifiers[] = {
>>> +static const u64 gud_plane_modifiers[] = {
>>>    	DRM_FORMAT_MOD_LINEAR,
>>>    	DRM_FORMAT_MOD_INVALID
>>>    };
>>> @@ -567,12 +588,17 @@ static int gud_probe(struct usb_interface
>>> *intf, const struct usb_device_id *id)
>>>    			return -ENOMEM;
>>>    	}
>>>    
>>> -	ret = drm_simple_display_pipe_init(drm, &gdrm->pipe,
>>> &gud_pipe_funcs,
>>> -					   formats, num_formats,
>>> -					   gud_pipe_modifiers,
>>> NULL);
>>> +	ret = drm_universal_plane_init(drm, &gdrm->plane, 0,
>>> +				       &gud_plane_funcs,
>>> +				       formats, num_formats,
>>> +				       gud_plane_modifiers,
>>> +				       DRM_PLANE_TYPE_PRIMARY,
>>> NULL);
>>>    	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);
>>>    
>>> @@ -582,7 +608,13 @@ static int gud_probe(struct usb_interface
>>> *intf, const struct usb_device_id *id)
>>>    		return ret;
>>>    	}
>>>    
>>> -	drm_plane_enable_fb_damage_clips(&gdrm->pipe.plane);
>>> +	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) {
>>> diff --git a/drivers/gpu/drm/gud/gud_internal.h
>>> b/drivers/gpu/drm/gud/gud_internal.h
>>> index d6fb25388722..6152a9b5da43 100644
>>> --- a/drivers/gpu/drm/gud/gud_internal.h
>>> +++ b/drivers/gpu/drm/gud/gud_internal.h
>>> @@ -15,7 +15,9 @@
>>>    
>>>    struct gud_device {
>>>    	struct drm_device drm;
>>> -	struct drm_simple_display_pipe pipe;
>>> +	struct drm_plane plane;
>>> +	struct drm_crtc crtc;
>>> +	struct drm_encoder encoder;
>> As mentioned above, the encoder field should be removed.
>>
>>>    	struct work_struct work;
>>>    	u32 flags;
>>>    	const struct drm_format_info *xrgb8888_emulation_format;
>>> @@ -62,11 +64,10 @@ int gud_usb_set_u8(struct gud_device *gdrm, u8
>>> request, u8 val);
>>>    
>>>    void gud_clear_damage(struct gud_device *gdrm);
>>>    void gud_flush_work(struct work_struct *work);
>>> -int gud_pipe_check(struct drm_simple_display_pipe *pipe,
>>> -		   struct drm_plane_state *new_plane_state,
>>> -		   struct drm_crtc_state *new_crtc_state);
>>> -void gud_pipe_update(struct drm_simple_display_pipe *pipe,
>>> -		     struct drm_plane_state *old_state);
>>> +int gud_plane_atomic_check(struct drm_plane *plane,
>>> +			   struct drm_atomic_state *state);
>>> +void gud_plane_atomic_update(struct drm_plane *plane,
>>> +			     struct drm_atomic_state
>>> *atomic_state);
>>>    int gud_connector_fill_properties(struct drm_connector_state
>>> *connector_state,
>>>    				  struct gud_property_req
>>> *properties);
>>>    int gud_get_connectors(struct gud_device *gdrm);
>>> diff --git a/drivers/gpu/drm/gud/gud_pipe.c
>>> b/drivers/gpu/drm/gud/gud_pipe.c
>>> index 8d548d08f127..6a0e6234224a 100644
>>> --- a/drivers/gpu/drm/gud/gud_pipe.c
>>> +++ b/drivers/gpu/drm/gud/gud_pipe.c
>>> @@ -451,14 +451,15 @@ 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);
>>>    }
>>>    
>>> -int gud_pipe_check(struct drm_simple_display_pipe *pipe,
>>> -		   struct drm_plane_state *new_plane_state,
>>> -		   struct drm_crtc_state *new_crtc_state)
>>> +int gud_plane_atomic_check(struct drm_plane *plane,
>>> +			   struct drm_atomic_state *state)
>>>    {
>>> -	struct gud_device *gdrm = to_gud_device(pipe->crtc.dev);
>>> -	struct drm_plane_state *old_plane_state = pipe-
>>>> plane.state;
>>> -	const struct drm_display_mode *mode = &new_crtc_state-
>>>> mode;
>>> -	struct drm_atomic_state *state = new_plane_state->state;
>>> +	struct gud_device *gdrm = to_gud_device(plane->dev);
>>> +	struct drm_plane_state *old_plane_state =
>>> drm_atomic_get_old_plane_state(state, plane);
>>> +	struct drm_plane_state *new_plane_state =
>>> drm_atomic_get_new_plane_state(state, plane);
>>> +	struct drm_crtc *crtc = new_plane_state->crtc;
>>> +	struct drm_crtc_state *crtc_state;
>>> +	const struct drm_display_mode *mode;
>>>    	struct drm_framebuffer *old_fb = old_plane_state->fb;
>>>    	struct drm_connector_state *connector_state = NULL;
>>>    	struct drm_framebuffer *fb = new_plane_state->fb;
>>> @@ -472,17 +473,35 @@ int gud_pipe_check(struct
>>> drm_simple_display_pipe *pipe,
>>>    	if (WARN_ON_ONCE(!fb))
>>>    		return -EINVAL;
>>>    
>>> +	if (WARN_ON_ONCE(!crtc))
>> Please use drm_WARN_ON_ONCE() here.
> Should the fb WARN_ON_ONCE be converted to drm_WARN_ON_ONCE as well?

Yes.

>>> +		return -EINVAL;
>>> +
>>> +	crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>>> +
>>> +	mode = &crtc_state->mode;
>>> +
>>> +	ret = drm_atomic_helper_check_plane_state(new_plane_state,
>>> crtc_state,
>>> +						
>>> DRM_PLANE_NO_SCALING,
>>> +						
>>> DRM_PLANE_NO_SCALING,
>>> +						  false, false);
>>> +
>> No empty line here and before similar "if (ret)" statements.
>>
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (!new_plane_state->visible)
>>> +		return 0;
>>> +
>>>    	if (old_plane_state->rotation != new_plane_state-
>>>> rotation)
>>> -		new_crtc_state->mode_changed = true;
>>> +		crtc_state->mode_changed = true;
>>>    
>>>    	if (old_fb && old_fb->format != format)
>>> -		new_crtc_state->mode_changed = true;
>>> +		crtc_state->mode_changed = true;
>>>    
>>> -	if (!new_crtc_state->mode_changed && !new_crtc_state-
>>>> connectors_changed)
>>> +	if (!crtc_state->mode_changed && !crtc_state-
>>>> connectors_changed)
>>>    		return 0;
>>>    
>>>    	/* Only one connector is supported */
>>> -	if (hweight32(new_crtc_state->connector_mask) != 1)
>>> +	if (hweight32(crtc_state->connector_mask) != 1)
>>>    		return -EINVAL;
>> In case you're wondering: this is really a problem with user-space
>> programs. Most compositors (Xorg, Gnome, etc) don't support a single
>> display that is mirrored to multiple outputs. It's only found in low-
>> end
>> hardware and complicated to get right; hence there's little incentive
>> for user space to fix the problem.
>>
>>
>>>    
>>>    	if (format->format == DRM_FORMAT_XRGB8888 && gdrm-
>>>> xrgb8888_emulation_format)
>>> @@ -500,7 +519,7 @@ int gud_pipe_check(struct
>>> drm_simple_display_pipe *pipe,
>>>    	if (!connector_state) {
>>>    		struct drm_connector_list_iter conn_iter;
>>>    
>>> -		drm_connector_list_iter_begin(pipe->crtc.dev,
>>> &conn_iter);
>>> +		drm_connector_list_iter_begin(plane->dev,
>>> &conn_iter);
>>>    		drm_for_each_connector_iter(connector, &conn_iter)
>>> {
>>>    			if (connector->state->crtc) {
>>>    				connector_state = connector-
>>>> state;
>>> @@ -567,16 +586,19 @@ int gud_pipe_check(struct
>>> drm_simple_display_pipe *pipe,
>>>    	return ret;
>>>    }
>>>    
>>> -void gud_pipe_update(struct drm_simple_display_pipe *pipe,
>>> -		     struct drm_plane_state *old_state)
>>> +void gud_plane_atomic_update(struct drm_plane *plane,
>>> +			     struct drm_atomic_state
>>> *atomic_state)
>>>    {
>>> -	struct drm_device *drm = pipe->crtc.dev;
>>> +	struct drm_device *drm = plane->dev;
>>>    	struct gud_device *gdrm = to_gud_device(drm);
>>> -	struct drm_plane_state *state = pipe->plane.state;
>>> -	struct drm_shadow_plane_state *shadow_plane_state =
>>> to_drm_shadow_plane_state(state);
>>> -	struct drm_framebuffer *fb = state->fb;
>>> -	struct drm_crtc *crtc = &pipe->crtc;
>>> +	struct drm_plane_state *old_state =
>>> drm_atomic_get_old_plane_state(atomic_state, plane);
>>> +	struct drm_plane_state *new_state =
>>> drm_atomic_get_new_plane_state(atomic_state, plane);
>>> +	struct drm_shadow_plane_state *shadow_plane_state =
>>> to_drm_shadow_plane_state(new_state);
>>> +	struct drm_framebuffer *fb = new_state->fb;
>>> +	struct drm_crtc *crtc = new_state->crtc;
>>>    	struct drm_rect damage;
>>> +	struct drm_rect dst_clip;
>>> +	struct drm_atomic_helper_damage_iter iter;
>>>    	int ret, idx;
>>>    
>>>    	if (crtc->state->mode_changed || !crtc->state->enable) {
>>> @@ -611,8 +633,15 @@ void gud_pipe_update(struct
>>> drm_simple_display_pipe *pipe,
>>>    	if (ret)
>>>    		goto ctrl_disable;
>>>    
>>> -	if (drm_atomic_helper_damage_merged(old_state, state,
>>> &damage))
>>> +	drm_atomic_helper_damage_iter_init(&iter, old_state,
>>> new_state);
>>> +	drm_atomic_for_each_plane_damage(&iter, &damage) {
>>> +		dst_clip = new_state->dst;
>>> +
>>> +		if (!drm_rect_intersect(&dst_clip, &damage))
>>> +			continue;
>> Where did you get this code snippet? This test should not be
>> necessary
>> here and can even lead to incorrect display updates. Just call
>> gud_fb_hand
> I found this in the hyperv conversion, I'm guessing then it may not be
> relevant here and is hyperv specific, (unless it's incorrect in hyperv
> as well)

The thing is that it refers to a plane that covers the whole screen. And 
for overlay planes, that dst_clip value should be programmed to hardware 
to do the clipping.(*) That's what overlay planes are for. So there's 
probably in issue with hyperv_drm as well.

To give some details: the source buffer for the frame is in system RAM. 
And there's a 1:1 copy in video RAM (on the RPi) for the output. The 
damage area is exactly what gud should copy from system memory to video 
ram. A hardware overlay plane could take that video data, modify and 
move it on the screen. That's unrelated to the damage transfer.

Best regards
Thomas

(*) There are drivers that handle state->dst in software, but not this 
one or hyperv_drm.

>> Best regards
>> Thomas
>>
>>> +
>>>    		gud_fb_handle_damage(gdrm, fb,
>>> &shadow_plane_state->data[0], &damage);
>>> +	}
>>>    
>>>    	drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>>>    

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