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 <b...@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 <kei...@keithp.com>
> Reported-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> Signed-off-by: Ben Widawsky <b...@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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
