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

[Lucas De Marchi]

On Tue, Nov 28, 2023 at 11:59:29PM -0600, Yang, Fei wrote:
>> 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?

I'd consider that small enough to just have in the same commit with a
note to the commit message. But it's something you will get a different
answer depending who you ask or when you ask. So... Let me just apply
this as is.

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

and applied to drm-xe-next.

Thanks
Lucas De Marchi


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