[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