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