Re: [PATCH 1/3] drm/i915: relative_constants_mode race fix

[Ben Widawsky <ben@bwidawsk.net>]

Keith, I believe this series belongs in -next. The first two could
actually go in fixes.

Ben

On Sat, 22 Oct 2011 19:41:23 -0700
Ben Widawsky <ben@bwidawsk.net> wrote:

> After my refactoring, Chris noticed that we had a bug.
> 
> dev_priv keeps track of the current addressing mode that gets set at
> execbuffer time. Unfortunately the existing code was doing this before
> acquiring struct_mutex which leaves a race with another thread also
> doing an execbuffer. If that wasn't bad enough, relocate_slow drops
> struct_mutex which opens a much more likely error where another thread
> comes in and modifies the state while relocate_slow is being slow.
> 
> The solution here is to just defer setting this state until we
> absolutely need it, and we know we'll have struct_mutex for the
> remainder of our code path.
> 
> Cc: Keith Packard <keithp@keithp.com>
> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   67 ++++++++++++++--------------
>  1 files changed, 34 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 3693e83..1d66c24 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1003,39 +1003,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		return -EINVAL;
>  	}
>  
> -	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> -	switch (mode) {
> -	case I915_EXEC_CONSTANTS_REL_GENERAL:
> -	case I915_EXEC_CONSTANTS_ABSOLUTE:
> -	case I915_EXEC_CONSTANTS_REL_SURFACE:
> -		if (ring == &dev_priv->ring[RCS] &&
> -		    mode != dev_priv->relative_constants_mode) {
> -			if (INTEL_INFO(dev)->gen < 4)
> -				return -EINVAL;
> -
> -			if (INTEL_INFO(dev)->gen > 5 &&
> -			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
> -				return -EINVAL;
> -
> -			ret = intel_ring_begin(ring, 4);
> -			if (ret)
> -				return ret;
> -
> -			intel_ring_emit(ring, MI_NOOP);
> -			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> -			intel_ring_emit(ring, INSTPM);
> -			intel_ring_emit(ring,
> -					I915_EXEC_CONSTANTS_MASK << 16 | mode);
> -			intel_ring_advance(ring);
> -
> -			dev_priv->relative_constants_mode = mode;
> -		}
> -		break;
> -	default:
> -		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
> -		return -EINVAL;
> -	}
> -
>  	if (args->buffer_count < 1) {
>  		DRM_ERROR("execbuf with %d buffers\n", args->buffer_count);
>  		return -EINVAL;
> @@ -1159,6 +1126,40 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  		}
>  	}
>  
> +	mode = args->flags & I915_EXEC_CONSTANTS_MASK;
> +	switch (mode) {
> +	case I915_EXEC_CONSTANTS_REL_GENERAL:
> +	case I915_EXEC_CONSTANTS_ABSOLUTE:
> +	case I915_EXEC_CONSTANTS_REL_SURFACE:
> +		if (ring == &dev_priv->ring[RCS] &&
> +		    mode != dev_priv->relative_constants_mode) {
> +			if (INTEL_INFO(dev)->gen < 4)
> +				return -EINVAL;
> +
> +			if (INTEL_INFO(dev)->gen > 5 &&
> +			    mode == I915_EXEC_CONSTANTS_REL_SURFACE)
> +				return -EINVAL;
> +
> +			ret = intel_ring_begin(ring, 4);
> +			if (ret)
> +				goto err;
> +
> +			intel_ring_emit(ring, MI_NOOP);
> +			intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1));
> +			intel_ring_emit(ring, INSTPM);
> +			intel_ring_emit(ring,
> +					I915_EXEC_CONSTANTS_MASK << 16 | mode);
> +			intel_ring_advance(ring);
> +
> +			dev_priv->relative_constants_mode = mode;
> +		}
> +		break;
> +	default:
> +		DRM_ERROR("execbuf with unknown constants: %d\n", mode);
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
>  	trace_i915_gem_ring_dispatch(ring, seqno);
>  
>  	exec_start = batch_obj->gtt_offset + args->batch_start_offset;