Re: [PATCH 3/4] drm/i915: Add whitelist workarounds for CFL

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 14/06/2019 01:28, Robert M. Fosha wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Updated whitelist table for CFL.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Robert M. Fosha <robert.m.fosha@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/gt/intel_workarounds.c | 35 ++++++++++++++++++++-
>   1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index 5308a0864e78..60bd515edaf1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1092,10 +1092,43 @@ static void glk_whitelist_build(struct intel_engine_cs *engine)
>   
>   static void cfl_whitelist_build(struct intel_engine_cs *engine)
>   {
> +	struct i915_wa_list *w = &engine->whitelist;
> +
>   	if (engine->class != RENDER_CLASS)
>   		return;
>   
> -	gen9_whitelist_build(&engine->whitelist);
> +	/* Whitelist entries from BSpec page 53688: */
> +	gen9_whitelist_build(w);
> +
> +	/* WaSendPushConstantsFromMMIO:cfl */
> +	whitelist_reg_ext(w, COMMON_SLICE_CHICKEN2,
> +			  RING_FORCE_TO_NONPRIV_RW);
> +
> +	/* GEN9_CS_DEBUG_MODE1 */
> +	whitelist_reg_ext(w, GEN9_CS_DEBUG_MODE1,
> +				  RING_FORCE_TO_NONPRIV_RW);
> +
> +	/* WaAllowUmdWriteTRTTRootTable:cfl */
> +	whitelist_reg_ext(w, _MMIO(0x4DE0), RING_FORCE_TO_NONPRIV_RW);
> +	whitelist_reg_ext(w, _MMIO(0x4DE4), RING_FORCE_TO_NONPRIV_RW);
> +
> +	/* WaAllowUMDToDisableVFAutoStrip:cfl */
> +	whitelist_reg_ext(w, _MMIO(0x83A8), RING_FORCE_TO_NONPRIV_RW);
> +
> +	/* WaAllowPMDepthAndInvocationCountAccessFromUMD:cfl
> +	 * NB: this also pulls in CL_PRIMITIVES_COUNT in same block
> +	 * as PS_INVOCATIONS_COUNT. Likewise, TIMESTAMP in the
> +	 * PS_DEPTH_COUNT block.
> +	 */

Nitpick but please use multi-line comments in style:

/*
  *
  */

We are trying not to add more of the unpopular style.

> +	whitelist_reg_ext(w, CL_PRIMITIVES_COUNT,
> +			  RING_FORCE_TO_NONPRIV_RW |
> +			  RING_FORCE_TO_NONPRIV_RANGE_4);
> +	whitelist_reg_ext(w, PS_DEPTH_COUNT, RING_FORCE_TO_NONPRIV_RW |
> +			  RING_FORCE_TO_NONPRIV_RANGE_4);
> +
> +	/* WaAllowUMDAccesstoOARegisters:cfl */
> +	whitelist_reg_ext(w, _MMIO(0x28A0), RING_FORCE_TO_NONPRIV_RW);
> +	whitelist_reg_ext(w, OAREPORTTRIG6, RING_FORCE_TO_NONPRIV_RW);

AFAICS majority of the register here shouldn't use the whitelist_reg_ext 
but normal whitelist_reg. It will help with readability a bit and the 
special ones would stick out more.

>   }
>   
>   static void cnl_whitelist_build(struct intel_engine_cs *engine)
> 

With the two tweaks:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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