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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Oct 12, 2011 at 03:56:19PM -0700, Ben Widawsky 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: Chris Wilson <chris@chris-wilson.co.uk>
> 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..0b343af 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;
> @@ -1132,6 +1099,40 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			goto err;
>  	}
>  
> +	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;

This probably wont compile ;-) I'd also move this block even further down
after gem_execbuffer_move_to_gpu. It doesn't matter for correctness, but
now it's sitting in the middle of the relocation and cache domain
handling, which doesn't make sense.

> +		goto err;
> +	}
> +
>  	/* Set the pending read domains for the batch buffer to COMMAND */
>  	if (batch_obj->base.pending_write_domain) {
>  		DRM_ERROR("Attempting to use self-modifying batch buffer\n");
> -- 
> 1.7.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48