[PATCH v3 7/7] drm/xe/pm: Capture errors and handle them
Ghimiray, Himal Prasad
himal.prasad.ghimiray at intel.com
Fri Apr 12 17:07:09 UTC 2024
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 at intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
>>>> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-xe/attachments/20240412/262311c0/attachment-0001.htm>
More information about the Intel-xe
mailing list