Re: [PATCH 16/31] drm/i915/slpc: Lay out SLPC init/enable/disable/cleanup helpers

[Sagar Arun Kamble <sagar.a.kamble@intel.com>]

On 9/21/2017 6:30 PM, Michal Wajdeczko wrote:
> On Tue, 19 Sep 2017 19:41:52 +0200, Sagar Arun Kamble 
> <sagar.a.kamble@intel.com> wrote:
>
>> SLPC operates based on parameters setup in shared data between
>> i915 and GuC SLPC. This is to be created/initialized in intel_slpc_init.
>> From there onwards i915 can control the SLPC operations by Enabling,
>> Disabling complete SLPC or changing SLPC parameters. During cleanup
>> SLPC shared data has to be freed.
>> With this patch on platforms with SLPC support we call intel_slpc_*()
>> functions from GuC setup functions and do not use Host rps functions.
>> With SLPC, intel_enable_gt_powersave will only handle RC6. In the later
>> patch intel_init_gt_powersave will check if SLPC has started running
>> through shared data and update initial state that i915 needs like
>> frequency limits if needed.
>>
>> 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.
>>
>> v7: Makefile rearrangement. Reducing usage of i915.enable_slpc and 
>> relying
>>     mostly on rps.rps_enabled to bypass Host RPS flows. Commit message
>>     update.
>>
>> v8: Changed parameters for SLPC functions to struct intel_slpc*.
>>
>> v9: Reinstated intel_slpc_active and intel_slpc_enabled as they are more
>>     meaningful.
>>
>> v10: Rebase changes due to creation of intel_guc.h. Updates in
>>      intel_guc_cleanup w.r.t slpc cleanup.
>>
>> Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> ---
>>  drivers/gpu/drm/i915/Makefile     |  1 +
>>  drivers/gpu/drm/i915/i915_drv.h   | 12 ++++++++++
>>  drivers/gpu/drm/i915/intel_guc.c  |  3 +++
>>  drivers/gpu/drm/i915/intel_guc.h  |  3 +++
>
> Hmm, this looks like cross dependency on other pending series
Yes. Will make this independent series.
>
>>  drivers/gpu/drm/i915/intel_pm.c   | 19 +++++++++++-----
>>  drivers/gpu/drm/i915/intel_slpc.c | 42
>
> It looks that SLPC is tight with Guc so maybe better names would be:
>
>     intel_guc_slpc.c and struct intel_guc_slpc
>
> ie. the same pattern as with Guc log.
Sure. Will update.
>
>
>> ++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_slpc.h | 47 
>> +++++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_uc.c   | 20 +++++++++++++++++
>>  8 files changed, 142 insertions(+), 5 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 d1327f6..62bf4f6e 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -64,6 +64,7 @@ i915-y += intel_uc.o \
>>        intel_guc_log.o \
>>        intel_guc_loader.o \
>>        intel_huc.o \
>> +      intel_slpc.o \
>>        i915_guc_submission.o
>> # autogenerated null render state
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 428cb1c..af633c6 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2748,6 +2748,18 @@ static inline struct drm_i915_private 
>> *guc_to_i915(struct intel_guc *guc)
>>      return container_of(guc, struct drm_i915_private, guc);
>>  }
>> +static inline struct intel_guc *slpc_to_guc(struct intel_slpc *slpc)
>> +{
>> +    return container_of(slpc, struct intel_guc, slpc);
>> +}
>> +
>> +static inline struct drm_i915_private *slpc_to_i915(struct 
>> intel_slpc *slpc)
>> +{
>> +    struct intel_guc *guc = slpc_to_guc(slpc);
>> +
>> +    return guc_to_i915(guc);
>> +}
>> +
>>  static inline struct drm_i915_private *huc_to_i915(struct intel_huc 
>> *huc)
>>  {
>>      return container_of(huc, struct drm_i915_private, huc);
>> diff --git a/drivers/gpu/drm/i915/intel_guc.c 
>> b/drivers/gpu/drm/i915/intel_guc.c
>> index f4dc708..a92c7e8 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.c
>> +++ b/drivers/gpu/drm/i915/intel_guc.c
>> @@ -226,6 +226,9 @@ void intel_guc_cleanup(struct intel_guc *guc)
>>     if (i915.enable_guc_submission)
>>          i915_guc_submission_cleanup(dev_priv);
>> +
>> +    if (intel_slpc_enabled())
>> +        intel_slpc_cleanup(&guc->slpc);
>>  }
>> /**
>> diff --git a/drivers/gpu/drm/i915/intel_guc.h 
>> b/drivers/gpu/drm/i915/intel_guc.h
>> index 3821bf2..b835d30 100644
>> --- a/drivers/gpu/drm/i915/intel_guc.h
>> +++ b/drivers/gpu/drm/i915/intel_guc.h
>> @@ -28,6 +28,7 @@
>>  #include "intel_guc_fwif.h"
>>  #include "i915_guc_reg.h"
>>  #include "intel_guc_ct.h"
>> +#include "intel_slpc.h"
>> /*
>>   * This structure primarily describes the GEM object shared with the 
>> GuC.
>> @@ -115,6 +116,8 @@ struct intel_guc {
>>          enum forcewake_domains fw_domains;
>>      } send_regs;
>> +    struct intel_slpc slpc;
>> +
>
> Pick better place - now you're inside 'send' related members.
Ok. Will update.
>
>>      /* To serialize the intel_guc_send actions */
>>      struct mutex send_mutex;
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 20ec8f4..e5607e5 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -7995,6 +7995,12 @@ void intel_suspend_gt_powersave(struct 
>> drm_i915_private *dev_priv)
>>          intel_runtime_pm_put(dev_priv);
>>     /* gen6_rps_idle() will be called later to disable interrupts */
>> +
>> +    if (intel_slpc_active(&dev_priv->guc.slpc)) {
>> +        intel_runtime_pm_get(dev_priv);
>> +        intel_slpc_disable(&dev_priv->guc.slpc);
>> +        intel_runtime_pm_put(dev_priv);
>> +    }
>>  }
>> void intel_sanitize_gt_powersave(struct drm_i915_private *dev_priv)
>> @@ -8121,7 +8127,8 @@ void intel_enable_gt_powersave(struct 
>> drm_i915_private *dev_priv)
>>      mutex_lock(&dev_priv->pm.pcu_lock);
>>     intel_enable_rc6(dev_priv);
>> -    intel_enable_rps(dev_priv);
>> +    if (!intel_slpc_active(&dev_priv->guc.slpc))
>> +        intel_enable_rps(dev_priv);
>>      intel_update_ring_freq(dev_priv);
>>     mutex_unlock(&dev_priv->pm.pcu_lock);
>> @@ -8134,8 +8141,9 @@ static void 
>> __intel_autoenable_gt_powersave(struct work_struct *work)
>>      struct intel_engine_cs *rcs;
>>      struct drm_i915_gem_request *req;
>> -    if (READ_ONCE(dev_priv->pm.rps.enabled) &&
>> -        READ_ONCE(dev_priv->pm.rc6.enabled) &&
>> +    if (READ_ONCE(dev_priv->pm.rc6.enabled) &&
>> +        !(!(intel_slpc_active(&dev_priv->guc.slpc)) ^
>> +          READ_ONCE(dev_priv->pm.rps.enabled)) &&
>>          !(NEEDS_RING_FREQ_UPDATE(dev_priv) ^
>>            READ_ONCE(dev_priv->pm.ring_pstate.configured)))
>>          goto out;
>> @@ -8167,8 +8175,9 @@ static void 
>> __intel_autoenable_gt_powersave(struct work_struct *work)
>> void intel_autoenable_gt_powersave(struct drm_i915_private *dev_priv)
>>  {
>> -    if (READ_ONCE(dev_priv->pm.rps.enabled) &&
>> -        READ_ONCE(dev_priv->pm.rc6.enabled) &&
>> +    if (READ_ONCE(dev_priv->pm.rc6.enabled) &&
>> +        !(!(intel_slpc_active(&dev_priv->guc.slpc)) ^
>> +          READ_ONCE(dev_priv->pm.rps.enabled)) &&
>>          !(NEEDS_RING_FREQ_UPDATE(dev_priv) ^
>>            READ_ONCE(dev_priv->pm.ring_pstate.configured)))
>>          return;
>> diff --git a/drivers/gpu/drm/i915/intel_slpc.c 
>> b/drivers/gpu/drm/i915/intel_slpc.c
>> new file mode 100644
>> index 0000000..06abda5
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_slpc.c
>> @@ -0,0 +1,42 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom 
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including 
>> the next
>> + * paragraph) shall be included in all copies or substantial 
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +#include <linux/firmware.h>
>> +#include "i915_drv.h"
>> +#include "intel_uc.h"
>> +
>> +void intel_slpc_init(struct intel_slpc *slpc)
>> +{
>> +}
>> +
>> +void intel_slpc_cleanup(struct intel_slpc *slpc)
>> +{
>> +}
>> +
>> +void intel_slpc_enable(struct intel_slpc *slpc)
>> +{
>> +}
>> +
>> +void intel_slpc_disable(struct intel_slpc *slpc)
>> +{
>> +}
>> diff --git a/drivers/gpu/drm/i915/intel_slpc.h 
>> b/drivers/gpu/drm/i915/intel_slpc.h
>> new file mode 100644
>> index 0000000..f68671f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_slpc.h
>> @@ -0,0 +1,47 @@
>> +/*
>> + * Copyright © 2017 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom 
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including 
>> the next
>> + * paragraph) shall be included in all copies or substantial 
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
>> EVENT SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + */
>> +#ifndef _INTEL_SLPC_H_
>> +#define _INTEL_SLPC_H_
>> +
>> +struct intel_slpc {
>> +    bool active;
>> +};
>> +
>> +static inline int intel_slpc_enabled(void)
>> +{
>> +    return i915.enable_slpc;
>> +}
>> +
>> +static inline bool intel_slpc_active(struct intel_slpc *slpc)
>> +{
>> +    return slpc->active;
>> +}
>> +
>> +/* intel_slpc.c */
>> +void intel_slpc_init(struct intel_slpc *slpc);
>> +void intel_slpc_cleanup(struct intel_slpc *slpc);
>> +void intel_slpc_enable(struct intel_slpc *slpc);
>> +void intel_slpc_disable(struct intel_slpc *slpc);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 350027f..990d84a 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -328,6 +328,9 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>          ret = i915_guc_submission_init(dev_priv);
>>          if (ret)
>>              goto err_guc;
>> +
>> +        if (intel_slpc_enabled())
>> +            intel_slpc_init(&dev_priv->guc.slpc);
>>      }
>>     /* init WOPCM */
>> @@ -369,6 +372,17 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>          goto err_log_capture;
>>     intel_huc_auth(&dev_priv->huc);
>> +
>> +    /*
>> +     * SLPC is enabled by setting up the shared data structure and
>> +     * sending reset event to GuC SLPC. Initial data is setup in
>> +     * intel_slpc_init. Here we send the reset event. SLPC enabling
>> +     * in GuC can happen in parallel in GuC with other initialization
>> +     * being done in i915.
>> +     */
>> +    if (intel_slpc_enabled())
>> +        intel_slpc_enable(&dev_priv->guc.slpc);
>> +
>>      if (i915.enable_guc_submission) {
>>          if (i915.guc_log_level >= 0)
>>              gen9_enable_guc_interrupts(dev_priv);
>> @@ -398,6 +412,12 @@ int intel_uc_init_hw(struct drm_i915_private 
>> *dev_priv)
>>      if (i915.enable_guc_submission)
>>          i915_guc_submission_cleanup(dev_priv);
>>  err_guc:
>> +    if (intel_slpc_enabled()) {
>> +        if (intel_slpc_active(&dev_priv->guc.slpc))
>> +            intel_slpc_disable(&dev_priv->guc.slpc);
>> +        intel_slpc_cleanup(&dev_priv->guc.slpc);
>> +    }
>> +
>>      i915_ggtt_disable_guc(dev_priv);
>>     DRM_ERROR("GuC init failed\n");

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx