> -----Original Message----- > From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf > Of Li, Samuel > Sent: Wednesday, September 20, 2017 11:44 AM > To: Koenig, Christian; amd-gfx at lists.freedesktop.org > Subject: RE: [PATCH v2 1/1] drm/amdgpu: Add gem_prime_mmap support > > > What happens when another thread is using amdgpu_dmabuf_ops to call > > begin_cpu_access/end_cpu_access when you are fixing it up again? > Right, that is an issue. > > > I would just completely drop the two callbacks, pinning is not necessary for > > CPU access and thinking more about it it actually has some unwanted side > > effects. > CPU access needs synchronization anyway, so the two callbacks cannot be > dropped (other drivers implemented too), so I would like to keep it there for > now. Pinning only needs to happen for device access (i.e., so the pages don't get swapped out). Alex > > Sam > > > > -----Original Message----- > > From: Koenig, Christian > > Sent: Wednesday, September 20, 2017 2:58 AM > > To: Li, Samuel <Samuel.Li at amd.com>; amd-gfx at lists.freedesktop.org > > Subject: Re: [PATCH v2 1/1] drm/amdgpu: Add gem_prime_mmap support > > > > > What do you mean "This isn't race free"? > > > > Take a look at the code again: > > > + dma_buf = drm_gem_prime_export(dev, gobj, flags); > > > + amdgpu_dmabuf_ops = *(dma_buf->ops); > > > + amdgpu_dmabuf_ops.begin_cpu_access = > > amdgpu_gem_begin_cpu_access; > > > + amdgpu_dmabuf_ops.end_cpu_access = > > amdgpu_gem_end_cpu_access; > > > + dma_buf->ops = &amdgpu_dmabuf_ops; > > > > What happens when another thread is using amdgpu_dmabuf_ops to call > > begin_cpu_access/end_cpu_access when you are fixing it up again? > > > > I would just completely drop the two callbacks, pinning is not necessary for > > CPU access and thinking more about it it actually has some unwanted side > > effects. > > > > Regards, > > Christian. > > > > Am 19.09.2017 um 23:22 schrieb Samuel Li: > > >>> + vma->vm_pgoff = amdgpu_bo_mmap_offset(bo) >> PAGE_SHIFT; > > >> Maybe better use "vma->vm_pgoff += amdgpu_bo_mmap_offset(bo) > >> > > PAGE_SHIFT;", but I'm not sure. > > >> How other drivers handle this? > > > This is a good catch. Looks like pgoff is honored during prime mmap, not a > > fake offset here. > > > > > >>> + dma_buf->ops = &amdgpu_dmabuf_ops; > > >> This isn't race free and needs to be fixed. > > >> Better add callbacks to drm_prime.c similar to > > drm_gem_dmabuf_mmap(). > > > What do you mean "This isn't race free"? > > > > > > Regards, > > > Sam > > > > > > > > > > > > On 2017-09-15 11:05 AM, Christian König wrote: > > >> Am 14.09.2017 um 00:39 schrieb Samuel Li: > > >>> v2: drop hdp invalidate/flush. > > >>> > > >>> Signed-off-by: Samuel Li <Samuel.Li at amd.com> > > >>> --- > > >>> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 3 ++ > > >>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +- > > >>> drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c | 77 > > ++++++++++++++++++++++++++++++- > > >>> 3 files changed, 81 insertions(+), 2 deletions(-) > > >>> > > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > >>> index d2aaad7..188b705 100644 > > >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > > >>> @@ -395,11 +395,14 @@ > amdgpu_gem_prime_import_sg_table(struct > > drm_device *dev, > > >>> struct dma_buf *amdgpu_gem_prime_export(struct drm_device > *dev, > > >>> struct drm_gem_object *gobj, > > >>> int flags); > > >>> +struct drm_gem_object *amdgpu_gem_prime_import(struct > > drm_device *dev, > > >>> + struct dma_buf *dma_buf); > > >>> int amdgpu_gem_prime_pin(struct drm_gem_object *obj); > > >>> void amdgpu_gem_prime_unpin(struct drm_gem_object *obj); > > >>> struct reservation_object *amdgpu_gem_prime_res_obj(struct > > drm_gem_object *); > > >>> void *amdgpu_gem_prime_vmap(struct drm_gem_object *obj); > > >>> void amdgpu_gem_prime_vunmap(struct drm_gem_object *obj, > void > > >>> *vaddr); > > >>> +int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, struct > > >>> +vm_area_struct *vma); > > >>> int amdgpu_gem_debugfs_init(struct amdgpu_device *adev); > > >>> /* sub-allocation manager, it has to be protected by another lock. > > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > >>> index 2cdf844..9b63ac5 100644 > > >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > > >>> @@ -835,7 +835,7 @@ static struct drm_driver kms_driver = { > > >>> .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > > >>> .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > > >>> .gem_prime_export = amdgpu_gem_prime_export, > > >>> - .gem_prime_import = drm_gem_prime_import, > > >>> + .gem_prime_import = amdgpu_gem_prime_import, > > >>> .gem_prime_pin = amdgpu_gem_prime_pin, > > >>> .gem_prime_unpin = amdgpu_gem_prime_unpin, > > >>> .gem_prime_res_obj = amdgpu_gem_prime_res_obj, @@ -843,6 > > >>> +843,7 @@ static struct drm_driver kms_driver = { > > >>> .gem_prime_import_sg_table = > > amdgpu_gem_prime_import_sg_table, > > >>> .gem_prime_vmap = amdgpu_gem_prime_vmap, > > >>> .gem_prime_vunmap = amdgpu_gem_prime_vunmap, > > >>> + .gem_prime_mmap = amdgpu_gem_prime_mmap, > > >>> .name = DRIVER_NAME, > > >>> .desc = DRIVER_DESC, > > >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c > > >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c > > >>> index 5b3f928..13c977a 100644 > > >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c > > >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_prime.c > > >>> @@ -57,6 +57,40 @@ void amdgpu_gem_prime_vunmap(struct > > drm_gem_object *obj, void *vaddr) > > >>> ttm_bo_kunmap(&bo->dma_buf_vmap); > > >>> } > > >>> +int amdgpu_gem_prime_mmap(struct drm_gem_object *obj, > struct > > >>> vm_area_struct *vma) > > >>> +{ > > >>> + struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); > > >>> + struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); > > >>> + unsigned asize = amdgpu_bo_size(bo); > > >>> + int ret; > > >>> + > > >>> + if (!vma->vm_file) > > >>> + return -ENODEV; > > >>> + > > >>> + if (adev == NULL) > > >>> + return -ENODEV; > > >>> + > > >>> + /* Check for valid size. */ > > >>> + if (asize < vma->vm_end - vma->vm_start) > > >>> + return -EINVAL; > > >>> + > > >>> + if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm) || > > >>> + (bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS)) { > > >>> + return -EPERM; > > >>> + } > > >>> + vma->vm_pgoff = amdgpu_bo_mmap_offset(bo) >> PAGE_SHIFT; > > >> Maybe better use "vma->vm_pgoff += amdgpu_bo_mmap_offset(bo) > >> > > PAGE_SHIFT;", but I'm not sure. > > >> > > >> How other drivers handle this? > > >> > > >>> + > > >>> + /* prime mmap does not need to check access, so allow here */ > > >>> + ret = drm_vma_node_allow(&obj->vma_node, vma->vm_file- > > >private_data); > > >>> + if (ret) > > >>> + return ret; > > >>> + > > >>> + ret = ttm_bo_mmap(vma->vm_file, vma, &adev->mman.bdev); > > >>> + drm_vma_node_revoke(&obj->vma_node, > > >>> + vma->vm_file->private_data); > > >>> + > > >>> + return ret; > > >>> +} > > >>> + > > >>> struct drm_gem_object * > > >>> amdgpu_gem_prime_import_sg_table(struct drm_device *dev, > > >>> struct dma_buf_attachment *attach, @@ -130,14 > > >>> +164,55 @@ struct reservation_object > > *amdgpu_gem_prime_res_obj(struct drm_gem_object *obj) > > >>> return bo->tbo.resv; > > >>> } > > >>> +static int amdgpu_gem_begin_cpu_access(struct dma_buf > *dma_buf, > > >>> enum dma_data_direction direction) > > >>> +{ > > >>> + return amdgpu_gem_prime_pin(dma_buf->priv); > > >>> +} > > >>> + > > >>> +static int amdgpu_gem_end_cpu_access(struct dma_buf *dma_buf, > > enum > > >>> +dma_data_direction direction) { > > >>> + amdgpu_gem_prime_unpin(dma_buf->priv); > > >>> + > > >>> + return 0; > > >>> +} > > >>> + > > >>> +static struct dma_buf_ops amdgpu_dmabuf_ops; > > >>> + > > >>> struct dma_buf *amdgpu_gem_prime_export(struct drm_device > *dev, > > >>> struct drm_gem_object *gobj, > > >>> int flags) > > >>> { > > >>> struct amdgpu_bo *bo = gem_to_amdgpu_bo(gobj); > > >>> + struct dma_buf *dma_buf; > > >>> if (amdgpu_ttm_tt_get_usermm(bo->tbo.ttm)) > > >>> return ERR_PTR(-EPERM); > > >>> - return drm_gem_prime_export(dev, gobj, flags); > > >>> + dma_buf = drm_gem_prime_export(dev, gobj, flags); > > >>> + amdgpu_dmabuf_ops = *(dma_buf->ops); > > >>> + amdgpu_dmabuf_ops.begin_cpu_access = > > amdgpu_gem_begin_cpu_access; > > >>> + amdgpu_dmabuf_ops.end_cpu_access = > > amdgpu_gem_end_cpu_access; > > >>> + dma_buf->ops = &amdgpu_dmabuf_ops; > > >> This isn't race free and needs to be fixed. > > >> > > >> Better add callbacks to drm_prime.c similar to > > drm_gem_dmabuf_mmap(). > > >> > > >> Alternative you could just completely drop > > amdgpu_gem_begin_cpu_access() and amdgpu_gem_end_cpu_access() > as > > well. > > >> > > >> When the buffer is shared between device it is pinned anyway and > when > > it isn't shared ttm_bo_mmap() is able to handle VRAM mappings as well. > > >> > > >> Regards, > > >> Christian. > > >> > > >>> + > > >>> + return dma_buf; > > >>> +} > > >>> + > > >>> +struct drm_gem_object *amdgpu_gem_prime_import(struct > > drm_device *dev, > > >>> + struct dma_buf *dma_buf) { > > >>> + struct drm_gem_object *obj; > > >>> + > > >>> + if (dma_buf->ops == &amdgpu_dmabuf_ops) { > > >>> + obj = dma_buf->priv; > > >>> + if (obj->dev == dev) { > > >>> + /* > > >>> + * Importing dmabuf exported from out own gem increases > > >>> + * refcount on gem itself instead of f_count of dmabuf. > > >>> + */ > > >>> + drm_gem_object_get(obj); > > >>> + return obj; > > >>> + } > > >>> + } > > >>> + > > >>> + return drm_gem_prime_import(dev, dma_buf); > > >>> } > > >> > > _______________________________________________ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
