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

[Jani Nikula <jani.nikula@linux.intel.com>]

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.

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