[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