On Tue, 2025-10-07 at 12:56 +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 07.10.25 um 11:52 schrieb Ruben Wauters:
> > On Tue, 2025-10-07 at 11:17 +0200, Thomas Zimmermann wrote:
> > > Hi Ruben,
> > >
> > > please see my comments below.
> > >
> > > Am 04.10.25 um 19:49 schrieb Ruben Wauters:
> > > > gud_probe() currently is a quite large function that does a lot of
> > > > different things, including USB detection, plane init, and several other
> > > > things.
> > > >
> > > > This patch moves the plane and crtc init into gud_plane_init() in
> > > > gud_pipe.c, which is a more appropriate file for this. Associated
> > > > variables and structs have also been moved to gud_pipe.c
> > > >
> > > > Signed-off-by: Ruben Wauters <[email protected]>
> > > > ---
> > > > It was somewhat difficult to determine what exactly should be moved
> > > > over, gud_probe() as a function quite a mess, so I need to figure out
> > > > exactly how to split this one up.
> > > Agreed. The probe function looks really chaotic.
> > >
> > > I think that just moving CRTC and plane is a not enough. In ast and udl,
> > > we have functions that init the whole display pipeline from
> > > drmm_mode_config_init() to _reset(). See [1] and [2] for examples. That
> > > would likely be a good model for gud as well, but gud's probe function
> > > mixes up pipeline init with other code.
> > >
> > > [1]
> > > https://elixir.bootlin.com/linux/v6.17.1/source/drivers/gpu/drm/ast/ast_mode.c#L1005
> > > [2]
> > > https://elixir.bootlin.com/linux/v6.17.1/source/drivers/gpu/drm/udl/udl_modeset.c#L482
> > >
> > >
> > > Looking over gud_probe, the following blocks are related to pipeline init:
> > >
> > > - lines 466-469 [3]
> > > - lines 486-489
> > > - lines 558-565
> > > - lines 590-599
> > > - lines 610-623
> > > - line 641
> > >
> > > [3]
> > > https://gitlab.freedesktop.org/drm/misc/kernel/-/blob/drm-misc-next/drivers/gpu/drm/gud/gud_drv.c#L466
> > >
> > > I'd try to move these lines into a new helper that initializes the full
> > > modesetting pipeline.
> > >
> > > The other code that happens in between is either preparation or clean up
> > > and should be done before or after creating the pipeline.
> > These changes will probably required another patch/possibly even a
> > patch series, so will be more extensive, as such they make take me
> > longer to do as I consider the best way to go about it.
>
> It's really just about moving code around and what you currently do
> (moving CRTC init into plane-init code) is generally not advised.
Ahh, I understand, that will probably have to be split into a separate
function then.
> Another step in the right direction would be to reorganize gud_probe()
> first. I mentioned the pipeline init, but anything that is between could
> either go before or after pipeline init. That could be done in a patch
> series or even individual patches at your preferred pace. In the end,
> you'd have a block of pipeline-init code on the middle of gud_probe,
> from where it can be moved into a helper easily. Would that work for you?
This might be a bit easier to do at least at first, I shall get working
on that.
Ruben
> Best regards
> Thomas
>
>
> >
> > Ruben
> > >
> > > > As an aside, I noticed that the driver doesn't have a version macro in
> > > > gud_drv.c, and therefore is shown as 1.0.0. I was thinking of
> > > > introducing a version, but I wanted to know how others generally deal
> > > > with driver versions. I'm not 100% sure if it's *necessary* for GUD but
> > > > it might be a good idea.
> > > I wouldn't bother at all about module versions. AFAIK no one cares about
> > > it anyway.
> > >
> > > Best regards
> > > Thomas
> > >
> > > > ---
> > > > drivers/gpu/drm/gud/gud_drv.c | 48 +-----------------------
> > > > drivers/gpu/drm/gud/gud_internal.h | 1 +
> > > > drivers/gpu/drm/gud/gud_pipe.c | 60
> > > > ++++++++++++++++++++++++++++++
> > > > 3 files changed, 62 insertions(+), 47 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/gud/gud_drv.c
> > > > b/drivers/gpu/drm/gud/gud_drv.c
> > > > index b7345c8d823d..967c16479b5c 100644
> > > > --- a/drivers/gpu/drm/gud/gud_drv.c
> > > > +++ b/drivers/gpu/drm/gud/gud_drv.c
> > > > @@ -16,7 +16,6 @@
> > > > #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>
> > > > @@ -338,43 +337,12 @@ static int gud_stats_debugfs(struct seq_file *m,
> > > > void *data)
> > > > return 0;
> > > > }
> > > >
> > > > -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 = {
> > > > .fb_create = drm_gem_fb_create_with_dirty,
> > > > .atomic_check = drm_atomic_helper_check,
> > > > .atomic_commit = drm_atomic_helper_commit,
> > > > };
> > > >
> > > > -static const u64 gud_plane_modifiers[] = {
> > > > - DRM_FORMAT_MOD_LINEAR,
> > > > - DRM_FORMAT_MOD_INVALID
> > > > -};
> > > > -
> > > > DEFINE_DRM_GEM_FOPS(gud_fops);
> > > >
> > > > static const struct drm_driver gud_drm_driver = {
> > > > @@ -587,17 +555,10 @@ static int gud_probe(struct usb_interface *intf,
> > > > const struct usb_device_id *id)
> > > > return -ENOMEM;
> > > > }
> > > >
> > > > - ret = drm_universal_plane_init(drm, &gdrm->plane, 0,
> > > > - &gud_plane_funcs,
> > > > - formats, num_formats,
> > > > - gud_plane_modifiers,
> > > > - DRM_PLANE_TYPE_PRIMARY, NULL);
> > > > + ret = gud_plane_init(gdrm, formats, num_formats);
> > > > 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);
> > > >
> > > > @@ -607,13 +568,6 @@ static int gud_probe(struct usb_interface *intf,
> > > > const struct usb_device_id *id)
> > > > return ret;
> > > > }
> > > >
> > > > - 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) {
> > > > dev_err(dev, "Failed to get connectors (error=%d)\n",
> > > > ret);
> > > > diff --git a/drivers/gpu/drm/gud/gud_internal.h
> > > > b/drivers/gpu/drm/gud/gud_internal.h
> > > > index d27c31648341..4a91aae61e50 100644
> > > > --- a/drivers/gpu/drm/gud/gud_internal.h
> > > > +++ b/drivers/gpu/drm/gud/gud_internal.h
> > > > @@ -69,6 +69,7 @@ void gud_plane_atomic_update(struct drm_plane *plane,
> > > > int gud_connector_fill_properties(struct drm_connector_state
> > > > *connector_state,
> > > > struct gud_property_req *properties);
> > > > int gud_get_connectors(struct gud_device *gdrm);
> > > > +int gud_plane_init(struct gud_device *gdrm, u32 *formats, unsigned int
> > > > num_formats);
> > > >
> > > > /* Driver internal fourcc transfer formats */
> > > > #define GUD_DRM_FORMAT_R1 0x00000122
> > > > diff --git a/drivers/gpu/drm/gud/gud_pipe.c
> > > > b/drivers/gpu/drm/gud/gud_pipe.c
> > > > index 3a208e956dff..1f7af86b28fd 100644
> > > > --- a/drivers/gpu/drm/gud/gud_pipe.c
> > > > +++ b/drivers/gpu/drm/gud/gud_pipe.c
> > > > @@ -10,6 +10,7 @@
> > > >
> > > > #include <drm/drm_atomic.h>
> > > > #include <drm/drm_connector.h>
> > > > +#include <drm/drm_crtc_helper.h>
> > > > #include <drm/drm_damage_helper.h>
> > > > #include <drm/drm_drv.h>
> > > > #include <drm/drm_format_helper.h>
> > > > @@ -450,6 +451,65 @@ 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);
> > > > }
> > > >
> > > > +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_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_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 u64 gud_plane_modifiers[] = {
> > > > + DRM_FORMAT_MOD_LINEAR,
> > > > + DRM_FORMAT_MOD_INVALID
> > > > +};
> > > > +
> > > > +int gud_plane_init(struct gud_device *gdrm, u32 *formats, unsigned int
> > > > num_formats)
> > > > +{
> > > > + struct drm_device *drm = &gdrm->drm;
> > > > + struct drm_plane *plane = &gdrm->plane;
> > > > + struct drm_crtc *crtc = &gdrm->crtc;
> > > > + int ret;
> > > > +
> > > > + ret = drm_universal_plane_init(drm, plane, 0,
> > > > + &gud_plane_funcs,
> > > > + formats, num_formats,
> > > > + gud_plane_modifiers,
> > > > + DRM_PLANE_TYPE_PRIMARY, NULL);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + drm_plane_helper_add(plane, &gud_plane_helper_funcs);
> > > > + drm_plane_enable_fb_damage_clips(plane);
> > > > +
> > > > + ret = drm_crtc_init_with_planes(drm, crtc, plane, NULL,
> > > > + &gud_crtc_funcs, NULL);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + drm_crtc_helper_add(crtc, &gud_crtc_helper_funcs);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > int gud_plane_atomic_check(struct drm_plane *plane,
> > > > struct drm_atomic_state *state)
> > > > {
