On Wed, 2022-09-28 at 12:31 +0000, Tangudu, Tilak wrote: > + > > > -----Original Message----- > > From: Tangudu, Tilak > > Sent: Wednesday, September 28, 2022 5:46 PM > > To: Tangudu, Tilak <tilak.tangudu@xxxxxxxxx>; Vivi, Rodrigo > > <rodrigo.vivi@xxxxxxxxx>; Nikula, Jani <jani.nikula@xxxxxxxxx> > > Cc: 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 > > > > @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 ). I believe this warnings shows that the solution 2 has some flaws or corner cases that we don't fully understand. I honestly believe we need to go with option 1, moving the runtime_pm_ {get,put} to higher levels. One way or another, we should not go partial here, but with full implementation so we can see if we are really covered. Jani, thoughts? > > > > 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 > > > > >
