Re: [PATCH] drm/i915: track relative-constants-mode per-context not per-device

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 13, 2015 at 02:20:05PM +0100, Dave Gordon wrote:
> 'relative_constants_mode' has always been tracked per-device, but this
> is wrong in execlists (or GuC) mode, as INSTPM is saved and restored
> with the logical context, and the per-context value could therefore get
> out of sync with the tracked value. This patch moves the tracking
> element from the dev_priv structure into the intel_context structure,
> with corresponding adjustments to the code that initialises and uses it.
> 
> Test case (if anyone wants to write it) would be to create two contexts,
> submit a batch with a non-default mode in the first context, submit a
> batch with the default mode in the other context, submit another batch
> in the first context, but this time in default mode. The driver will
> fail to insert the instructions to reset INSTPM into the first context's
> ringbuffer.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92448
> 
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>

Doesn't this break pre-gen8? Or maybe legacy mode instead of execlist,
dunno. If it's pre-gen8 we need a check, if it's execlist vs. legacy then
we could just move this into the ring instead.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.h            | 4 ++--
>  drivers/gpu/drm/i915/i915_gem.c            | 2 --
>  drivers/gpu/drm/i915/i915_gem_context.c    | 3 +++
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 +++---
>  drivers/gpu/drm/i915/intel_lrc.c           | 6 +++---
>  5 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 51eea29..2917370 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -872,6 +872,8 @@ struct intel_context {
>  	struct i915_ctx_hang_stats hang_stats;
>  	struct i915_hw_ppgtt *ppgtt;
>  
> +	int relative_constants_mode;
> +
>  	/* Legacy ring buffer submission */
>  	struct {
>  		struct drm_i915_gem_object *rcs_state;
> @@ -1707,8 +1709,6 @@ struct drm_i915_private {
>  
>  	const struct intel_device_info info;
>  
> -	int relative_constants_mode;
> -
>  	void __iomem *regs;
>  
>  	struct intel_uncore uncore;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f0cfbb9..374af2d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4894,8 +4894,6 @@ i915_gem_load(struct drm_device *dev)
>  			  i915_gem_idle_work_handler);
>  	init_waitqueue_head(&dev_priv->gpu_error.reset_queue);
>  
> -	dev_priv->relative_constants_mode = I915_EXEC_CONSTANTS_REL_GENERAL;
> -
>  	if (INTEL_INFO(dev)->gen >= 7 && !IS_VALLEYVIEW(dev))
>  		dev_priv->num_fence_regs = 32;
>  	else if (INTEL_INFO(dev)->gen >= 4 || IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 74aa0c9..465e3e0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -222,6 +222,9 @@ __create_hw_context(struct drm_device *dev,
>  	 * is no remap info, it will be a NOP. */
>  	ctx->remap_slice = (1 << NUM_L3_SLICES(dev)) - 1;
>  
> +	/* First execbuffer will override this */
> +	ctx->relative_constants_mode = -1;
> +
>  	ctx->hang_stats.ban_period_seconds = DRM_I915_CTX_BAN_PERIOD;
>  
>  	return ctx;
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index edc17be..9833c8a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1283,7 +1283,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  			goto error;
>  		}
>  
> -		if (instp_mode != dev_priv->relative_constants_mode) {
> +		if (instp_mode != params->ctx->relative_constants_mode) {
>  			if (INTEL_INFO(dev)->gen < 4) {
>  				DRM_DEBUG("no rel constants on pre-gen4\n");
>  				ret = -EINVAL;
> @@ -1309,7 +1309,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  	}
>  
>  	if (ring == &dev_priv->ring[RCS] &&
> -			instp_mode != dev_priv->relative_constants_mode) {
> +			instp_mode != params->ctx->relative_constants_mode) {
>  		ret = intel_ring_begin(params->request, 4);
>  		if (ret)
>  			goto error;
> @@ -1320,7 +1320,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params,
>  		intel_ring_emit(ring, instp_mask << 16 | instp_mode);
>  		intel_ring_advance(ring);
>  
> -		dev_priv->relative_constants_mode = instp_mode;
> +		params->ctx->relative_constants_mode = instp_mode;
>  	}
>  
>  	if (args->flags & I915_EXEC_GEN7_SOL_RESET) {
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 825fa7a..9ff409d 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -889,7 +889,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>  			return -EINVAL;
>  		}
>  
> -		if (instp_mode != dev_priv->relative_constants_mode) {
> +		if (instp_mode != params->ctx->relative_constants_mode) {
>  			if (instp_mode == I915_EXEC_CONSTANTS_REL_SURFACE) {
>  				DRM_DEBUG("rel surface constants mode invalid on gen5+\n");
>  				return -EINVAL;
> @@ -929,7 +929,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>  		return ret;
>  
>  	if (ring == &dev_priv->ring[RCS] &&
> -	    instp_mode != dev_priv->relative_constants_mode) {
> +	    instp_mode != params->ctx->relative_constants_mode) {
>  		ret = intel_logical_ring_begin(params->request, 4);
>  		if (ret)
>  			return ret;
> @@ -940,7 +940,7 @@ int intel_execlists_submission(struct i915_execbuffer_params *params,
>  		intel_logical_ring_emit(ringbuf, instp_mask << 16 | instp_mode);
>  		intel_logical_ring_advance(ringbuf);
>  
> -		dev_priv->relative_constants_mode = instp_mode;
> +		params->ctx->relative_constants_mode = instp_mode;
>  	}
>  
>  	exec_start = params->batch_obj_vm_offset +
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx