On Fri, 19 Apr 2024, Imre Deak <imre.deak@xxxxxxxxx> wrote: > On Thu, Apr 18, 2024 at 06:37:56PM -0400, Rodrigo Vivi wrote: >> In the past, the noresume function was used by the GEM code to ensure >> wakelocks were held and bump its usage. This is no longer the case >> and this function was totally unused until it started to be used again >> by display with commit 77e619a82fc3 ("drm/i915/display: convert inner >> wakeref get towards get_if_in_use") >> >> However, on the display code, most of the callers are using the >> raw wakeref, rather then the wakelock version. What caused a >> major regression caught by CI. >> >> Another option to this patch is to go with the original plan and >> use the get_if_in_use variant in the display code, what is enough >> to fulfil our needs. Then, an extra patch to delete the unused >> _noresume variant. >> >> v2: Keep grabbing wakelock but only assert for wakeref. (Imre) >> >> Cc: Imre Deak <imre.deak@xxxxxxxxx> >> Cc: Francois Dugast <francois.dugast@xxxxxxxxx> >> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> >> Fixes: 77e619a82fc3 ("drm/i915/display: convert inner wakeref get towards get_if_in_use") >> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/10875 >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx> Thanks for the patch and review. Pushed to drm-xe-next where the offending commit was pushed. BR, Jani. > >> --- >> drivers/gpu/drm/i915/display/intel_display_power.c | 6 ------ >> drivers/gpu/drm/i915/intel_runtime_pm.c | 14 +++++--------- >> 2 files changed, 5 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c >> index 048943d0a881..03dc7edcc443 100644 >> --- a/drivers/gpu/drm/i915/display/intel_display_power.c >> +++ b/drivers/gpu/drm/i915/display/intel_display_power.c >> @@ -640,12 +640,6 @@ release_async_put_domains(struct i915_power_domains *power_domains, >> enum intel_display_power_domain domain; >> intel_wakeref_t wakeref; >> >> - /* >> - * The caller must hold already raw wakeref, upgrade that to a proper >> - * wakeref to make the state checker happy about the HW access during >> - * power well disabling. >> - */ >> - assert_rpm_raw_wakeref_held(rpm); >> wakeref = intel_runtime_pm_get_noresume(rpm); >> >> for_each_power_domain(domain, mask) { >> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c >> index d4e844128826..2d0647aca964 100644 >> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c >> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c >> @@ -272,15 +272,11 @@ intel_wakeref_t intel_runtime_pm_get_if_active(struct intel_runtime_pm *rpm) >> * intel_runtime_pm_get_noresume - grab a runtime pm reference >> * @rpm: the intel_runtime_pm structure >> * >> - * This function grabs a device-level runtime pm reference (mostly used for GEM >> - * code to ensure the GTT or GT is on). >> + * This function grabs a device-level runtime pm reference. >> * >> - * It will _not_ power up the device but instead only check that it's powered >> - * on. Therefore it is only valid to call this functions from contexts where >> - * the device is known to be powered up and where trying to power it up would >> - * result in hilarity and deadlocks. That pretty much means only the system >> - * suspend/resume code where this is used to grab runtime pm references for >> - * delayed setup down in work items. >> + * It will _not_ resume the device but instead only get an extra wakeref. >> + * Therefore it is only valid to call this functions from contexts where >> + * the device is known to be active and with another wakeref previously hold. >> * >> * Any runtime pm reference obtained by this function must have a symmetric >> * call to intel_runtime_pm_put() to release the reference again. >> @@ -289,7 +285,7 @@ intel_wakeref_t intel_runtime_pm_get_if_active(struct intel_runtime_pm *rpm) >> */ >> intel_wakeref_t intel_runtime_pm_get_noresume(struct intel_runtime_pm *rpm) >> { >> - assert_rpm_wakelock_held(rpm); >> + assert_rpm_raw_wakeref_held(rpm); >> pm_runtime_get_noresume(rpm->kdev); >> >> intel_runtime_pm_acquire(rpm, true); >> -- >> 2.44.0 >> -- Jani Nikula, Intel