<!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 22:28, Lucas De Marchi wrote: </div> <blockquote type="cite" cite="mid:ub2u2jq47h5uy7zydrmoewdad644qf5eo6finfz4mfpwa4orm3@22uzbbikjtkw">On Fri, Apr 12, 2024 at 10:01:20PM +0530, Ghimiray, Himal Prasad wrote: <blockquote type="cite"> On 12-04-2024 19:12, Lucas De Marchi wrote: <blockquote type="cite">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"> 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> 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 class="moz-txt-link-freetext" href="https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1352">https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/1352</a>. </blockquote> 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. */ </blockquote> Thanks for the input. <p 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: ; margin: 0px 0px 1.25em; 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;">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. <blockquote type="cite" cite="mid:ub2u2jq47h5uy7zydrmoewdad644qf5eo6finfz4mfpwa4orm3@22uzbbikjtkw"> thanks Lucas De Marchi </blockquote> </body> </html>