<!DOCTYPE html><html><head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> </head> <body> <div class="moz-cite-prefix">On 12-04-2024 19:12, Lucas De Marchi wrote: </div> <blockquote type="cite" cite="mid:7ioxhkpm4jidj5m4hcc5jsatfk333d4sgso7o5bl2pqmq5xxyc@36dmyunxbviy">On Fri, Apr 12, 2024 at 01:32:45PM +0530, Himal Prasad Ghimiray wrote: <blockquote type="cite">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 <a class="moz-txt-link-rfc2396E" href="mailto:lucas.demarchi@intel.com"><lucas.demarchi@intel.com></a> Cc: Rodrigo Vivi <a class="moz-txt-link-rfc2396E" href="mailto:rodrigo.vivi@intel.com"><rodrigo.vivi@intel.com></a> Signed-off-by: Himal Prasad Ghimiray <a class="moz-txt-link-rfc2396E" href="mailto:himal.prasad.ghimiray@intel.com"><himal.prasad.ghimiray@intel.com></a> --- 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 */ </blockquote> 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; </blockquote> Looks clean. Will prefer using this. <blockquote type="cite" cite="mid:7ioxhkpm4jidj5m4hcc5jsatfk333d4sgso7o5bl2pqmq5xxyc@36dmyunxbviy"> 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. <blockquote type="cite"> /** @@ -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. + * </blockquote> wrong line <blockquote type="cite"> */ 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); </blockquote> 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 <blockquote type="cite">+ if (err) + drm_warn(&xe->drm, + "Sysfs create for user to set vram threshold failed\n"); </blockquote> just warning? </blockquote> <span style="color: rgb(13, 13, 13); font-family: Söhne, ui-sans-serif, system-ui, -apple-system, "Segoe UI", Roboto, Ubuntu, Cantarell, "Noto Sans", sans-serif, "Helvetica Neue", Arial, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji"; font-size: 16px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; white-space: pre-wrap; background-color: rgb(255, 255, 255); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;">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 <a target="_new" href="https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1352" style="border: 0px solid rgb(227, 227, 227); box-sizing: border-box; --tw-border-spacing-x: 0; --tw-border-spacing-y: 0; --tw-translate-x: 0; --tw-translate-y: 0; --tw-rotate: 0; --tw-skew-x: 0; --tw-skew-y: 0; --tw-scale-x: 1; --tw-scale-y: 1; --tw-pan-x: ; --tw-pan-y: ; --tw-pinch-zoom: ; --tw-scroll-snap-strictness: proximity; --tw-gradient-from-position: ; --tw-gradient-via-position: ; --tw-gradient-to-position: ; --tw-ordinal: ; --tw-slashed-zero: ; --tw-numeric-figure: ; --tw-numeric-spacing: ; --tw-numeric-fraction: ; --tw-ring-inset: ; --tw-ring-offset-width: 0px; --tw-ring-offset-color: #fff; --tw-ring-color: rgba(69,89,164,.5); --tw-ring-offset-shadow: 0 0 transparent; --tw-ring-shadow: 0 0 transparent; --tw-shadow: 0 0 transparent; --tw-shadow-colored: 0 0 transparent; --tw-blur: ; --tw-brightness: ; --tw-contrast: ; --tw-grayscale: ; --tw-hue-rotate: ; --tw-invert: ; --tw-saturate: ; --tw-sepia: ; --tw-drop-shadow: ; --tw-backdrop-blur: ; --tw-backdrop-brightness: ; --tw-backdrop-contrast: ; --tw-backdrop-grayscale: ; --tw-backdrop-hue-rotate: ; --tw-backdrop-invert: ; --tw-backdrop-opacity: ; --tw-backdrop-saturate: ; --tw-backdrop-sepia: ; --tw-contain-size: ; --tw-contain-layout: ; --tw-contain-paint: ; --tw-contain-style: ; color: var(--link); text-decoration: none; font-weight: 400; font-family: Söhne, ui-sans-serif, system-ui, -apple-system, "Segoe UI", Roboto, Ubuntu, Cantarell, "Noto Sans", sans-serif, "Helvetica Neue", Arial, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji"; font-size: 16px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; white-space: pre-wrap; background-color: rgb(255, 255, 255);" class="moz-txt-link-freetext">https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1352</a><span style="color: rgb(13, 13, 13); font-family: Söhne, ui-sans-serif, system-ui, -apple-system, "Segoe UI", Roboto, Ubuntu, Cantarell, "Noto Sans", sans-serif, "Helvetica Neue", Arial, "Apple Color Emoji", "Segoe UI Emoji", "Segoe UI Symbol", "Noto Color Emoji"; font-size: 16px; font-style: normal; font-variant-ligatures: normal; font-variant-caps: normal; font-weight: 400; letter-spacing: normal; orphans: 2; text-align: start; text-indent: 0px; text-transform: none; widows: 2; word-spacing: 0px; -webkit-text-stroke-width: 0px; white-space: pre-wrap; background-color: rgb(255, 255, 255); text-decoration-thickness: initial; text-decoration-style: initial; text-decoration-color: initial; display: inline !important; float: none;">. <blockquote type="cite" cite="mid:7ioxhkpm4jidj5m4hcc5jsatfk333d4sgso7o5bl2pqmq5xxyc@36dmyunxbviy"> Lucas De Marchi <blockquote type="cite">+ + 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 </blockquote> </blockquote> </body> </html>