Re: [PATCH 06/13] drm/i915/bdw: implement semaphore signal

From: Chris Wilson <chris@chris-wilson.co.uk>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>,
	Ben Widawsky <ben@bwidawsk.net>,
	Ben Widawsky <benjamin.widawsky@intel.com>
Subject: Re: [PATCH 06/13] drm/i915/bdw: implement semaphore signal
Date: Thu, 30 Jan 2014 12:46:58 +0000	[thread overview]
Message-ID: <20140130124658.GG29091@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20140130123817.GJ9454@intel.com>

On Thu, Jan 30, 2014 at 02:38:17PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 29, 2014 at 11:55:26AM -0800, Ben Widawsky wrote:
> > Semaphore signalling works similarly to previous GENs with the exception
> > that the per ring mailboxes no longer exist. Instead you must define
> > your own space, somewhere in the GTT.
> > 
> > The comments in the code define the layout I've opted for, which should
> > be fairly future proof. Ie. I tried to define offsets in abstract terms
> > (NUM_RINGS, seqno size, etc).
> > 
> > NOTE: If one wanted to move this to the HWSP they could. I've decided
> > one 4k object would be easier to deal with, and provide potential wins
> > with cache locality, but that's all speculative.
> > 
> > v2: Update the macro to not need the other ring's ring->id (Chris)
> > Update the comment to use the correct formula (Chris)
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h         |   1 +
> >  drivers/gpu/drm/i915/i915_reg.h         |   5 +-
> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 199 +++++++++++++++++++++++++-------
> >  drivers/gpu/drm/i915/intel_ringbuffer.h |  38 +++++-
> >  4 files changed, 197 insertions(+), 46 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 3673ba1..f521059 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1380,6 +1380,7 @@ typedef struct drm_i915_private {
> >  
> >  	struct pci_dev *bridge_dev;
> >  	struct intel_ring_buffer ring[I915_NUM_RINGS];
> > +	struct drm_i915_gem_object *semaphore_obj;
> >  	uint32_t last_seqno, next_seqno;
> >  
> >  	drm_dma_handle_t *status_page_dmah;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index cbbaf26..8b745dc 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -216,7 +216,7 @@
> >  #define   MI_DISPLAY_FLIP_IVB_SPRITE_B (3 << 19)
> >  #define   MI_DISPLAY_FLIP_IVB_PLANE_C  (4 << 19)
> >  #define   MI_DISPLAY_FLIP_IVB_SPRITE_C (5 << 19)
> > -#define MI_SEMAPHORE_MBOX	MI_INSTR(0x16, 1) /* gen6+ */
> > +#define MI_SEMAPHORE_MBOX	MI_INSTR(0x16, 1) /* gen6, gen7 */
> >  #define   MI_SEMAPHORE_GLOBAL_GTT    (1<<22)
> >  #define   MI_SEMAPHORE_UPDATE	    (1<<21)
> >  #define   MI_SEMAPHORE_COMPARE	    (1<<20)
> > @@ -241,6 +241,8 @@
> >  #define   MI_RESTORE_EXT_STATE_EN	(1<<2)
> >  #define   MI_FORCE_RESTORE		(1<<1)
> >  #define   MI_RESTORE_INHIBIT		(1<<0)
> > +#define MI_SEMAPHORE_SIGNAL	MI_INSTR(0x1b, 0) /* GEN8+ */
> > +#define   MI_SEMAPHORE_TARGET(engine)	((engine)<<15)
> >  #define MI_STORE_DWORD_IMM	MI_INSTR(0x20, 1)
> >  #define   MI_MEM_VIRTUAL	(1 << 22) /* 965+ only */
> >  #define MI_STORE_DWORD_INDEX	MI_INSTR(0x21, 1)
> > @@ -329,6 +331,7 @@
> >  #define   PIPE_CONTROL_TEXTURE_CACHE_INVALIDATE		(1<<10) /* GM45+ only */
> >  #define   PIPE_CONTROL_INDIRECT_STATE_DISABLE		(1<<9)
> >  #define   PIPE_CONTROL_NOTIFY				(1<<8)
> > +#define   PIPE_CONTROL_FLUSH_ENABLE			(1<<7) /* gen7+ */

Oh. So they changed how post-sync writes operated - this should be a
separate fix for stable I believe (so that batches are not run before we
have finished invalidating the TLBs required).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre