On 3/13/26 09:53, Tvrtko Ursulin wrote:
>
> On 13/03/2026 08:45, Khatri, Sunil wrote:
>>
>> On 13-03-2026 02:09 pm, Tvrtko Ursulin wrote:
>>>
>>> On 10/03/2026 13:55, Sunil Khatri wrote:
>>>> In function amdgpu_userq_wait_ioctl, call function drm_gem_objects_lookup
>>>> only if the count is valid i.e non zero.
>>>>
>>>> In case of object count is 0 set the pointer to NULL for proper clean
>>>> up.
>>>
>>> Hasn't this been discussed already with the conclusion that nothing is
>>> broken? Or I am missing something?
>>> Regards,
>>>
>>> Tvrtko
>> Issue was seen again since Alex pulled 6.19 fixes from DRM and the
>> drm_gem_objects_lookup again got reverted to old and we started to see the
>> issue. With that in mind Alex too suggested that nothing wrong if we
>> explicitly check in driver too. Also i thought its only when handles are
>> Zero case when the problem is actually seen and why not that have a check
>> targeting that and not to invoke the function drm_gem_objects_lookup itself
>> for that specific case.
>
> Yes nothing wrong with checking, thanks for clarifying. I was just worried I
> again missed something in the code.
I'm clearly against adding those checks. It adds additional complexity because
of an issue somewhere else.
So we should probably just cherry pick the correct fix over into
amd-staging-drm-next instead.
Regards,
Christian.
>
> Regards,
>
> Tvrtko
>
>>>> Signed-off-by: Sunil Khatri <[email protected]>
>>>> ---
>>>> .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 32 ++++++++++++-------
>>>> 1 file changed, 20 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>>> index 76f32fd768fb..a4fff90b190a 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c
>>>> @@ -665,19 +665,27 @@ int amdgpu_userq_wait_ioctl(struct drm_device *dev,
>>>> void *data,
>>>> goto free_timeline_handles;
>>>> }
>>>> - r = drm_gem_objects_lookup(filp,
>>>> - u64_to_user_ptr(wait_info->bo_read_handles),
>>>> - num_read_bo_handles,
>>>> - &gobj_read);
>>>> - if (r)
>>>> - goto free_timeline_points;
>>>> + if (num_read_bo_handles) {
>>>> + r = drm_gem_objects_lookup(filp,
>>>> + u64_to_user_ptr(wait_info->bo_read_handles),
>>>> + num_read_bo_handles,
>>>> + &gobj_read);
>>>> + if (r)
>>>> + goto free_timeline_points;
>>>> + } else {
>>>> + gobj_read = NULL;
>>>> + }
>>>> - r = drm_gem_objects_lookup(filp,
>>>> - u64_to_user_ptr(wait_info->bo_write_handles),
>>>> - num_write_bo_handles,
>>>> - &gobj_write);
>>>> - if (r)
>>>> - goto put_gobj_read;
>>>> + if (num_write_bo_handles) {
>>>> + r = drm_gem_objects_lookup(filp,
>>>> + u64_to_user_ptr(wait_info->bo_write_handles),
>>>> + num_write_bo_handles,
>>>> + &gobj_write);
>>>> + if (r)
>>>> + goto put_gobj_read;
>>>> + } else {
>>>> + gobj_write = NULL;
>>>> + }
>>>> drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT,
>>>> (num_read_bo_handles + num_write_bo_handles));
>>>
>
