Re: [PATCH 2/9] drm/xe: Sort xe_config_device fields and defaults alphabetically

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

On Mon, 2026-05-04 at 10:58 -0300, Gustavo Sousa wrote:
> Stuart Summers <stuart.summers@intel.com> writes:
> 
> > Sort the fields in struct xe_config_device and the corresponding
> > device_defaults initializer in alphabetical order. Non-SRIOV fields
> > come first (alphabetically), followed by the SRIOV sub-struct with
> > its own fields in alphabetical order. Also sort the PRI_CUSTOM_ATTR
> > calls in dump_custom_dev_config() to match and the documentation
> > for
> > each of the fields.
> 
> I think the sorting alphabetically seems fine, but I'll defer it to
> others to weigh in here on this decision though.
> 
> I guess we will not have many instances of that struct, so I guess
> potential padding resulting from the sorting requirement should be
> fine.
> 
> We probably want to add a note somewhere in the code asking to keep
> that
> stuff ordered alphabetically.
> 
> By the way, for other people looking at this, I find it easier to
> review
> this with: git show --color-moved --histogram.
> 
> Code-wise, looks good to me, so if this is the direction we take,
> 
> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

I had accidentally responded to the wrong patch with this... Also
thanks for the review!

Yeah this is something I wanted to discuss here too. I kind of just
blindly alphabetized things here, but we might want to sort by data
type or something else. also, we only have a few parameters here, but
I'd like to set this up to support a much larger set of parameters on
the debug side - we need much finer grained support for things like
power-on for instance. And I'd like a way that easily scales here.

Also I agree on the documentation.. I'll make sure to add that in the
next patch once we conclude on the approach here.

Thanks,
Stuart

> 
> > 
> > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > Assisted-by: Copilot:claude-opus-4.7
> > ---
> >  drivers/gpu/drm/xe/xe_configfs.c | 192 +++++++++++++++------------
> > ----
> >  1 file changed, 96 insertions(+), 96 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_configfs.c
> > b/drivers/gpu/drm/xe/xe_configfs.c
> > index 69abc69ec0f3..1e134057fae8 100644
> > --- a/drivers/gpu/drm/xe/xe_configfs.c
> > +++ b/drivers/gpu/drm/xe/xe_configfs.c
> > @@ -73,53 +73,79 @@
> >   * Configure Attributes
> >   * ====================
> >   *
> > - * Survivability mode:
> > - * -------------------
> > + * Context restore BB
> > + * ------------------
> >   *
> > - * Enable survivability mode on supported cards. This setting only
> > takes
> > - * effect when probing the device. Example to enable it::
> > + * Allow to execute a batch buffer during any context switches.
> > When the
> > + * GPU is restoring the context, it executes additional commands.
> > It's useful
> > + * for testing additional workarounds and validating certain HW
> > behaviors: it's
> > + * not intended for normal execution and will taint the kernel
> > with TAINT_TEST
> > + * when used.
> >   *
> > - *     # echo 1 >
> > /sys/kernel/config/xe/0000:03:00.0/enable_survivability_mode
> > + * The syntax allows to pass straight instructions to be executed
> > by the engine
> > + * in a batch buffer or set specific registers.
> >   *
> > - * This attribute can only be set before binding to the device.
> > + * #. Generic instruction::
> >   *
> > - * Allowed GT types:
> > - * -----------------
> > + *     <engine-class> cmd <instr> [[dword0] [dword1] [...]]
> >   *
> > - * Allow only specific types of GTs to be detected and initialized
> > by the
> > - * driver.  Any combination of GT types can be enabled/disabled,
> > although
> > - * some settings will cause the device to fail to probe.
> > + * #. Simple register setting::
> >   *
> > - * Writes support both comma- and newline-separated input format.
> > Reads
> > - * will always return one GT type per line. "primary" and "media"
> > are the
> > - * GT type names supported by this interface.
> > + *     <engine-class> reg <address> <value>
> >   *
> > - * This attribute can only be set before binding to the device.
> > + * Commands are saved per engine class: all instances of that
> > class will execute
> > + * those commands during context switch. The instruction, dword
> > arguments,
> > + * addresses and values are in hex format like in the examples
> > below.
> >   *
> > - * Examples:
> > + * #. Execute a LRI command to write 0xDEADBEEF to register 0x4f10
> > after the
> > + *    normal context restore::
> >   *
> > - * Allow both primary and media GTs to be initialized and used. 
> > This matches
> > - * the driver's default behavior::
> > + *     # echo 'rcs cmd 11000001 4F100 DEADBEEF' \
> > + *             >
> > /sys/kernel/config/xe/0000:03:00.0/ctx_restore_post_bb
> >   *
> > - *     # echo 'primary,media' >
> > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> > + * #. Execute a LRI command to write 0xDEADBEEF to register 0x4f10
> > at the
> > + *    beginning of the context restore::
> >   *
> > - * Allow only the primary GT of each tile to be initialized and
> > used,
> > - * effectively disabling the media GT if it exists on the
> > platform::
> > + *     # echo 'rcs cmd 11000001 4F100 DEADBEEF' \
> > + *             >
> > /sys/kernel/config/xe/0000:03:00.0/ctx_restore_mid_bb
> > +
> > + * #. Load certain values in a couple of registers (it can be used
> > as a simpler
> > + *    alternative to the `cmd`) action::
> >   *
> > - *     # echo 'primary' >
> > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> > + *     # cat >
> > /sys/kernel/config/xe/0000:03:00.0/ctx_restore_post_bb <<EOF
> > + *     rcs reg 4F100 DEADBEEF
> > + *     rcs reg 4F104 FFFFFFFF
> > + *     EOF
> >   *
> > - * Allow only the media GT of each tile to be initialized and
> > used,
> > - * effectively disabling the primary GT.  **This configuration
> > will cause
> > - * device probe failure on all current platforms, but may be
> > allowed on
> > - * igpu platforms in the future**::
> > + *    .. note::
> >   *
> > - *     # echo 'media' >
> > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> > + *       When using multiple lines, make sure to use a command
> > that is
> > + *       implemented with a single write syscall, like HEREDOC.
> >   *
> > - * Disable all GTs.  Only other GPU IP (such as display) is
> > potentially usable.
> > - * **This configuration will cause device probe failure on all
> > current
> > - * platforms, but may be allowed on igpu platforms in the
> > future**::
> > + * Currently this is implemented only for post and mid context
> > restore and
> > + * these attributes can only be set before binding to the device.
> >   *
> > - *     # echo '' >
> > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> > + * PSMI
> > + * ----
> > + *
> > + * Enable extra debugging capabilities to trace engine execution.
> > Only useful
> > + * during early platform enabling and requires additional hardware
> > connected.
> > + * Once it's enabled, additionals WAs are added and runtime
> > configuration is
> > + * done via debugfs. Example to enable it::
> > + *
> > + *     # echo 1 > /sys/kernel/config/xe/0000:03:00.0/enable_psmi
> > + *
> > + * This attribute can only be set before binding to the device.
> > + *
> > + * Survivability mode:
> > + * -------------------
> > + *
> > + * Enable survivability mode on supported cards. This setting only
> > takes
> > + * effect when probing the device. Example to enable it::
> > + *
> > + *     # echo 1 >
> > /sys/kernel/config/xe/0000:03:00.0/enable_survivability_mode
> > + *
> > + * This attribute can only be set before binding to the device.
> >   *
> >   * Allowed engines:
> >   * ----------------
> > @@ -147,69 +173,43 @@
> >   *
> >   * This attribute can only be set before binding to the device.
> >   *
> > - * PSMI
> > - * ----
> > + * Allowed GT types:
> > + * -----------------
> >   *
> > - * Enable extra debugging capabilities to trace engine execution.
> > Only useful
> > - * during early platform enabling and requires additional hardware
> > connected.
> > - * Once it's enabled, additionals WAs are added and runtime
> > configuration is
> > - * done via debugfs. Example to enable it::
> > + * Allow only specific types of GTs to be detected and initialized
> > by the
> > + * driver.  Any combination of GT types can be enabled/disabled,
> > although
> > + * some settings will cause the device to fail to probe.
> >   *
> > - *     # echo 1 > /sys/kernel/config/xe/0000:03:00.0/enable_psmi
> > + * Writes support both comma- and newline-separated input format.
> > Reads
> > + * will always return one GT type per line. "primary" and "media"
> > are the
> > + * GT type names supported by this interface.
> >   *
> >   * This attribute can only be set before binding to the device.
> >   *
> > - * Context restore BB
> > - * ------------------
> > - *
> > - * Allow to execute a batch buffer during any context switches.
> > When the
> > - * GPU is restoring the context, it executes additional commands.
> > It's useful
> > - * for testing additional workarounds and validating certain HW
> > behaviors: it's
> > - * not intended for normal execution and will taint the kernel
> > with TAINT_TEST
> > - * when used.
> > - *
> > - * The syntax allows to pass straight instructions to be executed
> > by the engine
> > - * in a batch buffer or set specific registers.
> > - *
> > - * #. Generic instruction::
> > - *
> > - *     <engine-class> cmd <instr> [[dword0] [dword1] [...]]
> > - *
> > - * #. Simple register setting::
> > - *
> > - *     <engine-class> reg <address> <value>
> > - *
> > - * Commands are saved per engine class: all instances of that
> > class will execute
> > - * those commands during context switch. The instruction, dword
> > arguments,
> > - * addresses and values are in hex format like in the examples
> > below.
> > + * Examples:
> >   *
> > - * #. Execute a LRI command to write 0xDEADBEEF to register 0x4f10
> > after the
> > - *    normal context restore::
> > + * Allow both primary and media GTs to be initialized and used. 
> > This matches
> > + * the driver's default behavior::
> >   *
> > - *     # echo 'rcs cmd 11000001 4F100 DEADBEEF' \
> > - *             >
> > /sys/kernel/config/xe/0000:03:00.0/ctx_restore_post_bb
> > + *     # echo 'primary,media' >
> > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> >   *
> > - * #. Execute a LRI command to write 0xDEADBEEF to register 0x4f10
> > at the
> > - *    beginning of the context restore::
> > + * Allow only the primary GT of each tile to be initialized and
> > used,
> > + * effectively disabling the media GT if it exists on the
> > platform::
> >   *
> > - *     # echo 'rcs cmd 11000001 4F100 DEADBEEF' \
> > - *             >
> > /sys/kernel/config/xe/0000:03:00.0/ctx_restore_mid_bb
> > -
> > - * #. Load certain values in a couple of registers (it can be used
> > as a simpler
> > - *    alternative to the `cmd`) action::
> > + *     # echo 'primary' >
> > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> >   *
> > - *     # cat >
> > /sys/kernel/config/xe/0000:03:00.0/ctx_restore_post_bb <<EOF
> > - *     rcs reg 4F100 DEADBEEF
> > - *     rcs reg 4F104 FFFFFFFF
> > - *     EOF
> > + * Allow only the media GT of each tile to be initialized and
> > used,
> > + * effectively disabling the primary GT.  **This configuration
> > will cause
> > + * device probe failure on all current platforms, but may be
> > allowed on
> > + * igpu platforms in the future**::
> >   *
> > - *    .. note::
> > + *     # echo 'media' >
> > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> >   *
> > - *       When using multiple lines, make sure to use a command
> > that is
> > - *       implemented with a single write syscall, like HEREDOC.
> > + * Disable all GTs.  Only other GPU IP (such as display) is
> > potentially usable.
> > + * **This configuration will cause device probe failure on all
> > current
> > + * platforms, but may be allowed on igpu platforms in the
> > future**::
> >   *
> > - * Currently this is implemented only for post and mid context
> > restore and
> > - * these attributes can only be set before binding to the device.
> > + *     # echo '' >
> > /sys/kernel/config/xe/0000:03:00.0/gt_types_allowed
> >   *
> >   * Max SR-IOV Virtual Functions
> >   * ----------------------------
> > @@ -256,15 +256,15 @@ struct xe_config_group_device {
> >         struct config_group sriov;
> >  
> >         struct xe_config_device {
> > -               u64 gt_types_allowed;
> > -               u64 engines_allowed;
> > -               struct wa_bb
> > ctx_restore_post_bb[XE_ENGINE_CLASS_MAX];
> >                 struct wa_bb
> > ctx_restore_mid_bb[XE_ENGINE_CLASS_MAX];
> > -               bool enable_survivability_mode;
> > +               struct wa_bb
> > ctx_restore_post_bb[XE_ENGINE_CLASS_MAX];
> >                 bool enable_psmi;
> > +               bool enable_survivability_mode;
> > +               u64 engines_allowed;
> > +               u64 gt_types_allowed;
> >                 struct {
> > -                       unsigned int max_vfs;
> >                         bool admin_only_pf;
> > +                       unsigned int max_vfs;
> >                 } sriov;
> >         } config;
> >  
> > @@ -277,13 +277,13 @@ struct xe_config_group_device {
> >  };
> >  
> >  static const struct xe_config_device device_defaults = {
> > -       .gt_types_allowed = U64_MAX,
> > -       .engines_allowed = U64_MAX,
> > -       .enable_survivability_mode = false,
> >         .enable_psmi = false,
> > +       .enable_survivability_mode = false,
> > +       .engines_allowed = U64_MAX,
> > +       .gt_types_allowed = U64_MAX,
> >         .sriov = {
> > -               .max_vfs = XE_DEFAULT_MAX_VFS,
> >                 .admin_only_pf = XE_DEFAULT_ADMIN_ONLY_PF,
> > +               .max_vfs = XE_DEFAULT_MAX_VFS,
> >         },
> >  };
> >  
> > @@ -814,17 +814,17 @@ static ssize_t
> > ctx_restore_post_bb_store(struct config_item *item,
> >  CONFIGFS_ATTR(, ctx_restore_mid_bb);
> >  CONFIGFS_ATTR(, ctx_restore_post_bb);
> >  CONFIGFS_ATTR(, enable_psmi);
> > +CONFIGFS_ATTR(, enable_survivability_mode);
> >  CONFIGFS_ATTR(, engines_allowed);
> >  CONFIGFS_ATTR(, gt_types_allowed);
> > -CONFIGFS_ATTR(, enable_survivability_mode);
> >  
> >  static struct configfs_attribute *xe_config_device_attrs[] = {
> >         &attr_ctx_restore_mid_bb,
> >         &attr_ctx_restore_post_bb,
> >         &attr_enable_psmi,
> > +       &attr_enable_survivability_mode,
> >         &attr_engines_allowed,
> >         &attr_gt_types_allowed,
> > -       &attr_enable_survivability_mode,
> >         NULL,
> >  };
> >  
> > @@ -929,12 +929,12 @@ static ssize_t
> > sriov_admin_only_pf_store(struct config_item *item, const char *p
> >         return len;
> >  }
> >  
> > -CONFIGFS_ATTR(sriov_, max_vfs);
> >  CONFIGFS_ATTR(sriov_, admin_only_pf);
> > +CONFIGFS_ATTR(sriov_, max_vfs);
> >  
> >  static struct configfs_attribute *xe_config_sriov_attrs[] = {
> > -       &sriov_attr_max_vfs,
> >         &sriov_attr_admin_only_pf,
> > +       &sriov_attr_max_vfs,
> >         NULL,
> >  };
> >  
> > @@ -1096,10 +1096,10 @@ static void dump_custom_dev_config(struct
> > pci_dev *pdev,
> >                                  dev->config.attr_); \
> >         } while (0)
> >  
> > -       PRI_CUSTOM_ATTR("%llx", gt_types_allowed);
> > -       PRI_CUSTOM_ATTR("%llx", engines_allowed);
> >         PRI_CUSTOM_ATTR("%d", enable_psmi);
> >         PRI_CUSTOM_ATTR("%d", enable_survivability_mode);
> > +       PRI_CUSTOM_ATTR("%llx", engines_allowed);
> > +       PRI_CUSTOM_ATTR("%llx", gt_types_allowed);
> >         PRI_CUSTOM_ATTR("%u", sriov.admin_only_pf);
> >  
> >  #undef PRI_CUSTOM_ATTR
> > -- 
> > 2.43.0