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

[Lucas De Marchi]

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
Lucas De Marchi