[Intel-gfx] [PATCH v6 06/23] drm/i915/slpc: Use intel_slpc_* functions if supported
Kamble, Sagar A
sagar.a.kamble at intel.com
Fri Mar 17 09:58:17 UTC 2017
On 3/17/2017 2:35 AM, Chris Wilson wrote:
> On Thu, Mar 16, 2017 at 11:58:10PM +0530, Sagar Arun Kamble wrote:
>> On platforms with SLPC support: call intel_slpc_*() functions from
>> intel_*_gt_powersave() functions and GuC setup functions and do not
>> use rps functions. intel_slpc_enable is tied to GuC setup.
>> With SLPC, intel_enable_gt_powersave will only handle RC6 and ring
>> frequencies. intel_init_gt_powersave will check if SLPC has started
>> running through shared data and update initial state that i915 needs
>> like frequency limits.
>> Host will not manage GT frequency with this change.
>>
>> v1: Return void instead of ignored error code (Paulo)
>> enable/disable RC6 in SLPC flows (Sagar)
>> replace HAS_SLPC() use with intel_slpc_enabled()
>> or intel_slpc_active() (Paulo)
>> Fix for renaming gen9_disable_rps to gen9_disable_rc6 in
>> "drm/i915/bxt: Explicitly clear the Turbo control register"
>> Defer RC6 and SLPC enabling to intel_gen6_powersave_work. (Sagar)
>> Performance drop with SLPC was happening as ring frequency table
>> was not programmed when SLPC was enabled. This patch programs ring
>> frequency table with SLPC. Initial reset of SLPC is based on kernel
>> parameter as planning to add slpc state in intel_slpc_active. Cleanup
>> is also based on kernel parameter as SLPC gets disabled in
>> disable/suspend.(Sagar)
>>
>> v2: Usage of INTEL_GEN instead of INTEL_INFO->gen (David)
>> Checkpatch update.
>>
>> v3: Rebase
>>
>> v4: Removed reset functions to comply with *_gt_powersave routines.
>> (Sagar)
>>
>> v5: Removed intel_slpc_active. Relying on slpc.active for control flows
>> that are based on SLPC active status in GuC. State setup/cleanup needed
>> for SLPC is handled using kernel parameter i915.enable_slpc. Moved SLPC
>> init and enabling to GuC enable path as SLPC in GuC can start doing the
>> setup post GuC init. Commit message update. (Sagar)
>>
>> v6: Rearranged function definitions.
>>
>> Signed-off-by: Tom O'Rourke <Tom.O'Rourke at intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> ---
>> drivers/gpu/drm/i915/Makefile | 3 +-
>> drivers/gpu/drm/i915/i915_drv.c | 2 ++
>> drivers/gpu/drm/i915/i915_gem.c | 8 +++++
>> drivers/gpu/drm/i915/i915_guc_submission.c | 3 ++
>> drivers/gpu/drm/i915/intel_pm.c | 51 ++++++++++++++++++++++++------
>> drivers/gpu/drm/i915/intel_slpc.c | 46 +++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_slpc.h | 38 ++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_uc.c | 11 +++++++
>> drivers/gpu/drm/i915/intel_uc.h | 3 ++
>> 9 files changed, 154 insertions(+), 11 deletions(-)
>> create mode 100644 drivers/gpu/drm/i915/intel_slpc.c
>> create mode 100644 drivers/gpu/drm/i915/intel_slpc.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index 2cf0450..a4a8e0b 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -60,7 +60,8 @@ i915-y += intel_uc.o \
>> intel_guc_log.o \
>> intel_guc_loader.o \
>> intel_huc.o \
>> - i915_guc_submission.o
>> + i915_guc_submission.o \
>> + intel_slpc.o
> Something here is not in alphabetical order.
Yes. Will fix this.
>
>>
>> # autogenerated null render state
>> i915-y += intel_renderstate_gen6.o \
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 0302d24..c0eb3d2 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1762,6 +1762,8 @@ static int i915_drm_resume_early(struct drm_device *dev)
>> hsw_disable_pc8(dev_priv);
>> }
>>
>> + dev_priv->guc.slpc.active = false;
>> +
>> intel_uncore_sanitize(dev_priv);
>>
>> if (IS_GEN9_LP(dev_priv) ||
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index d87983b..db55285 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -2900,6 +2900,14 @@ void i915_gem_reset(struct drm_i915_private *dev_priv)
>>
>> i915_gem_restore_fences(dev_priv);
>>
>> + /*
>> + * GPU is reset at this point, Hence mark SLPC as inactive to
> First sentence is redundant.
Will fix this.
>
>> + * not send h2g action to shutdown SLPC as that will fail.
>> + * enable_gt_powersave will setup RC6 and ring frequencies and
>> + * SLPC will be enabled post GuC initialization.
>> + */
>> + dev_priv->guc.slpc.active = false;
> It suggests we should be hooking guc into the reset prepare/do/finish
> more thoroughly.
Yes. Might have to rework w.r.t these flows. Will update.
>
>> +
>> if (dev_priv->gt.awake) {
>> intel_sanitize_gt_powersave(dev_priv);
>> intel_enable_gt_powersave(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index 5ec2cbd..1c9f859 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -902,6 +902,9 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>> intel_guc_log_create(guc);
>> guc_addon_create(guc);
>>
>> + if (i915.enable_slpc)
>> + intel_slpc_init(dev_priv);
> This is not a hotpath, move the test to the callee.
Ok. Will update.
>
>> guc->execbuf_client = guc_client_alloc(dev_priv,
>> INTEL_INFO(dev_priv)->ring_mask,
>> GUC_CTX_PRIORITY_KMD_NORMAL,
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 8e41596..9c47d65 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -5283,6 +5283,9 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
>>
>> void gen6_rps_busy(struct drm_i915_private *dev_priv)
>> {
>> + if (i915.enable_slpc)
>> + return;
> Feels very wrong. Anticipating this should be more akin to disabling at
> the intel_set_rps() (or a nop callback), or just using rps.enabled to
> skip.
Will make this return based on rps.enabled.
>
>> void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv)
>> {
>> - dev_priv->rps.enabled = true; /* force disabling */
>> + if (!i915.enable_slpc)
>> + dev_priv->rps.enabled = true; /* force disabling */
> Nope. Please consider the point of this function to ensure that whatever
> we inherit from the BIOS is cleared.
Forceful disabling here clears the RP_CONTROL register which controls the SLPC frequency requests.
Would need to preserve the BIOS set value so will copy prior to clearing here and set again when enabling SLPC.
>
>> dev_priv->rps.rc6_enabled = true;
>> +
>> intel_disable_gt_powersave(dev_priv);
>>
>> - gen6_reset_rps_interrupts(dev_priv);
>> + if (!i915.enable_slpc)
>> + gen6_reset_rps_interrupts(dev_priv);
>> }
>>
>> void intel_disable_gt_powersave(struct drm_i915_private *dev_priv)
>> {
>> - if (!READ_ONCE(dev_priv->rps.enabled)) {
> So what is preventing us from just not marking rps.enabled as enabled
> under slpc?
>
> In all, i915.enable_slpc should be rare.
> -Chris
RP_CONTROL clearing was the issue. Will update to rely on rps.enabled as much as possible.
Will hopefully be able to reduce the i915.enable_slpc occurrences.
Thanks for the review.
Thanks
Sagar
More information about the Intel-gfx
mailing list