[Intel-xe] [PATCH v2 08/17] drm/xe/guc: Convert GuC registers to REG_FIELD/REG_BIT

Lucas De Marchi lucas.demarchi at intel.com
Thu Apr 27 14:37:46 UTC 2023


On Thu, Apr 27, 2023 at 04:13:55PM +0200, Michal Wajdeczko wrote:
>
>
>On 26.04.2023 23:32, Lucas De Marchi wrote:
>> Cleanup GuC register declarations by converting them to use REG_FIELD,
>> REG_BIT and REG_GENMASK. While converting, also reorder the bitfields
>> so they follow the convention of declaring the higher bits first.
>>
>> v2:
>>   - Drop unused HUC_LOADING_AGENT_VCR and DMA_ADDRESS_SPACE_GTT (Matt Roper)
>>   - Simplify HUC_LOADING_AGENT_GUC define (Matt Roper)
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>  drivers/gpu/drm/xe/regs/xe_guc_regs.h | 165 ++++++++++++--------------
>>  drivers/gpu/drm/xe/xe_ggtt.c          |   6 +-
>>  drivers/gpu/drm/xe/xe_guc.c           |   8 +-
>>  drivers/gpu/drm/xe/xe_guc_ads.c       |   3 +-
>>  4 files changed, 86 insertions(+), 96 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/regs/xe_guc_regs.h b/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>> index 011868ff38aa..1960f9e78ec4 100644
>> --- a/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>> +++ b/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>> @@ -14,23 +14,18 @@
>>  /* Definitions of GuC H/W registers, bits, etc */
>>
>>  #define GUC_STATUS			_MMIO(0xc000)
>> -#define   GS_RESET_SHIFT		0
>> -#define   GS_MIA_IN_RESET		  (0x01 << GS_RESET_SHIFT)
>> -#define   GS_BOOTROM_SHIFT		1
>> -#define   GS_BOOTROM_MASK		  (0x7F << GS_BOOTROM_SHIFT)
>> -#define   GS_BOOTROM_RSA_FAILED		  (0x50 << GS_BOOTROM_SHIFT)
>> -#define   GS_BOOTROM_JUMP_PASSED	  (0x76 << GS_BOOTROM_SHIFT)
>> -#define   GS_UKERNEL_SHIFT		8
>> -#define   GS_UKERNEL_MASK		  (0xFF << GS_UKERNEL_SHIFT)
>> -#define   GS_MIA_SHIFT			16
>> -#define   GS_MIA_MASK			  (0x07 << GS_MIA_SHIFT)
>> -#define   GS_MIA_CORE_STATE		  (0x01 << GS_MIA_SHIFT)
>> -#define   GS_MIA_HALT_REQUESTED		  (0x02 << GS_MIA_SHIFT)
>> -#define   GS_MIA_ISR_ENTRY		  (0x04 << GS_MIA_SHIFT)
>> -#define   GS_AUTH_STATUS_SHIFT		30
>> -#define   GS_AUTH_STATUS_MASK		  (0x03 << GS_AUTH_STATUS_SHIFT)
>> -#define   GS_AUTH_STATUS_BAD		  (0x01 << GS_AUTH_STATUS_SHIFT)
>> -#define   GS_AUTH_STATUS_GOOD		  (0x02 << GS_AUTH_STATUS_SHIFT)
>> +#define   GS_AUTH_STATUS_MASK		REG_GENMASK(31, 30)
>> +#define   GS_AUTH_STATUS_BAD		REG_FIELD_PREP(GS_AUTH_STATUS_MASK, 0x1)
>> +#define   GS_AUTH_STATUS_GOOD		REG_FIELD_PREP(GS_AUTH_STATUS_MASK, 0x2)
>> +#define   GS_MIA_MASK			REG_GENMASK(18, 16)
>> +#define   GS_MIA_CORE_STATE		REG_FIELD_PREP(GS_MIA_MASK, 0x1)
>> +#define   GS_MIA_HALT_REQUESTED		REG_FIELD_PREP(GS_MIA_MASK, 0x2)
>> +#define   GS_MIA_ISR_ENTRY		REG_FIELD_PREP(GS_MIA_MASK, 0x4)
>> +#define   GS_UKERNEL_MASK		REG_GENMASK(15, 8)
>> +#define   GS_BOOTROM_MASK		REG_GENMASK(7, 1)
>> +#define   GS_BOOTROM_RSA_FAILED		REG_FIELD_PREP(GS_BOOTROM_MASK, 0x50)
>> +#define   GS_BOOTROM_JUMP_PASSED	REG_FIELD_PREP(GS_BOOTROM_MASK, 0x76)
>
>shouldn't we just define mask for Bootrom status here and define known
>codes in separate place/enum ? note that there could be more codes and

not what is usually done. We usually define the mask and the values
using REG_FIELD_PREP().

>we just want to extract that code from the register and print it (even
>if the value is not yet defined in the driver
>
>#define   GS_BOOTROM_STATUS_MASK	REG_GENMASK(7, 1)
>...
>enum {
>	BOOTROM_STATUS RSA_FAILED = 0x50,
>	BOOTROM_STATUS_JUMP_PASSED = 0x76,
>};

drivers/gpu/drm/xe/xe_guc.c:            if ((status & GS_BOOTROM_MASK) == GS_BOOTROM_RSA_FAILED) {

in this case it would change to:

	if (REG_FIELD_GET(GS_BOOTROM_MASK, status) == GS_BOOTROM_RSA_FAILED)

we do have some mixes of these styles around. I'm not sure one is
better than the other in all cases.

This patch however is only about the conversion to
REG_GENMASK/REG_FIELD_PREP. Other changes should be done on top

>
>> +#define   GS_MIA_IN_RESET		REG_BIT(0)>
>>  #define SOFT_SCRATCH(n)			_MMIO(0xc180 + (n) * 4)
>>  #define SOFT_SCRATCH_COUNT		16
>> @@ -42,90 +37,86 @@
>>  #define DMA_ADDR_0_HIGH			_MMIO(0xc304)
>>  #define DMA_ADDR_1_LOW			_MMIO(0xc308)
>>  #define DMA_ADDR_1_HIGH			_MMIO(0xc30c)
>> -#define   DMA_ADDRESS_SPACE_WOPCM	  (7 << 16)
>> -#define   DMA_ADDRESS_SPACE_GTT		  (8 << 16)
>> +#define   DMA_ADDR_SPACE_MASK		REG_GENMASK(20, 16)
>> +#define   DMA_ADDRESS_SPACE_WOPCM	REG_FIELD_PREP(DMA_ADDR_SPACE_MASK, 7)
>>  #define DMA_COPY_SIZE			_MMIO(0xc310)
>>  #define DMA_CTRL			_MMIO(0xc314)
>> -#define   HUC_UKERNEL			  (1<<9)
>> -#define   UOS_MOVE			  (1<<4)
>> -#define   START_DMA			  (1<<0)
>> +#define   HUC_UKERNEL			REG_BIT(9)
>> +#define   UOS_MOVE			REG_BIT(4)
>> +#define   START_DMA			REG_BIT(0)
>>  #define DMA_GUC_WOPCM_OFFSET		_MMIO(0xc340)
>> -#define   GUC_WOPCM_OFFSET_VALID	  (1<<0)
>> -#define   HUC_LOADING_AGENT_VCR		  (0<<1)
>> -#define   HUC_LOADING_AGENT_GUC		  (1<<1)
>>  #define   GUC_WOPCM_OFFSET_SHIFT	14
>> -#define   GUC_WOPCM_OFFSET_MASK		  (0x3ffff << GUC_WOPCM_OFFSET_SHIFT)
>> -#define GUC_MAX_IDLE_COUNT		_MMIO(0xC3E4)
>> +#define   GUC_WOPCM_OFFSET_MASK		REG_GENMASK(31, GUC_WOPCM_OFFSET_SHIFT)
>> +#define   HUC_LOADING_AGENT_GUC		REG_BIT(1)
>> +#define   GUC_WOPCM_OFFSET_VALID	REG_BIT(0)
>> +#define GUC_MAX_IDLE_COUNT		_MMIO(0xc3e4)
>>
>> -#define HUC_STATUS2             _MMIO(0xD3B0)
>> -#define   HUC_FW_VERIFIED       (1<<7)
>> +#define HUC_STATUS2			_MMIO(0xd3b0)
>> +#define   HUC_FW_VERIFIED		REG_BIT(7)
>>
>> -#define GEN11_HUC_KERNEL_LOAD_INFO	_MMIO(0xC1DC)
>> -#define   HUC_LOAD_SUCCESSFUL		  (1 << 0)
>> +#define GEN11_HUC_KERNEL_LOAD_INFO	_MMIO(0xc1dc)
>> +#define   HUC_LOAD_SUCCESSFUL		REG_BIT(0)
>>
>>  #define GUC_WOPCM_SIZE			_MMIO(0xc050)
>> -#define   GUC_WOPCM_SIZE_LOCKED		  (1<<0)
>> -#define   GUC_WOPCM_SIZE_SHIFT		12
>> -#define   GUC_WOPCM_SIZE_MASK		  (0xfffff << GUC_WOPCM_SIZE_SHIFT)
>> +#define   GUC_WOPCM_SIZE_MASK		REG_GENMASK(31, 12)
>> +#define   GUC_WOPCM_SIZE_LOCKED		REG_BIT(0)
>>
>>  #define GEN8_GT_PM_CONFIG		_MMIO(0x138140)
>>  #define GEN9LP_GT_PM_CONFIG		_MMIO(0x138140)
>>  #define GEN9_GT_PM_CONFIG		_MMIO(0x13816c)
>> -#define   GT_DOORBELL_ENABLE		  (1<<0)
>> +#define   GT_DOORBELL_ENABLE		REG_BIT(0)
>>
>>  #define GEN8_GTCR			_MMIO(0x4274)
>> -#define   GEN8_GTCR_INVALIDATE		  (1<<0)
>> -
>> -#define GEN12_GUC_TLB_INV_CR		_MMIO(0xcee8)
>> -#define   GEN12_GUC_TLB_INV_CR_INVALIDATE	(1 << 0)
>> -
>> -#define GUC_ARAT_C6DIS			_MMIO(0xA178)
>> -
>> -#define GUC_SHIM_CONTROL		_MMIO(0xc064)
>> -#define   GUC_DISABLE_SRAM_INIT_TO_ZEROES	(1<<0)
>> -#define   GUC_ENABLE_READ_CACHE_LOGIC		(1<<1)
>> -#define   GUC_ENABLE_MIA_CACHING		(1<<2)
>> -#define   GUC_GEN10_MSGCH_ENABLE		(1<<4)
>> -#define   GUC_ENABLE_READ_CACHE_FOR_SRAM_DATA	(1<<9)
>> -#define   GUC_ENABLE_READ_CACHE_FOR_WOPCM_DATA	(1<<10)
>> -#define   GUC_ENABLE_MIA_CLOCK_GATING		(1<<15)
>> -#define   GUC_GEN10_SHIM_WC_ENABLE		(1<<21)
>> +#define   GEN8_GTCR_INVALIDATE		REG_BIT(0)
>> +
>> +#define GEN12_GUC_TLB_INV_CR			_MMIO(0xcee8)
>> +#define   GEN12_GUC_TLB_INV_CR_INVALIDATE	REG_BIT(0)
>> +
>> +#define GUC_ARAT_C6DIS				_MMIO(0xa178)
>> +
>> +#define GUC_SHIM_CONTROL			_MMIO(0xc064)
>>  #define   PVC_GUC_MOCS_INDEX_MASK		REG_GENMASK(25, 24)
>> -#define   PVC_MOCS_UC_INDEX			1
>> -#define   PVC_GUC_MOCS_INDEX(index)		REG_FIELD_PREP(PVC_GUC_MOCS_INDEX_MASK,\
>> +#define   PVC_GUC_MOCS_UC_INDEX			1
>> +#define   PVC_GUC_MOCS_INDEX(index)		REG_FIELD_PREP(PVC_GUC_MOCS_INDEX_MASK, \
>>  							       index)
>> +#define   GUC_GEN10_SHIM_WC_ENABLE		REG_BIT(21)
>> +#define   GUC_ENABLE_MIA_CLOCK_GATING		REG_BIT(15)
>> +#define   GUC_ENABLE_READ_CACHE_FOR_WOPCM_DATA	REG_BIT(10)
>> +#define   GUC_ENABLE_READ_CACHE_FOR_SRAM_DATA	REG_BIT(9)
>> +#define   GUC_GEN10_MSGCH_ENABLE		REG_BIT(4)
>> +#define   GUC_ENABLE_MIA_CACHING		REG_BIT(2)
>> +#define   GUC_ENABLE_READ_CACHE_LOGIC		REG_BIT(1)
>> +#define   GUC_DISABLE_SRAM_INIT_TO_ZEROES	REG_BIT(0)
>>
>> -#define GUC_SEND_INTERRUPT		_MMIO(0xc4c8)
>> -#define   GUC_SEND_TRIGGER		  (1<<0)
>> -#define GEN11_GUC_HOST_INTERRUPT	_MMIO(0x1901f0)
>>
>> -#define GUC_NUM_DOORBELLS		256
>> +#define GUC_SEND_INTERRUPT			_MMIO(0xc4c8)
>> +#define   GUC_SEND_TRIGGER			REG_BIT(0)
>> +#define GEN11_GUC_HOST_INTERRUPT		_MMIO(0x1901f0)
>> +
>> +#define GUC_NUM_DOORBELLS			256
>>
>>  /* format of the HW-monitored doorbell cacheline */
>>  struct guc_doorbell_info {
>>  	u32 db_status;
>> -#define GUC_DOORBELL_DISABLED		0
>> -#define GUC_DOORBELL_ENABLED		1
>> +#define GUC_DOORBELL_DISABLED			0
>> +#define GUC_DOORBELL_ENABLED			1
>>
>>  	u32 cookie;
>>  	u32 reserved[14];
>>  } __packed;
>>
>> -#define GEN8_DRBREGL(x)			_MMIO(0x1000 + (x) * 8)
>> -#define   GEN8_DRB_VALID		  (1<<0)
>> -#define GEN8_DRBREGU(x)			_MMIO(0x1000 + (x) * 8 + 4)
>> +#define GEN8_DRBREGL(x)				_MMIO(0x1000 + (x) * 8)
>> +#define   GEN8_DRB_VALID			REG_BIT(0)
>> +#define GEN8_DRBREGU(x)				_MMIO(0x1000 + (x) * 8 + 4)
>>
>>  #define GEN12_DIST_DBS_POPULATED		_MMIO(0xd08)
>> -#define   GEN12_DOORBELLS_PER_SQIDI_SHIFT	16
>> -#define   GEN12_DOORBELLS_PER_SQIDI		(0xff)
>> -#define   GEN12_SQIDIS_DOORBELL_EXIST		(0xffff)
>> -
>> -#define DE_GUCRMR			_MMIO(0x44054)
>> +#define   GEN12_DOORBELLS_PER_SQIDI_MASK	REG_GENMASK(23, 16)
>> +#define   GEN12_SQIDIS_DOORBELL_EXIST_MASK	REG_GENMASK(15, 0)
>>
>> -#define GUC_BCS_RCS_IER			_MMIO(0xC550)
>> -#define GUC_VCS2_VCS1_IER		_MMIO(0xC554)
>> -#define GUC_WD_VECS_IER			_MMIO(0xC558)
>> -#define GUC_PM_P24C_IER			_MMIO(0xC55C)
>> +#define GUC_BCS_RCS_IER				_MMIO(0xC550)
>> +#define GUC_VCS2_VCS1_IER			_MMIO(0xC554)
>> +#define GUC_WD_VECS_IER				_MMIO(0xC558)
>> +#define GUC_PM_P24C_IER				_MMIO(0xC55C)
>>
>>  #define VF_SW_FLAG(n)			_MMIO(0x190240 + (n) * 4)
>>  #define VF_SW_FLAG_COUNT		4
>> @@ -134,21 +125,21 @@ struct guc_doorbell_info {
>>  #define MED_VF_SW_FLAG_COUNT		4
>>
>>  /* GuC Interrupt Vector */
>> -#define GUC_INTR_GUC2HOST		BIT(15)
>> -#define GUC_INTR_EXEC_ERROR		BIT(14)
>> -#define GUC_INTR_DISPLAY_EVENT		BIT(13)
>> -#define GUC_INTR_SEM_SIG		BIT(12)
>> -#define GUC_INTR_IOMMU2GUC		BIT(11)
>> -#define GUC_INTR_DOORBELL_RANG		BIT(10)
>> -#define GUC_INTR_DMA_DONE		BIT(9)
>> -#define GUC_INTR_FATAL_ERROR		BIT(8)
>> -#define GUC_INTR_NOTIF_ERROR		BIT(7)
>> -#define GUC_INTR_SW_INT_6		BIT(6)
>> -#define GUC_INTR_SW_INT_5		BIT(5)
>> -#define GUC_INTR_SW_INT_4		BIT(4)
>> -#define GUC_INTR_SW_INT_3		BIT(3)
>> -#define GUC_INTR_SW_INT_2		BIT(2)
>> -#define GUC_INTR_SW_INT_1		BIT(1)
>> -#define GUC_INTR_SW_INT_0		BIT(0)
>> +#define GUC_INTR_GUC2HOST			BIT(15)
>> +#define GUC_INTR_EXEC_ERROR			BIT(14)
>> +#define GUC_INTR_DISPLAY_EVENT			BIT(13)
>> +#define GUC_INTR_SEM_SIG			BIT(12)
>> +#define GUC_INTR_IOMMU2GUC			BIT(11)
>> +#define GUC_INTR_DOORBELL_RANG			BIT(10)
>> +#define GUC_INTR_DMA_DONE			BIT(9)
>> +#define GUC_INTR_FATAL_ERROR			BIT(8)
>> +#define GUC_INTR_NOTIF_ERROR			BIT(7)
>> +#define GUC_INTR_SW_INT_6			BIT(6)
>> +#define GUC_INTR_SW_INT_5			BIT(5)
>> +#define GUC_INTR_SW_INT_4			BIT(4)
>> +#define GUC_INTR_SW_INT_3			BIT(3)
>> +#define GUC_INTR_SW_INT_2			BIT(2)
>> +#define GUC_INTR_SW_INT_1			BIT(1)
>> +#define GUC_INTR_SW_INT_0			BIT(0)
>>
>>  #endif
>> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
>> index 9a5950024d9f..cc2d119bbbe8 100644
>> --- a/drivers/gpu/drm/xe/xe_ggtt.c
>> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
>> @@ -186,11 +186,11 @@ int xe_ggtt_init(struct xe_gt *gt, struct xe_ggtt *ggtt)
>>  }
>>
>>  #define GEN12_GUC_TLB_INV_CR                     _MMIO(0xcee8)
>> -#define   GEN12_GUC_TLB_INV_CR_INVALIDATE        (1 << 0)
>> +#define   GEN12_GUC_TLB_INV_CR_INVALIDATE        REG_BIT(0)
>>  #define PVC_GUC_TLB_INV_DESC0			_MMIO(0xcf7c)
>> -#define   PVC_GUC_TLB_INV_DESC0_VALID		 (1 << 0)
>> +#define   PVC_GUC_TLB_INV_DESC0_VALID		 REG_BIT(0)
>>  #define PVC_GUC_TLB_INV_DESC1			_MMIO(0xcf80)
>> -#define   PVC_GUC_TLB_INV_DESC1_INVALIDATE	 (1 << 6)
>> +#define   PVC_GUC_TLB_INV_DESC1_INVALIDATE	 REG_BIT(6)
>
>why we are still keeping some register definitions in .c when we already
>have separate regs/ folder where we can have all definitions?

in some cases, historical. In other, so that register isn't accessed
outside where it's supposed to be.

However as we improve ours regs definitions and even add some additional
checks in all headers in regs/*.h, we may want to eventually move them
all to regs/. In this series I did it for GuC as it was the biggest
offender


Lucas De Marchi

>
>>
>>  void xe_ggtt_invalidate(struct xe_gt *gt)
>>  {
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index e00177f4d294..d18f2e25ce56 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -357,7 +357,7 @@ static void guc_prepare_xfer(struct xe_guc *guc)
>>  				GUC_ENABLE_MIA_CACHING;
>>
>>  	if (xe->info.platform == XE_PVC)
>> -		shim_flags |= PVC_GUC_MOCS_INDEX(PVC_MOCS_UC_INDEX);
>> +		shim_flags |= PVC_GUC_MOCS_INDEX(PVC_GUC_MOCS_UC_INDEX);
>>
>>  	/* Must program this register before loading the ucode with DMA */
>>  	xe_mmio_write32(gt, GUC_SHIM_CONTROL.reg, shim_flags);
>> @@ -848,11 +848,11 @@ void xe_guc_print_info(struct xe_guc *guc, struct drm_printer *p)
>>
>>  	drm_printf(p, "\nGuC status 0x%08x:\n", status);
>>  	drm_printf(p, "\tBootrom status = 0x%x\n",
>> -		   (status & GS_BOOTROM_MASK) >> GS_BOOTROM_SHIFT);
>> +		   REG_FIELD_GET(GS_BOOTROM_MASK, status));
>>  	drm_printf(p, "\tuKernel status = 0x%x\n",
>> -		   (status & GS_UKERNEL_MASK) >> GS_UKERNEL_SHIFT);
>> +		   REG_FIELD_GET(GS_UKERNEL_MASK, status));
>>  	drm_printf(p, "\tMIA Core status = 0x%x\n",
>> -		   (status & GS_MIA_MASK) >> GS_MIA_SHIFT);
>> +		   REG_FIELD_GET(GS_MIA_MASK, status));
>
>maybe for indented logs we should use drm_printf_indent()
>and we can use %#x to get pretty hex output with 0x prefix
>
>>  	drm_printf(p, "\tLog level = %d\n",
>>  		   xe_guc_log_get_level(&guc->log));
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc_ads.c b/drivers/gpu/drm/xe/xe_guc_ads.c
>> index fe1d5be1241e..d4fc2d357a78 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_ads.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_ads.c
>> @@ -561,8 +561,7 @@ static void guc_doorbell_init(struct xe_guc_ads *ads)
>>
>>  		ads_blob_write(ads,
>>  			       system_info.generic_gt_sysinfo[GUC_GENERIC_GT_SYSINFO_DOORBELL_COUNT_PER_SQIDI],
>> -			       ((distdbreg >> GEN12_DOORBELLS_PER_SQIDI_SHIFT)
>> -				& GEN12_DOORBELLS_PER_SQIDI) + 1);
>> +			       REG_FIELD_GET(GEN12_DOORBELLS_PER_SQIDI_MASK, distdbreg) + 1);
>>  	}
>>  }
>>


More information about the Intel-xe mailing list