Re: [PATCH 2/2] drm/i915/lrc: Prevent preemption when lite-restore is disabled

[Arun Siluvery <arun.siluvery@linux.intel.com>]

On 04/09/2015 12:59, Michel Thierry wrote:
> When WaEnableForceRestoreInCtxtDescForVCS is required, it is only
> safe to send new contexts if the last reported event is "active to
> idle". Otherwise the same context can fully preempt itself because
> lite-restore is disabled.
>
> Testcase: igt/gem_concurrent_blit
> Reported-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Signed-off-by: Michel Thierry <michel.thierry@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lrc.c | 24 ++++++++++++++++++------
>   1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d8b605f..c3fca4b 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -277,10 +277,18 @@ u32 intel_execlists_ctx_id(struct drm_i915_gem_object *ctx_obj)
>   	return lrca >> 12;
>   }
>
> +static bool disable_lite_restore_wa(struct intel_engine_cs *ring)
> +{
> +	struct drm_device *dev = ring->dev;
> +
> +	return ((IS_SKYLAKE(dev) && INTEL_REVID(dev) <= SKL_REVID_B0) ||
> +		(IS_BROXTON(dev) && INTEL_REVID(dev) == BXT_REVID_A0)) &&
> +	       (ring->id == VCS || ring->id == VCS2);
> +}
> +
>   uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
>   				     struct intel_engine_cs *ring)
>   {
> -	struct drm_device *dev = ring->dev;
>   	struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state;
>   	uint64_t desc;
>   	uint64_t lrca = i915_gem_obj_ggtt_offset(ctx_obj) +
> @@ -302,9 +310,7 @@ uint64_t intel_lr_context_descriptor(struct intel_context *ctx,
>
>   	/* WaEnableForceRestoreInCtxtDescForVCS:skl */
>   	/* WaEnableForceRestoreInCtxtDescForVCS:bxt */
> -	if (((IS_SKYLAKE(dev) && INTEL_REVID(dev) <= SKL_REVID_B0) ||
> -	     (IS_BROXTON(dev) && INTEL_REVID(dev) == BXT_REVID_A0)) &&
> -	    (ring->id == VCS || ring->id == VCS2))
> +	if (disable_lite_restore_wa(ring))
>   		desc |= GEN8_CTX_FORCE_RESTORE;
>
>   	return desc;
> @@ -495,7 +501,7 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring)
>   	u32 status_pointer;
>   	u8 read_pointer;
>   	u8 write_pointer;
> -	u32 status;
> +	u32 status = 0;
>   	u32 status_id;
>   	u32 submit_contexts = 0;
>
> @@ -533,8 +539,14 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring)
>   		}
>   	}
>
> -	if (submit_contexts != 0)
> +	if (disable_lite_restore_wa(ring)) {
> +		/* Prevent a ctx to preempt itself */
> +		if ((status & GEN8_CTX_STATUS_ACTIVE_IDLE) &&
> +		    (submit_contexts != 0))
> +			execlists_context_unqueue(ring);
> +	} else if (submit_contexts != 0) {
>   		execlists_context_unqueue(ring);
> +	}
>
>   	spin_unlock(&ring->execlist_lock);
>
>

Need changes if we decide to drop SKL, otherwise also it looks good to me,
Reviewed-by: Arun Siluvery <arun.siluvery@linux.intel.com>

regards
Arun

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