[Mesa-dev] [PATCH resend 1/7] i965: Define HW-binding table and resource streamer control opcodes

[Kenneth Graunke]

On Monday, June 01, 2015 03:14:24 PM Abdiel Janulgue wrote:
> v2: Simplify HW binding table bit definitions and magic constants (Topi)
> v3: Add Broadwell support.
> 
> Cc: kristian.h.kristensen at intel.com
> Cc: topi.pohjolainen at intel.com
> Cc: kenneth at whitecape.org
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue at linux.intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_defines.h | 30 ++++++++++++++++++++++++++++++
>  src/mesa/drivers/dri/i965/intel_reg.h   |  3 +++
>  2 files changed, 33 insertions(+)

FWIW, you probably didn't need to repost this so quickly - I'd already
started reviewing and writing replies.  Sorry it's taken so long.  I've
was really burnt out and then went on vacation.  It's near the top of my
list of things to review now though.

> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
> index dedc381..b9b4d1c 100644
> --- a/src/mesa/drivers/dri/i965/brw_defines.h
> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> @@ -1618,6 +1618,36 @@ enum brw_message_target {
>  #define _3DSTATE_BINDING_TABLE_POINTERS_GS	0x7829 /* GEN7+ */
>  #define _3DSTATE_BINDING_TABLE_POINTERS_PS	0x782A /* GEN7+ */
>  
> +#define _3DSTATE_BINDING_TABLE_POOL_ALLOC       0x7919 /* GEN7.5+ */
> +#define BRW_HW_BINDING_TABLE_ENABLE             (1 << 11)
> +#define GEN7_HW_BT_MOCS_SHIFT                   7
> +#define GEN7_HW_BT_MOCS_MASK                    INTEL_MASK(10, 7)
> +#define GEN8_HW_BT_MOCS_SHIFT                   0
> +#define GEN8_HW_BT_MOCS_MASK                    INTEL_MASK(6, 0)

Usually, we try to put the name of the packet in the field defines, i.e.
3DSTATE_PS_BLEND -> GEN8_PS_BLEND_HAS_WRITEABLE_RT
3DSTATE_WM_DEPTH_STENCIL -> GEN8_WM_DS_DEPTH_FUNC_SHIFT

With "HW_BT" it's not immediately clear whether it applies to the pool
alloc commands or the edit commands.  What do you think about
GEN7_BT_POOL_ALLOC_MOCS?  Or GEN7_HW_BT_POOL_MOCS?

It's not that big of a deal, though, since there are really only two
types of commands (pool alloc, edit).

> +/* Only required in HSW */
> +#define HSW_HW_BINDING_TABLE_RESERVED           (3 << 5)

Could we add "must be one" to the name, i.e. either
HSW_BT_POOL_ALLOC_RESERVED_MUST_BE_ONE or HSW_BT_POOL_ALLOC_MUST_BE_ONE?

I think that makes it clear that you're just setting some "must be one"
bits.  When I saw "reserved" in a pool allocation command I thought it
might be something about reserving space in the buffer.

> +
> +#define _3DSTATE_BINDING_TABLE_EDIT_VS          0x7843 /* GEN7.5 */
> +#define _3DSTATE_BINDING_TABLE_EDIT_GS          0x7844 /* GEN7.5 */
> +#define _3DSTATE_BINDING_TABLE_EDIT_HS          0x7845 /* GEN7.5 */
> +#define _3DSTATE_BINDING_TABLE_EDIT_DS          0x7846 /* GEN7.5 */
> +#define _3DSTATE_BINDING_TABLE_EDIT_PS          0x7847 /* GEN7.5 */
> +#define BRW_BINDING_TABLE_INDEX_SHIFT           16
> +#define BRW_BINDING_TABLE_INDEX_MASK            INTEL_MASK(23, 16)
> +
> +#define BRW_BINDING_TABLE_EDIT_TARGET_ALL       3
> +#define BRW_BINDING_TABLE_EDIT_TARGET_CORE1     2
> +#define BRW_BINDING_TABLE_EDIT_TARGET_CORE0     1
> +/* In HSW, when editing binding table entries to surface state offsets,
> + * the surface state offset is a 16-bit value aligned to 32 bytes. But
> + * Surface State Pointer in dword 2 is [15:0]. Right shift surf_offset
> + * by 5 bits so it won't disturb bit 16 (which is used as the binding
> + * table index entry), otherwise it would hang the GPU.
> + */
> +#define HSW_SURFACE_STATE_EDIT(value)           (value >> 5)
> +/* Same as Haswell, but surface state offsets now aligned to 64 bytes.*/
> +#define BDW_SURFACE_STATE_EDIT(value)           (value >> 6)

Please use GEN8_* for Broadwell.  We use HSW because it's a half
generation, and I thought GEN75_* or GEN7_5_* looked funny.  (The jury's
still out on whether that was a good idea.  Probably wasn't.)

With those changes, this patch would get:
Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

> +
>  #define _3DSTATE_SAMPLER_STATE_POINTERS		0x7802 /* GEN6+ */
>  # define PS_SAMPLER_STATE_CHANGE				(1 << 12)
>  # define GS_SAMPLER_STATE_CHANGE				(1 << 9)
> diff --git a/src/mesa/drivers/dri/i965/intel_reg.h b/src/mesa/drivers/dri/i965/intel_reg.h
> index bd14e18..98adf45 100644
> --- a/src/mesa/drivers/dri/i965/intel_reg.h
> +++ b/src/mesa/drivers/dri/i965/intel_reg.h
> @@ -47,6 +47,9 @@
>  /* Load a value from memory into a register.  Only available on Gen7+. */
>  #define GEN7_MI_LOAD_REGISTER_MEM	(CMD_MI | (0x29 << 23))
>  # define MI_LOAD_REGISTER_MEM_USE_GGTT		(1 << 22)
> +/* Haswell RS control */
> +#define MI_RS_CONTROL                   (CMD_MI | (0x6 << 23))
> +#define MI_RS_STORE_DATA_IMM            (CMD_MI | (0x2b << 23))
>  
>  /* Manipulate the predicate bit based on some register values. Only on Gen7+ */
>  #define GEN7_MI_PREDICATE		(CMD_MI | (0xC << 23))
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150601/360430f5/attachment-0001.sig>