[Intel-gfx] [PATCH 4/6] drm/i915: Skip CS verification of L3 bank registers

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-07-17 19:06:22)
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Access to 0xb100 - 0xb3ff mmio range is controlled by the MCR selector
> which only affects CPU MMIO. Therefore these registers cannot be realiably
> read with MI_SRM from the command streamer so skip their verification.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_workarounds.c | 38 ++++++++++++++++++---
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index c2325b7ecf8d..619d42a2b81b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1436,26 +1436,50 @@ create_scratch(struct i915_address_space *vm, int count)
>         return ERR_PTR(err);
>  }
>  
> +static bool mcr_range(struct drm_i915_private *i915, u32 offset)
> +{
> +       /*
> +        * Registers in this range are affected by the MCR selector
> +        * which only controls CPU initiated MMIO. Routing does not
> +        * work for CS access so we cannot verify them on this path.
> +        */
> +       if (INTEL_GEN(i915) >= 8 && (offset >= 0xb100 && offset <= 0xb3ff))
> +               return true;

Bonus (). I was thinking maybe
	if (INTEL_GEN() < 8)
		return false;
	return offset >= 0xb100 && offset <= 0xb3ff;
to remove the apparent need. But structuring the checks with a
per-gen if-ladder will probably be easier to extend should the need
arise.

> +       return false;
> +}
> +
>  static int
>  wa_list_srm(struct i915_request *rq,
>             const struct i915_wa_list *wal,
>             struct i915_vma *vma)
>  {
> +       struct drm_i915_private *i915 = rq->i915;
> +       unsigned int i, count = 0;
>         const struct i915_wa *wa;
> -       unsigned int i;
>         u32 srm, *cs;
>  
>         srm = MI_STORE_REGISTER_MEM | MI_SRM_LRM_GLOBAL_GTT;
> -       if (INTEL_GEN(rq->i915) >= 8)
> +       if (INTEL_GEN(i915) >= 8)
>                 srm++;
>  
> -       cs = intel_ring_begin(rq, 4 * wal->count);
> +       for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> +               if (!mcr_range(i915, i915_mmio_reg_offset(wa->reg)))
> +                       count++;
> +       }
> +
> +       cs = intel_ring_begin(rq, 4 * count);
>         if (IS_ERR(cs))
>                 return PTR_ERR(cs);
>  
>         for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> +               u32 offset = i915_mmio_reg_offset(wa->reg);
> +
> +               if (mcr_range(i915, offset))
> +                       continue;
> +

I would have just done the reg read and ignored the result, rather than
add another loop to fixup the count.

>                 *cs++ = srm;
> -               *cs++ = i915_mmio_reg_offset(wa->reg);
> +               *cs++ = offset;
>                 *cs++ = i915_ggtt_offset(vma) + sizeof(u32) * i;
>                 *cs++ = 0;
>         }
> @@ -1505,9 +1529,13 @@ static int engine_wa_list_verify(struct intel_context *ce,
>         }
>  
>         err = 0;
> -       for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> +       for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> +               if (mcr_range(rq->i915, i915_mmio_reg_offset(wa->reg)))
> +                       continue;
> +
>                 if (!wa_verify(wa, results[i], wal->name, from))
>                         err = -ENXIO;
> +       }

Looks fine though
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris