[PATCH 2/4] drm/amdgpu: Add AQUIRE_MEM PACKET3 fields defintion

[Luben Tuikov]

On 2020-03-25 10:29, Andrey Grodzovsky wrote:
> Add this for gfx10 and gfx9.
> 
> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/nvd.h    | 48 +++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/amd/amdgpu/soc15d.h | 25 ++++++++++++++++++-
>  2 files changed, 72 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/nvd.h b/drivers/gpu/drm/amd/amdgpu/nvd.h
> index f3d8771..7785ea5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/nvd.h
> +++ b/drivers/gpu/drm/amd/amdgpu/nvd.h
> @@ -256,6 +256,54 @@
>  #define	PACKET3_BLK_CNTX_UPDATE				0x53
>  #define	PACKET3_INCR_UPDT_STATE				0x55
>  #define	PACKET3_ACQUIRE_MEM				0x58
> +/* 1.  HEADER
> + * 2.  COHER_CNTL [30:0]
> + * 2.1 ENGINE_SEL [31:31]
> + * 2.  COHER_SIZE [31:0]
> + * 3.  COHER_SIZE_HI [7:0]
> + * 4.  COHER_BASE_LO [31:0]
> + * 5.  COHER_BASE_HI [23:0]
> + * 7.  POLL_INTERVAL [15:0]
> + * 8.  GCR_CNTL [18:0]
> + */
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GLI_INV(x) ((x) << 0)

I think it's visually better to not break up the "#" and "define" and
to not bunch up "define" and the macro name, to intead look like this:

#define 	PACKET3_ACQUIRE_MEM_GCR_CNTL_GLI_INV(x)        ((x) << 0)

Which creates visual cadence, for the eyes to follow. It also creates
a vertical visual separation (reading down) since then the left column
is not just white space reading down, but breaks at the definition of
each register field. (once changed for all, you'll see it)

> +		/*
> +		 * 0:NOP
> +		 * 1:ALL
> +		 * 2:RANGE
> +		 * 3:FIRST_LAST
> +		 */
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GL1_RANGE(x) ((x) << 2)
> +		/*
> +		 * 0:ALL
> +		 * 1:reserved
> +		 * 2:RANGE
> +		 * 3:FIRST_LAST
> +		 */
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GLM_WB(x) ((x) << 4)
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GLM_INV(x) ((x) << 5)
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GLK_WB(x) ((x) << 6)
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GLK_INV(x) ((x) << 7)
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GLV_INV(x) ((x) << 8)
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GL1_INV(x) ((x) << 9)
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GL2_US(x) ((x) << 10)
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GL2_RANGE(x) ((x) << 11)
> +		/*
> +		 * 0:ALL
> +		 * 1:VOL
> +		 * 2:RANGE
> +		 * 3:FIRST_LAST
> +		 */
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GL2_DISCARD(x)  ((x) << 13)
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GL2_INV(x) ((x) << 14)
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_GL2_WB(x) ((x) << 15)
> +#              define PACKET3_ACQUIRE_MEM_GCR_CNTL_SEQ(x) ((x) << 16)
> +		/*
> +		 * 0: PARALLEL
> +		 * 1: FORWARD
> +		 * 2: REVERSE
> +		 */
> +#              define PACKET3_ACQUIRE_MEM_GCR_RANGE_IS_PA  (1 << 18)
>  #define	PACKET3_REWIND					0x59
>  #define	PACKET3_INTERRUPT				0x5A
>  #define	PACKET3_GEN_PDEPTE				0x5B
> diff --git a/drivers/gpu/drm/amd/amdgpu/soc15d.h b/drivers/gpu/drm/amd/amdgpu/soc15d.h
> index 295d68c..8983871 100644
> --- a/drivers/gpu/drm/amd/amdgpu/soc15d.h
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15d.h
> @@ -253,7 +253,30 @@
>  #              define PACKET3_DMA_DATA_CMD_SAIC    (1 << 28)
>  #              define PACKET3_DMA_DATA_CMD_DAIC    (1 << 29)
>  #              define PACKET3_DMA_DATA_CMD_RAW_WAIT  (1 << 30)
> -#define	PACKET3_AQUIRE_MEM				0x58
> +#define	PACKET3_ACQUIRE_MEM				0x58
> +/* 1.  HEADER
> + * 2.  COHER_CNTL [30:0]
> + * 2.1 ENGINE_SEL [31:31]
> + * 3.  COHER_SIZE [31:0]
> + * 4.  COHER_SIZE_HI [7:0]
> + * 5.  COHER_BASE_LO [31:0]
> + * 6.  COHER_BASE_HI [23:0]
> + * 7.  POLL_INTERVAL [15:0]
> + */
> +/* COHER_CNTL fields for CP_COHER_CNTL */
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_TC_NC_ACTION_ENA(x) ((x) << 3)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_TC_WC_ACTION_ENA(x) ((x) << 4)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_TC_INV_METADATA_ACTION_ENA(x) ((x) << 5)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_TCL1_VOL_ACTION_ENA(x) ((x) << 15)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_TC_WB_ACTION_ENA(x) ((x) << 18)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_TCL1_ACTION_ENA(x) ((x) << 22)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_TC_ACTION_ENA(x) ((x) << 23)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_CB_ACTION_ENA(x) ((x) << 25)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_DB_ACTION_ENA(x) ((x) << 26)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_SH_KCACHE_ACTION_ENA(x) ((x) << 27)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_SH_KCACHE_VOL_ACTION_ENA(x) ((x) << 28)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_SH_ICACHE_ACTION_ENA(x) ((x) << 29)
> +#              define PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_SH_KCACHE_WB_ACTION_ENA(x) ((x) << 30)

The same sentiment here. There is no reason to break the "#" and the "define",
which is usually seen as "#define" by the language. Instead keeping
the "#" and the "define" together and then white-space to the macro name would
make for a better and move familiar visual cadence. Like,

#define		PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_TC_NC_ACTION_ENA(x)           ((x) << 3)
#define		PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_TC_WC_ACTION_ENA(x)           ((x) << 4)
#define		PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_TC_INV_METADATA_ACTION_ENA(x) ((x) << 5)
#define		PACKET3_ACQUIRE_MEM_CP_COHER_CNTL_TCL1_VOL_ACTION_ENA(x)        ((x) << 15)
...


>  #define	PACKET3_REWIND					0x59
>  #define	PACKET3_LOAD_UCONFIG_REG			0x5E
>  #define	PACKET3_LOAD_SH_REG				0x5F

Like it is done here ^.

Regards,
Luben