[Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper

Wed Sep 28 12:16:10 UTC 2022

 @Nikula, Jani,

As you know we have reused i915_gem_backup_suspend, i915_gem_suspend_late and 
i915_gem_resume in runtime_pm_suspend/resume callbacks ,they use runtime pm 
helpers (intel_runtime_pm_get/put).
These need to be avoided in callbacks as they lead to deadlock.

This can be done in two ways 
1) push runtime pm helpers usage at higher level functions,
Which requires code refactoring (https://patchwork.freedesktop.org/series/105427/#rev2    is partially implemented following this)
2) Add is_intel_rpm_allowed helper and avoid the runtime helpers (https://patchwork.freedesktop.org/series/105427/#rev3 is completely implemented following this)

Hope I gave you the context,

As per your review comment in rev2,  the below is implemented in rev3

""""""""""""""""""""""""
v2: Return -2 if runtime pm is not allowed in runtime_pm_get and skip
wakeref release in runtime_pm_put if wakeref value is -2. - Jani N
Signed-off-by: Tilak Tangudu <tilak.tangudu at intel.com>
"""""""""""""""""""""""""

Rodrigo and myself want to trigger a discussion, if 2) is a proper approach or go with 1) which requires lot of code refactoring.
Or Is there any way we follow 1) with less code refactoring.?
Or Do you think there is any other proper approach ?

(Please note rev3 is not clean, d3cold off support need to be restricted to Headless clients for now , we see some Display related warnings in headed case ).

Thanks
Tilak

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of
> Tangudu, Tilak
> Sent: Thursday, August 4, 2022 11:03 AM
> To: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Cc: Nikula, Jani <jani.nikula at intel.com>; Wilson, Chris P
> <chris.p.wilson at intel.com>; Gupta, saurabhg <saurabhg.gupta at intel.com>;
> intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed
> helper
> 
> 
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> > Sent: Thursday, August 4, 2022 2:01 AM
> > To: Tangudu, Tilak <tilak.tangudu at intel.com>
> > Cc: Ewins, Jon <jon.ewins at intel.com>; Belgaumkar, Vinay
> > <vinay.belgaumkar at intel.com>; Roper, Matthew D
> > <matthew.d.roper at intel.com>; Wilson, Chris P
> > <chris.p.wilson at intel.com>; Nikula, Jani <jani.nikula at intel.com>;
> > Gupta, saurabhg <saurabhg.gupta at intel.com>; Gupta, Anshuman
> > <anshuman.gupta at intel.com>; Nilawar, Badal <badal.nilawar at intel.com>;
> > Deak, Imre <imre.deak at intel.com>; Iddamsetty, Aravind
> > <aravind.iddamsetty at intel.com>; intel-gfx at lists.freedesktop.org
> > Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> >
> > On Thu, Jul 21, 2022 at 03:29:48PM +0530, tilak.tangudu at intel.com wrote:
> > > From: Tilak Tangudu <tilak.tangudu at intel.com>
> > >
> > > Added is_intel_rpm_allowed function to query the runtime_pm status
> > > and disllow during suspending and resuming.
> >
> > >
> > > v2: Return -2 if runtime pm is not allowed in runtime_pm_get and
> > > skip wakeref release in runtime_pm_put if wakeref value is -2. -
> > > Jani N
> >
> > Should we have some defines instead of the -#s?
> Ok will address this.
> >
> > > Signed-off-by: Tilak Tangudu <tilak.tangudu at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 23
> > ++++++++++++++++++++++-
> > > drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
> > >  2 files changed, 23 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 6ed5786bcd29..704beeeb560b 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -113,7 +113,7 @@ static void
> > untrack_intel_runtime_pm_wakeref(struct intel_runtime_pm *rpm,
> > >  	unsigned long flags, n;
> > >  	bool found = false;
> > >
> > > -	if (unlikely(stack == -1))
> > > +	if (unlikely(stack == -1) || unlikely(stack == -2))
> > >  		return;
> > >
> > >  	spin_lock_irqsave(&rpm->debug.lock, flags); @@ -320,6 +320,21
> > @@
> > > untrack_all_intel_runtime_pm_wakerefs(struct intel_runtime_pm *rpm)
> > > }
> > >
> > >  #endif
> > > +static int intel_runtime_pm_status(struct intel_runtime_pm *rpm) {
> > > +	return rpm->kdev->power.runtime_status; }
> > > +
> > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm)
> >
> > why not static?
>  We need is_intel_rpm_allowed for rc6 helpers , the helpers use
> pm_runtime_get_sync, To avoid lock issue we need to use it here too.
> 
> See this patch " drm/i915: Guard rc6 helpers with is_intel_rpm_allowed"
> 
> >
> > > +{
> > > +	int rpm_status;
> > > +
> > > +	rpm_status = intel_runtime_pm_status(rpm);
> > > +	if (rpm_status == RPM_RESUMING
> >
> > I don't have a good feeling about this. If we are resuming we
> > shouldn't grab extra references... This seems a workaround for the lock
> mess.
> >
> > > || rpm_status == RPM_SUSPENDING)
> >
> > and when we are suspending and we call this function is because we
> > need to wake up, no?!
> 
> As we have re-used i915_gem_backup_suspend, i915_gem_suspend_late
> and i915_gem_resume , These functions use runtime helpers, which in-turn
> call  runtime suspend/resume callbacks and leads to deadlock.
>  So, these helpers need to be avoided.  we have added is_intel_rpm_allowed
> and disallowed rpm callbacks with above condition during suspending and
> resuming  to avoid deadlock.
> >
> > > +		return false;
> > > +	else
> > > +		return true;
> > > +}
> > >
> > >  static void
> > >  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool
> > > wakelock) @@ -354,6 +369,9 @@ static intel_wakeref_t
> > __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
> > >  						     runtime_pm);
> > >  	int ret;
> > >
> > > +	if (!is_intel_rpm_allowed(rpm))
> > > +		return -2;
> > > +
> > >  	ret = pm_runtime_get_sync(rpm->kdev);
> > >  	drm_WARN_ONCE(&i915->drm, ret < 0,
> > >  		      "pm_runtime_get_sync() failed: %d\n", ret); @@ -490,6
> > +508,9
> > > @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,
> > >
> > >  	untrack_intel_runtime_pm_wakeref(rpm, wref);
> > >
> > > +	if (wref == -2)
> > > +		return;
> > > +
> > >  	intel_runtime_pm_release(rpm, wakelock);
> > >
> > >  	pm_runtime_mark_last_busy(kdev);
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > index d9160e3ff4af..99418c3a934a 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.h
> > > @@ -173,6 +173,7 @@ void intel_runtime_pm_init_early(struct
> > > intel_runtime_pm *rpm);  void intel_runtime_pm_enable(struct
> > > intel_runtime_pm *rpm);  void intel_runtime_pm_disable(struct
> > > intel_runtime_pm *rpm);  void intel_runtime_pm_driver_release(struct
> > > intel_runtime_pm *rpm);
> > > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm);
> >
> > if really need to export please follow the naming convention.\
> 
> Ok will address this.
> 
> -Tilak
> >
> > >
> > >  intel_wakeref_t intel_runtime_pm_get(struct intel_runtime_pm *rpm);
> > > intel_wakeref_t intel_runtime_pm_get_if_in_use(struct
> > > intel_runtime_pm *rpm);
> > > --
> > > 2.25.1
> > >