On 3/12/26 17:34, Tvrtko Ursulin wrote:
>> + /* Retrieve timeline fences */
>> + num_points = wait_info->num_syncobj_timeline_handles;
>> + for (i = 0; i < num_points; i++) {
>> + r = drm_syncobj_find_fence(filp, timeline_handles[i],
>> + timeline_points[i],
>> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT,
>> + &fence);
>> + if (r)
>> + goto free_fences;
>> +
>> + dma_fence_unwrap_for_each(f, &iter, fence) {
>> + if (num_fences >= wait_info->num_fences) {
>> + r = -EINVAL;
>
> dma_fence_put(fence);
Fixed.
>
> Also, maybe -EAGAIN?
>
> Or even consider dma_fence_dedup_array() and only bail out if it couldn't
> compact it.
I've replaced the error with a fallback in the next patch anyway.
>
>> + goto free_fences;
>> + }
>> - /* Array of fences */
>> - fences = kmalloc_array(wait_info->num_fences, sizeof(*fences),
>> GFP_KERNEL);
>> - if (!fences) {
>> - r = -ENOMEM;
>> - goto free_fence_info;
>> + fences[num_fences++] = dma_fence_get(f);
>> }
>> - /* Retrieve GEM read objects fence */
>> - for (i = 0; i < num_read_bo_handles; i++) {
>> - struct dma_resv_iter resv_cursor;
>> - struct dma_fence *fence;
>> + dma_fence_put(fence);
>> + }
>> +
>> + /* Retrieve boolean fences */
>> + num_syncobj = wait_info->num_syncobj_handles;
>> + for (i = 0; i < num_syncobj; i++) {
>> + struct dma_fence *fence;
>> - dma_resv_for_each_fence(&resv_cursor, gobj_read[i]->resv,
>> - DMA_RESV_USAGE_READ, fence) {
>> - if (num_fences >= wait_info->num_fences) {
>> - r = -EINVAL;
>
> Same as above.
Those doesn't hold an extra reference, so dma_fence_put would underflow the
refcount.
>> + if (!userq_fence) {
>> /*
>> - * We need to make sure the user queue release their reference
>> - * to the fence drivers at some point before queue destruction.
>> - * Otherwise, we would gather those references until we don't
>> - * have any more space left and crash.
>> + * Just waiting on other driver fences should
>> + * be good for now
>> */
>> - r = xa_alloc(&waitq->fence_drv_xa, &index, fence_drv,
>> - xa_limit_32b, GFP_KERNEL);
>
> Does it need to put potentially the same fence_drv multiple times into the
> same xarray?
That would be quite unlikely. We have one fence_drv for each fence context and
de-duplicate them above.
It could only happen if you call the wait_ioctl multiple times, but I don't see
an use case for that.
Thanks,
Christian.
>
>> + r = dma_fence_wait(fences[i], true);
>> if (r)
>> - goto free_fences;
>> + goto put_waitq;
>> +
>> + continue;
>> + }
>> - amdgpu_userq_fence_driver_get(fence_drv);
>> + fence_drv = userq_fence->fence_drv;
>> + /*
>> + * We need to make sure the user queue release their reference
>> + * to the fence drivers at some point before queue destruction.
>> + * Otherwise, we would gather those references until we don't
>> + * have any more space left and crash.
>> + */
>> + r = xa_alloc(&waitq->fence_drv_xa, &index, fence_drv,
>> + xa_limit_32b, GFP_KERNEL);
>> + if (r)
>> + goto put_waitq;
>> - /* Store drm syncobj's gpu va address and value */
>> - fence_info[cnt].va = fence_drv->va;
>> - fence_info[cnt].value = fences[i]->seqno;
>> + amdgpu_userq_fence_driver_get(fence_drv);
>> - dma_fence_put(fences[i]);
>> - /* Increment the actual userq fence count */
>> - cnt++;
>> - }
>> + /* Store drm syncobj's gpu va address and value */
>> + fence_info[cnt].va = fence_drv->va;
>> + fence_info[cnt].value = fences[i]->seqno;
>> - wait_info->num_fences = cnt;
>> - /* Copy userq fence info to user space */
>> - if (copy_to_user(u64_to_user_ptr(wait_info->out_fences),
>> - fence_info, wait_info->num_fences * sizeof(*fence_info))) {
>> - r = -EFAULT;
>> - goto free_fences;
>> - }
>> + /* Increment the actual userq fence count */
>> + cnt++;
>> }
>> + wait_info->num_fences = cnt;
>> +
>> + /* Copy userq fence info to user space */
>> + if (copy_to_user(u64_to_user_ptr(wait_info->out_fences),
>> + fence_info, cnt * sizeof(*fence_info)))
>> + r = -EFAULT;
>> + else
>> + r = 0;
>> +
>> +put_waitq:
>> + amdgpu_userq_put(waitq);
>> free_fences:
>> - if (fences) {
>> - while (num_fences-- > 0)
>> - dma_fence_put(fences[num_fences]);
>> - kfree(fences);
>> - }
>> + while (num_fences--)
>> + dma_fence_put(fences[num_fences]);
>> + kfree(fences);
>> +
>> free_fence_info:
>> kfree(fence_info);
>> -exec_fini:
>> + return r;
>> +
>> +error_unlock:
>> drm_exec_fini(&exec);
>> -put_gobj_write:
>> - for (i = 0; i < num_write_bo_handles; i++)
>> - drm_gem_object_put(gobj_write[i]);
>> - kfree(gobj_write);
>> + goto free_fences;
>> +}
>> +
>> +int amdgpu_userq_wait_ioctl(struct drm_device *dev, void *data,
>> + struct drm_file *filp)
>> +{
>> + int num_points, num_syncobj, num_read_bo_handles, num_write_bo_handles;
>> + u32 *syncobj_handles, *timeline_points, *timeline_handles;
>> + struct drm_amdgpu_userq_wait *wait_info = data;
>> + struct drm_gem_object **gobj_write;
>> + struct drm_gem_object **gobj_read;
>> + void __user *ptr;
>> + int r;
>> +
>> + if (!amdgpu_userq_enabled(dev))
>> + return -ENOTSUPP;
>> +
>> + if (wait_info->num_bo_write_handles > AMDGPU_USERQ_MAX_HANDLES ||
>> + wait_info->num_bo_read_handles > AMDGPU_USERQ_MAX_HANDLES)
>> + return -EINVAL;
>> +
>> + num_syncobj = wait_info->num_syncobj_handles;
>> + ptr = u64_to_user_ptr(wait_info->syncobj_handles);
>> + syncobj_handles = memdup_array_user(ptr, num_syncobj, sizeof(u32));
>> + if (IS_ERR(syncobj_handles))
>> + return PTR_ERR(syncobj_handles);
>> +
>> + num_points = wait_info->num_syncobj_timeline_handles;
>> + ptr = u64_to_user_ptr(wait_info->syncobj_timeline_handles);
>> + timeline_handles = memdup_array_user(ptr, num_points, sizeof(u32));
>> + if (IS_ERR(timeline_handles)) {
>> + r = PTR_ERR(timeline_handles);
>> + goto free_syncobj_handles;
>> + }
>> +
>> + ptr = u64_to_user_ptr(wait_info->syncobj_timeline_points);
>> + timeline_points = memdup_array_user(ptr, num_points, sizeof(u32));
>> + if (IS_ERR(timeline_points)) {
>> + r = PTR_ERR(timeline_points);
>> + goto free_timeline_handles;
>> + }
>> +
>> + num_read_bo_handles = wait_info->num_bo_read_handles;
>> + ptr = u64_to_user_ptr(wait_info->bo_read_handles),
>> + r = drm_gem_objects_lookup(filp, ptr, num_read_bo_handles, &gobj_read);
>> + if (r)
>> + goto free_timeline_points;
>> +
>> + num_write_bo_handles = wait_info->num_bo_write_handles;
>> + ptr = u64_to_user_ptr(wait_info->bo_write_handles),
>> + r = drm_gem_objects_lookup(filp, ptr, num_write_bo_handles,
>> + &gobj_write);
>> + if (r)
>> + goto put_gobj_read;
>> +
>> + /*
>> + * Passing num_fences = 0 means that userspace doesn't want to
>> + * retrieve userq_fence_info. If num_fences = 0 we skip filling
>> + * userq_fence_info and return the actual number of fences on
>> + * args->num_fences.
>> + */
>> + if (!wait_info->num_fences) {
>> + r = amdgpu_userq_wait_count_fences(filp, wait_info,
>> + syncobj_handles,
>> + timeline_points,
>> + timeline_handles,
>> + gobj_write,
>> + gobj_read);
>> + } else {
>> + r = amdgpu_userq_wait_return_fence_info(filp, wait_info,
>> + syncobj_handles,
>> + timeline_points,
>> + timeline_handles,
>> + gobj_write,
>> + gobj_read);
>> + }
>> +
>> + while (num_write_bo_handles--)
>> + drm_gem_object_put(gobj_write[num_write_bo_handles]);
>> + kvfree(gobj_write);
>> +
>> put_gobj_read:
>> - for (i = 0; i < num_read_bo_handles; i++)
>> - drm_gem_object_put(gobj_read[i]);
>> - kfree(gobj_read);
>> + while (num_read_bo_handles--)
>> + drm_gem_object_put(gobj_read[num_read_bo_handles]);
>> + kvfree(gobj_read);
>> +
>> free_timeline_points:
>> kfree(timeline_points);
>> free_timeline_handles:
>> kfree(timeline_handles);
>> free_syncobj_handles:
>> kfree(syncobj_handles);
>> -
>> - if (waitq)
>> - amdgpu_userq_put(waitq);
>> -
>> return r;
>> }
>
> The rest looks good. In my RFC I found a way to not duplicate the various
> fence walks between the counting and waiting, but yours works as well.
>
> Regards,
>
> Tvrtko
>
