Re: [PATCH v3 4/6] drm/xe/configfs: Check if device was preconfigured

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 7/24/2025 5:17 PM, Lucas De Marchi wrote:
> On Tue, Jul 22, 2025 at 04:10:57PM +0200, Michal Wajdeczko wrote:
>> Since device configuration using configfs could be prepared long
>> time prior the driver load, add a debug log whether the current
>> device driver probe is using custom or default settings.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_configfs.c | 35 ++++++++++++++++++++++++++++++--
>> drivers/gpu/drm/xe/xe_configfs.h |  2 ++
>> drivers/gpu/drm/xe/xe_pci.c      |  3 +++
>> 3 files changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
>> index 36e2b45b305f..f83e8e9f070f 100644
>> --- a/drivers/gpu/drm/xe/xe_configfs.c
>> +++ b/drivers/gpu/drm/xe/xe_configfs.c
>> @@ -118,6 +118,17 @@ static struct xe_config_device *to_xe_config_device(struct config_item *item)
>>     return container_of(to_config_group(item), struct xe_config_device, group);
>> }
>>
>> +static void xe_config_set_defaults(struct xe_config_device *dev)
>> +{
>> +    dev->engines_allowed = U64_MAX;
>> +    dev->survivability_mode = false;
>> +}
> 
> other places here follow the file prefix, xe_configfs. I think mixing
> both is confusing.

but those are different classes:

xe_configfs() functions take struct pci_dev *pdev

while

xe_config() functions take struct xe_config_device *dev
 
so maybe this one should be just in full name:

xe_config_device_set_defaults()

or short name:

device_set_defaults()
set_device_defaults()

> 
>> +
>> +static bool xe_config_is_default(const struct xe_config_device *dev)
>> +{
>> +    return dev->engines_allowed == U64_MAX && !dev->survivability_mode;
> 
> maybe have something like below would be more maintainable? We may need
> to move the attribute values to an inner struct or just set the
> defaults very early.
> 
> At least aligning the variables would be good. On a first read I missed
> you were checking the 2 vars.
> 
> static const struct xe_config_device defaults = {
>     .engines_allowed = U64_MAX,
> };

could work, but this will likely prevent us from building defaults in the
runtime based on the data from xe_device_desc (if we ever want that way)

> 
> static void set_defaults(struct xe_config_device *dev)
> {
>     *dev = defaults;
> }
> 
> static bool is_default(const struct xe_config_device *dev)
> {
>     /*
>      * If a new attribute is added, there should be also a check for
>      * default. Assert it's not forgotten
>      */
>     static_assert(ARRAY_SIZE(xe_config_device_attrs) == 3);

maybe inner struct with attributes would be a better solution as then
we can just memcmp() actual item with fresh default, or at least keep
all non-attributes members first, followed by real attribute members:

struct xe_device_config {
	struct config_group group;
	struct mutex lock; /* protects attributes */

	/* attributes must be placed here: */
	bool survivability_mode;
	u64 engines_allowed;
	...
};

return !compare_after_member(def, dev, lock);

btw, do we really need mutex per each group?
can't we use our singleton xe_configfs.su_mutex?

> 
>     return dev->engines_allowed != defaults.engines_allowed &&
>            dev->survivability_mode != defaults.survivability_mode;

you likely mean ==

> }
> 
> 
>> +}
>> +
>> static ssize_t survivability_mode_show(struct config_item *item, char *page)
>> {
>>     struct xe_config_device *dev = to_xe_config_device(item);
>> @@ -281,8 +292,7 @@ static struct config_group *xe_config_make_device_group(struct config_group *gro
>>     if (!dev)
>>         return ERR_PTR(-ENOMEM);
>>
>> -    /* Default values */
>> -    dev->engines_allowed = U64_MAX;
>> +    xe_config_set_defaults(dev);
> 
> yeah, if we keep it here I think we don't need to an inner struct.
> 
>>
>>     config_group_init_type_name(&dev->group, name, &xe_config_device_type);
>>
>> @@ -323,6 +333,27 @@ static struct xe_config_device *configfs_find_group(struct pci_dev *pdev)
>>     return to_xe_config_device(item);
>> }
>>
>> +/**
>> + * xe_configfs_check_device() - Test if device was configured by configfs
>> + * @pdev: the &pci_dev device to test
>> + *
>> + * Try to find the configfs group that belongs to the specified pci device
>> + * and print a diagnostic message if found.
>> + *
>> + * Return: true if config group is found, false otherwise
>> + */
>> +bool xe_configfs_check_device(struct pci_dev *pdev)
>> +{
>> +    struct xe_config_device *cfg = configfs_find_group(pdev);
>> +
>> +    if (!cfg)
>> +        return false;
>> +
>> +    pci_dbg(pdev, "found %s settings in configfs\n",
>> +        xe_config_is_default(cfg) ? "default" : "custom");
>> +    return true;
> 
> return could be void.

my secret idea was that we could use this as a hint that there is
no need to check for modparams if there is config

but agree, currently unused

> 
> Lucas De Marchi
> 
>> +}
>> +
>> /**
>>  * xe_configfs_get_survivability_mode - get configfs survivability mode attribute
>>  * @pdev: pci device
>> diff --git a/drivers/gpu/drm/xe/xe_configfs.h b/drivers/gpu/drm/xe/xe_configfs.h
>> index fb8764008089..cdccf1f9b716 100644
>> --- a/drivers/gpu/drm/xe/xe_configfs.h
>> +++ b/drivers/gpu/drm/xe/xe_configfs.h
>> @@ -13,12 +13,14 @@ struct pci_dev;
>> #if IS_ENABLED(CONFIG_CONFIGFS_FS)
>> int xe_configfs_init(void);
>> void xe_configfs_exit(void);
>> +bool xe_configfs_check_device(struct pci_dev *pdev);
>> bool xe_configfs_get_survivability_mode(struct pci_dev *pdev);
>> void xe_configfs_clear_survivability_mode(struct pci_dev *pdev);
>> u64 xe_configfs_get_engines_allowed(struct pci_dev *pdev);
>> #else
>> static inline int xe_configfs_init(void) { return 0; }
>> static inline void xe_configfs_exit(void) { }
>> +static inline bool xe_configfs_check_device(struct pci_dev *pdev) { return false; }
>> static inline bool xe_configfs_get_survivability_mode(struct pci_dev *pdev) { return false; }
>> static inline void xe_configfs_clear_survivability_mode(struct pci_dev *pdev) { }
>> static inline u64 xe_configfs_get_engines_allowed(struct pci_dev *pdev) { return U64_MAX; }
>> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
>> index f4ea13527945..322dc52c4a0b 100644
>> --- a/drivers/gpu/drm/xe/xe_pci.c
>> +++ b/drivers/gpu/drm/xe/xe_pci.c
>> @@ -17,6 +17,7 @@
>>
>> #include "display/xe_display.h"
>> #include "regs/xe_gt_regs.h"
>> +#include "xe_configfs.h"
>> #include "xe_device.h"
>> #include "xe_drv.h"
>> #include "xe_gt.h"
>> @@ -770,6 +771,8 @@ static int xe_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>     struct xe_device *xe;
>>     int err;
>>
>> +    xe_configfs_check_device(pdev);
>> +
>>     if (desc->require_force_probe && !id_forced(pdev->device)) {
>>         dev_info(&pdev->dev,
>>              "Your graphics device %04x is not officially supported\n"
>> -- 
>> 2.47.1
>>