Re: [PATCH 3/3] drm/i915: Treat ringbuffer writes as write to normal memory

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 08, 2015 at 01:39:56PM +0100, Chris Wilson wrote:
> Ringbuffers are now being written to either through LLC or WC paths, so
> treating them as simply iomem is no longer adequate. However, for the
> older !llc hardware, the hardware is documentated as treating the TAIL
> register update as serialising, so we can relax the barriers when filling
> the rings (but even if it were not, it is still an uncached register write
> and so serialising anyway.).
> 
> For simplicity, let's ignore the iomem annotation.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c        |  7 +------
>  drivers/gpu/drm/i915/intel_lrc.h        |  6 +++---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  7 +------
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 17 ++++++++++++-----
>  4 files changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d38746c5370d..10020505be75 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -713,13 +713,8 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
>  
>  static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
>  {
> -	uint32_t __iomem *virt;
>  	int rem = ringbuf->size - ringbuf->tail;
> -
> -	virt = ringbuf->virtual_start + ringbuf->tail;
> -	rem /= 4;
> -	while (rem--)
> -		iowrite32(MI_NOOP, virt++);
> +	memset(ringbuf->virtual_start + ringbuf->tail, 0, rem);
>  
>  	ringbuf->tail = 0;
>  	intel_ring_update_space(ringbuf);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 64f89f9982a2..8b56fb934763 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -53,8 +53,9 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
>   */
>  static inline void intel_logical_ring_advance(struct intel_ringbuffer *ringbuf)
>  {
> -	ringbuf->tail &= ringbuf->size - 1;
> +	intel_ringbuffer_advance(ringbuf);
>  }
> +
>  /**
>   * intel_logical_ring_emit() - write a DWORD to the ringbuffer.
>   * @ringbuf: Ringbuffer to write to.
> @@ -63,8 +64,7 @@ static inline void intel_logical_ring_advance(struct intel_ringbuffer *ringbuf)
>  static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf,
>  					   u32 data)
>  {
> -	iowrite32(data, ringbuf->virtual_start + ringbuf->tail);
> -	ringbuf->tail += 4;
> +	intel_ringbuffer_emit(ringbuf, data);
>  }
>  
>  /* Logical Ring Contexts */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 49aa52440db2..0eaaab92dea0 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2180,13 +2180,8 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
>  
>  static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
>  {
> -	uint32_t __iomem *virt;
>  	int rem = ringbuf->size - ringbuf->tail;
> -
> -	virt = ringbuf->virtual_start + ringbuf->tail;
> -	rem /= 4;
> -	while (rem--)
> -		iowrite32(MI_NOOP, virt++);
> +	memset(ringbuf->virtual_start + ringbuf->tail, 0, rem);

Hmm. The lrc copy looks identical to this one. But looks like most of
intel_lrc.c ring handling is copy pasted anyway, so there's even more
that could be thrown out.

This patch seems OK to me
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	ringbuf->tail = 0;
>  	intel_ring_update_space(ringbuf);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 2e85fda94963..ff290765e6c9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -427,17 +427,24 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request);
>  
>  int __must_check intel_ring_begin(struct drm_i915_gem_request *req, int n);
>  int __must_check intel_ring_cacheline_align(struct drm_i915_gem_request *req);
> +static inline void intel_ringbuffer_emit(struct intel_ringbuffer *rb,
> +					 u32 data)
> +{
> +	*(uint32_t __force *)(rb->virtual_start + rb->tail) = data;
> +	rb->tail += 4;
> +}
> +static inline void intel_ringbuffer_advance(struct intel_ringbuffer *rb)
> +{
> +	rb->tail &= rb->size - 1;
> +}
>  static inline void intel_ring_emit(struct intel_engine_cs *ring,
>  				   u32 data)
>  {
> -	struct intel_ringbuffer *ringbuf = ring->buffer;
> -	iowrite32(data, ringbuf->virtual_start + ringbuf->tail);
> -	ringbuf->tail += 4;
> +	intel_ringbuffer_emit(ring->buffer, data);
>  }
>  static inline void intel_ring_advance(struct intel_engine_cs *ring)
>  {
> -	struct intel_ringbuffer *ringbuf = ring->buffer;
> -	ringbuf->tail &= ringbuf->size - 1;
> +	intel_ringbuffer_advance(ring->buffer);
>  }
>  int __intel_ring_space(int head, int tail, int size);
>  void intel_ring_update_space(struct intel_ringbuffer *ringbuf);
> -- 
> 2.6.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx