Re: [PATCH 019/190] drm/i915: Separate out the seqno-barrier from engine->get_seqno

[Dave Gordon <david.s.gordon@intel.com>]

On 11/01/16 09:16, Chris Wilson wrote:
> In order to simplify the next couple of patches, extract the
> lazy_coherency optimisation our of the engine->get_seqno() vfunc into
> its own callback.
>
> v2: Rename the barrier to engine->irq_seqno_barrier to try and better
> reflect that the barrier is only required after the user interrupt before
> reading the seqno (to ensure that the seqno update lands in time as we
> do not have strict seqno-irq ordering on all platforms).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c      |  6 ++---
>   drivers/gpu/drm/i915/i915_drv.h          | 12 ++++++----
>   drivers/gpu/drm/i915/i915_gpu_error.c    |  2 +-
>   drivers/gpu/drm/i915/i915_irq.c          |  4 ++--
>   drivers/gpu/drm/i915/i915_trace.h        |  2 +-
>   drivers/gpu/drm/i915/intel_breadcrumbs.c |  4 ++--
>   drivers/gpu/drm/i915/intel_lrc.c         | 39 ++++++++++++--------------------
>   drivers/gpu/drm/i915/intel_ringbuffer.c  | 36 +++++++++++++++--------------
>   drivers/gpu/drm/i915/intel_ringbuffer.h  |  4 ++--
>   9 files changed, 53 insertions(+), 56 deletions(-)

All looks OK, so

Reviewed-by: Dave Gordon <david.s.gordon@intel.com>

> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9396597b136d..1499e2337e5d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -600,7 +600,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
>   					   ring->name,
>   					   i915_gem_request_get_seqno(work->flip_queued_req),
>   					   dev_priv->next_seqno,
> -					   ring->get_seqno(ring, true),
> +					   ring->get_seqno(ring),
>   					   i915_gem_request_completed(work->flip_queued_req, true));
>   			} else
>   				seq_printf(m, "Flip not associated with any ring\n");
> @@ -734,7 +734,7 @@ static void i915_ring_seqno_info(struct seq_file *m,
>
>   	if (ring->get_seqno) {
>   		seq_printf(m, "Current sequence (%s): %x\n",
> -			   ring->name, ring->get_seqno(ring, false));
> +			   ring->name, ring->get_seqno(ring));
>   	}
>
>   	spin_lock(&ring->breadcrumbs.lock);
> @@ -1354,7 +1354,7 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>   	intel_runtime_pm_get(dev_priv);
>
>   	for_each_ring(ring, dev_priv, i) {
> -		seqno[i] = ring->get_seqno(ring, false);
> +		seqno[i] = ring->get_seqno(ring);
>   		acthd[i] = intel_ring_get_active_head(ring);
>   	}
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a9e8de57e848..9762aa76bb0a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2972,15 +2972,19 @@ i915_seqno_passed(uint32_t seq1, uint32_t seq2)
>   static inline bool i915_gem_request_started(struct drm_i915_gem_request *req,
>   					   bool lazy_coherency)
>   {
> -	u32 seqno = req->ring->get_seqno(req->ring, lazy_coherency);
> -	return i915_seqno_passed(seqno, req->previous_seqno);
> +	if (!lazy_coherency && req->ring->irq_seqno_barrier)
> +		req->ring->irq_seqno_barrier(req->ring);
> +	return i915_seqno_passed(req->ring->get_seqno(req->ring),
> +				 req->previous_seqno);
>   }

We have a different implementation of request_started() with the 
scheduler, but we'll just update this when the scheduler goes in.

.Dave.

>   static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
>   					      bool lazy_coherency)
>   {
> -	u32 seqno = req->ring->get_seqno(req->ring, lazy_coherency);
> -	return i915_seqno_passed(seqno, req->seqno);
> +	if (!lazy_coherency && req->ring->irq_seqno_barrier)
> +		req->ring->irq_seqno_barrier(req->ring);
> +	return i915_seqno_passed(req->ring->get_seqno(req->ring),
> +				 req->seqno);
>   }
>
>   int __must_check i915_gem_get_seqno(struct drm_device *dev, u32 *seqno);
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index f805d117f3d1..01d0206ca4dd 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -902,8 +902,8 @@ static void i915_record_ring_state(struct drm_device *dev,
>
>   	ering->waiting = intel_engine_has_waiter(ring);
>   	ering->instpm = I915_READ(RING_INSTPM(ring->mmio_base));
> -	ering->seqno = ring->get_seqno(ring, false);
>   	ering->acthd = intel_ring_get_active_head(ring);
> +	ering->seqno = ring->get_seqno(ring);
>   	ering->start = I915_READ_START(ring);
>   	ering->head = I915_READ_HEAD(ring);
>   	ering->tail = I915_READ_TAIL(ring);
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 95b997a57da8..d73669783045 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2903,7 +2903,7 @@ static int semaphore_passed(struct intel_engine_cs *ring)
>   	if (signaller->hangcheck.deadlock >= I915_NUM_RINGS)
>   		return -1;
>
> -	if (i915_seqno_passed(signaller->get_seqno(signaller, false), seqno))
> +	if (i915_seqno_passed(signaller->get_seqno(signaller), seqno))
>   		return 1;
>
>   	/* cursory check for an unkickable deadlock */
> @@ -3067,8 +3067,8 @@ static void i915_hangcheck_elapsed(struct work_struct *work)
>
>   		semaphore_clear_deadlocks(dev_priv);
>
> -		seqno = ring->get_seqno(ring, false);
>   		acthd = intel_ring_get_active_head(ring);
> +		seqno = ring->get_seqno(ring);
>
>   		if (ring->hangcheck.seqno == seqno) {
>   			if (ring_idle(ring, seqno)) {
> diff --git a/drivers/gpu/drm/i915/i915_trace.h b/drivers/gpu/drm/i915/i915_trace.h
> index 52b2d409945d..cfb5f78a6e84 100644
> --- a/drivers/gpu/drm/i915/i915_trace.h
> +++ b/drivers/gpu/drm/i915/i915_trace.h
> @@ -573,7 +573,7 @@ TRACE_EVENT(i915_gem_request_notify,
>   	    TP_fast_assign(
>   			   __entry->dev = ring->dev->primary->index;
>   			   __entry->ring = ring->id;
> -			   __entry->seqno = ring->get_seqno(ring, false);
> +			   __entry->seqno = ring->get_seqno(ring);
>   			   ),
>
>   	    TP_printk("dev=%u, ring=%u, seqno=%u",
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 9f756583a44e..10b0add54acf 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -127,7 +127,7 @@ bool intel_engine_add_wait(struct intel_engine_cs *engine,
>   			   struct intel_wait *wait)
>   {
>   	struct intel_breadcrumbs *b = &engine->breadcrumbs;
> -	u32 seqno = engine->get_seqno(engine, true);
> +	u32 seqno = engine->get_seqno(engine);
>   	struct rb_node **p, *parent, *completed;
>   	bool first;
>
> @@ -269,7 +269,7 @@ void intel_engine_remove_wait(struct intel_engine_cs *engine,
>   			 * the first_waiter. This is undesirable if that
>   			 * waiter is a high priority task.
>   			 */
> -			u32 seqno = engine->get_seqno(engine, true);
> +			u32 seqno = engine->get_seqno(engine);
>   			while (i915_seqno_passed(seqno,
>   						 to_wait(next)->seqno)) {
>   				struct rb_node *n = rb_next(next);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 16fa58a0a930..333e95bda78a 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1775,7 +1775,7 @@ static int gen8_emit_flush_render(struct drm_i915_gem_request *request,
>   	return 0;
>   }
>
> -static u32 gen8_get_seqno(struct intel_engine_cs *ring, bool lazy_coherency)
> +static u32 gen8_get_seqno(struct intel_engine_cs *ring)
>   {
>   	return intel_read_status_page(ring, I915_GEM_HWS_INDEX);
>   }
> @@ -1785,9 +1785,8 @@ static void gen8_set_seqno(struct intel_engine_cs *ring, u32 seqno)
>   	intel_write_status_page(ring, I915_GEM_HWS_INDEX, seqno);
>   }
>
> -static u32 bxt_a_get_seqno(struct intel_engine_cs *ring, bool lazy_coherency)
> +static void bxt_seqno_barrier(struct intel_engine_cs *ring)
>   {
> -
>   	/*
>   	 * On BXT A steppings there is a HW coherency issue whereby the
>   	 * MI_STORE_DATA_IMM storing the completed request's seqno
> @@ -1798,11 +1797,7 @@ static u32 bxt_a_get_seqno(struct intel_engine_cs *ring, bool lazy_coherency)
>   	 * bxt_a_set_seqno(), where we also do a clflush after the write. So
>   	 * this clflush in practice becomes an invalidate operation.
>   	 */
> -
> -	if (!lazy_coherency)
> -		intel_flush_status_page(ring, I915_GEM_HWS_INDEX);
> -
> -	return intel_read_status_page(ring, I915_GEM_HWS_INDEX);
> +	intel_flush_status_page(ring, I915_GEM_HWS_INDEX);
>   }
>
>   static void bxt_a_set_seqno(struct intel_engine_cs *ring, u32 seqno)
> @@ -2007,12 +2002,11 @@ static int logical_render_ring_init(struct drm_device *dev)
>   		ring->init_hw = gen8_init_render_ring;
>   	ring->init_context = gen8_init_rcs_context;
>   	ring->cleanup = intel_fini_pipe_control;
> +	ring->get_seqno = gen8_get_seqno;
> +	ring->set_seqno = gen8_set_seqno;
>   	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> -		ring->get_seqno = bxt_a_get_seqno;
> +		ring->irq_seqno_barrier = bxt_seqno_barrier;
>   		ring->set_seqno = bxt_a_set_seqno;
> -	} else {
> -		ring->get_seqno = gen8_get_seqno;
> -		ring->set_seqno = gen8_set_seqno;
>   	}
>   	ring->emit_request = gen8_emit_request;
>   	ring->emit_flush = gen8_emit_flush_render;
> @@ -2059,12 +2053,11 @@ static int logical_bsd_ring_init(struct drm_device *dev)
>   		GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VCS1_IRQ_SHIFT;
>
>   	ring->init_hw = gen8_init_common_ring;
> +	ring->get_seqno = gen8_get_seqno;
> +	ring->set_seqno = gen8_set_seqno;
>   	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> -		ring->get_seqno = bxt_a_get_seqno;
> +		ring->irq_seqno_barrier = bxt_seqno_barrier;
>   		ring->set_seqno = bxt_a_set_seqno;
> -	} else {
> -		ring->get_seqno = gen8_get_seqno;
> -		ring->set_seqno = gen8_set_seqno;
>   	}
>   	ring->emit_request = gen8_emit_request;
>   	ring->emit_flush = gen8_emit_flush;
> @@ -2114,12 +2107,11 @@ static int logical_blt_ring_init(struct drm_device *dev)
>   		GT_CONTEXT_SWITCH_INTERRUPT << GEN8_BCS_IRQ_SHIFT;
>
>   	ring->init_hw = gen8_init_common_ring;
> +	ring->get_seqno = gen8_get_seqno;
> +	ring->set_seqno = gen8_set_seqno;
>   	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> -		ring->get_seqno = bxt_a_get_seqno;
> +		ring->irq_seqno_barrier = bxt_seqno_barrier;
>   		ring->set_seqno = bxt_a_set_seqno;
> -	} else {
> -		ring->get_seqno = gen8_get_seqno;
> -		ring->set_seqno = gen8_set_seqno;
>   	}
>   	ring->emit_request = gen8_emit_request;
>   	ring->emit_flush = gen8_emit_flush;
> @@ -2144,12 +2136,11 @@ static int logical_vebox_ring_init(struct drm_device *dev)
>   		GT_CONTEXT_SWITCH_INTERRUPT << GEN8_VECS_IRQ_SHIFT;
>
>   	ring->init_hw = gen8_init_common_ring;
> +	ring->get_seqno = gen8_get_seqno;
> +	ring->set_seqno = gen8_set_seqno;
>   	if (IS_BXT_REVID(dev, 0, BXT_REVID_A1)) {
> -		ring->get_seqno = bxt_a_get_seqno;
> +		ring->irq_seqno_barrier = bxt_seqno_barrier;
>   		ring->set_seqno = bxt_a_set_seqno;
> -	} else {
> -		ring->get_seqno = gen8_get_seqno;
> -		ring->set_seqno = gen8_set_seqno;
>   	}
>   	ring->emit_request = gen8_emit_request;
>   	ring->emit_flush = gen8_emit_flush;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 60b0df2c5399..57ec21c5b1ab 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1485,8 +1485,8 @@ pc_render_add_request(struct drm_i915_gem_request *req)
>   	return 0;
>   }
>
> -static u32
> -gen6_ring_get_seqno(struct intel_engine_cs *ring, bool lazy_coherency)
> +static void
> +gen6_seqno_barrier(struct intel_engine_cs *ring)
>   {
>   	/* Workaround to force correct ordering between irq and seqno writes on
>   	 * ivb (and maybe also on snb) by reading from a CS register (like
> @@ -1500,18 +1500,14 @@ gen6_ring_get_seqno(struct intel_engine_cs *ring, bool lazy_coherency)
>   	 * a delay after every batch i.e. much more frequent than a delay
>   	 * when waiting for the interrupt (with the same net latency).
>   	 */
> -	if (!lazy_coherency) {
> -		struct drm_i915_private *dev_priv = ring->dev->dev_private;
> -		POSTING_READ_FW(RING_ACTHD(ring->mmio_base));
> -
> -		intel_flush_status_page(ring, I915_GEM_HWS_INDEX);
> -	}
> +	struct drm_i915_private *dev_priv = ring->i915;
> +	POSTING_READ_FW(RING_ACTHD(ring->mmio_base));
>
> -	return intel_read_status_page(ring, I915_GEM_HWS_INDEX);
> +	intel_flush_status_page(ring, I915_GEM_HWS_INDEX);
>   }
>
>   static u32
> -ring_get_seqno(struct intel_engine_cs *ring, bool lazy_coherency)
> +ring_get_seqno(struct intel_engine_cs *ring)
>   {
>   	return intel_read_status_page(ring, I915_GEM_HWS_INDEX);
>   }
> @@ -1523,7 +1519,7 @@ ring_set_seqno(struct intel_engine_cs *ring, u32 seqno)
>   }
>
>   static u32
> -pc_render_get_seqno(struct intel_engine_cs *ring, bool lazy_coherency)
> +pc_render_get_seqno(struct intel_engine_cs *ring)
>   {
>   	return ring->scratch.cpu_page[0];
>   }
> @@ -2698,7 +2694,8 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>   		ring->irq_get = gen8_ring_get_irq;
>   		ring->irq_put = gen8_ring_put_irq;
>   		ring->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
> -		ring->get_seqno = gen6_ring_get_seqno;
> +		ring->irq_seqno_barrier = gen6_seqno_barrier;
> +		ring->get_seqno = ring_get_seqno;
>   		ring->set_seqno = ring_set_seqno;
>   		if (i915_semaphore_is_enabled(dev)) {
>   			WARN_ON(!dev_priv->semaphore_obj);
> @@ -2715,7 +2712,8 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
>   		ring->irq_get = gen6_ring_get_irq;
>   		ring->irq_put = gen6_ring_put_irq;
>   		ring->irq_enable_mask = GT_RENDER_USER_INTERRUPT;
> -		ring->get_seqno = gen6_ring_get_seqno;
> +		ring->irq_seqno_barrier = gen6_seqno_barrier;
> +		ring->get_seqno = ring_get_seqno;
>   		ring->set_seqno = ring_set_seqno;
>   		if (i915_semaphore_is_enabled(dev)) {
>   			ring->semaphore.sync_to = gen6_ring_sync;
> @@ -2829,7 +2827,8 @@ int intel_init_bsd_ring_buffer(struct drm_device *dev)
>   			ring->write_tail = gen6_bsd_ring_write_tail;
>   		ring->flush = gen6_bsd_ring_flush;
>   		ring->add_request = gen6_add_request;
> -		ring->get_seqno = gen6_ring_get_seqno;
> +		ring->irq_seqno_barrier = gen6_seqno_barrier;
> +		ring->get_seqno = ring_get_seqno;
>   		ring->set_seqno = ring_set_seqno;
>   		if (INTEL_INFO(dev)->gen >= 8) {
>   			ring->irq_enable_mask =
> @@ -2901,7 +2900,8 @@ int intel_init_bsd2_ring_buffer(struct drm_device *dev)
>   	ring->mmio_base = GEN8_BSD2_RING_BASE;
>   	ring->flush = gen6_bsd_ring_flush;
>   	ring->add_request = gen6_add_request;
> -	ring->get_seqno = gen6_ring_get_seqno;
> +	ring->irq_seqno_barrier = gen6_seqno_barrier;
> +	ring->get_seqno = ring_get_seqno;
>   	ring->set_seqno = ring_set_seqno;
>   	ring->irq_enable_mask =
>   			GT_RENDER_USER_INTERRUPT << GEN8_VCS2_IRQ_SHIFT;
> @@ -2931,7 +2931,8 @@ int intel_init_blt_ring_buffer(struct drm_device *dev)
>   	ring->write_tail = ring_write_tail;
>   	ring->flush = gen6_ring_flush;
>   	ring->add_request = gen6_add_request;
> -	ring->get_seqno = gen6_ring_get_seqno;
> +	ring->irq_seqno_barrier = gen6_seqno_barrier;
> +	ring->get_seqno = ring_get_seqno;
>   	ring->set_seqno = ring_set_seqno;
>   	if (INTEL_INFO(dev)->gen >= 8) {
>   		ring->irq_enable_mask =
> @@ -2988,7 +2989,8 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev)
>   	ring->write_tail = ring_write_tail;
>   	ring->flush = gen6_ring_flush;
>   	ring->add_request = gen6_add_request;
> -	ring->get_seqno = gen6_ring_get_seqno;
> +	ring->irq_seqno_barrier = gen6_seqno_barrier;
> +	ring->get_seqno = ring_get_seqno;
>   	ring->set_seqno = ring_set_seqno;
>
>   	if (INTEL_INFO(dev)->gen >= 8) {
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 51fcb66bfc4a..3b49726b1732 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -219,8 +219,8 @@ struct  intel_engine_cs {
>   	 * seen value is good enough. Note that the seqno will always be
>   	 * monotonic, even if not coherent.
>   	 */
> -	u32		(*get_seqno)(struct intel_engine_cs *ring,
> -				     bool lazy_coherency);
> +	void		(*irq_seqno_barrier)(struct intel_engine_cs *ring);
> +	u32		(*get_seqno)(struct intel_engine_cs *ring);
>   	void		(*set_seqno)(struct intel_engine_cs *ring,
>   				     u32 seqno);
>   	int		(*dispatch_execbuffer)(struct drm_i915_gem_request *req,
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx