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

[Daniele Ceraolo Spurio]

On 11/27/2023 11:09 AM, Lucas De Marchi wrote:
> 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

This is still guaranteed to be a 32b number even after the sum, because 
we're adding an offset within the GGTT-mapped object and therefore the 
resulting value is still within the 32b address space.
AFAICT the reason we have upper_32_bits() calls in several places with 
GGTT offsets is because of a quirk of the HW specs: when 48b PPGTT was 
introduced back in gen8, all address fields in GT HW (not sure about the 
display side) have been updated to 48b, even the ones where the address 
was guaranteed to be GGTT and therefore 32b, and the specs have been 
updated to always say "write the upper 16 bits in the HDW reg/field" 
even if that is always guaranteed to be 0 for GGTT offsets. I'm guessing 
the aim on the specs side was to be ready if the GGTT ever gets bigger 
than 32b, so I think we should do the same in SW and proceed like you're 
suggesting with moving the variable to be a u64.

Daniele

> 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
>>