Message-ID: <8715a886-5416-4fd0-ac50-1cb948cc4546@suse.de>
Date: Mon, 1 Sep 2025 08:41:17 +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 v3] drm/gud: Replace simple display pipe with DRM atomic
 helpers

Hi,

apologies for the late review. I simply missed the mail. There's quite 
some traffic on the list and things get lost.

Am 18.08.25 um 21:35 schrieb Ruben Wauters:
> The simple display pipe is obsolete and the atomic helpers allow for
> more control over the rendering process. As such, this patch replaces
> the old simple display pipe system with the newer atomic helpers.
>
> As the code is mainly the same, merely replaced with the new atomic
> system, there should be no change in functionality.
>
> Signed-off-by: Ruben Wauters <rubenru09@....com>

Reviewed-by: Thomas Zimmermann <tzimmermann@...e.de>

Thank you so much for the update. I added the patch to the drm-misc-next 
tree. It'll show up in the next Linux release.

Best regards
Thomas

> ---
> I noticed as well elsewhere in the driver WARN_ON_ONCE was used instead
> of drm_WARN_ON_ONCE(). I have fixed it for the ones I've modified, and
> will fix it for the driver as a whole in another patch.
>
> v2 changes:
> - address review comments by reorganising gud_probe()
>
> v3 changes:
> - fix formatting and spacing
> - remove unnecessary includes
> - convert WARN_ON_ONCE() to drm_WARN_ON_ONCE()
> - remove dst rect intersect check
> - remove encoder from gud_device and switch to gconn->encoder
> ---
>   drivers/gpu/drm/gud/gud_connector.c | 25 +++++------
>   drivers/gpu/drm/gud/gud_drv.c       | 52 ++++++++++++++++++-----
>   drivers/gpu/drm/gud/gud_internal.h  | 13 +++---
>   drivers/gpu/drm/gud/gud_pipe.c      | 64 +++++++++++++++++++----------
>   4 files changed, 99 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/gud/gud_connector.c b/drivers/gpu/drm/gud/gud_connector.c
> index 0f07d77c5d52..4a15695fa933 100644
> --- a/drivers/gpu/drm/gud/gud_connector.c
> +++ b/drivers/gpu/drm/gud/gud_connector.c
> @@ -16,7 +16,6 @@
>   #include <drm/drm_modeset_helper_vtables.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_probe_helper.h>
> -#include <drm/drm_simple_kms_helper.h>
>   #include <drm/gud.h>
>   
>   #include "gud_internal.h"
> @@ -607,13 +606,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 +683,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;
> -	}
> +	gconn->encoder.possible_crtcs = drm_crtc_mask(&gdrm->crtc);
> +	ret = drm_encoder_init(drm, &gconn->encoder, &gud_drm_simple_encoder_funcs_cleanup,
> +			       DRM_MODE_ENCODER_NONE, NULL);
> +	if (ret)
> +		return ret;
>   
> -	return drm_connector_attach_encoder(connector, encoder);
> +	return drm_connector_attach_encoder(connector, &gconn->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..5f57f841e603 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>
> @@ -27,7 +28,6 @@
>   #include <drm/drm_managed.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_probe_helper.h>
> -#include <drm/drm_simple_kms_helper.h>
>   #include <drm/gud.h>
>   
>   #include "gud_internal.h"
> @@ -289,7 +289,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 +338,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 +370,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 +587,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 +607,12 @@ 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..d27c31648341 100644
> --- a/drivers/gpu/drm/gud/gud_internal.h
> +++ b/drivers/gpu/drm/gud/gud_internal.h
> @@ -11,11 +11,11 @@
>   #include <uapi/drm/drm_fourcc.h>
>   
>   #include <drm/drm_modes.h>
> -#include <drm/drm_simple_kms_helper.h>
>   
>   struct gud_device {
>   	struct drm_device drm;
> -	struct drm_simple_display_pipe pipe;
> +	struct drm_plane plane;
> +	struct drm_crtc crtc;
>   	struct work_struct work;
>   	u32 flags;
>   	const struct drm_format_info *xrgb8888_emulation_format;
> @@ -62,11 +62,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..54d9aa9998e5 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -20,7 +20,6 @@
>   #include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/drm_print.h>
>   #include <drm/drm_rect.h>
> -#include <drm/drm_simple_kms_helper.h>
>   #include <drm/gud.h>
>   
>   #include "gud_internal.h"
> @@ -451,14 +450,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;
> @@ -469,20 +469,37 @@ int gud_pipe_check(struct drm_simple_display_pipe *pipe,
>   	int idx, ret;
>   	size_t len;
>   
> -	if (WARN_ON_ONCE(!fb))
> +	if (drm_WARN_ON_ONCE(plane->dev, !fb))
>   		return -EINVAL;
>   
> +	if (drm_WARN_ON_ONCE(plane->dev, !crtc))
> +		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);
> +	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;
>   
>   	if (format->format == DRM_FORMAT_XRGB8888 && gdrm->xrgb8888_emulation_format)
> @@ -500,7 +517,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 +584,18 @@ 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_atomic_helper_damage_iter iter;
>   	int ret, idx;
>   
>   	if (crtc->state->mode_changed || !crtc->state->enable) {
> @@ -611,7 +630,8 @@ 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)
>   		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