Quoting Tilak. > -----Original Message----- > From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx> > Sent: Wednesday, September 28, 2022 8:00 PM > To: Nikula, Jani <jani.nikula@xxxxxxxxx>; Gupta, Anshuman > <anshuman.gupta@xxxxxxxxx>; Tangudu, Tilak <tilak.tangudu@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 > > 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 AFAIK pushing runtime PM to higher level need to asses case by case, Moving runtime PM wakeref to higher level will also burn more power as Wakeref will be active for longer period. This has to be resolve case by case, as a simple rule of thumb we don't need any wakeref in suspend path. So refactoring based upon suspend specific function and general use function would be the correct approach. Rest Jani and Rodrigo can provide their input here. Thanks, Anshuman Gupta. > > > (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 > > > > > >
