[Intel-xe] [PATCH 1/1] drm/xe: explicitly set GGTT access for GuC DMA

[Lucas De Marchi]

On Wed, Nov 22, 2023 at 12:45:01PM -0800, fei.yang at intel.com wrote:
>From: Fei Yang <fei.yang at intel.com>
>
>Confirmed with hardware that setting GGTT memory access for GuC
>firmware loading is correct for all platforms and required for
>new platforms going forward.
>
>Signed-off-by: Fei Yang <fei.yang at intel.com>
>---
> drivers/gpu/drm/xe/regs/xe_guc_regs.h | 1 +
> drivers/gpu/drm/xe/xe_uc_fw.c         | 3 ++-
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/xe/regs/xe_guc_regs.h b/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>index ba375fc51a87..92320bbc9d3d 100644
>--- a/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>+++ b/drivers/gpu/drm/xe/regs/xe_guc_regs.h
>@@ -70,6 +70,7 @@
> #define DMA_ADDR_1_HIGH				XE_REG(0xc30c)
> #define   DMA_ADDR_SPACE_MASK			REG_GENMASK(20, 16)
> #define   DMA_ADDRESS_SPACE_WOPCM		REG_FIELD_PREP(DMA_ADDR_SPACE_MASK, 7)
>+#define   DMA_ADDRESS_SPACE_GGTT		REG_FIELD_PREP(DMA_ADDR_SPACE_MASK, 8)
> #define DMA_COPY_SIZE				XE_REG(0xc310)
> #define DMA_CTRL				XE_REG(0xc314)
> #define   HUC_UKERNEL				REG_BIT(9)
>diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c b/drivers/gpu/drm/xe/xe_uc_fw.c
>index 3032c4f148d4..438ce51dd373 100644
>--- a/drivers/gpu/drm/xe/xe_uc_fw.c
>+++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>@@ -629,7 +629,8 @@ static int uc_fw_xfer(struct xe_uc_fw *uc_fw, u32 offset, u32 dma_flags)
> 	/* Set the source address for the uCode */
> 	src_offset = uc_fw_ggtt_offset(uc_fw) + uc_fw->css_offset;
> 	xe_mmio_write32(gt, DMA_ADDR_0_LOW, lower_32_bits(src_offset));
>-	xe_mmio_write32(gt, DMA_ADDR_0_HIGH, upper_32_bits(src_offset));

not that this patch changes anything but this upper_32_bits() is a code
smell. If uc_fw_ggtt_offset() can only return u32, why exactly we would
try to get the upper 32 bits (and suppress the warning by doing 2x >>
16?). Humn... commit ee2dee81e484 ("drm/xe/huc: Extract version and binary
offset from new HuC headers") extracted css_offset and now we are
adding 2 u32. So maybe keep the upper_32_bits(), but move src_offset to
u64. That also matches what we have in i915. Before that commit, doing
that write was basically always writting 0.

With that,

	Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>


+Daniele / +Matt Brost

Lucas De Marchi

>+	xe_mmio_write32(gt, DMA_ADDR_0_HIGH,
>+			upper_32_bits(src_offset) | DMA_ADDRESS_SPACE_GGTT);
>
> 	/* Set the DMA destination */
> 	xe_mmio_write32(gt, DMA_ADDR_1_LOW, offset);
>-- 
>2.25.1
>