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

[Vivi, Rodrigo]

On Thu, 2022-08-04 at 05:32 +0000, Tangudu, Tilak wrote:
> 
> 
> > -----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.

Why does these functions in suspend resume path are using the runtime
callbacks?
Can't we have a refactor on that area that allows the paths that we
need in a place where we are sure that device is not going to d3?
then we can have a clear infra?

> > 
> > > +               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
> > >