On 23.06.25 17:04, vitaly prosyak wrote:
>
> On 2025-06-23 10:34, Christian König wrote:
>> On 19.06.25 04:26, Alex Deucher wrote:
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 12 +++---------
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 3 +++
>>>> 2 files changed, 6 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> index 7fd233f160bf..204178d949e1 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> @@ -2914,16 +2914,10 @@ static int amdgpu_pmops_runtime_idle(struct device
>>>> *dev)
>>>>
>>>> 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_eviction_fence_destroy(&fpriv->evf_mgr);
>>>> - amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>>>> - }
>>>> + int r;
>>>>
>>>> - return drm_release(inode, filp);
>>>> + r = drm_release(inode, filp);
>>>> + return r;
>>>> }
>>> You can just drop amdgpu_drm_release() altogether and just assign
>>> drm_release() as the callback directly.
>> Stop, that change here is just broken.
>>
>> We need to call amdgpu_userq_mgr_fini() before drm_release() since the later
>> will release all BOs and so eventually also the ring buffer of the user
>> queues.
> Yes, we observe that amdgpu_driver_postclose_kms is invoked before
> drm_release. Within amdgpu_driver_postclose_kms, we perform all the necessary
> cleanup steps, including:
>
> amdgpu_eviction_fence_destroy()
>
> amdgpu_userq_mgr_fini()
>
> amdgpu_ctx_mgr_fini()
>
> amdgpu_vm_fini()
>
> These functions handle eviction fences, user queue management, context
> management, and VM cleanup respectively
That won't work. See drm_file_free():
...
if (drm_core_check_feature(dev, DRIVER_GEM))
drm_gem_release(dev, file);
if (drm_is_primary_client(file))
drm_master_release(file);
if (dev->driver->postclose)
dev->driver->postclose(dev, file);
...
The GEM handles are released first and then postclose is called.
So postclose is to late to stop the queues from processing.
Regards,
Christian.
>
> Thanks, Vitaly
>
>>
>> Regards,
>> Christian.
>>
>>> Alex
>>>
>>>> long amdgpu_drm_ioctl(struct file *filp,
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> index d2ce7d86dbc8..195ed81d39ff 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>> @@ -1501,6 +1501,9 @@ void amdgpu_driver_postclose_kms(struct drm_device
>>>> *dev,
>>>> amdgpu_vm_bo_del(adev, fpriv->prt_va);
>>>> amdgpu_bo_unreserve(pd);
>>>> }
>>>> + fpriv->evf_mgr.fd_closing = true;
>>>> + amdgpu_eviction_fence_destroy(&fpriv->evf_mgr);
>>>> + amdgpu_userq_mgr_fini(&fpriv->userq_mgr);
>>>>
>>>> amdgpu_ctx_mgr_fini(&fpriv->ctx_mgr);
>>>> amdgpu_vm_fini(adev, &fpriv->vm);
>>>> --
>>>> 2.34.1
>>>>
