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: 6781 bytes --]


On 12-04-2024 22:28, Lucas De Marchi wrote:
> On Fri, Apr 12, 2024 at 10:01:20PM +0530, Ghimiray, Himal Prasad wrote:
>>
>> 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.
>
> Ok, it seems xe_pci.c is not ready yet to handle the propagated error.
> Please add a comment above and in the commit message about this, so we
> remember to fix it later when xe_pci.c is fixed.
>
> /*
>  * FIXME: Just warn on error for now since caller is not completely
>  * ready to handle the fallout.
>  */

Thanks for the input.

Since errors come up after the DRM device probing, there's currently no 
cleanup mechanism for the driver. Adding handling via xe_pci_remove() 
might sort out issues popping up post driver probe. I'll give it a try, 
and if it works out, I'll update the patch accordingly. Otherwise, I'll 
resort to adding warning messages and fixme notes for now.

>
> thanks
> Lucas De Marchi

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