[Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
Gupta, Anshuman
anshuman.gupta at intel.com
Thu Sep 29 05:56:36 UTC 2022
Quoting Tilak.
> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Sent: Wednesday, September 28, 2022 8:00 PM
> To: Nikula, Jani <jani.nikula at intel.com>; Gupta, Anshuman
> <anshuman.gupta at intel.com>; Tangudu, Tilak <tilak.tangudu at intel.com>
> Cc: Wilson, Chris P <chris.p.wilson at intel.com>; Gupta, saurabhg
> <saurabhg.gupta at intel.com>; intel-gfx at lists.freedesktop.org
> 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 at intel.com>; Vivi, Rodrigo
> > > <rodrigo.vivi at intel.com>; Nikula, Jani <jani.nikula at intel.com>
> > > Cc: Wilson, Chris P <Chris.P.Wilson at intel.com>; Gupta, saurabhg
> > > <saurabhg.gupta at intel.com>; intel-gfx at lists.freedesktop.org
> > > 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 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 ).
>
> 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 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
> > > > > >
More information about the Intel-gfx
mailing list