Hello,
Here's the second version of the GEM CMA DMA-BUF support patches.
The code is based on the Exynos DRM DMA-BUF implementation. The exporter role has been successfully tested with the Renesas R-Car DU driver.
Dave, is there a chance this could make it to v3.11 ?
Changes compared to v1:
- Added a mixing sg_free_table() in drm_gem_cma_dmabuf_map() - Implement drm_gem_cma_dmabuf_mmap()
Laurent Pinchart (5): drm/gem: Split drm_gem_mmap() into object search and object mapping drm/omap: Use drm_gem_mmap_obj() to implement dma-buf mmap drm: GEM CMA: Split object creation into object alloc and DMA memory alloc drm: GEM CMA: Split object mapping into GEM mapping and CMA mapping drm: GEM CMA: Add DRM PRIME support
drivers/gpu/drm/drm_gem.c | 83 +++--- drivers/gpu/drm/drm_gem_cma_helper.c | 408 +++++++++++++++++++++++++++--- drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 32 +-- include/drm/drmP.h | 2 + include/drm/drm_gem_cma_helper.h | 9 + 5 files changed, 438 insertions(+), 96 deletions(-)
The drm_gem_mmap() function first finds the GEM object to be mapped based on the fake mmap offset and then maps the object. Split the object mapping code into a standalone drm_gem_mmap_obj() function that can be used to implement dma-buf mmap() operations.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/drm_gem.c | 83 +++++++++++++++++++++++++++++------------------ include/drm/drmP.h | 2 ++ 2 files changed, 54 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index cf919e3..4321713 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -644,6 +644,55 @@ void drm_gem_vm_close(struct vm_area_struct *vma) } EXPORT_SYMBOL(drm_gem_vm_close);
+/** + * drm_gem_mmap_obj - memory map a GEM object + * @obj: the GEM object to map + * @obj_size: the object size to be mapped, in bytes + * @vma: VMA for the area to be mapped + * + * Set up the VMA to prepare mapping of the GEM object using the gem_vm_ops + * provided by the driver. Depending on their requirements, drivers can either + * provide a fault handler in their gem_vm_ops (in which case any accesses to + * the object will be trapped, to perform migration, GTT binding, surface + * register allocation, or performance monitoring), or mmap the buffer memory + * synchronously after calling drm_gem_mmap_obj. + * + * This function is mainly intended to implement the DMABUF mmap operation, when + * the GEM object is not looked up based on its fake offset. To implement the + * DRM mmap operation, drivers should use the drm_gem_mmap() function. + * + * Return 0 or success or -EINVAL if the object size is smaller than the VMA + * size, or if no gem_vm_ops are provided. + */ +int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, + struct vm_area_struct *vma) +{ + struct drm_device *dev = obj->dev; + + /* Check for valid size. */ + if (obj_size < vma->vm_end - vma->vm_start) + return -EINVAL; + + if (!dev->driver->gem_vm_ops) + return -EINVAL; + + vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; + vma->vm_ops = dev->driver->gem_vm_ops; + vma->vm_private_data = obj; + vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); + + /* Take a ref for this mapping of the object, so that the fault + * handler can dereference the mmap offset's pointer to the object. + * This reference is cleaned up by the corresponding vm_close + * (which should happen whether the vma was created by this call, or + * by a vm_open due to mremap or partial unmap or whatever). + */ + drm_gem_object_reference(obj); + + drm_vm_open_locked(dev, vma); + return 0; +} +EXPORT_SYMBOL(drm_gem_mmap_obj);
/** * drm_gem_mmap - memory map routine for GEM objects @@ -653,11 +702,9 @@ EXPORT_SYMBOL(drm_gem_vm_close); * If a driver supports GEM object mapping, mmap calls on the DRM file * descriptor will end up here. * - * If we find the object based on the offset passed in (vma->vm_pgoff will + * Look up the GEM object based on the offset passed in (vma->vm_pgoff will * contain the fake offset we created when the GTT map ioctl was called on - * the object), we set up the driver fault handler so that any accesses - * to the object can be trapped, to perform migration, GTT binding, surface - * register allocation, or performance monitoring. + * the object) and map it with a call to drm_gem_mmap_obj(). */ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) { @@ -665,7 +712,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; struct drm_gem_mm *mm = dev->mm_private; struct drm_local_map *map = NULL; - struct drm_gem_object *obj; struct drm_hash_item *hash; int ret = 0;
@@ -686,32 +732,7 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) goto out_unlock; }
- /* Check for valid size. */ - if (map->size < vma->vm_end - vma->vm_start) { - ret = -EINVAL; - goto out_unlock; - } - - obj = map->handle; - if (!obj->dev->driver->gem_vm_ops) { - ret = -EINVAL; - goto out_unlock; - } - - vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; - vma->vm_ops = obj->dev->driver->gem_vm_ops; - vma->vm_private_data = map->handle; - vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); - - /* Take a ref for this mapping of the object, so that the fault - * handler can dereference the mmap offset's pointer to the object. - * This reference is cleaned up by the corresponding vm_close - * (which should happen whether the vma was created by this call, or - * by a vm_open due to mremap or partial unmap or whatever). - */ - drm_gem_object_reference(obj); - - drm_vm_open_locked(dev, vma); + ret = drm_gem_mmap_obj(map->handle, map->size, vma);
out_unlock: mutex_unlock(&dev->struct_mutex); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index b06f5af..79fb4c7 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1616,6 +1616,8 @@ int drm_gem_private_object_init(struct drm_device *dev, void drm_gem_object_handle_free(struct drm_gem_object *obj); void drm_gem_vm_open(struct vm_area_struct *vma); void drm_gem_vm_close(struct vm_area_struct *vma); +int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, + struct vm_area_struct *vma); int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
#include <drm/drm_global.h>
On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:
The drm_gem_mmap() function first finds the GEM object to be mapped based on the fake mmap offset and then maps the object. Split the object mapping code into a standalone drm_gem_mmap_obj() function that can be used to implement dma-buf mmap() operations.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Cool, thanks.. this was an old TODO of mine that I apparently forgot about :-)
Reviewed-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_gem.c | 83 +++++++++++++++++++++++++++++------------------ include/drm/drmP.h | 2 ++ 2 files changed, 54 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index cf919e3..4321713 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -644,6 +644,55 @@ void drm_gem_vm_close(struct vm_area_struct *vma) } EXPORT_SYMBOL(drm_gem_vm_close);
+/**
- drm_gem_mmap_obj - memory map a GEM object
- @obj: the GEM object to map
- @obj_size: the object size to be mapped, in bytes
- @vma: VMA for the area to be mapped
- Set up the VMA to prepare mapping of the GEM object using the gem_vm_ops
- provided by the driver. Depending on their requirements, drivers can either
- provide a fault handler in their gem_vm_ops (in which case any accesses to
- the object will be trapped, to perform migration, GTT binding, surface
- register allocation, or performance monitoring), or mmap the buffer memory
- synchronously after calling drm_gem_mmap_obj.
- This function is mainly intended to implement the DMABUF mmap operation, when
- the GEM object is not looked up based on its fake offset. To implement the
- DRM mmap operation, drivers should use the drm_gem_mmap() function.
- Return 0 or success or -EINVAL if the object size is smaller than the VMA
- size, or if no gem_vm_ops are provided.
- */
+int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
struct vm_area_struct *vma)+{
struct drm_device *dev = obj->dev;/* Check for valid size. */if (obj_size < vma->vm_end - vma->vm_start)return -EINVAL;if (!dev->driver->gem_vm_ops)return -EINVAL;vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;vma->vm_ops = dev->driver->gem_vm_ops;vma->vm_private_data = obj;vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));/* Take a ref for this mapping of the object, so that the fault* handler can dereference the mmap offset's pointer to the object.* This reference is cleaned up by the corresponding vm_close* (which should happen whether the vma was created by this call, or* by a vm_open due to mremap or partial unmap or whatever).*/drm_gem_object_reference(obj);drm_vm_open_locked(dev, vma);return 0;+} +EXPORT_SYMBOL(drm_gem_mmap_obj);
/**
- drm_gem_mmap - memory map routine for GEM objects
@@ -653,11 +702,9 @@ EXPORT_SYMBOL(drm_gem_vm_close);
- If a driver supports GEM object mapping, mmap calls on the DRM file
- descriptor will end up here.
- If we find the object based on the offset passed in (vma->vm_pgoff will
- Look up the GEM object based on the offset passed in (vma->vm_pgoff will
- contain the fake offset we created when the GTT map ioctl was called on
- the object), we set up the driver fault handler so that any accesses
- to the object can be trapped, to perform migration, GTT binding, surface
- register allocation, or performance monitoring.
*/
- the object) and map it with a call to drm_gem_mmap_obj().
int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) { @@ -665,7 +712,6 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) struct drm_device *dev = priv->minor->dev; struct drm_gem_mm *mm = dev->mm_private; struct drm_local_map *map = NULL;
struct drm_gem_object *obj; struct drm_hash_item *hash; int ret = 0;@@ -686,32 +732,7 @@ int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma) goto out_unlock; }
/* Check for valid size. */if (map->size < vma->vm_end - vma->vm_start) {ret = -EINVAL;goto out_unlock;}obj = map->handle;if (!obj->dev->driver->gem_vm_ops) {ret = -EINVAL;goto out_unlock;}vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;vma->vm_ops = obj->dev->driver->gem_vm_ops;vma->vm_private_data = map->handle;vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));/* Take a ref for this mapping of the object, so that the fault* handler can dereference the mmap offset's pointer to the object.* This reference is cleaned up by the corresponding vm_close* (which should happen whether the vma was created by this call, or* by a vm_open due to mremap or partial unmap or whatever).*/drm_gem_object_reference(obj);drm_vm_open_locked(dev, vma);
ret = drm_gem_mmap_obj(map->handle, map->size, vma);out_unlock: mutex_unlock(&dev->struct_mutex); diff --git a/include/drm/drmP.h b/include/drm/drmP.h index b06f5af..79fb4c7 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1616,6 +1616,8 @@ int drm_gem_private_object_init(struct drm_device *dev, void drm_gem_object_handle_free(struct drm_gem_object *obj); void drm_gem_vm_open(struct vm_area_struct *vma); void drm_gem_vm_close(struct vm_area_struct *vma); +int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size,
struct vm_area_struct *vma);int drm_gem_mmap(struct file *filp, struct vm_area_struct *vma);
#include <drm/drm_global.h>
1.8.1.5
The dma-buf mmap code was copied from the GEM mmap implementation. Replace it with the new drm_gem_mmap_obj() function.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 32 +++---------------------------- 1 file changed, 3 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c index be7cd97..3256693 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c +++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c @@ -136,10 +136,6 @@ static void omap_gem_dmabuf_kunmap(struct dma_buf *buffer, kunmap(pages[page_num]); }
-/* - * TODO maybe we can split up drm_gem_mmap to avoid duplicating - * some here.. or at least have a drm_dmabuf_mmap helper. - */ static int omap_gem_dmabuf_mmap(struct dma_buf *buffer, struct vm_area_struct *vma) { @@ -149,31 +145,9 @@ static int omap_gem_dmabuf_mmap(struct dma_buf *buffer, if (WARN_ON(!obj->filp)) return -EINVAL;
- /* Check for valid size. */ - if (omap_gem_mmap_size(obj) < vma->vm_end - vma->vm_start) { - ret = -EINVAL; - goto out_unlock; - } - - if (!obj->dev->driver->gem_vm_ops) { - ret = -EINVAL; - goto out_unlock; - } - - vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP; - vma->vm_ops = obj->dev->driver->gem_vm_ops; - vma->vm_private_data = obj; - vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags)); - - /* Take a ref for this mapping of the object, so that the fault - * handler can dereference the mmap offset's pointer to the object. - * This reference is cleaned up by the corresponding vm_close - * (which should happen whether the vma was created by this call, or - * by a vm_open due to mremap or partial unmap or whatever). - */ - vma->vm_ops->open(vma); - -out_unlock: + ret = drm_gem_mmap_obj(obj, omap_gem_mmap_size(obj), vma); + if (ret < 0) + return ret;
return omap_gem_mmap_obj(obj, vma); }
On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:
The dma-buf mmap code was copied from the GEM mmap implementation. Replace it with the new drm_gem_mmap_obj() function.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Reviewed-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 32 +++---------------------------- 1 file changed, 3 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c index be7cd97..3256693 100644 --- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c +++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c @@ -136,10 +136,6 @@ static void omap_gem_dmabuf_kunmap(struct dma_buf *buffer, kunmap(pages[page_num]); }
-/*
- TODO maybe we can split up drm_gem_mmap to avoid duplicating
- some here.. or at least have a drm_dmabuf_mmap helper.
- */
static int omap_gem_dmabuf_mmap(struct dma_buf *buffer, struct vm_area_struct *vma) { @@ -149,31 +145,9 @@ static int omap_gem_dmabuf_mmap(struct dma_buf *buffer, if (WARN_ON(!obj->filp)) return -EINVAL;
/* Check for valid size. */if (omap_gem_mmap_size(obj) < vma->vm_end - vma->vm_start) {ret = -EINVAL;goto out_unlock;}if (!obj->dev->driver->gem_vm_ops) {ret = -EINVAL;goto out_unlock;}vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP;vma->vm_ops = obj->dev->driver->gem_vm_ops;vma->vm_private_data = obj;vma->vm_page_prot = pgprot_writecombine(vm_get_page_prot(vma->vm_flags));/* Take a ref for this mapping of the object, so that the fault* handler can dereference the mmap offset's pointer to the object.* This reference is cleaned up by the corresponding vm_close* (which should happen whether the vma was created by this call, or* by a vm_open due to mremap or partial unmap or whatever).*/vma->vm_ops->open(vma);-out_unlock:
ret = drm_gem_mmap_obj(obj, omap_gem_mmap_size(obj), vma);if (ret < 0)return ret; return omap_gem_mmap_obj(obj, vma);}
1.8.1.5
Hi Rob,
On Tuesday 04 June 2013 07:33:42 Rob Clark wrote:
On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart wrote:
The dma-buf mmap code was copied from the GEM mmap implementation. Replace it with the new drm_gem_mmap_obj() function.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Reviewed-by: Rob Clark robdclark@gmail.com
Thanks. Any chance you would have time to review 3/5 to 5/5 ? :-)
This allows creating a GEM CMA object without an associated DMA memory buffer, and will be used to implement DRM PRIME support.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/drm_gem_cma_helper.c | 83 +++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 0a7e011..8cce330 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -32,62 +32,73 @@ static unsigned int get_gem_mmap_offset(struct drm_gem_object *obj) return (unsigned int)obj->map_list.hash.key << PAGE_SHIFT; }
-static void drm_gem_cma_buf_destroy(struct drm_device *drm, - struct drm_gem_cma_object *cma_obj) -{ - dma_free_writecombine(drm->dev, cma_obj->base.size, cma_obj->vaddr, - cma_obj->paddr); -} - /* - * drm_gem_cma_create - allocate an object with the given size + * __drm_gem_cma_create - Create a GEM CMA object without allocating memory + * @drm: The drm device + * @size: The GEM object size * - * returns a struct drm_gem_cma_object* on success or ERR_PTR values - * on failure. + * This function creates and initializes a GEM CMA object of the given size, but + * doesn't allocate any memory to back the object. + * + * Return a struct drm_gem_cma_object* on success or ERR_PTR values on failure. */ -struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, - unsigned int size) +static struct drm_gem_cma_object * +__drm_gem_cma_create(struct drm_device *drm, unsigned int size) { struct drm_gem_cma_object *cma_obj; struct drm_gem_object *gem_obj; int ret;
- size = round_up(size, PAGE_SIZE); - cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL); if (!cma_obj) return ERR_PTR(-ENOMEM);
- cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size, - &cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN); - if (!cma_obj->vaddr) { - dev_err(drm->dev, "failed to allocate buffer with size %d\n", size); - ret = -ENOMEM; - goto err_dma_alloc; - } - gem_obj = &cma_obj->base;
ret = drm_gem_object_init(drm, gem_obj, size); if (ret) - goto err_obj_init; + goto error;
ret = drm_gem_create_mmap_offset(gem_obj); - if (ret) - goto err_create_mmap_offset; + if (ret) { + drm_gem_object_release(gem_obj); + goto error; + }
return cma_obj;
-err_create_mmap_offset: - drm_gem_object_release(gem_obj); +error: + kfree(cma_obj); + return ERR_PTR(ret); +}
-err_obj_init: - drm_gem_cma_buf_destroy(drm, cma_obj); +/* + * drm_gem_cma_create - allocate an object with the given size + * + * returns a struct drm_gem_cma_object* on success or ERR_PTR values + * on failure. + */ +struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, + unsigned int size) +{ + struct drm_gem_cma_object *cma_obj;
-err_dma_alloc: - kfree(cma_obj); + size = round_up(size, PAGE_SIZE);
- return ERR_PTR(ret); + cma_obj = __drm_gem_cma_create(drm, size); + if (IS_ERR(cma_obj)) + return cma_obj; + + cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size, + &cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN); + if (!cma_obj->vaddr) { + dev_err(drm->dev, "failed to allocate buffer with size %d\n", + size); + drm_gem_cma_free_object(&cma_obj->base); + return ERR_PTR(-ENOMEM); + } + + return cma_obj; } EXPORT_SYMBOL_GPL(drm_gem_cma_create);
@@ -143,11 +154,13 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj) if (gem_obj->map_list.map) drm_gem_free_mmap_offset(gem_obj);
- drm_gem_object_release(gem_obj); - cma_obj = to_drm_gem_cma_obj(gem_obj);
- drm_gem_cma_buf_destroy(gem_obj->dev, cma_obj); + if (cma_obj->vaddr) + dma_free_writecombine(gem_obj->dev->dev, cma_obj->base.size, + cma_obj->vaddr, cma_obj->paddr); + + drm_gem_object_release(gem_obj);
kfree(cma_obj); }
On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:
This allows creating a GEM CMA object without an associated DMA memory buffer, and will be used to implement DRM PRIME support.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Reviewed-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_gem_cma_helper.c | 83 +++++++++++++++++++++--------------- 1 file changed, 48 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 0a7e011..8cce330 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -32,62 +32,73 @@ static unsigned int get_gem_mmap_offset(struct drm_gem_object *obj) return (unsigned int)obj->map_list.hash.key << PAGE_SHIFT; }
-static void drm_gem_cma_buf_destroy(struct drm_device *drm,
struct drm_gem_cma_object *cma_obj)-{
dma_free_writecombine(drm->dev, cma_obj->base.size, cma_obj->vaddr,cma_obj->paddr);-}
/*
- drm_gem_cma_create - allocate an object with the given size
- __drm_gem_cma_create - Create a GEM CMA object without allocating memory
- @drm: The drm device
- @size: The GEM object size
- returns a struct drm_gem_cma_object* on success or ERR_PTR values
- on failure.
- This function creates and initializes a GEM CMA object of the given size, but
- doesn't allocate any memory to back the object.
*/
- Return a struct drm_gem_cma_object* on success or ERR_PTR values on failure.
-struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
unsigned int size)+static struct drm_gem_cma_object * +__drm_gem_cma_create(struct drm_device *drm, unsigned int size) { struct drm_gem_cma_object *cma_obj; struct drm_gem_object *gem_obj; int ret;
size = round_up(size, PAGE_SIZE);cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL); if (!cma_obj) return ERR_PTR(-ENOMEM);cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size,&cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN);if (!cma_obj->vaddr) {dev_err(drm->dev, "failed to allocate buffer with size %d\n", size);ret = -ENOMEM;goto err_dma_alloc;}gem_obj = &cma_obj->base; ret = drm_gem_object_init(drm, gem_obj, size); if (ret)goto err_obj_init;
goto error; ret = drm_gem_create_mmap_offset(gem_obj);
if (ret)goto err_create_mmap_offset;
if (ret) {drm_gem_object_release(gem_obj);goto error;} return cma_obj;-err_create_mmap_offset:
drm_gem_object_release(gem_obj);+error:
kfree(cma_obj);return ERR_PTR(ret);+}
-err_obj_init:
drm_gem_cma_buf_destroy(drm, cma_obj);+/*
- drm_gem_cma_create - allocate an object with the given size
- returns a struct drm_gem_cma_object* on success or ERR_PTR values
- on failure.
- */
+struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm,
unsigned int size)+{
struct drm_gem_cma_object *cma_obj;-err_dma_alloc:
kfree(cma_obj);
size = round_up(size, PAGE_SIZE);
return ERR_PTR(ret);
cma_obj = __drm_gem_cma_create(drm, size);if (IS_ERR(cma_obj))return cma_obj;cma_obj->vaddr = dma_alloc_writecombine(drm->dev, size,&cma_obj->paddr, GFP_KERNEL | __GFP_NOWARN);if (!cma_obj->vaddr) {dev_err(drm->dev, "failed to allocate buffer with size %d\n",size);drm_gem_cma_free_object(&cma_obj->base);return ERR_PTR(-ENOMEM);}return cma_obj;} EXPORT_SYMBOL_GPL(drm_gem_cma_create);
@@ -143,11 +154,13 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj) if (gem_obj->map_list.map) drm_gem_free_mmap_offset(gem_obj);
drm_gem_object_release(gem_obj);cma_obj = to_drm_gem_cma_obj(gem_obj);drm_gem_cma_buf_destroy(gem_obj->dev, cma_obj);
if (cma_obj->vaddr)dma_free_writecombine(gem_obj->dev->dev, cma_obj->base.size,cma_obj->vaddr, cma_obj->paddr);drm_gem_object_release(gem_obj); kfree(cma_obj);}
1.8.1.5
The CMA-specific mapping code will be used to implement dma-buf mmap support.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/drm_gem_cma_helper.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 8cce330..7a4db4e 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -228,13 +228,26 @@ const struct vm_operations_struct drm_gem_cma_vm_ops = { }; EXPORT_SYMBOL_GPL(drm_gem_cma_vm_ops);
+static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj, + struct vm_area_struct *vma) +{ + int ret; + + ret = remap_pfn_range(vma, vma->vm_start, cma_obj->paddr >> PAGE_SHIFT, + vma->vm_end - vma->vm_start, vma->vm_page_prot); + if (ret) + drm_gem_vm_close(vma); + + return ret; +} + /* * drm_gem_cma_mmap - (struct file_operation)->mmap callback function */ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) { - struct drm_gem_object *gem_obj; struct drm_gem_cma_object *cma_obj; + struct drm_gem_object *gem_obj; int ret;
ret = drm_gem_mmap(filp, vma); @@ -244,12 +257,7 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) gem_obj = vma->vm_private_data; cma_obj = to_drm_gem_cma_obj(gem_obj);
- ret = remap_pfn_range(vma, vma->vm_start, cma_obj->paddr >> PAGE_SHIFT, - vma->vm_end - vma->vm_start, vma->vm_page_prot); - if (ret) - drm_gem_vm_close(vma); - - return ret; + return drm_gem_cma_mmap_obj(cma_obj, vma); } EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:
The CMA-specific mapping code will be used to implement dma-buf mmap support.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Signed-off-by: Rob Clark robdclark@gmail.com
drivers/gpu/drm/drm_gem_cma_helper.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 8cce330..7a4db4e 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -228,13 +228,26 @@ const struct vm_operations_struct drm_gem_cma_vm_ops = { }; EXPORT_SYMBOL_GPL(drm_gem_cma_vm_ops);
+static int drm_gem_cma_mmap_obj(struct drm_gem_cma_object *cma_obj,
struct vm_area_struct *vma)+{
int ret;ret = remap_pfn_range(vma, vma->vm_start, cma_obj->paddr >> PAGE_SHIFT,vma->vm_end - vma->vm_start, vma->vm_page_prot);if (ret)drm_gem_vm_close(vma);return ret;+}
/*
- drm_gem_cma_mmap - (struct file_operation)->mmap callback function
*/ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) {
struct drm_gem_object *gem_obj; struct drm_gem_cma_object *cma_obj;
struct drm_gem_object *gem_obj; int ret; ret = drm_gem_mmap(filp, vma);@@ -244,12 +257,7 @@ int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma) gem_obj = vma->vm_private_data; cma_obj = to_drm_gem_cma_obj(gem_obj);
ret = remap_pfn_range(vma, vma->vm_start, cma_obj->paddr >> PAGE_SHIFT,vma->vm_end - vma->vm_start, vma->vm_page_prot);if (ret)drm_gem_vm_close(vma);return ret;
return drm_gem_cma_mmap_obj(cma_obj, vma);} EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
-- 1.8.1.5
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com --- drivers/gpu/drm/drm_gem_cma_helper.c | 321 ++++++++++++++++++++++++++++++++++- include/drm/drm_gem_cma_helper.h | 9 + 2 files changed, 327 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 7a4db4e..1dc2157 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -21,6 +21,9 @@ #include <linux/slab.h> #include <linux/mutex.h> #include <linux/export.h> +#if CONFIG_DMA_SHARED_BUFFER +#include <linux/dma-buf.h> +#endif #include <linux/dma-mapping.h>
#include <drm/drmP.h> @@ -82,6 +85,8 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, unsigned int size) { struct drm_gem_cma_object *cma_obj; + struct sg_table *sgt = NULL; + int ret;
size = round_up(size, PAGE_SIZE);
@@ -94,11 +99,29 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, if (!cma_obj->vaddr) { dev_err(drm->dev, "failed to allocate buffer with size %d\n", size); - drm_gem_cma_free_object(&cma_obj->base); - return ERR_PTR(-ENOMEM); + ret = -ENOMEM; + goto error; }
+ sgt = kzalloc(sizeof(*cma_obj->sgt), GFP_KERNEL); + if (sgt == NULL) { + ret = -ENOMEM; + goto error; + } + + ret = dma_get_sgtable(drm->dev, sgt, cma_obj->vaddr, + cma_obj->paddr, size); + if (ret < 0) + goto error; + + cma_obj->sgt = sgt; + return cma_obj; + +error: + kfree(sgt); + drm_gem_cma_free_object(&cma_obj->base); + return ERR_PTR(ret); } EXPORT_SYMBOL_GPL(drm_gem_cma_create);
@@ -156,9 +179,16 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
cma_obj = to_drm_gem_cma_obj(gem_obj);
- if (cma_obj->vaddr) + if (cma_obj->vaddr) { dma_free_writecombine(gem_obj->dev->dev, cma_obj->base.size, cma_obj->vaddr, cma_obj->paddr); + if (cma_obj->sgt) { + sg_free_table(cma_obj->sgt); + kfree(cma_obj->sgt); + } + } else if (gem_obj->import_attach) { + drm_prime_gem_destroy(gem_obj, cma_obj->sgt); + }
drm_gem_object_release(gem_obj);
@@ -291,3 +321,288 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m } EXPORT_SYMBOL_GPL(drm_gem_cma_describe); #endif + +/* ----------------------------------------------------------------------------- + * DMA-BUF + */ + +#if CONFIG_DMA_SHARED_BUFFER +struct drm_gem_cma_dmabuf_attachment { + struct sg_table sgt; + enum dma_data_direction dir; +}; + +static int drm_gem_cma_dmabuf_attach(struct dma_buf *dmabuf, struct device *dev, + struct dma_buf_attachment *attach) +{ + struct drm_gem_cma_dmabuf_attachment *cma_attach; + + cma_attach = kzalloc(sizeof(*cma_attach), GFP_KERNEL); + if (!cma_attach) + return -ENOMEM; + + cma_attach->dir = DMA_NONE; + attach->priv = cma_attach; + + return 0; +} + +static void drm_gem_cma_dmabuf_detach(struct dma_buf *dmabuf, + struct dma_buf_attachment *attach) +{ + struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv; + struct sg_table *sgt; + + if (cma_attach == NULL) + return; + + sgt = &cma_attach->sgt; + + if (cma_attach->dir != DMA_NONE) + dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, + cma_attach->dir); + + sg_free_table(sgt); + kfree(cma_attach); + attach->priv = NULL; +} + +static struct sg_table * +drm_gem_cma_dmabuf_map(struct dma_buf_attachment *attach, + enum dma_data_direction dir) +{ + struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv; + struct drm_gem_cma_object *cma_obj = attach->dmabuf->priv; + struct drm_device *drm = cma_obj->base.dev; + struct scatterlist *rd, *wr; + struct sg_table *sgt; + unsigned int i; + int nents, ret; + + DRM_DEBUG_PRIME("\n"); + + if (WARN_ON(dir == DMA_NONE)) + return ERR_PTR(-EINVAL); + + /* Return the cached mapping when possible. */ + if (cma_attach->dir == dir) + return &cma_attach->sgt; + + /* Two mappings with different directions for the same attachment are + * not allowed. + */ + if (WARN_ON(cma_attach->dir != DMA_NONE)) + return ERR_PTR(-EBUSY); + + sgt = &cma_attach->sgt; + + ret = sg_alloc_table(sgt, cma_obj->sgt->orig_nents, GFP_KERNEL); + if (ret) { + DRM_ERROR("failed to alloc sgt.\n"); + return ERR_PTR(-ENOMEM); + } + + mutex_lock(&drm->struct_mutex); + + rd = cma_obj->sgt->sgl; + wr = sgt->sgl; + for (i = 0; i < sgt->orig_nents; ++i) { + sg_set_page(wr, sg_page(rd), rd->length, rd->offset); + rd = sg_next(rd); + wr = sg_next(wr); + } + + nents = dma_map_sg(attach->dev, sgt->sgl, sgt->orig_nents, dir); + if (!nents) { + DRM_ERROR("failed to map sgl with iommu.\n"); + sg_free_table(sgt); + sgt = ERR_PTR(-EIO); + goto done; + } + + cma_attach->dir = dir; + attach->priv = cma_attach; + + DRM_DEBUG_PRIME("buffer size = %zu\n", cma_obj->base.size); + +done: + mutex_unlock(&drm->struct_mutex); + return sgt; +} + +static void drm_gem_cma_dmabuf_unmap(struct dma_buf_attachment *attach, + struct sg_table *sgt, + enum dma_data_direction dir) +{ + /* Nothing to do. */ +} + +static void drm_gem_cma_dmabuf_release(struct dma_buf *dmabuf) +{ + struct drm_gem_cma_object *cma_obj = dmabuf->priv; + + DRM_DEBUG_PRIME("%s\n", __FILE__); + + /* + * drm_gem_cma_dmabuf_release() call means that file object's + * f_count is 0 and it calls drm_gem_object_handle_unreference() + * to drop the references that these values had been increased + * at drm_prime_handle_to_fd() + */ + if (cma_obj->base.export_dma_buf == dmabuf) { + cma_obj->base.export_dma_buf = NULL; + + /* + * drop this gem object refcount to release allocated buffer + * and resources. + */ + drm_gem_object_unreference_unlocked(&cma_obj->base); + } +} + +static void *drm_gem_cma_dmabuf_kmap_atomic(struct dma_buf *dmabuf, + unsigned long page_num) +{ + /* TODO */ + + return NULL; +} + +static void drm_gem_cma_dmabuf_kunmap_atomic(struct dma_buf *dmabuf, + unsigned long page_num, void *addr) +{ + /* TODO */ +} + +static void *drm_gem_cma_dmabuf_kmap(struct dma_buf *dmabuf, + unsigned long page_num) +{ + /* TODO */ + + return NULL; +} + +static void drm_gem_cma_dmabuf_kunmap(struct dma_buf *dmabuf, + unsigned long page_num, void *addr) +{ + /* TODO */ +} + +static int drm_gem_cma_dmabuf_mmap(struct dma_buf *dmabuf, + struct vm_area_struct *vma) +{ + struct drm_gem_cma_object *cma_obj = dmabuf->priv; + struct drm_gem_object *gem_obj = &cma_obj->base; + int ret; + + ret = drm_gem_mmap_obj(gem_obj, gem_obj->size, vma); + if (ret < 0) + return ret; + + return drm_gem_cma_mmap_obj(cma_obj, vma); +} + +static void *drm_gem_cma_dmabuf_vmap(struct dma_buf *dmabuf) +{ + struct drm_gem_cma_object *cma_obj = dmabuf->priv; + + return cma_obj->vaddr; +} + +static struct dma_buf_ops drm_gem_cma_dmabuf_ops = { + .attach = drm_gem_cma_dmabuf_attach, + .detach = drm_gem_cma_dmabuf_detach, + .map_dma_buf = drm_gem_cma_dmabuf_map, + .unmap_dma_buf = drm_gem_cma_dmabuf_unmap, + .kmap = drm_gem_cma_dmabuf_kmap, + .kmap_atomic = drm_gem_cma_dmabuf_kmap_atomic, + .kunmap = drm_gem_cma_dmabuf_kunmap, + .kunmap_atomic = drm_gem_cma_dmabuf_kunmap_atomic, + .mmap = drm_gem_cma_dmabuf_mmap, + .vmap = drm_gem_cma_dmabuf_vmap, + .release = drm_gem_cma_dmabuf_release, +}; + +struct dma_buf *drm_gem_cma_dmabuf_export(struct drm_device *drm, + struct drm_gem_object *obj, int flags) +{ + struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj); + + return dma_buf_export(cma_obj, &drm_gem_cma_dmabuf_ops, + cma_obj->base.size, flags); +} +EXPORT_SYMBOL_GPL(drm_gem_cma_dmabuf_export); + +struct drm_gem_object *drm_gem_cma_dmabuf_import(struct drm_device *drm, + struct dma_buf *dma_buf) +{ + struct drm_gem_cma_object *cma_obj; + struct dma_buf_attachment *attach; + struct sg_table *sgt; + int ret; + + DRM_DEBUG_PRIME("%s\n", __FILE__); + + /* is this one of own objects? */ + if (dma_buf->ops == &drm_gem_cma_dmabuf_ops) { + struct drm_gem_object *obj; + + cma_obj = dma_buf->priv; + obj = &cma_obj->base; + + /* is it from our device? */ + if (obj->dev == drm) { + /* + * Importing dmabuf exported from out own gem increases + * refcount on gem itself instead of f_count of dmabuf. + */ + drm_gem_object_reference(obj); + dma_buf_put(dma_buf); + return obj; + } + } + + /* Create a CMA GEM buffer. */ + cma_obj = __drm_gem_cma_create(drm, dma_buf->size); + if (IS_ERR(cma_obj)) + return ERR_PTR(PTR_ERR(cma_obj)); + + /* Attach to the buffer and map it. Make sure the mapping is contiguous + * on the device memory bus, as that's all we support. + */ + attach = dma_buf_attach(dma_buf, drm->dev); + if (IS_ERR(attach)) { + ret = -EINVAL; + goto error_gem_free; + } + + sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); + if (IS_ERR_OR_NULL(sgt)) { + ret = sgt ? PTR_ERR(sgt) : -ENOMEM; + goto error_buf_detach; + } + + if (sgt->nents != 1) { + ret = -EINVAL; + goto error_buf_unmap; + } + + cma_obj->base.import_attach = attach; + cma_obj->paddr = sg_dma_address(sgt->sgl); + cma_obj->sgt = sgt; + + DRM_DEBUG_PRIME("dma_addr = 0x%x, size = %zu\n", cma_obj->paddr, + dma_buf->size); + + return &cma_obj->base; + +error_buf_unmap: + dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL); +error_buf_detach: + dma_buf_detach(dma_buf, attach); +error_gem_free: + drm_gem_cma_free_object(&cma_obj->base); + return ERR_PTR(ret); +} +EXPORT_SYMBOL_GPL(drm_gem_cma_dmabuf_import); +#endif diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index 63397ce..6e17251 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -4,6 +4,9 @@ struct drm_gem_cma_object { struct drm_gem_object base; dma_addr_t paddr; + struct sg_table *sgt; + + /* For objects with DMA memory allocated by GEM CMA */ void *vaddr; };
@@ -45,4 +48,10 @@ extern const struct vm_operations_struct drm_gem_cma_vm_ops; void drm_gem_cma_describe(struct drm_gem_cma_object *obj, struct seq_file *m); #endif
+struct dma_buf *drm_gem_cma_dmabuf_export(struct drm_device *drm_dev, + struct drm_gem_object *obj, + int flags); +struct drm_gem_object *drm_gem_cma_dmabuf_import(struct drm_device *drm_dev, + struct dma_buf *dma_buf); + #endif /* __DRM_GEM_CMA_HELPER_H__ */
couple small comments, other than those it looks ok
BR, -R
On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com wrote:
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/drm_gem_cma_helper.c | 321 ++++++++++++++++++++++++++++++++++- include/drm/drm_gem_cma_helper.h | 9 + 2 files changed, 327 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 7a4db4e..1dc2157 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -21,6 +21,9 @@ #include <linux/slab.h> #include <linux/mutex.h> #include <linux/export.h> +#if CONFIG_DMA_SHARED_BUFFER +#include <linux/dma-buf.h> +#endif
I don't think we need the #if, since drm selects DMA_SHARED_BUFFER
and same for other spot below
#include <linux/dma-mapping.h>
#include <drm/drmP.h> @@ -82,6 +85,8 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, unsigned int size) { struct drm_gem_cma_object *cma_obj;
struct sg_table *sgt = NULL;int ret; size = round_up(size, PAGE_SIZE);@@ -94,11 +99,29 @@ struct drm_gem_cma_object *drm_gem_cma_create(struct drm_device *drm, if (!cma_obj->vaddr) { dev_err(drm->dev, "failed to allocate buffer with size %d\n", size);
drm_gem_cma_free_object(&cma_obj->base);return ERR_PTR(-ENOMEM);
ret = -ENOMEM;goto error; }sgt = kzalloc(sizeof(*cma_obj->sgt), GFP_KERNEL);if (sgt == NULL) {ret = -ENOMEM;goto error;}ret = dma_get_sgtable(drm->dev, sgt, cma_obj->vaddr,cma_obj->paddr, size);if (ret < 0)goto error;cma_obj->sgt = sgt;return cma_obj;+error:
kfree(sgt);drm_gem_cma_free_object(&cma_obj->base);return ERR_PTR(ret);} EXPORT_SYMBOL_GPL(drm_gem_cma_create);
@@ -156,9 +179,16 @@ void drm_gem_cma_free_object(struct drm_gem_object *gem_obj)
cma_obj = to_drm_gem_cma_obj(gem_obj);
if (cma_obj->vaddr)
if (cma_obj->vaddr) { dma_free_writecombine(gem_obj->dev->dev, cma_obj->base.size, cma_obj->vaddr, cma_obj->paddr);if (cma_obj->sgt) {sg_free_table(cma_obj->sgt);kfree(cma_obj->sgt);}} else if (gem_obj->import_attach) {drm_prime_gem_destroy(gem_obj, cma_obj->sgt);} drm_gem_object_release(gem_obj);@@ -291,3 +321,288 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m } EXPORT_SYMBOL_GPL(drm_gem_cma_describe); #endif
+/* -----------------------------------------------------------------------------
- DMA-BUF
- */
+#if CONFIG_DMA_SHARED_BUFFER +struct drm_gem_cma_dmabuf_attachment {
struct sg_table sgt;enum dma_data_direction dir;+};
+static int drm_gem_cma_dmabuf_attach(struct dma_buf *dmabuf, struct device *dev,
struct dma_buf_attachment *attach)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach;cma_attach = kzalloc(sizeof(*cma_attach), GFP_KERNEL);if (!cma_attach)return -ENOMEM;cma_attach->dir = DMA_NONE;attach->priv = cma_attach;return 0;+}
+static void drm_gem_cma_dmabuf_detach(struct dma_buf *dmabuf,
struct dma_buf_attachment *attach)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;struct sg_table *sgt;if (cma_attach == NULL)return;sgt = &cma_attach->sgt;if (cma_attach->dir != DMA_NONE)dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents,cma_attach->dir);sg_free_table(sgt);kfree(cma_attach);attach->priv = NULL;+}
+static struct sg_table * +drm_gem_cma_dmabuf_map(struct dma_buf_attachment *attach,
enum dma_data_direction dir)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;struct drm_gem_cma_object *cma_obj = attach->dmabuf->priv;struct drm_device *drm = cma_obj->base.dev;struct scatterlist *rd, *wr;struct sg_table *sgt;unsigned int i;int nents, ret;DRM_DEBUG_PRIME("\n");if (WARN_ON(dir == DMA_NONE))return ERR_PTR(-EINVAL);/* Return the cached mapping when possible. */if (cma_attach->dir == dir)return &cma_attach->sgt;/* Two mappings with different directions for the same attachment are* not allowed.*/if (WARN_ON(cma_attach->dir != DMA_NONE))return ERR_PTR(-EBUSY);sgt = &cma_attach->sgt;ret = sg_alloc_table(sgt, cma_obj->sgt->orig_nents, GFP_KERNEL);if (ret) {DRM_ERROR("failed to alloc sgt.\n");return ERR_PTR(-ENOMEM);}mutex_lock(&drm->struct_mutex);rd = cma_obj->sgt->sgl;wr = sgt->sgl;for (i = 0; i < sgt->orig_nents; ++i) {sg_set_page(wr, sg_page(rd), rd->length, rd->offset);rd = sg_next(rd);wr = sg_next(wr);}nents = dma_map_sg(attach->dev, sgt->sgl, sgt->orig_nents, dir);if (!nents) {DRM_ERROR("failed to map sgl with iommu.\n");sg_free_table(sgt);sgt = ERR_PTR(-EIO);goto done;}cma_attach->dir = dir;attach->priv = cma_attach;DRM_DEBUG_PRIME("buffer size = %zu\n", cma_obj->base.size);+done:
mutex_unlock(&drm->struct_mutex);return sgt;+}
+static void drm_gem_cma_dmabuf_unmap(struct dma_buf_attachment *attach,
struct sg_table *sgt,enum dma_data_direction dir)+{
/* Nothing to do. */
I kinda think that if you don't support multiple mappings with different direction, that you should at least support unmap and then let someone else map with other direction
+}
+static void drm_gem_cma_dmabuf_release(struct dma_buf *dmabuf) +{
struct drm_gem_cma_object *cma_obj = dmabuf->priv;DRM_DEBUG_PRIME("%s\n", __FILE__);/** drm_gem_cma_dmabuf_release() call means that file object's* f_count is 0 and it calls drm_gem_object_handle_unreference()* to drop the references that these values had been increased* at drm_prime_handle_to_fd()*/if (cma_obj->base.export_dma_buf == dmabuf) {cma_obj->base.export_dma_buf = NULL;/** drop this gem object refcount to release allocated buffer* and resources.*/drm_gem_object_unreference_unlocked(&cma_obj->base);}+}
+static void *drm_gem_cma_dmabuf_kmap_atomic(struct dma_buf *dmabuf,
unsigned long page_num)+{
/* TODO */return NULL;+}
+static void drm_gem_cma_dmabuf_kunmap_atomic(struct dma_buf *dmabuf,
unsigned long page_num, void *addr)+{
/* TODO */+}
+static void *drm_gem_cma_dmabuf_kmap(struct dma_buf *dmabuf,
unsigned long page_num)+{
/* TODO */return NULL;+}
+static void drm_gem_cma_dmabuf_kunmap(struct dma_buf *dmabuf,
unsigned long page_num, void *addr)+{
/* TODO */+}
+static int drm_gem_cma_dmabuf_mmap(struct dma_buf *dmabuf,
struct vm_area_struct *vma)+{
struct drm_gem_cma_object *cma_obj = dmabuf->priv;struct drm_gem_object *gem_obj = &cma_obj->base;int ret;ret = drm_gem_mmap_obj(gem_obj, gem_obj->size, vma);if (ret < 0)return ret;return drm_gem_cma_mmap_obj(cma_obj, vma);+}
+static void *drm_gem_cma_dmabuf_vmap(struct dma_buf *dmabuf) +{
struct drm_gem_cma_object *cma_obj = dmabuf->priv;return cma_obj->vaddr;+}
+static struct dma_buf_ops drm_gem_cma_dmabuf_ops = {
.attach = drm_gem_cma_dmabuf_attach,.detach = drm_gem_cma_dmabuf_detach,.map_dma_buf = drm_gem_cma_dmabuf_map,.unmap_dma_buf = drm_gem_cma_dmabuf_unmap,.kmap = drm_gem_cma_dmabuf_kmap,.kmap_atomic = drm_gem_cma_dmabuf_kmap_atomic,.kunmap = drm_gem_cma_dmabuf_kunmap,.kunmap_atomic = drm_gem_cma_dmabuf_kunmap_atomic,.mmap = drm_gem_cma_dmabuf_mmap,.vmap = drm_gem_cma_dmabuf_vmap,.release = drm_gem_cma_dmabuf_release,+};
+struct dma_buf *drm_gem_cma_dmabuf_export(struct drm_device *drm,
struct drm_gem_object *obj, int flags)+{
struct drm_gem_cma_object *cma_obj = to_drm_gem_cma_obj(obj);return dma_buf_export(cma_obj, &drm_gem_cma_dmabuf_ops,cma_obj->base.size, flags);+} +EXPORT_SYMBOL_GPL(drm_gem_cma_dmabuf_export);
+struct drm_gem_object *drm_gem_cma_dmabuf_import(struct drm_device *drm,
struct dma_buf *dma_buf)+{
struct drm_gem_cma_object *cma_obj;struct dma_buf_attachment *attach;struct sg_table *sgt;int ret;DRM_DEBUG_PRIME("%s\n", __FILE__);/* is this one of own objects? */if (dma_buf->ops == &drm_gem_cma_dmabuf_ops) {struct drm_gem_object *obj;cma_obj = dma_buf->priv;obj = &cma_obj->base;/* is it from our device? */if (obj->dev == drm) {/** Importing dmabuf exported from out own gem increases* refcount on gem itself instead of f_count of dmabuf.*/drm_gem_object_reference(obj);dma_buf_put(dma_buf);return obj;}}/* Create a CMA GEM buffer. */cma_obj = __drm_gem_cma_create(drm, dma_buf->size);if (IS_ERR(cma_obj))return ERR_PTR(PTR_ERR(cma_obj));/* Attach to the buffer and map it. Make sure the mapping is contiguous* on the device memory bus, as that's all we support.*/attach = dma_buf_attach(dma_buf, drm->dev);if (IS_ERR(attach)) {ret = -EINVAL;goto error_gem_free;}sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);if (IS_ERR_OR_NULL(sgt)) {ret = sgt ? PTR_ERR(sgt) : -ENOMEM;goto error_buf_detach;}if (sgt->nents != 1) {ret = -EINVAL;goto error_buf_unmap;}cma_obj->base.import_attach = attach;cma_obj->paddr = sg_dma_address(sgt->sgl);cma_obj->sgt = sgt;DRM_DEBUG_PRIME("dma_addr = 0x%x, size = %zu\n", cma_obj->paddr,dma_buf->size);return &cma_obj->base;+error_buf_unmap:
dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);+error_buf_detach:
dma_buf_detach(dma_buf, attach);+error_gem_free:
drm_gem_cma_free_object(&cma_obj->base);return ERR_PTR(ret);+} +EXPORT_SYMBOL_GPL(drm_gem_cma_dmabuf_import); +#endif diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index 63397ce..6e17251 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -4,6 +4,9 @@ struct drm_gem_cma_object { struct drm_gem_object base; dma_addr_t paddr;
struct sg_table *sgt;/* For objects with DMA memory allocated by GEM CMA */ void *vaddr;};
@@ -45,4 +48,10 @@ extern const struct vm_operations_struct drm_gem_cma_vm_ops; void drm_gem_cma_describe(struct drm_gem_cma_object *obj, struct seq_file *m); #endif
+struct dma_buf *drm_gem_cma_dmabuf_export(struct drm_device *drm_dev,
struct drm_gem_object *obj,int flags);+struct drm_gem_object *drm_gem_cma_dmabuf_import(struct drm_device *drm_dev,
struct dma_buf *dma_buf);#endif /* __DRM_GEM_CMA_HELPER_H__ */
1.8.1.5
Hi Rob,
On Tuesday 04 June 2013 17:56:36 Rob Clark wrote:
couple small comments, other than those it looks ok
Thanks for the review.
On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart wrote:
Signed-off-by: Laurent Pinchart
laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/drm_gem_cma_helper.c | 321 +++++++++++++++++++++++++++++- include/drm/drm_gem_cma_helper.h | 9 + 2 files changed, 327 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 7a4db4e..1dc2157 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -21,6 +21,9 @@ #include <linux/slab.h> #include <linux/mutex.h> #include <linux/export.h> +#if CONFIG_DMA_SHARED_BUFFER +#include <linux/dma-buf.h> +#endif
I don't think we need the #if, since drm selects DMA_SHARED_BUFFER
and same for other spot below
Indeed. Will be fixed in the next version.
#include <linux/dma-mapping.h>
#include <drm/drmP.h>
[snip]
@@ -291,3 +321,288 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m> } EXPORT_SYMBOL_GPL(drm_gem_cma_describe); #endif
+/*
---- + * DMA-BUF
- */
+#if CONFIG_DMA_SHARED_BUFFER +struct drm_gem_cma_dmabuf_attachment {
struct sg_table sgt;enum dma_data_direction dir;+};
+static int drm_gem_cma_dmabuf_attach(struct dma_buf *dmabuf, struct device *dev, + struct dma_buf_attachment *attach) +{
struct drm_gem_cma_dmabuf_attachment *cma_attach;cma_attach = kzalloc(sizeof(*cma_attach), GFP_KERNEL);if (!cma_attach)return -ENOMEM;cma_attach->dir = DMA_NONE;attach->priv = cma_attach;return 0;+}
+static void drm_gem_cma_dmabuf_detach(struct dma_buf *dmabuf,
struct dma_buf_attachment *attach)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;struct sg_table *sgt;if (cma_attach == NULL)return;sgt = &cma_attach->sgt;if (cma_attach->dir != DMA_NONE)dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents,cma_attach->dir);sg_free_table(sgt);kfree(cma_attach);attach->priv = NULL;+}
+static struct sg_table * +drm_gem_cma_dmabuf_map(struct dma_buf_attachment *attach,
enum dma_data_direction dir)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;struct drm_gem_cma_object *cma_obj = attach->dmabuf->priv;struct drm_device *drm = cma_obj->base.dev;struct scatterlist *rd, *wr;struct sg_table *sgt;unsigned int i;int nents, ret;DRM_DEBUG_PRIME("\n");if (WARN_ON(dir == DMA_NONE))return ERR_PTR(-EINVAL);/* Return the cached mapping when possible. */if (cma_attach->dir == dir)return &cma_attach->sgt;/* Two mappings with different directions for the same attachmentare + * not allowed.
*/if (WARN_ON(cma_attach->dir != DMA_NONE))return ERR_PTR(-EBUSY);sgt = &cma_attach->sgt;ret = sg_alloc_table(sgt, cma_obj->sgt->orig_nents, GFP_KERNEL);if (ret) {DRM_ERROR("failed to alloc sgt.\n");return ERR_PTR(-ENOMEM);}mutex_lock(&drm->struct_mutex);rd = cma_obj->sgt->sgl;wr = sgt->sgl;for (i = 0; i < sgt->orig_nents; ++i) {sg_set_page(wr, sg_page(rd), rd->length, rd->offset);rd = sg_next(rd);wr = sg_next(wr);}nents = dma_map_sg(attach->dev, sgt->sgl, sgt->orig_nents, dir);if (!nents) {DRM_ERROR("failed to map sgl with iommu.\n");sg_free_table(sgt);sgt = ERR_PTR(-EIO);goto done;}cma_attach->dir = dir;attach->priv = cma_attach;DRM_DEBUG_PRIME("buffer size = %zu\n", cma_obj->base.size);+done:
mutex_unlock(&drm->struct_mutex);return sgt;+}
+static void drm_gem_cma_dmabuf_unmap(struct dma_buf_attachment *attach,
struct sg_table *sgt,enum dma_data_direction dir)+{
/* Nothing to do. */I kinda think that if you don't support multiple mappings with different direction, that you should at least support unmap and then let someone else map with other direction
That would make sense, but in that case I wouldn't be able to cache the mapping. It would probably be better to add proper support for multiple mappings with different directions.
Given that the mapping is cached in the attachment, this will only be an issue if a driver tries to map the same attachment twice with different directions. Isn't that an uncommon use case that could be fixed later ? :-) I'd like to get this set in v3.11 if possible.
+}
On Tue, Jun 4, 2013 at 9:22 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
On Tuesday 04 June 2013 17:56:36 Rob Clark wrote:
couple small comments, other than those it looks ok
Thanks for the review.
On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart wrote:
Signed-off-by: Laurent Pinchart
laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/drm_gem_cma_helper.c | 321 +++++++++++++++++++++++++++++- include/drm/drm_gem_cma_helper.h | 9 + 2 files changed, 327 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 7a4db4e..1dc2157 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -21,6 +21,9 @@ #include <linux/slab.h> #include <linux/mutex.h> #include <linux/export.h> +#if CONFIG_DMA_SHARED_BUFFER +#include <linux/dma-buf.h> +#endif
I don't think we need the #if, since drm selects DMA_SHARED_BUFFER
and same for other spot below
Indeed. Will be fixed in the next version.
#include <linux/dma-mapping.h>
#include <drm/drmP.h>
[snip]
@@ -291,3 +321,288 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m> } EXPORT_SYMBOL_GPL(drm_gem_cma_describe); #endif
+/*
---- + * DMA-BUF
- */
+#if CONFIG_DMA_SHARED_BUFFER +struct drm_gem_cma_dmabuf_attachment {
struct sg_table sgt;enum dma_data_direction dir;+};
+static int drm_gem_cma_dmabuf_attach(struct dma_buf *dmabuf, struct device *dev, + struct dma_buf_attachment *attach) +{
struct drm_gem_cma_dmabuf_attachment *cma_attach;cma_attach = kzalloc(sizeof(*cma_attach), GFP_KERNEL);if (!cma_attach)return -ENOMEM;cma_attach->dir = DMA_NONE;attach->priv = cma_attach;return 0;+}
+static void drm_gem_cma_dmabuf_detach(struct dma_buf *dmabuf,
struct dma_buf_attachment *attach)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;struct sg_table *sgt;if (cma_attach == NULL)return;sgt = &cma_attach->sgt;if (cma_attach->dir != DMA_NONE)dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents,cma_attach->dir);sg_free_table(sgt);kfree(cma_attach);attach->priv = NULL;+}
+static struct sg_table * +drm_gem_cma_dmabuf_map(struct dma_buf_attachment *attach,
enum dma_data_direction dir)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;struct drm_gem_cma_object *cma_obj = attach->dmabuf->priv;struct drm_device *drm = cma_obj->base.dev;struct scatterlist *rd, *wr;struct sg_table *sgt;unsigned int i;int nents, ret;DRM_DEBUG_PRIME("\n");if (WARN_ON(dir == DMA_NONE))return ERR_PTR(-EINVAL);/* Return the cached mapping when possible. */if (cma_attach->dir == dir)return &cma_attach->sgt;/* Two mappings with different directions for the same attachmentare + * not allowed.
*/if (WARN_ON(cma_attach->dir != DMA_NONE))return ERR_PTR(-EBUSY);sgt = &cma_attach->sgt;ret = sg_alloc_table(sgt, cma_obj->sgt->orig_nents, GFP_KERNEL);if (ret) {DRM_ERROR("failed to alloc sgt.\n");return ERR_PTR(-ENOMEM);}mutex_lock(&drm->struct_mutex);rd = cma_obj->sgt->sgl;wr = sgt->sgl;for (i = 0; i < sgt->orig_nents; ++i) {sg_set_page(wr, sg_page(rd), rd->length, rd->offset);rd = sg_next(rd);wr = sg_next(wr);}nents = dma_map_sg(attach->dev, sgt->sgl, sgt->orig_nents, dir);if (!nents) {DRM_ERROR("failed to map sgl with iommu.\n");sg_free_table(sgt);sgt = ERR_PTR(-EIO);goto done;}cma_attach->dir = dir;attach->priv = cma_attach;DRM_DEBUG_PRIME("buffer size = %zu\n", cma_obj->base.size);+done:
mutex_unlock(&drm->struct_mutex);return sgt;+}
+static void drm_gem_cma_dmabuf_unmap(struct dma_buf_attachment *attach,
struct sg_table *sgt,enum dma_data_direction dir)+{
/* Nothing to do. */I kinda think that if you don't support multiple mappings with different direction, that you should at least support unmap and then let someone else map with other direction
That would make sense, but in that case I wouldn't be able to cache the mapping. It would probably be better to add proper support for multiple mappings with different directions.
well, you could still cache it, you'd just have to invalidate that cache on transition in direction
Given that the mapping is cached in the attachment, this will only be an issue if a driver tries to map the same attachment twice with different directions. Isn't that an uncommon use case that could be fixed later ? :-) I'd like to get this set in v3.11 if possible.
I don't feel strongly that this should block merging, vs fixing at a (not too much later) date
BR, -R
+}
-- Regards,
Laurent Pinchart
On Tue, Jun 04, 2013 at 11:05:36PM -0400, Rob Clark wrote:
On Tue, Jun 4, 2013 at 9:22 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
On Tuesday 04 June 2013 17:56:36 Rob Clark wrote:
couple small comments, other than those it looks ok
Thanks for the review.
On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart wrote:
Signed-off-by: Laurent Pinchart
laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/drm_gem_cma_helper.c | 321 +++++++++++++++++++++++++++++- include/drm/drm_gem_cma_helper.h | 9 + 2 files changed, 327 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 7a4db4e..1dc2157 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -21,6 +21,9 @@ #include <linux/slab.h> #include <linux/mutex.h> #include <linux/export.h> +#if CONFIG_DMA_SHARED_BUFFER +#include <linux/dma-buf.h> +#endif
I don't think we need the #if, since drm selects DMA_SHARED_BUFFER
and same for other spot below
Indeed. Will be fixed in the next version.
#include <linux/dma-mapping.h>
#include <drm/drmP.h>
[snip]
@@ -291,3 +321,288 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m> } EXPORT_SYMBOL_GPL(drm_gem_cma_describe); #endif
+/*
---- + * DMA-BUF
- */
+#if CONFIG_DMA_SHARED_BUFFER +struct drm_gem_cma_dmabuf_attachment {
struct sg_table sgt;enum dma_data_direction dir;+};
+static int drm_gem_cma_dmabuf_attach(struct dma_buf *dmabuf, struct device *dev, + struct dma_buf_attachment *attach) +{
struct drm_gem_cma_dmabuf_attachment *cma_attach;cma_attach = kzalloc(sizeof(*cma_attach), GFP_KERNEL);if (!cma_attach)return -ENOMEM;cma_attach->dir = DMA_NONE;attach->priv = cma_attach;return 0;+}
+static void drm_gem_cma_dmabuf_detach(struct dma_buf *dmabuf,
struct dma_buf_attachment *attach)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;struct sg_table *sgt;if (cma_attach == NULL)return;sgt = &cma_attach->sgt;if (cma_attach->dir != DMA_NONE)dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents,cma_attach->dir);sg_free_table(sgt);kfree(cma_attach);attach->priv = NULL;+}
+static struct sg_table * +drm_gem_cma_dmabuf_map(struct dma_buf_attachment *attach,
enum dma_data_direction dir)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;struct drm_gem_cma_object *cma_obj = attach->dmabuf->priv;struct drm_device *drm = cma_obj->base.dev;struct scatterlist *rd, *wr;struct sg_table *sgt;unsigned int i;int nents, ret;DRM_DEBUG_PRIME("\n");if (WARN_ON(dir == DMA_NONE))return ERR_PTR(-EINVAL);/* Return the cached mapping when possible. */if (cma_attach->dir == dir)return &cma_attach->sgt;/* Two mappings with different directions for the same attachmentare + * not allowed.
*/if (WARN_ON(cma_attach->dir != DMA_NONE))return ERR_PTR(-EBUSY);sgt = &cma_attach->sgt;ret = sg_alloc_table(sgt, cma_obj->sgt->orig_nents, GFP_KERNEL);if (ret) {DRM_ERROR("failed to alloc sgt.\n");return ERR_PTR(-ENOMEM);}mutex_lock(&drm->struct_mutex);rd = cma_obj->sgt->sgl;wr = sgt->sgl;for (i = 0; i < sgt->orig_nents; ++i) {sg_set_page(wr, sg_page(rd), rd->length, rd->offset);rd = sg_next(rd);wr = sg_next(wr);}nents = dma_map_sg(attach->dev, sgt->sgl, sgt->orig_nents, dir);if (!nents) {DRM_ERROR("failed to map sgl with iommu.\n");sg_free_table(sgt);sgt = ERR_PTR(-EIO);goto done;}cma_attach->dir = dir;attach->priv = cma_attach;DRM_DEBUG_PRIME("buffer size = %zu\n", cma_obj->base.size);+done:
mutex_unlock(&drm->struct_mutex);return sgt;+}
+static void drm_gem_cma_dmabuf_unmap(struct dma_buf_attachment *attach,
struct sg_table *sgt,enum dma_data_direction dir)+{
/* Nothing to do. */I kinda think that if you don't support multiple mappings with different direction, that you should at least support unmap and then let someone else map with other direction
That would make sense, but in that case I wouldn't be able to cache the mapping. It would probably be better to add proper support for multiple mappings with different directions.
well, you could still cache it, you'd just have to invalidate that cache on transition in direction
Given that the mapping is cached in the attachment, this will only be an issue if a driver tries to map the same attachment twice with different directions. Isn't that an uncommon use case that could be fixed later ? :-) I'd like to get this set in v3.11 if possible.
I don't feel strongly that this should block merging, vs fixing at a (not too much later) date
I'm not sure whether we should even allow this at all. If an importer wants to switch dma directions he should probably map it bidirectional and we should finally bite the bullet and add a attachment_sync callback to flush caches without dropping the mapping on the floor ...
Tbh I'm not even sure whether multiple mappings make any sense at all for one attachment. Imo that sounds like the importer seriously lost track of things ;-) -Daniel
On Wed, Jun 5, 2013 at 4:44 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Jun 04, 2013 at 11:05:36PM -0400, Rob Clark wrote:
On Tue, Jun 4, 2013 at 9:22 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Rob,
On Tuesday 04 June 2013 17:56:36 Rob Clark wrote:
couple small comments, other than those it looks ok
Thanks for the review.
On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart wrote:
Signed-off-by: Laurent Pinchart
laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/drm_gem_cma_helper.c | 321 +++++++++++++++++++++++++++++- include/drm/drm_gem_cma_helper.h | 9 + 2 files changed, 327 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 7a4db4e..1dc2157 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -21,6 +21,9 @@ #include <linux/slab.h> #include <linux/mutex.h> #include <linux/export.h> +#if CONFIG_DMA_SHARED_BUFFER +#include <linux/dma-buf.h> +#endif
I don't think we need the #if, since drm selects DMA_SHARED_BUFFER
and same for other spot below
Indeed. Will be fixed in the next version.
#include <linux/dma-mapping.h>
#include <drm/drmP.h>
[snip]
@@ -291,3 +321,288 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m> } EXPORT_SYMBOL_GPL(drm_gem_cma_describe); #endif
+/*
---- + * DMA-BUF
- */
+#if CONFIG_DMA_SHARED_BUFFER +struct drm_gem_cma_dmabuf_attachment {
struct sg_table sgt;enum dma_data_direction dir;+};
+static int drm_gem_cma_dmabuf_attach(struct dma_buf *dmabuf, struct device *dev, + struct dma_buf_attachment *attach) +{
struct drm_gem_cma_dmabuf_attachment *cma_attach;cma_attach = kzalloc(sizeof(*cma_attach), GFP_KERNEL);if (!cma_attach)return -ENOMEM;cma_attach->dir = DMA_NONE;attach->priv = cma_attach;return 0;+}
+static void drm_gem_cma_dmabuf_detach(struct dma_buf *dmabuf,
struct dma_buf_attachment *attach)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;struct sg_table *sgt;if (cma_attach == NULL)return;sgt = &cma_attach->sgt;if (cma_attach->dir != DMA_NONE)dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents,cma_attach->dir);sg_free_table(sgt);kfree(cma_attach);attach->priv = NULL;+}
+static struct sg_table * +drm_gem_cma_dmabuf_map(struct dma_buf_attachment *attach,
enum dma_data_direction dir)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach = attach->priv;struct drm_gem_cma_object *cma_obj = attach->dmabuf->priv;struct drm_device *drm = cma_obj->base.dev;struct scatterlist *rd, *wr;struct sg_table *sgt;unsigned int i;int nents, ret;DRM_DEBUG_PRIME("\n");if (WARN_ON(dir == DMA_NONE))return ERR_PTR(-EINVAL);/* Return the cached mapping when possible. */if (cma_attach->dir == dir)return &cma_attach->sgt;/* Two mappings with different directions for the same attachmentare + * not allowed.
*/if (WARN_ON(cma_attach->dir != DMA_NONE))return ERR_PTR(-EBUSY);sgt = &cma_attach->sgt;ret = sg_alloc_table(sgt, cma_obj->sgt->orig_nents, GFP_KERNEL);if (ret) {DRM_ERROR("failed to alloc sgt.\n");return ERR_PTR(-ENOMEM);}mutex_lock(&drm->struct_mutex);rd = cma_obj->sgt->sgl;wr = sgt->sgl;for (i = 0; i < sgt->orig_nents; ++i) {sg_set_page(wr, sg_page(rd), rd->length, rd->offset);rd = sg_next(rd);wr = sg_next(wr);}nents = dma_map_sg(attach->dev, sgt->sgl, sgt->orig_nents, dir);if (!nents) {DRM_ERROR("failed to map sgl with iommu.\n");sg_free_table(sgt);sgt = ERR_PTR(-EIO);goto done;}cma_attach->dir = dir;attach->priv = cma_attach;DRM_DEBUG_PRIME("buffer size = %zu\n", cma_obj->base.size);+done:
mutex_unlock(&drm->struct_mutex);return sgt;+}
+static void drm_gem_cma_dmabuf_unmap(struct dma_buf_attachment *attach,
struct sg_table *sgt,enum dma_data_direction dir)+{
/* Nothing to do. */I kinda think that if you don't support multiple mappings with different direction, that you should at least support unmap and then let someone else map with other direction
That would make sense, but in that case I wouldn't be able to cache the mapping. It would probably be better to add proper support for multiple mappings with different directions.
well, you could still cache it, you'd just have to invalidate that cache on transition in direction
Given that the mapping is cached in the attachment, this will only be an issue if a driver tries to map the same attachment twice with different directions. Isn't that an uncommon use case that could be fixed later ? :-) I'd like to get this set in v3.11 if possible.
I don't feel strongly that this should block merging, vs fixing at a (not too much later) date
I'm not sure whether we should even allow this at all. If an importer wants to switch dma directions he should probably map it bidirectional and we should finally bite the bullet and add a attachment_sync callback to flush caches without dropping the mapping on the floor ...
Tbh I'm not even sure whether multiple mappings make any sense at all for one attachment. Imo that sounds like the importer seriously lost track of things ;-)
oh, good point.. I was thinking more the case of multiple importers, but you are right, there would be multiple attachments in that case so this wouldn't be the problem
BR, -R
-Daniel
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wednesday 05 June 2013 07:00:45 Rob Clark wrote:
On Wed, Jun 5, 2013 at 4:44 AM, Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Jun 04, 2013 at 11:05:36PM -0400, Rob Clark wrote:
On Tue, Jun 4, 2013 at 9:22 PM, Laurent Pinchart wrote:
On Tuesday 04 June 2013 17:56:36 Rob Clark wrote:
couple small comments, other than those it looks ok
Thanks for the review.
On Mon, Jun 3, 2013 at 10:20 PM, Laurent Pinchart wrote:
Signed-off-by: Laurent Pinchart
laurent.pinchart+renesas@ideasonboard.com
drivers/gpu/drm/drm_gem_cma_helper.c | 321 +++++++++++++++++++++++++++++- include/drm/drm_gem_cma_helper.h | 9 + 2 files changed, 327 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 7a4db4e..1dc2157 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -21,6 +21,9 @@
#include <linux/slab.h> #include <linux/mutex.h> #include <linux/export.h>
+#if CONFIG_DMA_SHARED_BUFFER +#include <linux/dma-buf.h> +#endif
I don't think we need the #if, since drm selects DMA_SHARED_BUFFER
and same for other spot below
Indeed. Will be fixed in the next version.
#include <linux/dma-mapping.h>
#include <drm/drmP.h>
[snip]
@@ -291,3 +321,288 @@ void drm_gem_cma_describe(struct drm_gem_cma_object *cma_obj, struct seq_file *m>
} EXPORT_SYMBOL_GPL(drm_gem_cma_describe); #endif
+/*
---- + * DMA-BUF
- */
+#if CONFIG_DMA_SHARED_BUFFER +struct drm_gem_cma_dmabuf_attachment {
struct sg_table sgt;enum dma_data_direction dir;+};
+static int drm_gem_cma_dmabuf_attach(struct dma_buf *dmabuf, struct device *dev, + struct dma_buf_attachment *attach) +{
struct drm_gem_cma_dmabuf_attachment *cma_attach;cma_attach = kzalloc(sizeof(*cma_attach), GFP_KERNEL);if (!cma_attach)return -ENOMEM;cma_attach->dir = DMA_NONE;attach->priv = cma_attach;return 0;+}
+static void drm_gem_cma_dmabuf_detach(struct dma_buf *dmabuf,
struct dma_buf_attachment*attach) +{
struct drm_gem_cma_dmabuf_attachment *cma_attach =attach->priv;
struct sg_table *sgt;if (cma_attach == NULL)return;sgt = &cma_attach->sgt;if (cma_attach->dir != DMA_NONE)dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents,cma_attach->dir);sg_free_table(sgt);kfree(cma_attach);attach->priv = NULL;+}
+static struct sg_table * +drm_gem_cma_dmabuf_map(struct dma_buf_attachment *attach,
enum dma_data_direction dir)+{
struct drm_gem_cma_dmabuf_attachment *cma_attach =attach->priv;
struct drm_gem_cma_object *cma_obj = attach->dmabuf->priv;struct drm_device *drm = cma_obj->base.dev;struct scatterlist *rd, *wr;struct sg_table *sgt;unsigned int i;int nents, ret;DRM_DEBUG_PRIME("\n");if (WARN_ON(dir == DMA_NONE))return ERR_PTR(-EINVAL);/* Return the cached mapping when possible. */if (cma_attach->dir == dir)return &cma_attach->sgt;/* Two mappings with different directions for the sameattachment are + * not allowed.
*/if (WARN_ON(cma_attach->dir != DMA_NONE))return ERR_PTR(-EBUSY);sgt = &cma_attach->sgt;ret = sg_alloc_table(sgt, cma_obj->sgt->orig_nents,GFP_KERNEL);
if (ret) {DRM_ERROR("failed to alloc sgt.\n");return ERR_PTR(-ENOMEM);}mutex_lock(&drm->struct_mutex);rd = cma_obj->sgt->sgl;wr = sgt->sgl;for (i = 0; i < sgt->orig_nents; ++i) {sg_set_page(wr, sg_page(rd), rd->length,rd->offset);
rd = sg_next(rd);wr = sg_next(wr);}nents = dma_map_sg(attach->dev, sgt->sgl, sgt->orig_nents,dir);
if (!nents) {DRM_ERROR("failed to map sgl with iommu.\n");sg_free_table(sgt);sgt = ERR_PTR(-EIO);goto done;}cma_attach->dir = dir;attach->priv = cma_attach;DRM_DEBUG_PRIME("buffer size = %zu\n", cma_obj->base.size);+done:
mutex_unlock(&drm->struct_mutex);return sgt;+}
+static void drm_gem_cma_dmabuf_unmap(struct dma_buf_attachment *attach,
struct sg_table *sgt,enum dma_data_direction dir)+{
/* Nothing to do. */I kinda think that if you don't support multiple mappings with different direction, that you should at least support unmap and then let someone else map with other direction
That would make sense, but in that case I wouldn't be able to cache the mapping. It would probably be better to add proper support for multiple mappings with different directions.
well, you could still cache it, you'd just have to invalidate that cache on transition in direction
Given that the mapping is cached in the attachment, this will only be an issue if a driver tries to map the same attachment twice with different directions. Isn't that an uncommon use case that could be fixed later ? :-) I'd like to get this set in v3.11 if possible.
I don't feel strongly that this should block merging, vs fixing at a (not too much later) date
I'm not sure whether we should even allow this at all. If an importer wants to switch dma directions he should probably map it bidirectional and we should finally bite the bullet and add a attachment_sync callback to flush caches without dropping the mapping on the floor ...
Tbh I'm not even sure whether multiple mappings make any sense at all for one attachment. Imo that sounds like the importer seriously lost track of things ;-)
oh, good point.. I was thinking more the case of multiple importers, but you are right, there would be multiple attachments in that case so this wouldn't be the problem
Problem solved, great :-) I've posted v3.
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Laurent Pinchart -
Laurent Pinchart -
Rob Clark