On Thu, 23 Jun 2022, "Tangudu, Tilak" <tilak.tangudu@xxxxxxxxx> wrote:
>> -----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.
The code below will lead to get/put inbalance if is_intel_rpm_allowed()
status changes between the get/put calls. I don't know how likely that
is, but if it happens it's nasty.
intel_wakeref_t is depot_stack_handle_t, which is actually just u32. We
already abuse -1 value to not track wakeref (when
CONFIG_DRM_I915_DEBUG_RUNTIME_PM=n or track_intel_runtime_pm_wakeref()
fails.
It's a bit of a hack, but we could have __intel_runtime_pm_get() early
return -2 as the wakeref when !is_intel_rpm_allowed(), and
intel_runtime_pm_put() (both versions for both kconfig option values!)
ignore the put when the passed in wakeref == -2.
This requires no changes in the calling code anywhere, even though the
implementation is a hack. A pedantically correct implementation would
turn intel_wakeref_t into a struct that wraps depot_stack_handle_t
inside, and has a separate field for validity, but that probably has a
non-trivial code size penalty.
BR,
Jani.
>
> 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
--
Jani Nikula, Intel Open Source Graphics Center
