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

[Gustavo Sousa <gustavo.sousa@intel.com>]

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>

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