> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@xxxxxxxxx>
> Sent: Thursday, August 4, 2022 2:01 AM
> To: Tangudu, Tilak <tilak.tangudu@xxxxxxxxx>
> Cc: Ewins, Jon <jon.ewins@xxxxxxxxx>; Belgaumkar, Vinay
> <vinay.belgaumkar@xxxxxxxxx>; Roper, Matthew D
> <matthew.d.roper@xxxxxxxxx>; Wilson, Chris P <chris.p.wilson@xxxxxxxxx>;
> Nikula, Jani <jani.nikula@xxxxxxxxx>; Gupta, saurabhg
> <saurabhg.gupta@xxxxxxxxx>; Gupta, Anshuman
> <anshuman.gupta@xxxxxxxxx>; Nilawar, Badal <badal.nilawar@xxxxxxxxx>;
> Deak, Imre <imre.deak@xxxxxxxxx>; Iddamsetty, Aravind
> <aravind.iddamsetty@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxx wrote:
> > From: Tilak Tangudu <tilak.tangudu@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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.
>
> > + 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
> >
