Re: [PATCH] drm/i915/execlists: Add a comment for the extra MI_ARB_ENABLE

[Michel Thierry <michel.thierry@intel.com>]

On 10/5/2017 12:10 PM, Chris Wilson wrote:
> Michel Thierry noticed that we were applying WaDisableCtxRestoreArbitration
> even to gen9, which does not require the w/a. The rationale is that we
> need to enable MI arbitration for execlists to work, and to be safe we
> do that before every batch (in addition to every context switch into the
> batch). Since this is not clear from the single line comment suggesting
> the MI_ARB_ENABLE is solely for the w/a, add a little more detail.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>

It can't be clearer. Thanks!

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 476881f63505..fa66e4f58642 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1608,7 +1608,23 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
>   	if (IS_ERR(cs))
>   		return PTR_ERR(cs);
>   
> -	/* WaDisableCtxRestoreArbitration:bdw,chv */
> +	/*
> +	 * WaDisableCtxRestoreArbitration:bdw,chv
> +	 *
> +	 * We don't need to perform MI_ARB_ENABLE as often as we do (in
> +	 * particular all the gen that do not need the w/a at all!), if we
> +	 * took care to make sure that on every switch into this context
> +	 * (both ordinary and for preemption) that arbitrartion was enabled
> +	 * we would be fine. However, there doesn't seem to be a downside to
> +	 * being paranoid and making sure it is set before each batch and
> +	 * every context-switch.
> +	 *
> +	 * Note that if we fail to enable arbitration before the request
> +	 * is complete, then we do not see the context-switch interrupt and
> +	 * the engine hangs (with RING_HEAD == RING_TAIL).
> +	 *
> +	 * That satisfies both the GPGPU w/a and our heavy-handed paranoia.
> +	 */
>   	*cs++ = MI_ARB_ON_OFF | MI_ARB_ENABLE;
>   
>   	/* FIXME(BDW): Address space and security selectors. */
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx