Re: [Intel-gfx] [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed helper

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

On Wed, Jun 22, 2022 at 03:55:03PM +0300, Jani Nikula wrote:
> On Tue, 21 Jun 2022, "Tangudu, Tilak" <tilak.tangudu@intel.com> wrote:
> >> -----Original Message-----
> >> From: Gupta, Anshuman <anshuman.gupta@intel.com>
> >> Sent: Tuesday, June 21, 2022 7:47 PM
> >> To: Tangudu, Tilak <tilak.tangudu@intel.com>; intel-gfx@lists.freedesktop.org;
> >> Ewins, Jon <jon.ewins@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>;
> >> Belgaumkar, Vinay <vinay.belgaumkar@intel.com>; Wilson, Chris P
> >> <chris.p.wilson@intel.com>; Dixit, Ashutosh <ashutosh.dixit@intel.com>;
> >> Nilawar, Badal <badal.nilawar@intel.com>; Roper, Matthew D
> >> <matthew.d.roper@intel.com>; Gupta, saurabhg
> >> <saurabhg.gupta@intel.com>; Iddamsetty, Aravind
> >> <aravind.iddamsetty@intel.com>; Sundaresan, Sujaritha
> >> <sujaritha.sundaresan@intel.com>
> >> Subject: RE: [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed helper
> >> 
> >> 
> >> 
> >> > -----Original Message-----
> >> > From: Tangudu, Tilak <tilak.tangudu@intel.com>
> >> > Sent: Tuesday, June 21, 2022 6:05 PM
> >> > To: intel-gfx@lists.freedesktop.org; Ewins, Jon <jon.ewins@intel.com>;
> >> > Vivi, Rodrigo <rodrigo.vivi@intel.com>; Belgaumkar, Vinay
> >> > <vinay.belgaumkar@intel.com>; Wilson, Chris P
> >> > <chris.p.wilson@intel.com>; Dixit, Ashutosh
> >> > <ashutosh.dixit@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
> >> > Gupta, Anshuman <anshuman.gupta@intel.com>; Tangudu, Tilak
> >> > <tilak.tangudu@intel.com>; Roper, Matthew D
> >> > <matthew.d.roper@intel.com>; Gupta, saurabhg
> >> > <saurabhg.gupta@intel.com>; Iddamsetty, Aravind
> >> > <aravind.iddamsetty@intel.com>; Sundaresan, Sujaritha
> >> > <sujaritha.sundaresan@intel.com>
> >> > Subject: [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed helper
> >> >
> >> > Added is_intel_rpm_allowed function to query the runtime_pm status and
> >> > disllow during suspending and resuming.
> >> This seems a hack,
> >> Not sure if we have better way to handle it.
> >> May be check this in intel_pm_runtime_{get,put} to keep entire code simple ?
> > Yes, that would be simple without code refactoring.
> > Checked the same with Chris, he suggested unbalancing of wakeref might popup
> > If used at intel_pm_runtime_{get,put}  . So used like this,
> >  @Wilson, Chris P , Please comment .
> > @Vivi, Rodrigo , Any suggestion ?
> 
> One option would be to track this in intel_wakeref_t, i.e. _get flags
> the case in the returned wakeref and _put skips in that case.

yeap, this seems to be the quick path at this moment...

Imre, do you see any other quick option?

In general I don't like much the big wakeref infra we end up
creating here because all of the historical unbalanced cases we got.
We should be able to get something cleaner and use the rpm infra as
other drivers are using, or improve in the rpm side itself whatever
we feel that we are missing to deal with these cases.

But back to this specific case/usage here we might need to duplicate
some functions to be called just from the inside the resuming/suspending
path... and/or moving the gets & puts upper on the stack...

The quick hacks will solve our short term problems and continue bloating
our rpm infra.

>
> BR,
> Jani.
> 
> 
> >  
> >> >
> >> > Signed-off-by: Tilak Tangudu <tilak.tangudu@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +++++++++++++++
> >> > drivers/gpu/drm/i915/intel_runtime_pm.h |  1 +
> >> >  2 files changed, 16 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >> > index 6ed5786bcd29..3759a8596084 100644
> >> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> >> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> >> > @@ -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; }
> >> This is racy in principal, we need a kdev->power lock here.
> >> Regards,
> >> Anshuman Gupta.
> >> > +
> >> > +bool is_intel_rpm_allowed(struct intel_runtime_pm *rpm) { int
> >> > +rpm_status;
> >> > +
> >> > +rpm_status = intel_runtime_pm_status(rpm); if (rpm_status ==
> >> > +RPM_RESUMING || rpm_status ==
> >> > RPM_SUSPENDING)
> >> > +return false;
> >> > +else
> >> > +return true;
> >> > +}
> >> >
> >> >  static void
> >> >  intel_runtime_pm_acquire(struct intel_runtime_pm *rpm, bool wakelock)
> >> > 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);
> >> >
> >> >  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
> >> 
> >
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center