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

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

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

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 {