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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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

> 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.
>
> v2: Remove iomem from ringbuffer->virtual_address
>

iomem annotation is for sparse. i915_irq.c
still uses ioread32 thus mixing the address spaces.

I see this has already r-b but something like this
as a followup would make sparse quiet:

diff --git a/drivers/gpu/drm/i915/i915_irq.c
b/drivers/gpu/drm/i915/i915_irq.c
index 6e0a568..b5e9383 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2831,7 +2831,7 @@ semaphore_waits_for(struct intel_engine_cs *ring,
u32 *seqno)
        head &= ring->buffer->size - 1;
 
                /* This here seems to blow up */
-                  cmd = ioread32(ring->buffer->virtual_start + head);
+                      cmd = intel_ringbuffer_read(ring->buffer, head);
                           if (cmd == ipehr)
                                   break;
 
@@ -2841,11 +2841,11 @@ semaphore_waits_for(struct intel_engine_cs
*ring, u32 *seqno)
       if (!i)
          return NULL;
 
-       *seqno = ioread32(ring->buffer->virtual_start + head + 4) + 1;
+       *seqno = intel_ringbuffer_read(ring->buffer, head + 4) + 1;
        if (INTEL_INFO(ring->dev)->gen >= 8) {
-          offset = ioread32(ring->buffer->virtual_start + head + 12);
+                 offset = intel_ringbuffer_read(ring->buffer, head +
12);
                offset <<= 32;
-                      offset = ioread32(ring->buffer->virtual_start +
head + 8);
+      offset = intel_ringbuffer_read(ring->buffer, head + 8);
       }
       return semaphore_wait_to_signaller_ring(ring, ipehr, offset);
 }
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index ea6f8a7..48fdfc3 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1999,7 +1999,7 @@ void intel_unpin_ringbuffer_obj(struct
intel_ringbuffer *ringbuf)
                 if (HAS_LLC(ringbuf->obj->base.dev) &&
                 !ringbuf->obj->stolen)
                        vunmap(ringbuf->virtual_start);
                        else
-                               iounmap(ringbuf->virtual_start);
+                                       iounmap((void __iomem
*)ringbuf->virtual_start);
        ringbuf->virtual_start = NULL;
        i915_gem_object_ggtt_unpin(ringbuf->obj);
 }
@@ -2059,7 +2059,7 @@ int intel_pin_and_map_ringbuffer_obj(struct
drm_device *dev,
                        return ret;
                               }
 
-               ringbuf->virtual_start =
                ioremap_wc(dev_priv->gtt.mappable_base +
+
                ringbuf->virtual_start = (void __force
                *)ioremap_wc(dev_priv->gtt.mappable_base +
                                                                                            i915_gem_obj_ggtt_offset(obj),
                ringbuf->size);
                        if (ringbuf->virtual_start == NULL) {
                                                   i915_gem_object_ggtt_unpin(obj);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
                b/drivers/gpu/drm/i915/intel_ringbuffer.h
index a15f6c2..335e632 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -448,6 +448,13 @@ static inline void intel_ringbuffer_advance(struct
                intel_ringbuffer *rb)
                rb->tail &= rb->size - 1;
 }
 
+static inline u32 intel_ringbuffer_read(const struct intel_ringbuffer
*rb,
+                                       const u32 offset)
+{
+       WARN_ON(offset >= rb->size);
+       return *(uint32_t *)(rb->virtual_start + offset);
+}
+
 static inline void intel_ring_emit(struct intel_engine_cs *ring,
                                              u32 data)
 {

-Mika

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  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 | 19 +++++++++++++------
>  4 files changed, 18 insertions(+), 21 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);
>  
>  	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..10f80df5f121 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -97,7 +97,7 @@ struct intel_ring_hangcheck {
>  
>  struct intel_ringbuffer {
>  	struct drm_i915_gem_object *obj;
> -	void __iomem *virtual_start;
> +	void *virtual_start;
>  
>  	struct intel_engine_cs *ring;
>  
> @@ -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 *)(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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx