> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
> Sent: Thursday, June 23, 2022 2:11 AM
> To: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Tangudu, Tilak <tilak.tangudu@xxxxxxxxx>; Gupta, Anshuman
> <anshuman.gupta@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Ewins, Jon
> <jon.ewins@xxxxxxxxx>; Belgaumkar, Vinay <vinay.belgaumkar@xxxxxxxxx>;
> Wilson, Chris P <chris.p.wilson@xxxxxxxxx>; Dixit, Ashutosh
> <ashutosh.dixit@xxxxxxxxx>; Nilawar, Badal <badal.nilawar@xxxxxxxxx>;
> Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; Gupta, saurabhg
> <saurabhg.gupta@xxxxxxxxx>; Iddamsetty, Aravind
> <aravind.iddamsetty@xxxxxxxxx>; Sundaresan, Sujaritha
> <sujaritha.sundaresan@xxxxxxxxx>; Deak, Imre <imre.deak@xxxxxxxxx>
> Subject: Re: [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed
> helper
>
> On Wed, Jun 22, 2022 at 03:55:03PM +0300, Jani Nikula wrote:
> > On Tue, 21 Jun 2022, "Tangudu, Tilak" <tilak.tangudu@xxxxxxxxx> wrote:
> > >> -----Original Message-----
> > >> From: Gupta, Anshuman <anshuman.gupta@xxxxxxxxx>
> > >> Sent: Tuesday, June 21, 2022 7:47 PM
> > >> To: Tangudu, Tilak <tilak.tangudu@xxxxxxxxx>;
> > >> intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Ewins, Jon <jon.ewins@xxxxxxxxx>;
> > >> Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>; Belgaumkar, Vinay
> > >> <vinay.belgaumkar@xxxxxxxxx>; Wilson, Chris P
> > >> <chris.p.wilson@xxxxxxxxx>; Dixit, Ashutosh
> > >> <ashutosh.dixit@xxxxxxxxx>; Nilawar, Badal
> > >> <badal.nilawar@xxxxxxxxx>; Roper, Matthew D
> > >> <matthew.d.roper@xxxxxxxxx>; Gupta, saurabhg
> > >> <saurabhg.gupta@xxxxxxxxx>; Iddamsetty, Aravind
> > >> <aravind.iddamsetty@xxxxxxxxx>; Sundaresan, Sujaritha
> > >> <sujaritha.sundaresan@xxxxxxxxx>
> > >> Subject: RE: [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed
> > >> helper
> > >>
> > >>
> > >>
> > >> > -----Original Message-----
> > >> > From: Tangudu, Tilak <tilak.tangudu@xxxxxxxxx>
> > >> > Sent: Tuesday, June 21, 2022 6:05 PM
> > >> > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Ewins, Jon
> > >> > <jon.ewins@xxxxxxxxx>; Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>;
> > >> > Belgaumkar, Vinay <vinay.belgaumkar@xxxxxxxxx>; Wilson, Chris P
> > >> > <chris.p.wilson@xxxxxxxxx>; Dixit, Ashutosh
> > >> > <ashutosh.dixit@xxxxxxxxx>; Nilawar, Badal
> > >> > <badal.nilawar@xxxxxxxxx>; Gupta, Anshuman
> > >> > <anshuman.gupta@xxxxxxxxx>; Tangudu, Tilak
> > >> > <tilak.tangudu@xxxxxxxxx>; Roper, Matthew D
> > >> > <matthew.d.roper@xxxxxxxxx>; Gupta, saurabhg
> > >> > <saurabhg.gupta@xxxxxxxxx>; Iddamsetty, Aravind
> > >> > <aravind.iddamsetty@xxxxxxxxx>; Sundaresan, Sujaritha
> > >> > <sujaritha.sundaresan@xxxxxxxxx>
> > >> > 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.
@Jani Nikula
I did not understand the suggestion, Can you please elaborate ?
Did you mean below or something more ? please help clarify.
8< ------------------------------
linux-desk:~/Code/drm-tip$ git diff
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 3759a8596084..ce272c569a89 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -369,12 +369,16 @@ static intel_wakeref_t __intel_runtime_pm_get(struct intel_runtime_pm *rpm,
runtime_pm);
int ret;
+ if (!is_intel_rpm_allowed(rpm))
+ goto out:
+
ret = pm_runtime_get_sync(rpm->kdev);
drm_WARN_ONCE(&i915->drm, ret < 0,
"pm_runtime_get_sync() failed: %d\n", ret);
intel_runtime_pm_acquire(rpm, wakelock);
+out:
return track_intel_runtime_pm_wakeref(rpm);
}
@@ -505,6 +509,9 @@ static void __intel_runtime_pm_put(struct intel_runtime_pm *rpm,
untrack_intel_runtime_pm_wakeref(rpm, wref);
+ if (!is_intel_rpm_allowed(rpm))
+ return;
+
intel_runtime_pm_release(rpm, wakelock);
pm_runtime_mark_last_busy(kdev);
---------------------------------------------------------- >8
Thanks
Tilak
>
> 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@xxxxxxxxx>
> > >> > ---
> > >> > 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
