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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Sep 29, 2022 at 05:56:36AM +0000, Gupta, Anshuman wrote:
> 
> Quoting Tilak.
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Wednesday, September 28, 2022 8:00 PM
> > To: Nikula, Jani <jani.nikula@intel.com>; Gupta, Anshuman
> > <anshuman.gupta@intel.com>; Tangudu, Tilak <tilak.tangudu@intel.com>
> > Cc: Wilson, Chris P <chris.p.wilson@intel.com>; Gupta, saurabhg
> > <saurabhg.gupta@intel.com>; intel-gfx@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@intel.com>; Vivi, Rodrigo
> > > > <rodrigo.vivi@intel.com>; Nikula, Jani <jani.nikula@intel.com>
> > > > Cc: Wilson, Chris P <Chris.P.Wilson@intel.com>; Gupta, saurabhg
> > > > <saurabhg.gupta@intel.com>; intel-gfx@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.

Okay, I'm convinced now that the better path is to use the status.
But this patch needs some clean-up and split.

Hopefully we can get the runtime_is_transitioninig function in the
linux/pm_runtime.h directly later, but one way or another, this
part of the patch needs to be separated with the '-2' return.

And that one with a different explanation and probably some enums?!

> 
> 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@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@lists.freedesktop.org> On
> > > > > Behalf Of Tangudu, Tilak
> > > > > Sent: Thursday, August 4, 2022 11:03 AM
> > > > > To: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > > Cc: Nikula, Jani <jani.nikula@intel.com>; Wilson, Chris P
> > > > > <chris.p.wilson@intel.com>; Gupta, saurabhg
> > > > > <saurabhg.gupta@intel.com>; intel-gfx@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@intel.com>
> > > > > > Sent: Thursday, August 4, 2022 2:01 AM
> > > > > > To: Tangudu, Tilak <tilak.tangudu@intel.com>
> > > > > > Cc: Ewins, Jon <jon.ewins@intel.com>; Belgaumkar, Vinay
> > > > > > <vinay.belgaumkar@intel.com>; Roper, Matthew D
> > > > > > <matthew.d.roper@intel.com>; Wilson, Chris P
> > > > > > <chris.p.wilson@intel.com>; Nikula, Jani
> > > > > > <jani.nikula@intel.com>; Gupta, saurabhg
> > > > > > <saurabhg.gupta@intel.com>; Gupta, Anshuman
> > > > > > <anshuman.gupta@intel.com>; Nilawar, Badal
> > > > > > <badal.nilawar@intel.com>; Deak, Imre <imre.deak@intel.com>;
> > > > > > Iddamsetty, Aravind <aravind.iddamsetty@intel.com>;
> > > > > > intel-gfx@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@intel.com
> > > > wrote:
> > > > > > > From: Tilak Tangudu <tilak.tangudu@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@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
> > > > > > >
>