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

Joonas Lahtinen
2016-01-08T12:26:43Z

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