[PATCH v3 7/7] drm/xe/pm: Capture errors and handle them
Lucas De Marchi
lucas.demarchi at intel.com
Fri Apr 12 13:42:48 UTC 2024
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;
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?
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
>
More information about the Intel-xe
mailing list