On 7/3/26 12:37, Tvrtko Ursulin wrote:
...
>>> +/**
>>> + * amdgpu_gem_copy_buffer_ioctl - copy buffer object content
>>> + *
>>> + * @dev: drm device pointer
>>> + * @data: drm_amdgpu_gem_copy_buffer
>>> + * @filp: drm file pointer
>>> + *
>>> + * Returns:
>>> + * 0 for success, -errno for errors.
>>> + */
>>> +int amdgpu_gem_copy_buffer_ioctl(struct drm_device *dev, void *data,
>>> + struct drm_file *filp)
>>> +{
>>> + struct amdgpu_copy_mem src_mem = {}, dst_mem = {};
>>> + union drm_amdgpu_gem_copy_buffer *args = data;
>>> + struct amdgpu_device *adev = drm_to_adev(dev);
>>> + struct drm_gem_object *src_gobj, *dst_gobj;
>>> + struct amdgpu_bo *src_bo, *dst_bo;
>>> + struct dma_fence *fence = NULL;
>>> + struct sync_file *sync_file;
>>> + struct drm_exec exec;
>>> + unsigned int e;
>>> + int r;
>>> +
>>> + src_gobj = drm_gem_object_lookup(filp, args->in.src_handle);
>>> + if (!src_gobj)
>>> + return -ENOENT;
>>> +
>>> + dst_gobj = drm_gem_object_lookup(filp, args->in.dst_handle);
>>> + if (!dst_gobj) {
>>> + r = -ENOENT;
>>> + goto err_dst;
>>> + }
>>> +
>>> + src_bo = gem_to_amdgpu_bo(src_gobj);
>>> + dst_bo = gem_to_amdgpu_bo(dst_gobj);
>>> +
>>> + if (amdgpu_bo_size(src_bo) < amdgpu_bo_size(dst_bo)) {
>>> + r = -E2BIG;
>>> + goto err_sizes;
>>> + }
>>
>> No sub range copy? I would expect something like a ~8MiB transfer buffer to
>> copy a 1GiB buffer in chunks of 2MiB.
>
> To preemptively make it generic or you already have an use case in mind? But
> sure, I can add that no problem.
If I'm not completely mistaken that will be needed for CRIU of large buffers at
some point. Especially for system without a large BAR.
Additional to that we need this for KFD/DRM render node unification as well and
IIRC the existing KFD IOCTL can do sub range copies.
>>> +
>>> + r = get_unused_fd_flags(O_CLOEXEC);
>>> + if (r < 0)
>>> + goto err_put_fence;
>>> +
>>> + sync_file = sync_file_create(fence);
>>
>> Oh, please no sync_file!
>>
>> Use a drm_syncobj, e.g. drm_syncobj_find() + dma_fence_chain_alloc() before
>> the copy and then drm_syncobj_add_point() when it is done.
>
> Why not, is sync_file so out of fashion? Syncobj feels heavy weight for what
> this is and it would require the CRIU side to create and destroy the syncobj.
The sync_file framework is only used by Android. It has quite a number of
shortcommings and is pretty much dead everywhere else.
For basically all modern IOCTLs we only use drm_syncobj timeline semaphores.
Regards,
Christian.
