Re: [PATCH v4 06/10] drm/amdgpu: set FW parameters in v11_struct

[Shashank Sharma <shashank.sharma@amd.com>]

On 25/04/2023 14:32, Christian König wrote:
> Am 24.04.23 um 19:38 schrieb Shashank Sharma:
>> From: Arvind Yadav <arvind.yadav@amd.com>
>>
>> This patch sets new parameters defined in v11_struct for
>> usermode queue with the respective allocated gpu context
>> space addresses.
>>
>> V4: Split the patch into two parts, keep only the set parameter
>>      part in this series (Alex).
>>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Christian Koenig <christian.koenig@amd.com>
>> Cc: Shashank Sharma <shashank.sharma@amd.com>
>> Signed-off-by: Arvind Yadav <arvind.yadav@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c 
>> b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
>> index f6b33faea86f..86de35292f69 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v11_0.c
>> @@ -6411,6 +6411,21 @@ const struct amdgpu_ip_block_version 
>> gfx_v11_0_ip_block =
>>       .funcs = &gfx_v11_0_ip_funcs,
>>   };
>>   +static void gfx_v11_userq_set_ctx_space(struct amdgpu_userq_mgr 
>> *uq_mgr,
>> +                    struct amdgpu_usermode_queue *queue)
>> +{
>> +    struct v11_gfx_mqd *mqd = queue->mqd.cpu_ptr;
>> +
>> +    mqd->shadow_base_lo = queue->shadow_ctx_gpu_addr & 0xfffffffc;
>> +    mqd->shadow_base_hi = upper_32_bits(queue->shadow_ctx_gpu_addr);
>> +
>> +    mqd->gds_bkup_base_lo = queue->gds_ctx_gpu_addr & 0xfffffffc;
>> +    mqd->gds_bkup_base_hi = upper_32_bits(queue->gds_ctx_gpu_addr);
>> +
>> +    mqd->fw_work_area_base_lo = queue->fw_ctx_gpu_addr & 0xfffffffc;
>> +    mqd->fw_work_area_base_lo = upper_32_bits(queue->fw_ctx_gpu_addr);
>
> Mhm, first of all please use lower_32_bits() here instead of the mask.
>
Noted,
> Then are you sure that is correct? The shadow, csa, gds are supposed 
> to be VAs as far as I know, but you use MC addresses here.

In my understanding, only the shadow area needs to be VA, which is true 
here as its being allocated by the user. We have tested the code and we 
did not see any problem yet, but I can cross check this and come back.

- Shashank

>
>
> Christian.
>
>> +}
>> +
>>   static int gfx_v11_userq_create_ctx_space(struct amdgpu_userq_mgr 
>> *uq_mgr,
>>                         struct amdgpu_usermode_queue *queue)
>>   {
>> @@ -6484,12 +6499,14 @@ gfx_v11_userq_mqd_create(struct 
>> amdgpu_userq_mgr *uq_mgr, struct amdgpu_usermode
>>       queue->userq_prop.use_doorbell = true;
>>       queue->userq_prop.mqd_gpu_addr = mqd->gpu_addr;
>>       r = gfx_v11_mqd->init_mqd(adev, (void *)mqd->cpu_ptr, 
>> &queue->userq_prop);
>> -    amdgpu_bo_unreserve(mqd->obj);
>>       if (r) {
>> +        amdgpu_bo_unreserve(mqd->obj);
>>           DRM_ERROR("Failed to init MQD for queue\n");
>>           goto free_ctx;
>>       }
>>   +    gfx_v11_userq_set_ctx_space(uq_mgr, queue);
>> +    amdgpu_bo_unreserve(mqd->obj);
>>       DRM_DEBUG_DRIVER("MQD for queue %d created\n", queue->queue_id);
>>       return 0;
>