[Intel-gfx] [PATCH 019/190] drm/i915: Separate out the seqno-barrier from engine->get_seqno
Dave Gordon
david.s.gordon at intel.com
Mon Jan 11 07:43:32 PST 2016
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 at 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 at 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,
>
More information about the Intel-gfx
mailing list