Re: [Intel-gfx] [PATCH] drm/i915/tgl: Fix Media power gate sequence.

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Rodrigo Vivi (2020-11-11 14:09:36)
> Some media power gates are disabled by default. commit 5d86923060fc
> ("drm/i915/tgl: Enable VD HCP/MFX sub-pipe power gating")
> tried to enable it, but it duplicated an existent register.
> So, the main PG setup sequences ended up overwriting it.
> 
> So, let's now merge this to the main PG setup sequence.
> 
> v2: (Chris): s/REG_BIT/BIT, remove useless comment,
>              remove useless =0, use the right gt,
>              remove rc6 sequence doubt from commit message.
> 
> Fixes: 5d86923060fc ("drm/i915/tgl: Enable VD HCP/MFX sub-pipe power gating")
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: stable@vger.kernel.org#v5.5+
> Cc: Dale B Stimson <dale.b.stimson@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/intel_rc6.c | 19 +++++++++++++++----
>  drivers/gpu/drm/i915/i915_reg.h     | 12 +++++-------
>  drivers/gpu/drm/i915/intel_pm.c     | 16 ----------------
>  3 files changed, 20 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_rc6.c b/drivers/gpu/drm/i915/gt/intel_rc6.c
> index ab675d35030d..c01fa86e0d07 100644
> --- a/drivers/gpu/drm/i915/gt/intel_rc6.c
> +++ b/drivers/gpu/drm/i915/gt/intel_rc6.c
> @@ -56,9 +56,12 @@ static inline void set(struct intel_uncore *uncore, i915_reg_t reg, u32 val)
>  
>  static void gen11_rc6_enable(struct intel_rc6 *rc6)
>  {
> +       struct drm_i915_private *i915 = rc6_to_i915(rc6);
>         struct intel_uncore *uncore = rc6_to_uncore(rc6);
>         struct intel_engine_cs *engine;
>         enum intel_engine_id id;
> +       u32 pg_enable;
> +       int i;
>  
>         /* 2b: Program RC6 thresholds.*/
>         set(uncore, GEN6_RC6_WAKE_RATE_LIMIT, 54 << 16 | 85);
> @@ -102,10 +105,18 @@ static void gen11_rc6_enable(struct intel_rc6 *rc6)
>                 GEN6_RC_CTL_RC6_ENABLE |
>                 GEN6_RC_CTL_EI_MODE(1);
>  
> -       set(uncore, GEN9_PG_ENABLE,
> -           GEN9_RENDER_PG_ENABLE |
> -           GEN9_MEDIA_PG_ENABLE |
> -           GEN11_MEDIA_SAMPLER_PG_ENABLE);
> +       pg_enable = GEN9_RENDER_PG_ENABLE |
> +               GEN9_MEDIA_PG_ENABLE |
> +               GEN11_MEDIA_SAMPLER_PG_ENABLE;
> +
> +       if (INTEL_GEN(i915) >= 12) {
> +               for (i = 0; i < I915_MAX_VCS; i++)
> +                       if (HAS_ENGINE(rc6_to_gt(rc6), _VCS(i)))

I would have put gt = rc6_to_gt(rc6) into the locals and used gt->i915
for the singular instance.

Code motion looks ok,

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

Now as to why this escaped notice? I see that our rc6/power measurement test
is not reporting energy consumption for tgl. This is concerning, give me
a moment to see if there's anything obvious blocking the test.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx