Re: [PATCH 03/13] drm/i915: Initial command parser table definitions

[Jani Nikula <jani.nikula@linux.intel.com>]

N.B. I'll likely make multiple passes on the patches while reviewing,
for example I did not check any of the #define values here.


On Wed, 29 Jan 2014, bradley.d.volkin@intel.com wrote:
> From: Brad Volkin <bradley.d.volkin@intel.com>
>
> Add command tables defining irregular length commands for each ring.
> This requires a few new command opcode definitions.
>
> OTC-Tracker: AXIA-4631
> Change-Id: I064bceb457e15f46928058352afe76d918c58ef5
> Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_cmd_parser.c | 157 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h        |  46 ++++++++++
>  2 files changed, 203 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 7639dbc..2e27bad 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -27,6 +27,148 @@
>  
>  #include "i915_drv.h"
>  
> +#define STD_MI_OPCODE_MASK  0xFF800000
> +#define STD_3D_OPCODE_MASK  0xFFFF0000
> +#define STD_2D_OPCODE_MASK  0xFFC00000
> +#define STD_MFX_OPCODE_MASK 0xFFFF0000
> +
> +#define CMD(op, opm, f, lm, fl, ...)		\
> +	{					\
> +		.flags = (fl) | (f),		\

Sparse gives me 

drivers/gpu/drm/i915/i915_cmd_parser.c:64:9: warning: dubious: x | !y

for the !F cases (bitwise OR with a logical NOT). I can see it's not a
bug here, but we want to keep those warnings down. Maybe just s/!F/0/ in
the tables? Or make the f argument to CMD a boolean, and make that

	.flags = (fl) | (f ? CMD_DESC_FIXED : 0),

> +		.cmd = { (op), (opm) }, 	\
> +		.length = { (lm) },		\
> +		__VA_ARGS__			\
> +	}
> +
> +/* Convenience macros to compress the tables */
> +#define SMI STD_MI_OPCODE_MASK
> +#define S3D STD_3D_OPCODE_MASK
> +#define S2D STD_2D_OPCODE_MASK
> +#define SMFX STD_MFX_OPCODE_MASK
> +#define F CMD_DESC_FIXED
> +#define S CMD_DESC_SKIP
> +#define R CMD_DESC_REJECT
> +#define W CMD_DESC_REGISTER
> +#define B CMD_DESC_BITMASK
> +#define M CMD_DESC_MASTER
> +
> +/*            Command                          Mask   Fixed Len   Action
> +	      ---------------------------------------------------------- */
> +static const struct drm_i915_cmd_descriptor common_cmds[] = {
> +	CMD(  MI_NOOP,                          SMI,    F,  1,      S  ),
> +	CMD(  MI_USER_INTERRUPT,                SMI,    F,  1,      S  ),
> +	CMD(  MI_WAIT_FOR_EVENT,                SMI,    F,  1,      S  ),
> +	CMD(  MI_ARB_CHECK,                     SMI,    F,  1,      S  ),
> +	CMD(  MI_REPORT_HEAD,                   SMI,    F,  1,      S  ),
> +	CMD(  MI_SUSPEND_FLUSH,                 SMI,    F,  1,      S  ),
> +	CMD(  MI_SEMAPHORE_MBOX,                SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_STORE_DWORD_INDEX,             SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_LOAD_REGISTER_IMM(1),          SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_STORE_REGISTER_MEM(1),         SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_LOAD_REGISTER_MEM,             SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_BATCH_BUFFER_START,            SMI,   !F,  0xFF,   S  ),
> +};
> +
> +static const struct drm_i915_cmd_descriptor render_cmds[] = {
> +	CMD(  MI_FLUSH,                         SMI,    F,  1,      S  ),
> +	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      S  ),
> +	CMD(  MI_PREDICATE,                     SMI,    F,  1,      S  ),
> +	CMD(  MI_TOPOLOGY_FILTER,               SMI,    F,  1,      S  ),
> +	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_SET_CONTEXT,                   SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_URB_CLEAR,                     SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_UPDATE_GTT,                    SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_CLFLUSH,                       SMI,   !F,  0x3FF,  S  ),
> +	CMD(  MI_CONDITIONAL_BATCH_BUFFER_END,  SMI,   !F,  0xFF,   S  ),
> +	CMD(  GFX_OP_3DSTATE_VF_STATISTICS,     S3D,    F,  1,      S  ),
> +	CMD(  PIPELINE_SELECT,                  S3D,    F,  1,      S  ),
> +	CMD(  GPGPU_OBJECT,                     S3D,   !F,  0xFF,   S  ),
> +	CMD(  GPGPU_WALKER,                     S3D,   !F,  0xFF,   S  ),
> +	CMD(  GFX_OP_3DSTATE_SO_DECL_LIST,      S3D,   !F,  0x1FF,  S  ),
> +};
> +
> +static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> +	CMD(  MI_SET_PREDICATE,                 SMI,    F,  1,      S  ),
> +	CMD(  MI_RS_CONTROL,                    SMI,    F,  1,      S  ),
> +	CMD(  MI_URB_ATOMIC_ALLOC,              SMI,    F,  1,      S  ),
> +	CMD(  MI_RS_CONTEXT,                    SMI,    F,  1,      S  ),
> +	CMD(  MI_LOAD_REGISTER_REG,             SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_RS_STORE_DATA_IMM,             SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_LOAD_URB_MEM,                  SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_STORE_URB_MEM,                 SMI,   !F,  0xFF,   S  ),
> +	CMD(  GFX_OP_3DSTATE_DX9_CONSTANTF_VS,  S3D,   !F,  0x7FF,  S  ),
> +	CMD(  GFX_OP_3DSTATE_DX9_CONSTANTF_PS,  S3D,   !F,  0x7FF,  S  ),
> +
> +	CMD(  GFX_OP_3DSTATE_BINDING_TABLE_EDIT_VS,  S3D,   !F,  0x1FF,  S  ),
> +	CMD(  GFX_OP_3DSTATE_BINDING_TABLE_EDIT_GS,  S3D,   !F,  0x1FF,  S  ),
> +	CMD(  GFX_OP_3DSTATE_BINDING_TABLE_EDIT_HS,  S3D,   !F,  0x1FF,  S  ),
> +	CMD(  GFX_OP_3DSTATE_BINDING_TABLE_EDIT_DS,  S3D,   !F,  0x1FF,  S  ),
> +	CMD(  GFX_OP_3DSTATE_BINDING_TABLE_EDIT_PS,  S3D,   !F,  0x1FF,  S  ),
> +};
> +
> +static const struct drm_i915_cmd_descriptor video_cmds[] = {
> +	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      S  ),
> +	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_CONDITIONAL_BATCH_BUFFER_END,  SMI,   !F,  0xFF,   S  ),
> +	/*
> +	 * MFX_WAIT doesn't fit the way we handle length for most commands.
> +	 * It has a length field but it uses a non-standard length bias.
> +	 * It is always 1 dword though, so just treat it as fixed length.
> +	 */
> +	CMD(  MFX_WAIT,                         SMFX,   F,  1,      S  ),
> +};
> +
> +static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
> +	CMD(  MI_ARB_ON_OFF,                    SMI,    F,  1,      S  ),
> +	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_CONDITIONAL_BATCH_BUFFER_END,  SMI,   !F,  0xFF,   S  ),
> +};
> +
> +static const struct drm_i915_cmd_descriptor blt_cmds[] = {
> +	CMD(  MI_DISPLAY_FLIP,                  SMI,   !F,  0xFF,   S  ),
> +	CMD(  MI_STORE_DWORD_IMM,               SMI,   !F,  0x3FF,  S  ),
> +	CMD(  COLOR_BLT,                        S2D,   !F,  0x3F,   S  ),
> +	CMD(  SRC_COPY_BLT,                     S2D,   !F,  0x3F,   S  ),
> +};
> +
> +#undef CMD
> +#undef SMI
> +#undef S3D
> +#undef S2D
> +#undef SMFX
> +#undef F
> +#undef S
> +#undef R
> +#undef W
> +#undef B
> +#undef M
> +
> +static const struct drm_i915_cmd_table gen7_render_cmds[] = {
> +	{ common_cmds, ARRAY_SIZE(common_cmds) },
> +	{ render_cmds, ARRAY_SIZE(render_cmds) },
> +};
> +
> +static const struct drm_i915_cmd_table hsw_render_ring_cmds[] = {
> +	{ common_cmds, ARRAY_SIZE(common_cmds) },
> +	{ render_cmds, ARRAY_SIZE(render_cmds) },
> +	{ hsw_render_cmds, ARRAY_SIZE(hsw_render_cmds) },
> +};
> +
> +static const struct drm_i915_cmd_table gen7_video_cmds[] = {
> +	{ common_cmds, ARRAY_SIZE(common_cmds) },
> +	{ video_cmds, ARRAY_SIZE(video_cmds) },
> +};
> +
> +static const struct drm_i915_cmd_table hsw_vebox_cmds[] = {
> +	{ common_cmds, ARRAY_SIZE(common_cmds) },
> +	{ vecs_cmds, ARRAY_SIZE(vecs_cmds) },
> +};
> +
> +static const struct drm_i915_cmd_table gen7_blt_cmds[] = {
> +	{ common_cmds, ARRAY_SIZE(common_cmds) },
> +	{ blt_cmds, ARRAY_SIZE(blt_cmds) },
> +};

A thought, if you added an end-of-array cell to all of these tables, I
think a lot of the initialization would be neater. If that seems like
too much trouble for too little gain, feel free to file this in the
bikeshedding bin.

> +
>  #define CLIENT_MASK      0xE0000000
>  #define SUBCLIENT_MASK   0x18000000
>  #define MI_CLIENT        0x00000000
> @@ -146,15 +288,30 @@ void i915_cmd_parser_init_ring(struct intel_ring_buffer *ring)
>  
>  	switch (ring->id) {
>  	case RCS:
> +		if (IS_HASWELL(ring->dev)) {
> +			ring->cmd_tables = hsw_render_ring_cmds;
> +			ring->cmd_table_count =
> +				ARRAY_SIZE(hsw_render_ring_cmds);
> +		} else {
> +			ring->cmd_tables = gen7_render_cmds;
> +			ring->cmd_table_count = ARRAY_SIZE(gen7_render_cmds);
> +		}
> +
>  		ring->get_cmd_length_mask = gen7_render_get_cmd_length_mask;
>  		break;
>  	case VCS:
> +		ring->cmd_tables = gen7_video_cmds;
> +		ring->cmd_table_count = ARRAY_SIZE(gen7_video_cmds);
>  		ring->get_cmd_length_mask = gen7_bsd_get_cmd_length_mask;
>  		break;
>  	case BCS:
> +		ring->cmd_tables = gen7_blt_cmds;
> +		ring->cmd_table_count = ARRAY_SIZE(gen7_blt_cmds);
>  		ring->get_cmd_length_mask = gen7_blt_get_cmd_length_mask;
>  		break;
>  	case VECS:
> +		ring->cmd_tables = hsw_vebox_cmds;
> +		ring->cmd_table_count = ARRAY_SIZE(hsw_vebox_cmds);
>  		/* VECS can use the same length_mask function as VCS */
>  		ring->get_cmd_length_mask = gen7_bsd_get_cmd_length_mask;
>  		break;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cbbaf26..13ed6ed 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -336,6 +336,52 @@
>  #define   PIPE_CONTROL_DEPTH_CACHE_FLUSH		(1<<0)
>  #define   PIPE_CONTROL_GLOBAL_GTT (1<<2) /* in addr dword */
>  
> +/*
> + * Commands used only by the command parser
> + */
> +#define MI_SET_PREDICATE       MI_INSTR(0x01, 0)
> +#define MI_ARB_CHECK           MI_INSTR(0x05, 0)
> +#define MI_RS_CONTROL          MI_INSTR(0x06, 0)
> +#define MI_URB_ATOMIC_ALLOC    MI_INSTR(0x09, 0)
> +#define MI_PREDICATE           MI_INSTR(0x0C, 0)
> +#define MI_RS_CONTEXT          MI_INSTR(0x0F, 0)
> +#define MI_TOPOLOGY_FILTER     MI_INSTR(0x0D, 0)
> +#define MI_URB_CLEAR           MI_INSTR(0x19, 0)
> +#define MI_UPDATE_GTT          MI_INSTR(0x23, 0)
> +#define MI_CLFLUSH             MI_INSTR(0x27, 0)
> +#define MI_LOAD_REGISTER_MEM   MI_INSTR(0x29, 0)
> +#define MI_LOAD_REGISTER_REG   MI_INSTR(0x2A, 0)
> +#define MI_RS_STORE_DATA_IMM   MI_INSTR(0x2B, 0)
> +#define MI_LOAD_URB_MEM        MI_INSTR(0x2C, 0)
> +#define MI_STORE_URB_MEM       MI_INSTR(0x2D, 0)
> +#define MI_CONDITIONAL_BATCH_BUFFER_END MI_INSTR(0x36, 0)
> +
> +#define PIPELINE_SELECT                ((0x3<<29)|(0x1<<27)|(0x1<<24)|(0x4<<16))
> +#define GFX_OP_3DSTATE_VF_STATISTICS   ((0x3<<29)|(0x1<<27)|(0x0<<24)|(0xB<<16))
> +#define GPGPU_OBJECT                   ((0x3<<29)|(0x2<<27)|(0x1<<24)|(0x4<<16))
> +#define GPGPU_WALKER                   ((0x3<<29)|(0x2<<27)|(0x1<<24)|(0x5<<16))
> +#define GFX_OP_3DSTATE_DX9_CONSTANTF_VS \
> +	((0x3<<29)|(0x3<<27)|(0x0<<24)|(0x39<<16))
> +#define GFX_OP_3DSTATE_DX9_CONSTANTF_PS \
> +	((0x3<<29)|(0x3<<27)|(0x0<<24)|(0x3A<<16))
> +#define GFX_OP_3DSTATE_SO_DECL_LIST \
> +	((0x3<<29)|(0x3<<27)|(0x1<<24)|(0x17<<16))
> +
> +#define GFX_OP_3DSTATE_BINDING_TABLE_EDIT_VS \
> +	((0x3<<29)|(0x3<<27)|(0x0<<24)|(0x43<<16))
> +#define GFX_OP_3DSTATE_BINDING_TABLE_EDIT_GS \
> +	((0x3<<29)|(0x3<<27)|(0x0<<24)|(0x44<<16))
> +#define GFX_OP_3DSTATE_BINDING_TABLE_EDIT_HS \
> +	((0x3<<29)|(0x3<<27)|(0x0<<24)|(0x45<<16))
> +#define GFX_OP_3DSTATE_BINDING_TABLE_EDIT_DS \
> +	((0x3<<29)|(0x3<<27)|(0x0<<24)|(0x46<<16))
> +#define GFX_OP_3DSTATE_BINDING_TABLE_EDIT_PS \
> +	((0x3<<29)|(0x3<<27)|(0x0<<24)|(0x47<<16))
> +
> +#define MFX_WAIT  ((0x3<<29)|(0x1<<27)|(0x0<<16))
> +
> +#define COLOR_BLT     ((0x2<<29)|(0x40<<22))
> +#define SRC_COPY_BLT  ((0x2<<29)|(0x43<<22))
>  
>  /*
>   * Reset registers
> -- 
> 1.8.5.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center