Re: [PATCH 02/39] drm/i915: Updating assorted register and status page definitions

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jan 08, 2016 at 02:26:40PM +0200, Joonas Lahtinen wrote:
> Hi,
> 
> Comments below.
> 
> On ma, 2015-11-23 at 11:38 +0000, John.C.Harrison@Intel.com wrote:
> > From: Dave Gordon <david.s.gordon@intel.com>
> > 
> > Added various definitions that will be useful for the scheduler in
> > general and
> > pre-emptive context switching in particular.
> > 
> > Change-Id: Ica805b94160426def51f5d520f5ce51c60864a98
> > For: VIZ-1587
> > Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h         | 30
> > +++++++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/intel_ringbuffer.h | 32
> > +++++++++++++++++++++++++++++---
> >  2 files changed, 58 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index db72f98..b986050 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -259,6 +259,10 @@
> >  #define  MI_GLOBAL_GTT    (1<<22)
> >  
> >  #define MI_NOOP			MI_INSTR(0, 0)
> > +#define   MI_NOOP_WRITE_ID		(1<<22)
> > +#define   MI_NOOP_ID_MASK		((1<<22) - 1)
> > +#define   MI_NOOP_MID(id)		((id) & MI_NOOP_ID_MASK)
> 
> I'd stay consistent with the style of other _MASK/_SHIFT pairs
> elsewhere in this header. I find this rather unintuitive compared to
> 0xff notation especially if it would be with nonzero shift. MI_NOOP_MID
> is extra, too.
> 
> Most of these new definitions are not used anywhere in the series?
> Partially due to that I would prefer these to be introduced in smaller
> patches prior to first use, at least to an extent that one patch per
> series, with only stuff being used. Would make reviewing much easier to
> know they have been actually tested too.
> 
> Daniel, how do you want to see the register updates? As much added at once as possible, or only needed?

I'm fine with a separate patch to just update them all. We do that
regularly for feature work.
-Daniel

> 
> > +#define MI_NOOP_WITH_ID(id)	MI_INSTR(0,
> > MI_NOOP_WRITE_ID|MI_NOOP_MID(id))
> >  #define MI_USER_INTERRUPT	MI_INSTR(0x02, 0)
> >  #define MI_WAIT_FOR_EVENT       MI_INSTR(0x03, 0)
> >  #define   MI_WAIT_FOR_OVERLAY_FLIP	(1<<16)
> > @@ -276,6 +280,7 @@
> >  #define MI_ARB_ON_OFF		MI_INSTR(0x08, 0)
> >  #define   MI_ARB_ENABLE			(1<<0)
> >  #define   MI_ARB_DISABLE		(0<<0)
> > +#define MI_ARB_CHECK		MI_INSTR(0x05, 0)
> >  #define MI_BATCH_BUFFER_END	MI_INSTR(0x0a, 0)
> >  #define MI_SUSPEND_FLUSH	MI_INSTR(0x0b, 0)
> >  #define   MI_SUSPEND_FLUSH_EN	(1<<0)
> > @@ -325,6 +330,8 @@
> >  #define   MI_SEMAPHORE_SYNC_INVALID (3<<16)
> >  #define   MI_SEMAPHORE_SYNC_MASK    (3<<16)
> >  #define MI_SET_CONTEXT		MI_INSTR(0x18, 0)
> > +#define   MI_CONTEXT_ADDR_MASK		((~0)<<12)
> > +#define   MI_SET_CONTEXT_FLAG_MASK	((1<<12)-1)
> >  #define   MI_MM_SPACE_GTT		(1<<8)
> >  #define   MI_MM_SPACE_PHYSICAL		(0<<8)
> >  #define   MI_SAVE_EXT_STATE_EN		(1<<3)
> > @@ -344,6 +351,10 @@
> >  #define   MI_USE_GGTT		(1 << 22) /* g4x+ */
> >  #define MI_STORE_DWORD_INDEX	MI_INSTR(0x21, 1)
> >  #define   MI_STORE_DWORD_INDEX_SHIFT 2
> > +#define MI_STORE_REG_MEM	MI_INSTR(0x24, 1)
> > +#define   MI_STORE_REG_MEM_GTT		(1 << 22)
> > +#define   MI_STORE_REG_MEM_PREDICATE	(1 << 21)
> > +
> >  /* Official intel docs are somewhat sloppy concerning
> > MI_LOAD_REGISTER_IMM:
> >   * - Always issue a MI_NOOP _before_ the MI_LOAD_REGISTER_IMM -
> > otherwise hw
> >   *   simply ignores the register load under certain conditions.
> > @@ -358,7 +369,10 @@
> >  #define MI_FLUSH_DW		MI_INSTR(0x26, 1) /* for GEN6 */
> >  #define   MI_FLUSH_DW_STORE_INDEX	(1<<21)
> >  #define   MI_INVALIDATE_TLB		(1<<18)
> > +#define   MI_FLUSH_DW_OP_NONE		(0<<14)
> >  #define   MI_FLUSH_DW_OP_STOREDW	(1<<14)
> > +#define   MI_FLUSH_DW_OP_RSVD		(2<<14)
> > +#define   MI_FLUSH_DW_OP_STAMP		(3<<14)
> >  #define   MI_FLUSH_DW_OP_MASK		(3<<14)
> >  #define   MI_FLUSH_DW_NOTIFY		(1<<8)
> >  #define   MI_INVALIDATE_BSD		(1<<7)
> > @@ -1533,6 +1547,19 @@ enum skl_disp_power_wells {
> >  
> >  #define HSW_GTT_CACHE_EN	0x4024
> >  #define   GTT_CACHE_EN_ALL	0xF0007FFF
> > +
> > +/*
> > + * Premption-related registers
> > + */
> > +#define RING_UHPTR(base)	((base)+0x134)
> > +#define   UHPTR_GFX_ADDR_ALIGN		(0x7)
> > +#define   UHPTR_VALID			(0x1)
> > +#define RING_PREEMPT_ADDR	0x0214c
> > +#define   PREEMPT_BATCH_LEVEL_MASK	(0x3)
> > +#define BB_PREEMPT_ADDR		0x02148
> > +#define SBB_PREEMPT_ADDR	0x0213c
> > +#define RS_PREEMPT_STATUS	0x0215c
> > +
> >  #define GEN7_WR_WATERMARK	0x4028
> >  #define GEN7_GFX_PRIO_CTRL	0x402C
> >  #define ARB_MODE		0x4030
> > @@ -6736,7 +6763,8 @@ enum skl_disp_power_wells {
> >  #define  VLV_SPAREG2H				0xA194
> >  
> >  #define  GTFIFODBG				0x120000
> > -#define    GT_FIFO_SBDROPERR			(1<<6)
> > +#define    GT_FIFO_CPU_ERROR_MASK		0xf
> > +#define    GT_FIFO_SDDROPERR			(1<<6)
> >  #define    GT_FIFO_BLOBDROPERR			(1<<5)
> >  #define    GT_FIFO_SB_READ_ABORTERR		(1<<4)
> >  #define    GT_FIFO_DROPERR			(1<<3)
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > index 1987abd..48f60cc 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> > @@ -49,6 +49,12 @@ struct  intel_hw_status_page {
> >  #define I915_READ_MODE(ring) I915_READ(RING_MI_MODE((ring)
> > ->mmio_base))
> >  #define I915_WRITE_MODE(ring, val) I915_WRITE(RING_MI_MODE((ring)
> > ->mmio_base), val)
> >  
> > +#define I915_READ_UHPTR(ring) \
> > +		I915_READ(RING_UHPTR((ring)->mmio_base))
> > +#define I915_WRITE_UHPTR(ring, val) \
> > +		I915_WRITE(RING_UHPTR((ring)->mmio_base), val)
> > +#define I915_READ_NOPID(ring) I915_READ(RING_NOPID((ring)
> > ->mmio_base))
> > +
> >  /* seqno size is actually only a uint32, but since we plan to use
> > MI_FLUSH_DW to
> >   * do the writes, and that must have qw aligned offsets, simply
> > pretend it's 8b.
> >   */
> > @@ -426,10 +432,30 @@ intel_write_status_page(struct intel_engine_cs
> > *ring,
> >   * 0x20-0x2f: Reserved (Gen6+)
> >   *
> >   * The area from dword 0x30 to 0x3ff is available for driver usage.
> > + *
> > + * Note: in general the allocation of these indices is arbitrary, as
> > long
> > + * as they are all unique. But a few of them are used with
> > instructions that
> > + * have specific alignment requirements, those particular indices
> > must be
> > + * chosen carefully to meet those requirements. The list below shows
> > the
> > + * currently-known alignment requirements:
> > + *
> > + *	I915_GEM_SCRATCH_INDEX	    must be EVEN
> > + */
> > +
> > +/*
> > + * Tracking; these are updated by the GPU at the beginning and/or
> > end of every
> > + * batch. One pair for regular buffers, the other for preemptive
> > ones.
> >   */
> > -#define I915_GEM_HWS_INDEX		0x30
> > -#define I915_GEM_HWS_SCRATCH_INDEX	0x40
> > -#define I915_GEM_HWS_SCRATCH_ADDR (I915_GEM_HWS_SCRATCH_INDEX <<
> > MI_STORE_DWORD_INDEX_SHIFT)
> > +#define I915_BATCH_DONE_SEQNO		0x30  /* Completed
> > batch seqno        */
> > +#define I915_BATCH_ACTIVE_SEQNO		0x31  /* In progress
> > batch seqno      */
> > +#define I915_PREEMPTIVE_DONE_SEQNO	0x32  /* Completed
> > preemptive batch   */
> > +#define I915_PREEMPTIVE_ACTIVE_SEQNO	0x33  /* In progress
> > preemptive batch */
> > +
> > +#define I915_GEM_HWS_INDEX		I915_BATCH_DONE_SEQNO	
> > /* alias */
> > +#define I915_GEM_ACTIVE_SEQNO_INDEX	I915_BATCH_ACTIVE_SEQNO	
> > /* alias */
> > +
> > +#define I915_GEM_HWS_SCRATCH_INDEX	0x38  /* QWord */
> > +#define I915_GEM_HWS_SCRATCH_ADDR	(I915_GEM_HWS_SCRATCH_INDEX
> > << MI_STORE_DWORD_INDEX_SHIFT)
> >  
> >  struct intel_ringbuffer *
> >  intel_engine_create_ringbuffer(struct intel_engine_cs *engine, int
> > size);
> -- 
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx