Re: [PATCH 4/9] drm/xe: Add a new debug focused configfs group

["Summers, Stuart" <stuart.summers@intel.com>]

On Mon, 2026-05-04 at 15:50 +0000, Summers, Stuart wrote:
> On Mon, 2026-05-04 at 12:42 -0300, Gustavo Sousa wrote:
> > Stuart Summers <stuart.summers@intel.com> writes:
> > 
> > > Add the skeleton code for a new debug specific configfs group.
> > > 
> > > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > > Assisted-by: Copilot:claude-opus-4.7
> > > ---
> > >  drivers/gpu/drm/xe/Makefile            |  2 ++
> > >  drivers/gpu/drm/xe/xe_configfs.c       |  4 ++++
> > >  drivers/gpu/drm/xe/xe_configfs_debug.c | 14 ++++++++++++++
> > >  drivers/gpu/drm/xe/xe_configfs_debug.h |  8 ++++++++
> > >  drivers/gpu/drm/xe/xe_configfs_types.h |  7 +++++++
> > >  5 files changed, 35 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/xe/xe_configfs_debug.c
> > >  create mode 100644 drivers/gpu/drm/xe/xe_configfs_debug.h
> > > 
> > > diff --git a/drivers/gpu/drm/xe/Makefile
> > > b/drivers/gpu/drm/xe/Makefile
> > > index 09661f079d03..b58667b0b18e 100644
> > > --- a/drivers/gpu/drm/xe/Makefile
> > > +++ b/drivers/gpu/drm/xe/Makefile
> > > @@ -161,6 +161,8 @@ xe-$(CONFIG_HWMON) += xe_hwmon.o
> > >  
> > >  xe-$(CONFIG_PERF_EVENTS) += xe_pmu.o
> > >  xe-$(CONFIG_CONFIGFS_FS) += xe_configfs.o
> > > +xe_debug_configfs_obj-$(CONFIG_DRM_XE_DEBUG) :=
> > > xe_configfs_debug.o
> > > +xe-$(CONFIG_CONFIGFS_FS) += $(xe_debug_configfs_obj-y)
> > 
> > What are the downsides of keeping this available for non-debug
> > builds?
> > 
> > I imagine that the debug configfs could be useful for someone not
> > involved in kernel development working together with the developers
> > to
> > debug issues without needing to rebuild their kernel.
> 
> So the focal parameter we had in i915 was enable_rc6. Something like
> this is what we explicitly don't want to expose to a general user as
> it
> can have unforeseen consequences (like having very high power usage).
> But, having the ability to disable rc6 for debug purposes as a
> developer is extremely useful at narrowing down certain types of bugs
> at the hardware/software interface.
> 
> That's just one example, but there are a bunch of things that could
> fit
> this category and I expect we'll have even more use cases over time. 
> 
> Right now we have to hold these types of changes in a side branch not
> exposed externally and I really want to converge our processes so we
> can more easily make these kinds of changes and let us debug directly
> out of drm-tip.
> 
> > 
> > >  
> > >  # graphics virtualization (SR-IOV) support
> > >  xe-y += \
> > > diff --git a/drivers/gpu/drm/xe/xe_configfs.c
> > > b/drivers/gpu/drm/xe/xe_configfs.c
> > > index 12b7fe65446d..85df8ce5cf2a 100644
> > > --- a/drivers/gpu/drm/xe/xe_configfs.c
> > > +++ b/drivers/gpu/drm/xe/xe_configfs.c
> > > @@ -1006,6 +1006,10 @@ static struct config_group
> > > *xe_config_make_device_group(struct config_group *gro
> > >                 config_group_init_type_name(&dev->sriov, "sriov",
> > > &xe_config_sriov_type);
> > >                 configfs_add_default_group(&dev->sriov, &dev-
> > > > group);
> > >         }
> > > +#if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
> > > +       config_group_init_type_name(&dev->debug, "debug",
> > > &xe_configfs_debug_type);
> > > +       configfs_add_default_group(&dev->debug, &dev->group);
> > > +#endif
> > 
> > I guess there isn't much we can do about it today, but, based on
> > what
> > I
> > read in configfs documentation a while ago, I suspect it wasn't
> > really
> > designed with arbitrary hierarchies of config items in mind.  It
> > would
> > be nice if there was an API for that though.
> > 
> > Basically, a config group is expected to be a directory to contain
> > a
> > set
> > of config items of the same type, which is not the case on our
> > current
> > usage (i.e. the "sriov" and now the "debug" group).
> > 
> > I guess we are able to get our desired behavior (basically just a
> > subdirectory behaving as a config item) by not implementing the
> > "make_item" hook.  Not sure if that's an expected use of the API or
> > if
> > that's more of a hack on our side and could come back to bite us in
> > the
> > future.
> 
> Hm.. ok that's good feedback. I do like bucketing these in groups and
> subdirectories. It also makes it easy to show or not show based on
> the
> Kconfig. But let me go through and make sure there isn't something
> more
> suited here. Happy to hear any other feedback as well here.

Hey so I don't think it's necessarily a bad idea to use make_item here
for the leaf nodes. But make_group does cover. I see other drivers
going both directions here. That said, we aren't doing anything new in
this patch series with respect to configfs item creation. I'd prefer to
keep things consistent in this series if possible. We can look at
dividing out into config_item and config_group entries separately.

Thanks,
Stuart

> 
> Thanks,
> Stuart
> 
> > 
> > --
> > Gustavo Sousa
> > 
> > >  
> > >         mutex_init(&dev->lock);
> > >  
> > > diff --git a/drivers/gpu/drm/xe/xe_configfs_debug.c
> > > b/drivers/gpu/drm/xe/xe_configfs_debug.c
> > > new file mode 100644
> > > index 000000000000..45617282cec5
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/xe/xe_configfs_debug.c
> > > @@ -0,0 +1,14 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright © 2026 Intel Corporation
> > > + */
> > > +
> > > +#include <linux/configfs.h>
> > > +#include <linux/module.h>
> > > +
> > > +#include "xe_configfs_debug.h"
> > > +#include "xe_configfs_types.h"
> > > +
> > > +const struct config_item_type xe_configfs_debug_type = {
> > > +       .ct_owner       = THIS_MODULE,
> > > +};
> > > diff --git a/drivers/gpu/drm/xe/xe_configfs_debug.h
> > > b/drivers/gpu/drm/xe/xe_configfs_debug.h
> > > new file mode 100644
> > > index 000000000000..01170dc2f97e
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/xe/xe_configfs_debug.h
> > > @@ -0,0 +1,8 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright © 2026 Intel Corporation
> > > + */
> > > +#ifndef _XE_CONFIGFS_DEBUG_H_
> > > +#define _XE_CONFIGFS_DEBUG_H_
> > > +
> > > +#endif /* _XE_CONFIGFS_DEBUG_H_ */
> > > diff --git a/drivers/gpu/drm/xe/xe_configfs_types.h
> > > b/drivers/gpu/drm/xe/xe_configfs_types.h
> > > index 935097aafa96..c9d94a3c26a7 100644
> > > --- a/drivers/gpu/drm/xe/xe_configfs_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_configfs_types.h
> > > @@ -24,6 +24,9 @@ struct wa_bb {
> > >  struct xe_config_group_device {
> > >         struct config_group group;
> > >         struct config_group sriov;
> > > +#if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
> > > +       struct config_group debug;
> > > +#endif
> > >  
> > >         struct xe_config_device {
> > >                 struct wa_bb
> > > ctx_restore_mid_bb[XE_ENGINE_CLASS_MAX];
> > > @@ -56,4 +59,8 @@ static inline struct xe_config_device
> > > *xe_configfs_to_device(struct config_item
> > >         return &xe_configfs_to_group_device(item)->config;
> > >  }
> > >  
> > > +#if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
> > > +extern const struct config_item_type xe_configfs_debug_type;
> > > +#endif
> > > +
> > >  #endif /* _XE_CONFIGFS_TYPES_H_ */
> > > -- 
> > > 2.43.0
>