Re: [PATCH v3 7/7] drm/xe/pm: Capture errors and handle them

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 6455 bytes --]


On 12-04-2024 19:12, Lucas De Marchi wrote:
> On Fri, Apr 12, 2024 at 01:32:45PM +0530, Himal Prasad Ghimiray wrote:
>> xe_pm_init may encounter failures for various reasons, such as a failure
>> in initializing drmm_mutex, or when dealing with a d3cold-capable device
>> for vram_threshold sysfs creation and setting default threshold.
>> Presently, all these potential failures are disregarded.
>>
>> Move d3cold.lock initialization to xe_pm_init_early and cause driver
>> abort if mutex initialization has failed.
>>
>> Warn about failure to create and setting default threshold.
>>
>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_device_sysfs.c | 12 ++++------
>> drivers/gpu/drm/xe/xe_device_sysfs.h |  2 +-
>> drivers/gpu/drm/xe/xe_pm.c           | 34 +++++++++++++++++++++++-----
>> drivers/gpu/drm/xe/xe_pm.h           |  2 +-
>> 4 files changed, 34 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.c 
>> b/drivers/gpu/drm/xe/xe_device_sysfs.c
>> index e47c8ad1bb17..21677b8cd977 100644
>> --- a/drivers/gpu/drm/xe/xe_device_sysfs.c
>> +++ b/drivers/gpu/drm/xe/xe_device_sysfs.c
>> @@ -76,18 +76,14 @@ static void xe_device_sysfs_fini(struct 
>> drm_device *drm, void *arg)
>>     sysfs_remove_file(&xe->drm.dev->kobj, 
>> &dev_attr_vram_d3cold_threshold.attr);
>> }
>>
>> -void xe_device_sysfs_init(struct xe_device *xe)
>> +int xe_device_sysfs_init(struct xe_device *xe)
>> {
>>     struct device *dev = xe->drm.dev;
>>     int ret;
>>
>>     ret = sysfs_create_file(&dev->kobj, 
>> &dev_attr_vram_d3cold_threshold.attr);
>> -    if (ret) {
>> -        drm_warn(&xe->drm, "Failed to create sysfs file\n");
>> -        return;
>> -    }
>> -
>> -    ret = drmm_add_action_or_reset(&xe->drm, xe_device_sysfs_fini, xe);
>>     if (ret)
>> -        drm_warn(&xe->drm, "Failed to add sysfs fini drm action\n");
>> +        return ret;
>> +
>> +    return drmm_add_action_or_reset(&xe->drm, xe_device_sysfs_fini, 
>> xe);
>> }
>> diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.h 
>> b/drivers/gpu/drm/xe/xe_device_sysfs.h
>> index 38b240684bee..f9e83d8bd2c7 100644
>> --- a/drivers/gpu/drm/xe/xe_device_sysfs.h
>> +++ b/drivers/gpu/drm/xe/xe_device_sysfs.h
>> @@ -8,6 +8,6 @@
>>
>> struct xe_device;
>>
>> -void xe_device_sysfs_init(struct xe_device *xe);
>> +int xe_device_sysfs_init(struct xe_device *xe);
>>
>> #endif
>> diff --git a/drivers/gpu/drm/xe/xe_pm.c b/drivers/gpu/drm/xe/xe_pm.c
>> index f1fc83845c01..f4d9441720b4 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.c
>> +++ b/drivers/gpu/drm/xe/xe_pm.c
>> @@ -208,10 +208,25 @@ static void xe_pm_runtime_init(struct xe_device 
>> *xe)
>>     pm_runtime_put(dev);
>> }
>>
>> -void xe_pm_init_early(struct xe_device *xe)
>> +int xe_pm_init_early(struct xe_device *xe)
>> {
>> +    int err;
>> +
>>     INIT_LIST_HEAD(&xe->mem_access.vram_userfault.list);
>> -    drmm_mutex_init(&xe->drm, &xe->mem_access.vram_userfault.lock);
>> +
>> +    err = drmm_mutex_init(&xe->drm, 
>> &xe->mem_access.vram_userfault.lock);
>> +    if (err)
>> +        return err;
>> +
>> +    /* Currently d3cold.lock will be used only with GuC */
>
> it's cheaper to just initialize it regardless so this can be simpler
>
> int xe_pm_init_early(struct xe_device *xe)
> {
>     int err;
>
>     INIT_LIST_HEAD(&xe->mem_access.vram_userfault.list);
>
>     if ((err = drmm_mutex_init(&xe->drm, 
> &xe->mem_access.vram_userfault.lock) ||
>         (err = drmm_mutex_init(&xe->drm, &xe->d3cold.lock)))
>         return err;
Looks clean. Will prefer using this.
>
>     return 0;
> }
>
> or keep the err assignment separate, doesn't matter much. But
> when we mix success and failure for a return-early style it makes
> it harder to read.
>
>>
>> /**
>> @@ -219,20 +234,27 @@ void xe_pm_init_early(struct xe_device *xe)
>>  * @xe: xe device instance
>>  *
>>  * This component is responsible for System and Device sleep states.
>> + *
>
> wrong line
>
>>  */
>> void xe_pm_init(struct xe_device *xe)
>> {
>> +    int err;
>> +
>>     /* For now suspend/resume is only allowed with GuC */
>>     if (!xe_device_uc_enabled(xe))
>>         return;
>>
>> -    drmm_mutex_init(&xe->drm, &xe->d3cold.lock);
>> -
>>     xe->d3cold.capable = xe_pm_pci_d3cold_capable(xe);
>>
>>     if (xe->d3cold.capable) {
>> -        xe_device_sysfs_init(xe);
>> -        xe_pm_set_vram_threshold(xe, DEFAULT_VRAM_THRESHOLD);
>> +        err = xe_device_sysfs_init(xe);
>
> apparently not because of this patch, but why do we call a function
> named xe_device_sysfs_init() iff xe->d3cold.capable? And from within
> xe_pm. That seems totally misplaced. +Rodrigo
>
>> +        if (err)
>> +            drm_warn(&xe->drm,
>> +                 "Sysfs create for user to set vram threshold 
>> failed\n");
>
> just warning?

If I propagate xe_pm_int errors to xe_pci.c, it exhibits unexpected 
behavior. The PCI module throws errors as anticipated, but when I remove 
the xe module with "module -r xe", it doesn't properly clean up the 
"/sys/class/drm/card*" directory. Subsequently, upon reloading, it 
complains about existing sysfs entries and fails to load. This behavior 
aligns with the issue described in 
https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1352.

>
> Lucas De Marchi
>
>> +
>> +        err = xe_pm_set_vram_threshold(xe, DEFAULT_VRAM_THRESHOLD);
>> +        if (err)
>> +            drm_warn(&xe->drm, "Setting default vram threshold 
>> failed\n");
>>     }
>>
>>     xe_pm_runtime_init(xe);
>> diff --git a/drivers/gpu/drm/xe/xe_pm.h b/drivers/gpu/drm/xe/xe_pm.h
>> index 0cb38ca244fe..1e6ec58878d2 100644
>> --- a/drivers/gpu/drm/xe/xe_pm.h
>> +++ b/drivers/gpu/drm/xe/xe_pm.h
>> @@ -20,7 +20,7 @@ struct xe_device;
>> int xe_pm_suspend(struct xe_device *xe);
>> int xe_pm_resume(struct xe_device *xe);
>>
>> -void xe_pm_init_early(struct xe_device *xe);
>> +int xe_pm_init_early(struct xe_device *xe);
>> void xe_pm_init(struct xe_device *xe);
>> void xe_pm_runtime_fini(struct xe_device *xe);
>> bool xe_pm_runtime_suspended(struct xe_device *xe);
>> -- 
>> 2.25.1
>>

[-- Attachment #2: Type: text/html, Size: 15643 bytes --]