Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
+ > -Original Message- > From: Tangudu, Tilak > Sent: Wednesday, September 28, 2022 5:46 PM > To: Tangudu, Tilak ; Vivi, Rodrigo > ; Nikula, Jani > Cc: Wilson, Chris P ; Gupta, saurabhg > ; intel-gfx@lists.freedesktop.org > Subject: RE: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper > > @Nikula, Jani, > > As you know we have reused i915_gem_backup_suspend, > i915_gem_suspend_late and i915_gem_resume in > runtime_pm_suspend/resume callbacks ,they use runtime pm helpers > (intel_runtime_pm_get/put). > These need to be avoided in callbacks as they lead to deadlock. > > This can be done in two ways > 1) push runtime pm helpers usage at higher level functions, > Which requires code refactoring > (https://patchwork.freedesktop.org/series/105427/#rev2is partially > implemented following this) > 2) Add is_intel_rpm_allowed helper and avoid the runtime helpers > (https://patchwork.freedesktop.org/series/105427/#rev3 is completely > implemented following this) > > Hope I gave you the context, > > As per your review comment in rev2, the below is implemented in rev3 > > """""""""""""""""""""""" > 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 > Signed-off-by: Tilak Tangudu > """"""""""""""""""""""""" > > Rodrigo and myself want to trigger a discussion, if 2) is a proper approach or > go with 1) which requires lot of code refactoring. > Or Is there any way we follow 1) with less code refactoring.? > Or Do you think there is any other proper approach ? > > (Please note rev3 is not clean, d3cold off support need to be restricted to > Headless clients for now , we see some Display related warnings in headed > case ). > > Thanks > Tilak > > > > -Original Message- > > From: Intel-gfx On Behalf Of > > Tangudu, Tilak > > Sent: Thursday, August 4, 2022 11:03 AM > > To: Vivi, Rodrigo > > Cc: Nikula, Jani ; Wilson, Chris P > > ; Gupta, saurabhg > > ; intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added > > is_intel_rpm_allowed helper > > > > > > > > > -Original Message- > > > From: Vivi, Rodrigo > > > Sent: Thursday, August 4, 2022 2:01 AM > > > To: Tangudu, Tilak > > > Cc: Ewins, Jon ; Belgaumkar, Vinay > > > ; Roper, Matthew D > > > ; Wilson, Chris P > > > ; Nikula, Jani ; > > > Gupta, saurabhg ; Gupta, Anshuman > > > ; Nilawar, Badal > > > ; Deak, Imre ; > > > Iddamsetty, Aravind ; > > > intel-gfx@lists.freedesktop.org > > > Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper > > > > > > On Thu, Jul 21, 2022 at 03:29:48PM +0530, tilak.tang...@intel.com > wrote: > > > > From: Tilak Tangudu > > > > > > > > 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 > > > > --- > > > > 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; > >
Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
@Nikula, Jani,
As you know we have reused i915_gem_backup_suspend, i915_gem_suspend_late and
i915_gem_resume in runtime_pm_suspend/resume callbacks ,they use runtime pm
helpers (intel_runtime_pm_get/put).
These need to be avoided in callbacks as they lead to deadlock.
This can be done in two ways
1) push runtime pm helpers usage at higher level functions,
Which requires code refactoring
(https://patchwork.freedesktop.org/series/105427/#rev2is partially
implemented following this)
2) Add is_intel_rpm_allowed helper and avoid the runtime helpers
(https://patchwork.freedesktop.org/series/105427/#rev3 is completely
implemented following this)
Hope I gave you the context,
As per your review comment in rev2, the below is implemented in rev3
""""""""""""""""""""""""
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
Signed-off-by: Tilak Tangudu
"""""""""""""""""""""""""
Rodrigo and myself want to trigger a discussion, if 2) is a proper approach or
go with 1) which requires lot of code refactoring.
Or Is there any way we follow 1) with less code refactoring.?
Or Do you think there is any other proper approach ?
(Please note rev3 is not clean, d3cold off support need to be restricted to
Headless clients for now , we see some Display related warnings in headed case
).
Thanks
Tilak
> -Original Message-
> From: Intel-gfx On Behalf Of
> Tangudu, Tilak
> Sent: Thursday, August 4, 2022 11:03 AM
> To: Vivi, Rodrigo
> Cc: Nikula, Jani ; Wilson, Chris P
> ; Gupta, saurabhg ;
> intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed
> helper
>
>
>
> > -Original Message-
> > From: Vivi, Rodrigo
> > Sent: Thursday, August 4, 2022 2:01 AM
> > To: Tangudu, Tilak
> > Cc: Ewins, Jon ; Belgaumkar, Vinay
> > ; Roper, Matthew D
> > ; Wilson, Chris P
> > ; Nikula, Jani ;
> > Gupta, saurabhg ; Gupta, Anshuman
> > ; Nilawar, Badal ;
> > Deak, Imre ; Iddamsetty, Aravind
> > ; intel-gfx@lists.freedesktop.org
> > Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> >
> > On Thu, Jul 21, 2022 at 03:29:48PM +0530, tilak.tang...@intel.com wrote:
> > > From: Tilak Tangudu
> > >
> > > 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
> > > ---
> > > 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)
Re: [Intel-gfx] [PATCH 4/8] drm/i915: sanitize dc state in rpm resume
> -Original Message-
> From: Vivi, Rodrigo
> Sent: Thursday, August 4, 2022 2:03 AM
> To: Tangudu, Tilak ; Srivatsa, Anusha
> ; Deak, Imre
> Cc: Ewins, Jon ; Belgaumkar, Vinay
> ; Roper, Matthew D
> ; Wilson, Chris P ;
> Nikula, Jani ; Gupta, saurabhg
> ; Gupta, Anshuman
> ; Nilawar, Badal ;
> Deak, Imre ; Iddamsetty, Aravind
> ; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 4/8] drm/i915: sanitize dc state in rpm resume
>
> On Thu, Jul 21, 2022 at 03:29:51PM +0530, tilak.tang...@intel.com wrote:
> > From: Tilak Tangudu
> >
> > During runtime resume the display init sequence is called via
> > intel_display_power_resume() -> icl_display_core_init() which should
> > restore the display HW state. For restoring the DC9 enabled state in
> > DC_STATE_EN, gen9_sanitize_dc_state() should be called on the runtime
> > resume path too to avoid the
> >
> > [ 513.818190] i915 :03:00.0: [drm] *ERROR DC state mismatch (0x8
> > -> 0x0)*
> >
> > Signed-off-by: Tilak Tangudu
> Cc: Anusha Srivatsa
> Cc: Imre Deak
This is suggested by Imre in the JIRA,
In next patches I will add the below
Suggested-by: Imre Deak
>
> > ---
> > drivers/gpu/drm/i915/display/intel_display_power.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c
> > b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 589af257edeb..799f84d3eed6 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -2229,6 +2229,7 @@ void intel_display_power_suspend(struct
> > drm_i915_private *i915) void intel_display_power_resume(struct
> > drm_i915_private *i915) {
> > if (DISPLAY_VER(i915) >= 11) {
> > + gen9_sanitize_dc_state(i915);
> > bxt_disable_dc9(i915);
> > icl_display_core_init(i915, true);
> > if (intel_dmc_has_payload(i915)) {
> > --
> > 2.25.1
> >
Re: [Intel-gfx] [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
> -Original Message-
> From: Vivi, Rodrigo
> Sent: Thursday, August 4, 2022 2:01 AM
> To: Tangudu, Tilak
> Cc: Ewins, Jon ; Belgaumkar, Vinay
> ; Roper, Matthew D
> ; Wilson, Chris P ;
> Nikula, Jani ; Gupta, saurabhg
> ; Gupta, Anshuman
> ; Nilawar, Badal ;
> Deak, Imre ; Iddamsetty, Aravind
> ; intel-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/8] drm/i915: Added is_intel_rpm_allowed helper
>
> On Thu, Jul 21, 2022 at 03:29:48PM +0530, tilak.tang...@intel.com wrote:
> > From: Tilak Tangudu
> >
> > 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
> > ---
> > 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
> >
Re: [Intel-gfx] [PATCH 6/8] drm/i915/rpm: d3cold Policy
> -Original Message- > From: Vivi, Rodrigo > Sent: Thursday, August 4, 2022 2:16 AM > To: Tangudu, Tilak > Cc: Ewins, Jon ; Belgaumkar, Vinay > ; Roper, Matthew D > ; Wilson, Chris P ; > Nikula, Jani ; Gupta, saurabhg > ; Gupta, Anshuman > ; Nilawar, Badal ; > Deak, Imre ; Iddamsetty, Aravind > ; intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH 6/8] drm/i915/rpm: d3cold Policy > > On Thu, Jul 21, 2022 at 03:29:53PM +0530, tilak.tang...@intel.com wrote: > > From: Tilak Tangudu > > This needs to be sorted out... or the patch split or use the Co-developed- > by:... Anshuman is sole author of the patch, I have modified a little in rev2 with a FIXME to fix the broken patch and to unblock my series and I missed to explicitly specify Anushman as author, Anshuman has a fix in rev3. I will specify him as author expliclty for next patches. No conflicts here. 😊 > > > > > Add d3cold_sr_lmem_threshold modparam to choose between d3cold-off > > zero watt and d3hot/d3cold-VRAM Self Refresh. > > i915 requires to evict the lmem objects to smem in order to support > > d3cold-Off. if platform does not supports vram_sr feature then fall > > back to d3hot by disabling d3cold to avoid the rpm suspend/resume > > latency. > > Extend the d3cold_sr_lmem_threshold modparam to debugfs i915_params > so > > that, it can be used by igt test. > > > > If gfx root port is not capable of sending PME from d3cold or doesn't > > have _PR3 power resources then only d3hot state can be supported. > > > > Adding intel_pm_prepare_targeted_d3_state() to choose the correct > > target d3 state and cache it to intel_runtime_pm structure, it can be > > used in rpm suspend/resume callback accordingly. > > > > v2: lmem->avail stopped tracking lmem usage since ttm is introduced, > > so removed lmem->avail usage in policy. > > FIXME here, lmem usage is not added, need to be added by using query > > functions. > > FIXME, Forcing the policy to enter D3COLD_OFF for validation purpose. > > > > Cc: Rodrigo Vivi > > Signed-off-by: Anshuman Gupta > > Signed-off-by: Tilak Tangudu > > --- > > drivers/gpu/drm/i915/i915_driver.c | 6 + > > drivers/gpu/drm/i915/i915_params.c | 5 > > drivers/gpu/drm/i915/i915_params.h | 1 + > > drivers/gpu/drm/i915/intel_pm.c | 35 + > > drivers/gpu/drm/i915/intel_pm.h | 1 + > > drivers/gpu/drm/i915/intel_runtime_pm.h | 7 + > > 6 files changed, 55 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_driver.c > > b/drivers/gpu/drm/i915/i915_driver.c > > index 4c36554567fd..2b2e9563f149 100644 > > --- a/drivers/gpu/drm/i915/i915_driver.c > > +++ b/drivers/gpu/drm/i915/i915_driver.c > > @@ -1581,6 +1581,12 @@ static int intel_runtime_idle(struct device *kdev) > > struct drm_i915_private *i915 = kdev_to_i915(kdev); > > int ret = 1; > > > > + disable_rpm_wakeref_asserts(&i915->runtime_pm); > > + ret = intel_pm_prepare_targeted_d3_state(i915); > > + if (!ret) > > + ret = 1; > > why? > > > + > > + enable_rpm_wakeref_asserts(&i915->runtime_pm); > > pm_runtime_mark_last_busy(kdev); > > pm_runtime_autosuspend(kdev); > > > > diff --git a/drivers/gpu/drm/i915/i915_params.c > > b/drivers/gpu/drm/i915/i915_params.c > > index 6fc475a5db61..4603f5c2ed77 100644 > > --- a/drivers/gpu/drm/i915/i915_params.c > > +++ b/drivers/gpu/drm/i915/i915_params.c > > @@ -197,6 +197,11 @@ i915_param_named(enable_gvt, bool, 0400, > > "Enable support for Intel GVT-g graphics virtualization host > > support(default:false)"); #endif > > > > +i915_param_named_unsafe(d3cold_sr_lmem_threshold, int, 0600, > > + "Enable VRAM Self refresh when size of lmem is greater to this > threshold. " > > + "If VRAM Self Refresh is not available then fall back to d3cold. " > > + "It helps to optimize the suspend/resume latecy. (default: 300mb)"); > > + > > #if CONFIG_DRM_I915_REQUEST_TIMEOUT > > i915_param_named_unsafe(request_timeout_ms, uint, 0600, > > "Default request/fence/batch buffer expiration > timeout."); diff > > --git a/drivers/gpu/drm/i915/i915_params.h > > b/drivers/gpu/drm/i915/i915_params.h > > index 2733cb6cfe09..1a86711038da 100644 > > --- a/drivers/gpu/drm/i915/i915_params.h > > +++ b/drivers/gpu/drm/i915/i915_params.h > > @@ -75,6 +75,7 @@ struct drm_printer; > > param(uns
Re: [Intel-gfx] [PATCH 6/8] drm/i915/rpm: d3cold Policy
> -Original Message- > From: Gupta, Anshuman > Sent: Thursday, July 21, 2022 5:00 PM > To: Tangudu, Tilak ; Ewins, Jon > ; Belgaumkar, Vinay ; > Roper, Matthew D ; Wilson, Chris P > ; Nikula, Jani ; Gupta, > saurabhg ; Vivi, Rodrigo > ; Nilawar, Badal ; Deak, > Imre ; Iddamsetty, Aravind > ; intel-gfx@lists.freedesktop.org > Subject: RE: [PATCH 6/8] drm/i915/rpm: d3cold Policy > > > > > -Original Message- > > From: Tangudu, Tilak > > Sent: Thursday, July 21, 2022 3:30 PM > > To: Ewins, Jon ; Belgaumkar, Vinay > > ; Roper, Matthew D > > ; Wilson, Chris P > > ; Nikula, Jani ; > > Gupta, saurabhg ; Vivi, Rodrigo > > ; Gupta, Anshuman > ; > > Nilawar, Badal ; Tangudu, Tilak > > ; Deak, Imre ; > > Iddamsetty, Aravind ; > > intel-gfx@lists.freedesktop.org > > Subject: [PATCH 6/8] drm/i915/rpm: d3cold Policy > > > > From: Tilak Tangudu > Please don't change the authorship of patch. I have not changed and at the same time I missed to add author explicitly I will make a note of it > > > > Add d3cold_sr_lmem_threshold modparam to choose between d3cold-off > > zero watt and d3hot/d3cold-VRAM Self Refresh. > > i915 requires to evict the lmem objects to smem in order to support d3cold- > Off. > > if platform does not supports vram_sr feature then fall back to d3hot > > by disabling d3cold to avoid the rpm suspend/resume latency. > > Extend the d3cold_sr_lmem_threshold modparam to debugfs i915_params > so > > that, it can be used by igt test. > > > > If gfx root port is not capable of sending PME from d3cold or doesn't > > have _PR3 power resources then only d3hot state can be supported. > > > > Adding intel_pm_prepare_targeted_d3_state() to choose the correct > > target d3 state and cache it to intel_runtime_pm structure, it can be > > used in rpm suspend/resume callback accordingly. > > > > v2: lmem->avail stopped tracking lmem usage since ttm is introduced, > > so removed lmem->avail usage in policy. > > FIXME here, lmem usage is not added, need to be added by using query > > functions. > > FIXME, Forcing the policy to enter D3COLD_OFF for validation purpose. > > > > Cc: Rodrigo Vivi > > Signed-off-by: Anshuman Gupta > > Signed-off-by: Tilak Tangudu > > --- > > drivers/gpu/drm/i915/i915_driver.c | 6 + > > drivers/gpu/drm/i915/i915_params.c | 5 > > drivers/gpu/drm/i915/i915_params.h | 1 + > > drivers/gpu/drm/i915/intel_pm.c | 35 + > > drivers/gpu/drm/i915/intel_pm.h | 1 + > > drivers/gpu/drm/i915/intel_runtime_pm.h | 7 + > > 6 files changed, 55 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_driver.c > > b/drivers/gpu/drm/i915/i915_driver.c > > index 4c36554567fd..2b2e9563f149 100644 > > --- a/drivers/gpu/drm/i915/i915_driver.c > > +++ b/drivers/gpu/drm/i915/i915_driver.c > > @@ -1581,6 +1581,12 @@ static int intel_runtime_idle(struct device *kdev) > > struct drm_i915_private *i915 = kdev_to_i915(kdev); > > int ret = 1; > > > > + disable_rpm_wakeref_asserts(&i915->runtime_pm); > > + ret = intel_pm_prepare_targeted_d3_state(i915); > > + if (!ret) > > + ret = 1; > > + > > + enable_rpm_wakeref_asserts(&i915->runtime_pm); > > pm_runtime_mark_last_busy(kdev); > > pm_runtime_autosuspend(kdev); > > > > diff --git a/drivers/gpu/drm/i915/i915_params.c > > b/drivers/gpu/drm/i915/i915_params.c > > index 6fc475a5db61..4603f5c2ed77 100644 > > --- a/drivers/gpu/drm/i915/i915_params.c > > +++ b/drivers/gpu/drm/i915/i915_params.c > > @@ -197,6 +197,11 @@ i915_param_named(enable_gvt, bool, 0400, > > "Enable support for Intel GVT-g graphics virtualization host > > support(default:false)"); #endif > > > > +i915_param_named_unsafe(d3cold_sr_lmem_threshold, int, 0600, > > + "Enable VRAM Self refresh when size of lmem is greater to this > > threshold. " > > + "If VRAM Self Refresh is not available then fall back to d3cold. " > > + "It helps to optimize the suspend/resume latecy. (default: 300mb)"); > > + > > #if CONFIG_DRM_I915_REQUEST_TIMEOUT > > i915_param_named_unsafe(request_timeout_ms, uint, 0600, > > "Default request/fence/batch buffer expiration > timeout."); diff > > --git a/drivers/gpu/drm/i915/i915_params.h > > b/drivers/gpu/drm/i915/i915_params.h > > index 2733cb6cfe09..1a86711
Re: [Intel-gfx] [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed helper
> -Original Message-
> From: Vivi, Rodrigo
> Sent: Thursday, June 23, 2022 2:11 AM
> To: Jani Nikula
> Cc: Tangudu, Tilak ; Gupta, Anshuman
> ; intel-gfx@lists.freedesktop.org; Ewins, Jon
> ; Belgaumkar, Vinay ;
> Wilson, Chris P ; Dixit, Ashutosh
> ; Nilawar, Badal ;
> Roper, Matthew D ; Gupta, saurabhg
> ; Iddamsetty, Aravind
> ; Sundaresan, Sujaritha
> ; Deak, Imre
> Subject: Re: [Intel-gfx] [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" wrote:
> > >> -Original Message-
> > >> From: Gupta, Anshuman
> > >> Sent: Tuesday, June 21, 2022 7:47 PM
> > >> To: Tangudu, Tilak ;
> > >> intel-gfx@lists.freedesktop.org; Ewins, Jon ;
> > >> Vivi, Rodrigo ; Belgaumkar, Vinay
> > >> ; Wilson, Chris P
> > >> ; Dixit, Ashutosh
> > >> ; Nilawar, Badal
> > >> ; Roper, Matthew D
> > >> ; Gupta, saurabhg
> > >> ; Iddamsetty, Aravind
> > >> ; Sundaresan, Sujaritha
> > >>
> > >> Subject: RE: [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed
> > >> helper
> > >>
> > >>
> > >>
> > >> > -Original Message-----
> > >> > From: Tangudu, Tilak
> > >> > Sent: Tuesday, June 21, 2022 6:05 PM
> > >> > To: intel-gfx@lists.freedesktop.org; Ewins, Jon
> > >> > ; Vivi, Rodrigo ;
> > >> > Belgaumkar, Vinay ; Wilson, Chris P
> > >> > ; Dixit, Ashutosh
> > >> > ; Nilawar, Badal
> > >> > ; Gupta, Anshuman
> > >> > ; Tangudu, Tilak
> > >> > ; Roper, Matthew D
> > >> > ; Gupta, saurabhg
> > >> > ; Iddamsetty, Aravind
> > >> > ; Sundaresan, Sujaritha
> > >> >
> > >> > 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
Re: [Intel-gfx] [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed helper
> -Original Message-
> From: Gupta, Anshuman
> Sent: Tuesday, June 21, 2022 7:47 PM
> To: Tangudu, Tilak ; intel-gfx@lists.freedesktop.org;
> Ewins, Jon ; Vivi, Rodrigo ;
> Belgaumkar, Vinay ; Wilson, Chris P
> ; Dixit, Ashutosh ;
> Nilawar, Badal ; Roper, Matthew D
> ; Gupta, saurabhg
> ; Iddamsetty, Aravind
> ; Sundaresan, Sujaritha
>
> Subject: RE: [PATCH 04/11] drm/i915: Added is_intel_rpm_allowed helper
>
>
>
> > -Original Message-
> > From: Tangudu, Tilak
> > Sent: Tuesday, June 21, 2022 6:05 PM
> > To: intel-gfx@lists.freedesktop.org; Ewins, Jon ;
> > Vivi, Rodrigo ; Belgaumkar, Vinay
> > ; Wilson, Chris P
> > ; Dixit, Ashutosh
> > ; Nilawar, Badal ;
> > Gupta, Anshuman ; Tangudu, Tilak
> > ; Roper, Matthew D
> > ; Gupta, saurabhg
> > ; Iddamsetty, Aravind
> > ; Sundaresan, Sujaritha
> >
> > 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 ?
> >
> > Signed-off-by: Tilak Tangudu
> > ---
> > 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
>
