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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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?

> +#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

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