Re: [PATCH v3 2/4] drm/amdgpu: add drm_file reference in userq_mgr

["Khatri, Sunil" <sukhatri@amd.com>]

On 4/16/2025 12:59 PM, Tvrtko Ursulin wrote:
>
> On 15/04/2025 19:43, Sunil Khatri wrote:
>> drm_file will be used in usermode queues code to
>> enable better process information in logging and hence
>> add drm_file part of the userq_mgr struct.
>>
>> update the drm_file pointer in userq_mgr for each
>> amdgpu_driver_open_kms.
>>
>> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c       | 1 +
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h | 1 +
>>   2 files changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 3d319687c1c9..3de3071d66ee 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -1436,6 +1436,7 @@ int amdgpu_driver_open_kms(struct drm_device 
>> *dev, struct drm_file *file_priv)
>>         amdgpu_ctx_mgr_init(&fpriv->ctx_mgr, adev);
>>   +    fpriv->userq_mgr.file = file_priv;
>>       r = amdgpu_userq_mgr_init(&fpriv->userq_mgr, adev);
>
> It's a bit of a layering violation since amdgpu_userq_mgr_init() is 
> the place which otherwise initialises fpriv->user_mgr. One day someome 
> might put a memset in there for example. Anyway, I think it would be 
> nicer if you passed fpriv to that function. Potentially instead of 
> adev. Looks like that would be cleaner "design".
>
I agree totally this should be inside amdgpu_userq_mgr_init with fpriv 
passed to function. But i guess whoever wrote it in first place thought 
to make it same as done in a line above fot ctx_mgr. Once we have these 
patches merge i will push these fixes separately.

Regards
Sunil Khatri

> Regards,
>
> Tvrtko
>
>>       if (r)
>>           DRM_WARN("Can't setup usermode queues, use legacy workload 
>> submission only\n");
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
>> index 381b9c6f0573..fe51a45f7ee4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userqueue.h
>> @@ -77,6 +77,7 @@ struct amdgpu_userq_mgr {
>>       struct amdgpu_device        *adev;
>>       struct delayed_work        resume_work;
>>       struct list_head        list;
>> +    struct drm_file            *file;
>>   };
>>     struct amdgpu_db_info {
>