Hi,

Comments below.

On ma, 2015-11-23 at 11:38 +0000, [email protected] wrote:
> From: Dave Gordon <[email protected]>
> 
> 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 <[email protected]>
> ---
>  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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to