[PATCH v2] drm/amdgpu: Fix Illegal opcode in command stream Error

[Christian König]

Am 23.12.24 um 16:34 schrieb Arvind Yadav:
> When applications closes, it triggers the drm_file_free
> function which subsequently releases all allocated buffer
> objects. Concurrently, the resume_worker thread will attempt
> to map the usermode queue. However, since the wptr buffer
> object has already been deallocated, this will result in
> an Illegal opcode error being raised in the command stream.
>
> Now replacing drm_release() with a new function
> amdgpu_drm_release(). This function will set the flag to
> prevent the scheduling of any new queue resume/map, stop
> all queues and then call drm_release().
>
> V2:
>    - Replace drm_release with amdgpu_drm_release(Christian).
>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Christian Koenig <christian.koenig at amd.com>
> Cc: Shashank Sharma <shashank.sharma at amd.com>
> Signed-off-by: Arvind Yadav <arvind.yadav at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 16 +++++++++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  4 ----
>   2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index 62de668e9ff8..acb9dc3705ac 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2794,6 +2794,20 @@ static int amdgpu_pmops_runtime_idle(struct device *dev)
>   	return ret;
>   }
>   
> +static int amdgpu_drm_release(struct inode *inode, struct file *filp)
> +{
> +	struct drm_file *file_priv = filp->private_data;
> +	struct amdgpu_fpriv *fpriv = file_priv->driver_priv;
> +
> +	if (fpriv) {
> +		fpriv->evf_mgr.fd_closing = true;
> +		amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
> +		amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
> +	}
> +
> +	return drm_release(inode, filp);
> +}
> +
For now the patch is Reviewed-by: Christian König 
<christian.koenig at amd.com>.

But unrelated to this patch we should probably clean up all the DRM 
callback functions and move them into a separate file.

Somebody should probably setup a confluence page for small cleanup ideas.

Regards,
Christian.

>   long amdgpu_drm_ioctl(struct file *filp,
>   		      unsigned int cmd, unsigned long arg)
>   {
> @@ -2845,7 +2859,7 @@ static const struct file_operations amdgpu_driver_kms_fops = {
>   	.owner = THIS_MODULE,
>   	.open = drm_open,
>   	.flush = amdgpu_flush,
> -	.release = drm_release,
> +	.release = amdgpu_drm_release,
>   	.unlocked_ioctl = amdgpu_drm_ioctl,
>   	.mmap = drm_gem_mmap,
>   	.poll = drm_poll,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index d2a046736edd..f908355df07c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1478,10 +1478,6 @@ void amdgpu_driver_postclose_kms(struct drm_device *dev,
>   		amdgpu_bo_unreserve(pd);
>   	}
>   
> -	fpriv->evf_mgr.fd_closing = true;
> -	amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
> -	amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
> -
>   	amdgpu_ctx_mgr_fini(&fpriv->ctx_mgr);
>   	amdgpu_vm_fini(adev, &fpriv->vm);
>