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 <ch...@chris-wilson.co.uk>
> 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..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: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
