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

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

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@xxxxxxxxx> 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@xxxxxxxxx>
> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> > ---
> >  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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx