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

Lucas De Marchi lucas.demarchi at intel.com
Thu Jul 24 16:41:49 UTC 2025


On Thu, Jul 24, 2025 at 05:51:43PM +0200, Michal Wajdeczko wrote:
>
>
>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 at intel.com>
>>> Cc: Lucas De Marchi <lucas.demarchi at 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()

lgtm

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

I think it would be fine. This is very rarely changed and doesn't matter
much it crosses all devices.

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

yes

Lucas De Marchi


More information about the Intel-xe mailing list