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

[Yang, Fei]

> 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

I agree we can define src_offset as u64, but that has to be a separate
patch, right? Should I send that change together with this one in the
same series, or this patch itself can be merged as is?

-Fei

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