* Re: [PATCH v2] drm/gud: Replace simple display pipe with DRM atomic helpers
2025-08-16 9:57 ` [PATCH v2] drm/gud: Replace simple display pipe with DRM atomic helpers Ruben Wauters
@ 2025-08-16 13:55 ` Ruben Wauters
2025-08-18 12:27 ` Thomas Zimmermann
2025-08-18 13:03 ` Thomas Zimmermann
2 siblings, 0 replies; 6+ messages in thread
From: Ruben Wauters @ 2025-08-16 13:55 UTC (permalink / raw)
To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, David Airlie,
Simona Vetter, Ingo Molnar, Thomas Gleixner, Jani Nikula,
Jocelyn Falempe
Cc: dri-devel, linux-kernel
On Sat, 2025-08-16 at 10:57 +0100, Ruben Wauters wrote:
> 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@aol.com>
> ---
> I have been able to test this now, having obtained the hardware, and
> it
> works fine, though some other unrelated issues have popped up, which
> I
> shall try and debug and address in other patches.
>
> 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(-)
>
> 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);
> + 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;
> 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))
> + 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 +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;
> +
> gud_fb_handle_damage(gdrm, fb, &shadow_plane_state-
> >data[0], &damage);
> + }
>
> drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>
My apologies, I appear to have typo'd the mailing list, I am replying
to ensure that it is included here
Ruben
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] drm/gud: Replace simple display pipe with DRM atomic helpers
2025-08-16 9:57 ` [PATCH v2] drm/gud: Replace simple display pipe with DRM atomic helpers Ruben Wauters
2025-08-16 13:55 ` Ruben Wauters
@ 2025-08-18 12:27 ` Thomas Zimmermann
2025-08-18 13:07 ` Ruben Wauters
2025-08-18 13:03 ` Thomas Zimmermann
2 siblings, 1 reply; 6+ messages in thread
From: Thomas Zimmermann @ 2025-08-18 12:27 UTC (permalink / raw)
To: Ruben Wauters, Maarten Lankhorst, Maxime Ripard, David Airlie,
Simona Vetter, Ingo Molnar, Thomas Gleixner, Jani Nikula,
Jocelyn Falempe
Cc: dri-devel, linux-kernel
Hi
Am 16.08.25 um 11:57 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@aol.com>
> ---
> I have been able to test this now, having obtained the hardware, and it
> works fine, though some other unrelated issues have popped up, which I
> shall try and debug and address in other patches.
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.
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.
> + 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_handle_damage() with the damage area.
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)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] drm/gud: Replace simple display pipe with DRM atomic helpers
2025-08-18 12:27 ` Thomas Zimmermann
@ 2025-08-18 13:07 ` Ruben Wauters
2025-08-18 13:29 ` Thomas Zimmermann
0 siblings, 1 reply; 6+ messages in thread
From: Ruben Wauters @ 2025-08-18 13:07 UTC (permalink / raw)
To: Thomas Zimmermann, Maarten Lankhorst, Maxime Ripard, David Airlie,
Simona Vetter, Ingo Molnar, Thomas Gleixner, Jani Nikula,
Jocelyn Falempe
Cc: dri-devel, linux-kernel
On Mon, 2025-08-18 at 14:27 +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 16.08.25 um 11:57 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@aol.com>
> > ---
> > I have been able to test this now, having obtained the hardware,
> > and it
> > works fine, though some other unrelated issues have popped up,
> > which I
> > shall try and debug and address in other patches.
>
> 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.
>
> 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?
>
> > + 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)
>
> 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);
> >
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2] drm/gud: Replace simple display pipe with DRM atomic helpers
2025-08-18 13:07 ` Ruben Wauters
@ 2025-08-18 13:29 ` Thomas Zimmermann
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Zimmermann @ 2025-08-18 13:29 UTC (permalink / raw)
To: Ruben Wauters, Maarten Lankhorst, Maxime Ripard, David Airlie,
Simona Vetter, Ingo Molnar, Thomas Gleixner, Jani Nikula,
Jocelyn Falempe
Cc: dri-devel, linux-kernel
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)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/gud: Replace simple display pipe with DRM atomic helpers
2025-08-16 9:57 ` [PATCH v2] drm/gud: Replace simple display pipe with DRM atomic helpers Ruben Wauters
2025-08-16 13:55 ` Ruben Wauters
2025-08-18 12:27 ` Thomas Zimmermann
@ 2025-08-18 13:03 ` Thomas Zimmermann
2 siblings, 0 replies; 6+ messages in thread
From: Thomas Zimmermann @ 2025-08-18 13:03 UTC (permalink / raw)
To: Ruben Wauters, Maarten Lankhorst, Maxime Ripard, David Airlie,
Simona Vetter, Ingo Molnar, Thomas Gleixner, Jani Nikula,
Jocelyn Falempe
Cc: dri-devel, linux-kernel
Hi
Am 16.08.25 um 11:57 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@aol.com>
> ---
> I have been able to test this now, having obtained the hardware, and it
> works fine, though some other unrelated issues have popped up, which I
> shall try and debug and address in other patches.
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.
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.
> + 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_handle_damage() with the damage area.
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)
^ permalink raw reply [flat|nested] 6+ messages in thread