| Message ID | 20240812065906.241398-2-viro@zeniv.linux.org.uk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/4] new helper: drm_gem_prime_handle_to_dmabuf() | expand |
On 2024-08-12 02:59, Al Viro wrote: > Using drm_gem_prime_handle_to_fd() to set dmabuf up and insert it into > descriptor table, only to have it looked up by file descriptor and > remove it from descriptor table is not just too convoluted - it's > racy; another thread might have modified the descriptor table while > we'd been going through that song and dance. > > Switch kfd_mem_export_dmabuf() to using drm_gem_prime_handle_to_dmabuf() > and leave the descriptor table alone... > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> This patch is Reviewed-by: Felix Kuehling <felix.kuehling@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > index 11672bfe4fad..bc5401de2948 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c > @@ -25,7 +25,6 @@ > #include <linux/pagemap.h> > #include <linux/sched/mm.h> > #include <linux/sched/task.h> > -#include <linux/fdtable.h> > #include <drm/ttm/ttm_tt.h> > > #include <drm/drm_exec.h> > @@ -818,18 +817,13 @@ static int kfd_mem_export_dmabuf(struct kgd_mem *mem) > if (!mem->dmabuf) { > struct amdgpu_device *bo_adev; > struct dma_buf *dmabuf; > - int r, fd; > > bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev); > - r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, bo_adev->kfd.client.file, > + dmabuf = drm_gem_prime_handle_to_dmabuf(&bo_adev->ddev, bo_adev->kfd.client.file, > mem->gem_handle, > mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ? > - DRM_RDWR : 0, &fd); > - if (r) > - return r; > - dmabuf = dma_buf_get(fd); > - close_fd(fd); > - if (WARN_ON_ONCE(IS_ERR(dmabuf))) > + DRM_RDWR : 0); > + if (IS_ERR(dmabuf)) > return PTR_ERR(dmabuf); > mem->dmabuf = dmabuf; > }
On Wed, Aug 14, 2024 at 06:15:46PM -0400, Felix Kuehling wrote: > > On 2024-08-12 02:59, Al Viro wrote: > > Using drm_gem_prime_handle_to_fd() to set dmabuf up and insert it into > > descriptor table, only to have it looked up by file descriptor and > > remove it from descriptor table is not just too convoluted - it's > > racy; another thread might have modified the descriptor table while > > we'd been going through that song and dance. > > > > Switch kfd_mem_export_dmabuf() to using drm_gem_prime_handle_to_dmabuf() > > and leave the descriptor table alone... > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > This patch is > > Reviewed-by: Felix Kuehling <felix.kuehling@amd.com> Umm... So which tree should that series go through? I can put it through vfs.git, or send a pull request to drm folks, or... Preferences?
On Wed, Aug 21, 2024 at 8:29 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > On Wed, Aug 14, 2024 at 06:15:46PM -0400, Felix Kuehling wrote: > > > > On 2024-08-12 02:59, Al Viro wrote: > > > Using drm_gem_prime_handle_to_fd() to set dmabuf up and insert it into > > > descriptor table, only to have it looked up by file descriptor and > > > remove it from descriptor table is not just too convoluted - it's > > > racy; another thread might have modified the descriptor table while > > > we'd been going through that song and dance. > > > > > > Switch kfd_mem_export_dmabuf() to using drm_gem_prime_handle_to_dmabuf() > > > and leave the descriptor table alone... > > > > > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > > > > This patch is > > > > Reviewed-by: Felix Kuehling <felix.kuehling@amd.com> > > Umm... So which tree should that series go through? > > I can put it through vfs.git, or send a pull request to drm folks, or... > > Preferences? I'm happy to take these via the amdgpu tree once the other patches get reviewed. Alex
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c index 11672bfe4fad..bc5401de2948 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c @@ -25,7 +25,6 @@ #include <linux/pagemap.h> #include <linux/sched/mm.h> #include <linux/sched/task.h> -#include <linux/fdtable.h> #include <drm/ttm/ttm_tt.h> #include <drm/drm_exec.h> @@ -818,18 +817,13 @@ static int kfd_mem_export_dmabuf(struct kgd_mem *mem) if (!mem->dmabuf) { struct amdgpu_device *bo_adev; struct dma_buf *dmabuf; - int r, fd; bo_adev = amdgpu_ttm_adev(mem->bo->tbo.bdev); - r = drm_gem_prime_handle_to_fd(&bo_adev->ddev, bo_adev->kfd.client.file, + dmabuf = drm_gem_prime_handle_to_dmabuf(&bo_adev->ddev, bo_adev->kfd.client.file, mem->gem_handle, mem->alloc_flags & KFD_IOC_ALLOC_MEM_FLAGS_WRITABLE ? - DRM_RDWR : 0, &fd); - if (r) - return r; - dmabuf = dma_buf_get(fd); - close_fd(fd); - if (WARN_ON_ONCE(IS_ERR(dmabuf))) + DRM_RDWR : 0); + if (IS_ERR(dmabuf)) return PTR_ERR(dmabuf); mem->dmabuf = dmabuf; }
Using drm_gem_prime_handle_to_fd() to set dmabuf up and insert it into descriptor table, only to have it looked up by file descriptor and remove it from descriptor table is not just too convoluted - it's racy; another thread might have modified the descriptor table while we'd been going through that song and dance. Switch kfd_mem_export_dmabuf() to using drm_gem_prime_handle_to_dmabuf() and leave the descriptor table alone... Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)