[Intel-gfx] [PATCH 1/2] drm/i915: Mark the end of intel_ring_begin() and check in intel_ring_advance()

[Mika Kuoppala]

Chris Wilson <chris at chris-wilson.co.uk> writes:

> It is required that the caller declare the exact number of dwords they
> wish to write into the ring. This is required for two reasons, we need
> to allocate sufficient space for the entire command packet and we need
> to be sure that the contents are completely written to avoid executing
> stale data. The current interface requires for any bug to be caught in
> review, the reader has to carefully count the number of
> intel_ring_emit() between intel_ring_begin() and intel_ring_advance().
> If we record the end of the packet of each intel_ring_begin() we can
> also have CI check for us.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>

Reviewed-by: Mika Kuoppala <mika.kuoppala at intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.h         | 9 +++++++++
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 1 +
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 3 +++
>  3 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.h b/drivers/gpu/drm/i915/i915_gem.h
> index a585d47c420a..412fa2794cbb 100644
> --- a/drivers/gpu/drm/i915/i915_gem.h
> +++ b/drivers/gpu/drm/i915/i915_gem.h
> @@ -28,9 +28,18 @@
>  #ifdef CONFIG_DRM_I915_DEBUG_GEM
>  #define GEM_BUG_ON(expr) BUG_ON(expr)
>  #define GEM_WARN_ON(expr) WARN_ON(expr)
> +
> +#define GEM_BUG_ONLY(expr) expr
> +#define GEM_BUG_ONLY_DECLARE(var) var
> +#define GEM_BUG_ONLY_ON(expr) GEM_BUG_ON(expr)
> +
>  #else
>  #define GEM_BUG_ON(expr) BUILD_BUG_ON_INVALID(expr)
>  #define GEM_WARN_ON(expr) (BUILD_BUG_ON_INVALID(expr), 0)
> +
> +#define GEM_BUG_ONLY(expr) do { } while (0)
> +#define GEM_BUG_ONLY_DECLARE(var)
> +#define GEM_BUG_ONLY_ON(expr)
>  #endif
>  
>  #define I915_NUM_ENGINES 5
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d32cbba25d98..383083ef2210 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2238,6 +2238,7 @@ int intel_ring_begin(struct drm_i915_gem_request *req, int num_dwords)
>  
>  	ring->space -= bytes;
>  	GEM_BUG_ON(ring->space < 0);
> +	GEM_BUG_ONLY(ring->advance = ring->tail + bytes);
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index b9c15cd40fbf..2c6d3655985e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -144,6 +144,8 @@ struct intel_ring {
>  
>  	u32 head;
>  	u32 tail;
> +	GEM_BUG_ONLY_DECLARE(u32 advance);
> +
>  	int space;
>  	int size;
>  	int effective_size;
> @@ -516,6 +518,7 @@ static inline void intel_ring_advance(struct intel_ring *ring)
>  	 * reserved for the command packet (i.e. the value passed to
>  	 * intel_ring_begin()).
>  	 */
> +	GEM_BUG_ONLY_ON(ring->tail != ring->advance);
>  }
>  
>  static inline u32 intel_ring_offset(struct intel_ring *ring, void *addr)
> -- 
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx