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 <rubenr...@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);
> >   

Reply via email to