Re: [PATCH v2] drm/gud: Replace simple display pipe with DRM atomic helpers

Thomas Zimmermann Mon, 18 Aug 2025 07:05:02 -0700

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. Andfor overlay planes, that dst_clip value should be programmed to hardwareto do the clipping.(*) That's what overlay planes are for. So there'sprobably 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. Thedamage area is exactly what gud should copy from system memory to videoram. A hardware overlay plane could take that video data, modify andmove 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 thisone 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)

Re: [PATCH v2] drm/gud: Replace simple display pipe with DRM atomic helpers

Reply via email to