[Intel-gfx] [PATCH v3 5/5] drm/i915/execlists: Read the context-status HEAD from the HWSP

Mika Kuoppala mika.kuoppala at linux.intel.com
Thu Jul 20 13:38:40 UTC 2017


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

> The engine also provides a mirror of the CSB write pointer in the HWSP,
> but not of our read pointer. To take advantage of this we need to
> remember where we read up to on the last interrupt and continue off from
> there. This poses a problem following a reset, as we don't know where
> the hw will start writing from, and due to the use of power contexts we
> cannot perform that query during the reset itself. So we continue the
> current modus operandi of delaying the first read of the context-status
> read/write pointers until after the first interrupt. With this we should
> now have eliminated all uncached mmio reads in handling the
> context-status interrupt, though we still have the uncached mmio writes
> for submitting new work, and many uncached mmio reads in the global
> interrupt handler itself. Still a step in the right direction towards
> reducing our resubmit latency, although it appears lost in the noise!
>
> v2: Cannonlake moved the CSB write index
> v3: Include the sw/hwsp state in debugfs/i915_engine_info
> v4: Also revert to using CSB mmio for GVT-g
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michel Thierry <michel.thierry at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Zhenyu Wang <zhenyuw at linux.intel.com>
> Cc: Zhi Wang <zhi.a.wang at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c     |  6 ++++--
>  drivers/gpu/drm/i915/i915_drv.h         |  8 ++++++++
>  drivers/gpu/drm/i915/intel_lrc.c        | 25 ++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_ringbuffer.h |  3 +++
>  4 files changed, 35 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 5fd01c14a3ec..552aef61b47b 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -3395,8 +3395,10 @@ static int i915_engine_info(struct seq_file *m, void *unused)
>  			ptr = I915_READ(RING_CONTEXT_STATUS_PTR(engine));
>  			read = GEN8_CSB_READ_PTR(ptr);
>  			write = GEN8_CSB_WRITE_PTR(ptr);
> -			seq_printf(m, "\tExeclist CSB read %d, write %d\n",
> -				   read, write);
> +			seq_printf(m, "\tExeclist CSB read %d [%d cached], write %d [%d from hws]\n",
> +				   read, engine->csb_head,
> +				   write,
> +				   intel_read_status_page(engine, intel_hws_csb_write_index(engine->i915)));
>  			if (read >= GEN8_CSB_ENTRIES)
>  				read = 0;
>  			if (write >= GEN8_CSB_ENTRIES)
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 81cd21ecfa7d..f62c9db8a9a8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -4228,4 +4228,12 @@ static inline bool i915_gem_object_is_coherent(struct drm_i915_gem_object *obj)
>  		HAS_LLC(to_i915(obj->base.dev)));
>  }
>  
> +static inline int intel_hws_csb_write_index(struct drm_i915_private *i915)
> +{
> +	if (INTEL_GEN(i915) >= 10)
> +		return CNL_HWS_CSB_WRITE_INDEX;
> +	else
> +		return I915_HWS_CSB_WRITE_INDEX;
> +}
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 5b721f65d232..7c3dce27e504 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -556,6 +556,7 @@ static void intel_lrc_irq_handler(unsigned long data)
>  		if (unlikely(intel_vgpu_active(dev_priv))) {
>  			buf = (u32 * __force)
>  				(dev_priv->regs + i915_mmio_reg_offset(RING_CONTEXT_STATUS_PTR(engine)));
> +			engine->csb_head = -1;
>  		}
>  
>  		/* The write will be ordered by the uncached read (itself
> @@ -569,9 +570,19 @@ static void intel_lrc_irq_handler(unsigned long data)
>  		 * is set and we do a new loop.
>  		 */
>  		__clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> -		head = readl(csb_mmio);
> -		tail = GEN8_CSB_WRITE_PTR(head);
> -		head = GEN8_CSB_READ_PTR(head);
> +		if (unlikely(engine->csb_head == -1)) { /* following a reset */
> +			head = readl(csb_mmio);
> +			tail = GEN8_CSB_WRITE_PTR(head);
> +			head = GEN8_CSB_READ_PTR(head);
> +			engine->csb_head = head;
> +		} else {
> +			const int write_idx =
> +				intel_hws_csb_write_index(dev_priv) -
> +				I915_HWS_CSB_BUF0_INDEX;
> +
> +			head = engine->csb_head;
> +			tail = buf[write_idx];
> +		}

I have discussed this with Chris already in irc but I have a kbl
that can't survive the patch. The hwsp tail seems to update
inside the loop without corresponding interrupt.

And it results in a system hang instead of hangcheck firing.
Which is a another mystery byitself.

Resampling the tail inside the loop makes it work.

-Mika

>  		while (head != tail) {
>  			struct drm_i915_gem_request *rq;
>  			unsigned int status;
> @@ -625,8 +636,11 @@ static void intel_lrc_irq_handler(unsigned long data)
>  				   !(status & GEN8_CTX_STATUS_ACTIVE_IDLE));
>  		}
>  
> -		writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
> -		       csb_mmio);
> +		if (head != engine->csb_head) {
> +			engine->csb_head = head;
> +			writel(_MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, head << 8),
> +			       csb_mmio);
> +		}
>  	}
>  
>  	if (execlists_elsp_ready(engine))
> @@ -1253,6 +1267,7 @@ static int gen8_init_common_ring(struct intel_engine_cs *engine)
>  
>  	/* After a GPU reset, we may have requests to replay */
>  	clear_bit(ENGINE_IRQ_EXECLIST, &engine->irq_posted);
> +	engine->csb_head = -1;
>  
>  	submit = false;
>  	for (n = 0; n < ARRAY_SIZE(engine->execlist_port); n++) {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 2c55cfa14fb5..a182da7eb9a9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -391,6 +391,7 @@ struct intel_engine_cs {
>  	struct rb_root execlist_queue;
>  	struct rb_node *execlist_first;
>  	unsigned int fw_domains;
> +	unsigned int csb_head;
>  
>  	/* Contexts are pinned whilst they are active on the GPU. The last
>  	 * context executed remains active whilst the GPU is idle - the
> @@ -497,6 +498,8 @@ intel_write_status_page(struct intel_engine_cs *engine, int reg, u32 value)
>  #define I915_GEM_HWS_SCRATCH_ADDR (I915_GEM_HWS_SCRATCH_INDEX << MI_STORE_DWORD_INDEX_SHIFT)
>  
>  #define I915_HWS_CSB_BUF0_INDEX		0x10
> +#define I915_HWS_CSB_WRITE_INDEX	0x1f
> +#define CNL_HWS_CSB_WRITE_INDEX		0x2f
>  
>  struct intel_ring *
>  intel_engine_create_ring(struct intel_engine_cs *engine, int size);
> -- 
> 2.13.2


More information about the Intel-gfx mailing list