@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@xxxxxxxxx> """"""""""""""""""""""""" 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@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of > Tangudu, Tilak > Sent: Thursday, August 4, 2022 11:03 AM > To: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx> > Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>; Wilson, Chris P > <chris.p.wilson@xxxxxxxxx>; Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; > intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed > helper > > > > > -----Original Message----- > > From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx> > > Sent: Thursday, August 4, 2022 2:01 AM > > To: Tangudu, Tilak <tilak.tangudu@xxxxxxxxx> > > Cc: Ewins, Jon <jon.ewins@xxxxxxxxx>; Belgaumkar, Vinay > > <vinay.belgaumkar@xxxxxxxxx>; Roper, Matthew D > > <matthew.d.roper@xxxxxxxxx>; Wilson, Chris P > > <chris.p.wilson@xxxxxxxxx>; Nikula, Jani <jani.nikula@xxxxxxxxx>; > > Gupta, saurabhg <saurabhg.gupta@xxxxxxxxx>; Gupta, Anshuman > > <anshuman.gupta@xxxxxxxxx>; Nilawar, Badal <badal.nilawar@xxxxxxxxx>; > > Deak, Imre <imre.deak@xxxxxxxxx>; Iddamsetty, Aravind > > <aravind.iddamsetty@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > 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@xxxxxxxxx wrote: > > > From: Tilak Tangudu <tilak.tangudu@xxxxxxxxx> > > > > > > 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@xxxxxxxxx> > > > --- > > > 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 > > >
