Re: [PATCH] drm/i915: Convert intel_runtime_pm_get_noresume towards raw wakeref

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Apr 19, 2024 at 01:30:21AM +0300, Imre Deak wrote:
> On Thu, Apr 18, 2024 at 06:13:20PM -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.
> > 
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Francois Dugast <francois.dugast@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 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@intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_display_power.c    |  6 ------
> >  drivers/gpu/drm/i915/intel_runtime_pm.c           | 15 +++++----------
> >  2 files changed, 5 insertions(+), 16 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..e27b2ab82da0 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 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,10 +285,9 @@ 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);
> >  	pm_runtime_get_noresume(rpm->kdev);
> >  
> > -	intel_runtime_pm_acquire(rpm, true);
> > +	intel_runtime_pm_acquire(rpm, false);
> 
> This needs to stay a wakelock, so that the HW access in
> release_async_put_domains() will not lead to a wakelock not held assert.
> Only the above assert_rpm_wakelock_held() needs to be changed to
> assert_rpm_raw_wakeref_held().

hmm I see. That was actually my first attempt here, but then went down
to check the intel_runtime_pm_acquire and got confused and ended up
trying to convert everything... will resend it right now.

Thank you!

> 
> >  
> >  	return track_intel_runtime_pm_wakeref(rpm);
> >  }
> > -- 
> > 2.44.0
> >