Re: [PATCH] drm/xe/uc: Use u64 for offsets for which we use upper_32_bits()

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 3/18/2024 3:33 PM, John Harrison wrote:
> On 3/18/2024 11:42, Daniele Ceraolo Spurio wrote:
>> The GGTT is currently a 32 bit address space, but the HW and GuC
>> support 48b addresses in GGTT-related operations, both to keep the
>> interface/HW paths common between PPGTT and GGTT and to allow for
>> future increase of the GGTT size.
>> This leaves us having to program a 64b field with a 32b offset, which
>> currently we're in some cases doing this by using an upper_32_bits()
>> call on a 32b variable, which doesn't make any sense. To do this cleanly
>> we have 2 options:
>>
>> 1 - Set the upper 32 bits directly to zero.
>> 2 - Use 64b variables for the offset and keep programming the whole 
>> thing,
>>      so we're ready if we ever have bigger offsets.
>>
>> This patch goes with option #2 and switches the related variables to 
>> u64.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> Cc: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_guc.c          | 2 +-
>>   drivers/gpu/drm/xe/xe_guc_hwconfig.c | 2 +-
>>   drivers/gpu/drm/xe/xe_guc_submit.c   | 2 +-
>>   drivers/gpu/drm/xe/xe_uc_fw.c        | 3 ++-
>>   4 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
>> index 9ed939c20602..1dfef2c29650 100644
>> --- a/drivers/gpu/drm/xe/xe_guc.c
>> +++ b/drivers/gpu/drm/xe/xe_guc.c
>> @@ -74,7 +74,7 @@ static u32 guc_ctl_feature_flags(struct xe_guc *guc)
>>     static u32 guc_ctl_log_params_flags(struct xe_guc *guc)
>>   {
>> -    u32 offset = guc_bo_ggtt_addr(guc, guc->log.bo) >> PAGE_SHIFT;
>> +    u64 offset = guc_bo_ggtt_addr(guc, guc->log.bo) >> PAGE_SHIFT;
> This one does not do upper/lower(offset). The offset is simply shifted 
> into a field position and OR'd into the u32 flags word below.
>
> The whole flags construction should probably be updated to use the reg 
> field macros, but I don't think making offset 64 bits gains anything 
> here.

True, I'll drop this one.

Daniele

>
> John.
>
>>       u32 flags;
>>         #if (((CRASH_BUFFER_SIZE) % SZ_1M) == 0)
>> diff --git a/drivers/gpu/drm/xe/xe_guc_hwconfig.c 
>> b/drivers/gpu/drm/xe/xe_guc_hwconfig.c
>> index ea49f3885c10..525e51cc7aa7 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_hwconfig.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_hwconfig.c
>> @@ -14,7 +14,7 @@
>>   #include "xe_guc.h"
>>   #include "xe_map.h"
>>   -static int send_get_hwconfig(struct xe_guc *guc, u32 ggtt_addr, 
>> u32 size)
>> +static int send_get_hwconfig(struct xe_guc *guc, u64 ggtt_addr, u32 
>> size)
>>   {
>>       u32 action[] = {
>>           XE_GUC_ACTION_GET_HWCONFIG,
>> diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c 
>> b/drivers/gpu/drm/xe/xe_guc_submit.c
>> index d51cb9a4a6b7..9d3771ec5ceb 100644
>> --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> @@ -533,7 +533,7 @@ static void register_engine(struct xe_exec_queue *q)
>>       info.flags = CONTEXT_REGISTRATION_FLAG_KMD;
>>         if (xe_exec_queue_is_parallel(q)) {
>> -        u32 ggtt_addr = xe_lrc_parallel_ggtt_addr(lrc);
>> +        u64 ggtt_addr = xe_lrc_parallel_ggtt_addr(lrc);
>>           struct iosys_map map = xe_lrc_parallel_map(lrc);
>>             info.wq_desc_lo = lower_32_bits(ggtt_addr +
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c 
>> b/drivers/gpu/drm/xe/xe_uc_fw.c
>> index 44b8c5f58fd8..7e44c468eb0f 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> @@ -788,7 +788,8 @@ static int uc_fw_xfer(struct xe_uc_fw *uc_fw, u32 
>> offset, u32 dma_flags)
>>   {
>>       struct xe_device *xe = uc_fw_to_xe(uc_fw);
>>       struct xe_gt *gt = uc_fw_to_gt(uc_fw);
>> -    u32 src_offset, dma_ctrl;
>> +    u64 src_offset;
>> +    u32 dma_ctrl;
>>       int ret;
>>         xe_force_wake_assert_held(gt_to_fw(gt), XE_FW_GT);
>