This series adds helper functions that abstract the core parts of .gem_prime_import and .gem_prime_export so that drivers don't have to worry about the low-level details. These helpers are optional. A driver can use them by plugging in drm_gem_prime_import and drm_gem_prime_export into the drm_driver structure, or it can bypass them by plugging in its own functions. The first patch adds these helpers, and the later patches switch three drivers over to using them.
This version of the series addresses concerns raised by Daniel Vetter, and to leave the vmap locking and refcounting to the dma-buf core code. It also drops Exynos from the series since its driver diverged between when I first sent out this series and now.
This series depends on commit 90b6e90cb03352a352015ca213ac9f4fab3308f3 of the for-next branch of git://git.linaro.org/people/sumitsemwal/linux-dma-buf
Aaron Plattner (3): drm: add prime helpers drm/nouveau: use prime helpers drm/radeon: use prime helpers
Documentation/DocBook/drm.tmpl | 4 + drivers/gpu/drm/drm_prime.c | 186 +++++++++++++++++++++++++++++++- drivers/gpu/drm/nouveau/nouveau_bo.h | 1 - drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +- drivers/gpu/drm/nouveau/nouveau_gem.c | 2 - drivers/gpu/drm/nouveau/nouveau_gem.h | 10 +- drivers/gpu/drm/nouveau/nouveau_prime.c | 173 ++++------------------------- drivers/gpu/drm/radeon/radeon.h | 1 - drivers/gpu/drm/radeon/radeon_drv.c | 21 ++-- drivers/gpu/drm/radeon/radeon_prime.c | 170 ++++------------------------- include/drm/drmP.h | 12 +++ 11 files changed, 270 insertions(+), 319 deletions(-)
Instead of reimplementing all of the dma_buf functionality in every driver, create helpers drm_prime_import and drm_prime_export that implement them in terms of new, lower-level hook functions:
gem_prime_pin: callback when a buffer is created, used to pin buffers into GTT gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for export gem_prime_import_sg_table: convert an sg_table into a drm_gem_object gem_prime_vmap, gem_prime_vunmap: map and unmap an object
These hooks are optional; drivers can opt in by using drm_gem_prime_import and drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of struct drm_driver.
v2: - Drop .begin_cpu_access. None of the drivers this code replaces implemented it. Having it here was a leftover from when I was trying to include i915 in this rework. - Use mutex_lock instead of mutex_lock_interruptible, as these three drivers did. This patch series shouldn't change that behavior. - Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table. Rename struct sg_table* variables to 'sgt' for clarity. - Update drm.tmpl for these new hooks.
v3: - Pass the vaddr down to the driver. This lets drivers that just call vunmap on the pointer avoid having to store the pointer in their GEM private structures. - Move documentation into a /** DOC */ comment in drm_prime.c and include it in drm.tmpl with a !P line. I tried to use !F lines to include documentation of the individual functions from drmP.h, but the docproc / kernel-doc scripts barf on that file, so hopefully this is good enough for now. - apply refcount fix from commit be8a42ae60addd8b6092535c11b42d099d6470ec ("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner aplattner@nvidia.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Airlie airlied@linux.ie --- Documentation/DocBook/drm.tmpl | 4 + drivers/gpu/drm/drm_prime.c | 186 ++++++++++++++++++++++++++++++++++++++++- include/drm/drmP.h | 12 +++ 3 files changed, 201 insertions(+), 1 deletion(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 4ee2304..ed40576 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -743,6 +743,10 @@ char *date;</synopsis> These two operations are mandatory for GEM drivers that support DRM PRIME. </para> + <sect4> + <title>DRM PRIME Helper Functions Reference</title> +!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers + </sect4> </sect3> <sect3 id="drm-gem-objects-mapping"> <title>GEM Objects Mapping</title> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7f12573..366910d 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -53,7 +53,8 @@ * Self-importing: if userspace is using PRIME as a replacement for flink * then it will get a fd->handle request for a GEM object that it created. * Drivers should detect this situation and return back the gem object - * from the dma-buf private. + * from the dma-buf private. Prime will do this automatically for drivers that + * use the drm_gem_prime_{import,export} helpers. */
struct drm_prime_member { @@ -62,6 +63,137 @@ struct drm_prime_member { uint32_t handle; };
+static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach, + enum dma_data_direction dir) +{ + struct drm_gem_object *obj = attach->dmabuf->priv; + struct sg_table *sgt; + + mutex_lock(&obj->dev->struct_mutex); + + sgt = obj->dev->driver->gem_prime_get_sg_table(obj); + + if (!IS_ERR_OR_NULL(sgt)) + dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir); + + mutex_unlock(&obj->dev->struct_mutex); + return sgt; +} + +static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach, + struct sg_table *sgt, enum dma_data_direction dir) +{ + dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir); + sg_free_table(sgt); + kfree(sgt); +} + +static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) +{ + struct drm_gem_object *obj = dma_buf->priv; + + if (obj->export_dma_buf == dma_buf) { + /* drop the reference on the export fd holds */ + obj->export_dma_buf = NULL; + drm_gem_object_unreference_unlocked(obj); + } +} + +static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) +{ + struct drm_gem_object *obj = dma_buf->priv; + struct drm_device *dev = obj->dev; + + return dev->driver->gem_prime_vmap(obj); +} + +static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) +{ + struct drm_gem_object *obj = dma_buf->priv; + struct drm_device *dev = obj->dev; + + dev->driver->gem_prime_vunmap(obj, vaddr); +} + +static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf, + unsigned long page_num) +{ + return NULL; +} + +static void drm_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf, + unsigned long page_num, void *addr) +{ + +} +static void *drm_gem_dmabuf_kmap(struct dma_buf *dma_buf, + unsigned long page_num) +{ + return NULL; +} + +static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf, + unsigned long page_num, void *addr) +{ + +} + +static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf, + struct vm_area_struct *vma) +{ + return -EINVAL; +} + +static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = { + .map_dma_buf = drm_gem_map_dma_buf, + .unmap_dma_buf = drm_gem_unmap_dma_buf, + .release = drm_gem_dmabuf_release, + .kmap = drm_gem_dmabuf_kmap, + .kmap_atomic = drm_gem_dmabuf_kmap_atomic, + .kunmap = drm_gem_dmabuf_kunmap, + .kunmap_atomic = drm_gem_dmabuf_kunmap_atomic, + .mmap = drm_gem_dmabuf_mmap, + .vmap = drm_gem_dmabuf_vmap, + .vunmap = drm_gem_dmabuf_vunmap, +}; + +/** + * DOC: PRIME Helpers + * + * Drivers can implement @gem_prime_export and @gem_prime_import in terms of + * simpler APIs by using the helper functions @drm_gem_prime_export and + * @drm_gem_prime_import. These functions implement dma-buf support in terms of + * five lower-level driver callbacks: + * + * Export callbacks: + * + * - @gem_prime_pin (optional): prepare a GEM object for exporting + * + * - @gem_prime_get_sg_table: provide a scatter/gather table of pinned pages + * + * - @gem_prime_vmap: vmap a buffer exported by your driver + * + * - @gem_prime_vunmap: vunmap a buffer exported by your driver + * + * Import callback: + * + * - @gem_prime_import_sg_table (import): produce a GEM object from another + * driver's scatter/gather table + */ + +struct dma_buf *drm_gem_prime_export(struct drm_device *dev, + struct drm_gem_object *obj, int flags) +{ + if (dev->driver->gem_prime_pin) { + int ret = dev->driver->gem_prime_pin(obj); + if (ret) + return ERR_PTR(ret); + } + return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size, + 0600); +} +EXPORT_SYMBOL(drm_gem_prime_export); + int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd) @@ -117,6 +249,58 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, } EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
+struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev, + struct dma_buf *dma_buf) +{ + struct dma_buf_attachment *attach; + struct sg_table *sgt; + struct drm_gem_object *obj; + int ret; + + if (!dev->driver->gem_prime_import_sg_table) + return ERR_PTR(-EINVAL); + + if (dma_buf->ops == &drm_gem_prime_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_reference(obj); + dma_buf_put(dma_buf); + return obj; + } + } + + attach = dma_buf_attach(dma_buf, dev->dev); + if (IS_ERR(attach)) + return ERR_PTR(PTR_ERR(attach)); + + sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); + if (IS_ERR_OR_NULL(sgt)) { + ret = PTR_ERR(sgt); + goto fail_detach; + } + + obj = dev->driver->gem_prime_import_sg_table(dev, dma_buf->size, sgt); + if (IS_ERR(obj)) { + ret = PTR_ERR(obj); + goto fail_unmap; + } + + obj->import_attach = attach; + + return obj; + +fail_unmap: + dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL); +fail_detach: + dma_buf_detach(dma_buf, attach); + return ERR_PTR(ret); +} +EXPORT_SYMBOL(drm_gem_prime_import); + int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle) { diff --git a/include/drm/drmP.h b/include/drm/drmP.h index fad21c9..2d4ca6f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -919,6 +919,14 @@ struct drm_driver { /* import dmabuf -> GEM */ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev, struct dma_buf *dma_buf); + /* low-level interface used by drm_gem_prime_{import,export} */ + int (*gem_prime_pin)(struct drm_gem_object *obj); + struct sg_table *(*gem_prime_get_sg_table)(struct drm_gem_object *obj); + struct drm_gem_object *(*gem_prime_import_sg_table)( + struct drm_device *dev, size_t size, + struct sg_table *sgt); + void *(*gem_prime_vmap)(struct drm_gem_object *obj); + void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr);
/* vga arb irq handler */ void (*vgaarb_irq)(struct drm_device *dev, bool state); @@ -1540,9 +1548,13 @@ extern int drm_clients_info(struct seq_file *m, void* data); extern int drm_gem_name_info(struct seq_file *m, void *data);
+extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev, + struct drm_gem_object *obj, int flags); extern int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd); +extern struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev, + struct dma_buf *dma_buf); extern int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle);
On Tue, Jan 15, 2013 at 12:47:42PM -0800, Aaron Plattner wrote:
Instead of reimplementing all of the dma_buf functionality in every driver, create helpers drm_prime_import and drm_prime_export that implement them in terms of new, lower-level hook functions:
gem_prime_pin: callback when a buffer is created, used to pin buffers into GTT gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for export gem_prime_import_sg_table: convert an sg_table into a drm_gem_object gem_prime_vmap, gem_prime_vunmap: map and unmap an object
These hooks are optional; drivers can opt in by using drm_gem_prime_import and drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of struct drm_driver.
v2:
- Drop .begin_cpu_access. None of the drivers this code replaces implemented it. Having it here was a leftover from when I was trying to include i915 in this rework.
- Use mutex_lock instead of mutex_lock_interruptible, as these three drivers did. This patch series shouldn't change that behavior.
- Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table. Rename struct sg_table* variables to 'sgt' for clarity.
- Update drm.tmpl for these new hooks.
v3:
- Pass the vaddr down to the driver. This lets drivers that just call vunmap on the pointer avoid having to store the pointer in their GEM private structures.
- Move documentation into a /** DOC */ comment in drm_prime.c and include it in drm.tmpl with a !P line. I tried to use !F lines to include documentation of the individual functions from drmP.h, but the docproc / kernel-doc scripts barf on that file, so hopefully this is good enough for now.
- apply refcount fix from commit be8a42ae60addd8b6092535c11b42d099d6470ec ("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner aplattner@nvidia.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Airlie airlied@linux.ie
Two minor things, but since you're not touching drm/i915 I don't care that much ...
+static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
enum dma_data_direction dir)+{
- struct drm_gem_object *obj = attach->dmabuf->priv;
- struct sg_table *sgt;
- mutex_lock(&obj->dev->struct_mutex);
- sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
- if (!IS_ERR_OR_NULL(sgt))
dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);- mutex_unlock(&obj->dev->struct_mutex);
The locking here looks superflous and not required to protect anything drm_gem_map_dma_buf does. If drivers need this (and ttm based ones shouldn't really), they can grab it in their ->gem_prime_get_sg_table callback. So I think a follow-up patch to ditch this would be good.
- return sgt;
+}
+static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
struct sg_table *sgt, enum dma_data_direction dir)+{
- dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
- sg_free_table(sgt);
- kfree(sgt);
+}
+static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) +{
- struct drm_gem_object *obj = dma_buf->priv;
- if (obj->export_dma_buf == dma_buf) {
/* drop the reference on the export fd holds */obj->export_dma_buf = NULL;drm_gem_object_unreference_unlocked(obj);- }
+}
+static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) +{
- struct drm_gem_object *obj = dma_buf->priv;
- struct drm_device *dev = obj->dev;
- return dev->driver->gem_prime_vmap(obj);
+}
+static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) +{
- struct drm_gem_object *obj = dma_buf->priv;
- struct drm_device *dev = obj->dev;
- dev->driver->gem_prime_vunmap(obj, vaddr);
+}
Imo these two don't add value, simpler to add a typechecking drm_dmabuf_to_gem_obj static inline somewhere. But then you'd need to pass in the dmabuf fops struct from drivers and export the helpers. More flexible, but also a bit work to redo. Like I've said, I don't care too much ... -Daniel
+static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
unsigned long page_num)+{
- return NULL;
+}
+static void drm_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
unsigned long page_num, void *addr)+{
+} +static void *drm_gem_dmabuf_kmap(struct dma_buf *dma_buf,
unsigned long page_num)+{
- return NULL;
+}
+static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
unsigned long page_num, void *addr)+{
+}
+static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
struct vm_area_struct *vma)+{
- return -EINVAL;
+}
+static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
- .map_dma_buf = drm_gem_map_dma_buf,
- .unmap_dma_buf = drm_gem_unmap_dma_buf,
- .release = drm_gem_dmabuf_release,
- .kmap = drm_gem_dmabuf_kmap,
- .kmap_atomic = drm_gem_dmabuf_kmap_atomic,
- .kunmap = drm_gem_dmabuf_kunmap,
- .kunmap_atomic = drm_gem_dmabuf_kunmap_atomic,
- .mmap = drm_gem_dmabuf_mmap,
- .vmap = drm_gem_dmabuf_vmap,
- .vunmap = drm_gem_dmabuf_vunmap,
+};
+/**
- DOC: PRIME Helpers
- Drivers can implement @gem_prime_export and @gem_prime_import in terms of
- simpler APIs by using the helper functions @drm_gem_prime_export and
- @drm_gem_prime_import. These functions implement dma-buf support in terms of
- five lower-level driver callbacks:
- Export callbacks:
- @gem_prime_pin (optional): prepare a GEM object for exporting
- @gem_prime_get_sg_table: provide a scatter/gather table of pinned pages
- @gem_prime_vmap: vmap a buffer exported by your driver
- @gem_prime_vunmap: vunmap a buffer exported by your driver
- Import callback:
- @gem_prime_import_sg_table (import): produce a GEM object from another
- driver's scatter/gather table
- */
+struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags)+{
- if (dev->driver->gem_prime_pin) {
int ret = dev->driver->gem_prime_pin(obj);if (ret)return ERR_PTR(ret);- }
- return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
0600);+} +EXPORT_SYMBOL(drm_gem_prime_export);
int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd) @@ -117,6 +249,58 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, } EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
+struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
struct dma_buf *dma_buf)+{
- struct dma_buf_attachment *attach;
- struct sg_table *sgt;
- struct drm_gem_object *obj;
- int ret;
- if (!dev->driver->gem_prime_import_sg_table)
return ERR_PTR(-EINVAL);- if (dma_buf->ops == &drm_gem_prime_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_reference(obj);dma_buf_put(dma_buf);return obj;}- }
- attach = dma_buf_attach(dma_buf, dev->dev);
- if (IS_ERR(attach))
return ERR_PTR(PTR_ERR(attach));- sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
- if (IS_ERR_OR_NULL(sgt)) {
ret = PTR_ERR(sgt);goto fail_detach;- }
- obj = dev->driver->gem_prime_import_sg_table(dev, dma_buf->size, sgt);
- if (IS_ERR(obj)) {
ret = PTR_ERR(obj);goto fail_unmap;- }
- obj->import_attach = attach;
- return obj;
+fail_unmap:
- dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
+fail_detach:
- dma_buf_detach(dma_buf, attach);
- return ERR_PTR(ret);
+} +EXPORT_SYMBOL(drm_gem_prime_import);
int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle) { diff --git a/include/drm/drmP.h b/include/drm/drmP.h index fad21c9..2d4ca6f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -919,6 +919,14 @@ struct drm_driver { /* import dmabuf -> GEM */ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev, struct dma_buf *dma_buf);
/* low-level interface used by drm_gem_prime_{import,export} */
int (*gem_prime_pin)(struct drm_gem_object *obj);
struct sg_table *(*gem_prime_get_sg_table)(struct drm_gem_object *obj);
struct drm_gem_object *(*gem_prime_import_sg_table)(
struct drm_device *dev, size_t size,struct sg_table *sgt);void *(*gem_prime_vmap)(struct drm_gem_object *obj);
void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr);
/* vga arb irq handler */ void (*vgaarb_irq)(struct drm_device *dev, bool state);
@@ -1540,9 +1548,13 @@ extern int drm_clients_info(struct seq_file *m, void* data); extern int drm_gem_name_info(struct seq_file *m, void *data);
+extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags);extern int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd); +extern struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
struct dma_buf *dma_buf);extern int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle);
-- 1.8.1.1
On 01/16/2013 01:50 AM, Daniel Vetter wrote:
On Tue, Jan 15, 2013 at 12:47:42PM -0800, Aaron Plattner wrote:
Instead of reimplementing all of the dma_buf functionality in every driver, create helpers drm_prime_import and drm_prime_export that implement them in terms of new, lower-level hook functions:
gem_prime_pin: callback when a buffer is created, used to pin buffers into GTT gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for export gem_prime_import_sg_table: convert an sg_table into a drm_gem_object gem_prime_vmap, gem_prime_vunmap: map and unmap an object
These hooks are optional; drivers can opt in by using drm_gem_prime_import and drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of struct drm_driver.
v2:
- Drop .begin_cpu_access. None of the drivers this code replaces implemented it. Having it here was a leftover from when I was trying to include i915 in this rework.
- Use mutex_lock instead of mutex_lock_interruptible, as these three drivers did. This patch series shouldn't change that behavior.
- Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table. Rename struct sg_table* variables to 'sgt' for clarity.
- Update drm.tmpl for these new hooks.
v3:
- Pass the vaddr down to the driver. This lets drivers that just call vunmap on the pointer avoid having to store the pointer in their GEM private structures.
- Move documentation into a /** DOC */ comment in drm_prime.c and include it in drm.tmpl with a !P line. I tried to use !F lines to include documentation of the individual functions from drmP.h, but the docproc / kernel-doc scripts barf on that file, so hopefully this is good enough for now.
- apply refcount fix from commit be8a42ae60addd8b6092535c11b42d099d6470ec ("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner aplattner@nvidia.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Airlie airlied@linux.ie
Two minor things, but since you're not touching drm/i915 I don't care that much ...
+static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
enum dma_data_direction dir)+{
struct drm_gem_object *obj = attach->dmabuf->priv;struct sg_table *sgt;mutex_lock(&obj->dev->struct_mutex);sgt = obj->dev->driver->gem_prime_get_sg_table(obj);if (!IS_ERR_OR_NULL(sgt))dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);mutex_unlock(&obj->dev->struct_mutex);The locking here looks superflous and not required to protect anything drm_gem_map_dma_buf does. If drivers need this (and ttm based ones shouldn't really), they can grab it in their ->gem_prime_get_sg_table callback. So I think a follow-up patch to ditch this would be good.
If you look at the later patches, this was just moved from the drivers. I agree that evaluating whether or not it's actually necessary should be a separate follow-up.
return sgt;+}
+static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
struct sg_table *sgt, enum dma_data_direction dir)+{
dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);sg_free_table(sgt);kfree(sgt);+}
+static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) +{
struct drm_gem_object *obj = dma_buf->priv;if (obj->export_dma_buf == dma_buf) {/* drop the reference on the export fd holds */obj->export_dma_buf = NULL;drm_gem_object_unreference_unlocked(obj);}+}
+static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) +{
struct drm_gem_object *obj = dma_buf->priv;struct drm_device *dev = obj->dev;return dev->driver->gem_prime_vmap(obj);+}
+static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) +{
struct drm_gem_object *obj = dma_buf->priv;struct drm_device *dev = obj->dev;dev->driver->gem_prime_vunmap(obj, vaddr);+}
Imo these two don't add value, simpler to add a typechecking drm_dmabuf_to_gem_obj static inline somewhere. But then you'd need to pass in the dmabuf fops struct from drivers and export the helpers. More flexible, but also a bit work to redo. Like I've said, I don't care too much ...
I considered that, but it seems like the wrong abstraction. The advantage here is that drivers don't have to care even a little bit about the buffer sharing mechanism, which simplifies the drivers.
I.e., from patch #2,
diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c index 8bf695c..24e0aab 100644 --- a/drivers/gpu/drm/nouveau/nouveau_gem.c +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c @@ -24,8 +24,6 @@ * */
-#include <linux/dma-buf.h>
Can I consider this a Reviewed-by?
On Thu, Jan 17, 2013 at 12:36 AM, Aaron Plattner aplattner@nvidia.com wrote:
Can I consider this a Reviewed-by?
Essentially it was just a drive-by bikeshed ;-) I think it'd be good if Maarten takes a look at this and checks whether it complies with his massive prime/dma_buf rework to use fences and ticketing reservations ... -Daniel
Op 17-01-13 09:40, Daniel Vetter schreef:
On Thu, Jan 17, 2013 at 12:36 AM, Aaron Plattner aplattner@nvidia.com wrote:
Can I consider this a Reviewed-by?
Essentially it was just a drive-by bikeshed ;-) I think it'd be good if Maarten takes a look at this and checks whether it complies with his massive prime/dma_buf rework to use fences and ticketing reservations ... -Daniel
Looks ok to me, I just wish there was a unpin being called on dma-buf release or if dma_buf_export fails, instead of only implicitly during destruction. But that's something you couldn't have known, since it seems darktama still didn't accept my patch for that. :(
~Maarten
On Tue, Jan 15, 2013 at 12:47:42PM -0800, Aaron Plattner wrote:
Instead of reimplementing all of the dma_buf functionality in every driver, create helpers drm_prime_import and drm_prime_export that implement them in terms of new, lower-level hook functions:
gem_prime_pin: callback when a buffer is created, used to pin buffers into GTT gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for export gem_prime_import_sg_table: convert an sg_table into a drm_gem_object gem_prime_vmap, gem_prime_vunmap: map and unmap an object
These hooks are optional; drivers can opt in by using drm_gem_prime_import and drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of struct drm_driver.
v2:
- Drop .begin_cpu_access. None of the drivers this code replaces implemented it. Having it here was a leftover from when I was trying to include i915 in this rework.
- Use mutex_lock instead of mutex_lock_interruptible, as these three drivers did. This patch series shouldn't change that behavior.
- Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table. Rename struct sg_table* variables to 'sgt' for clarity.
- Update drm.tmpl for these new hooks.
v3:
- Pass the vaddr down to the driver. This lets drivers that just call vunmap on the pointer avoid having to store the pointer in their GEM private structures.
- Move documentation into a /** DOC */ comment in drm_prime.c and include it in drm.tmpl with a !P line. I tried to use !F lines to include documentation of the individual functions from drmP.h, but the docproc / kernel-doc scripts barf on that file, so hopefully this is good enough for now.
- apply refcount fix from commit be8a42ae60addd8b6092535c11b42d099d6470ec ("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner aplattner@nvidia.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Airlie airlied@linux.ie
Documentation/DocBook/drm.tmpl | 4 + drivers/gpu/drm/drm_prime.c | 186 ++++++++++++++++++++++++++++++++++++++++- include/drm/drmP.h | 12 +++ 3 files changed, 201 insertions(+), 1 deletion(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 4ee2304..ed40576 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -743,6 +743,10 @@ char *date;</synopsis> These two operations are mandatory for GEM drivers that support DRM PRIME. </para>
<sect4><title>DRM PRIME Helper Functions Reference</title>+!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
</sect4> </sect3> <sect3 id="drm-gem-objects-mapping"> <title>GEM Objects Mapping</title>diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7f12573..366910d 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -53,7 +53,8 @@
- Self-importing: if userspace is using PRIME as a replacement for flink
- then it will get a fd->handle request for a GEM object that it created.
- Drivers should detect this situation and return back the gem object
- from the dma-buf private.
- from the dma-buf private. Prime will do this automatically for drivers that
*/
- use the drm_gem_prime_{import,export} helpers.
struct drm_prime_member { @@ -62,6 +63,137 @@ struct drm_prime_member { uint32_t handle; };
+static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
enum dma_data_direction dir)+{
- struct drm_gem_object *obj = attach->dmabuf->priv;
- struct sg_table *sgt;
- mutex_lock(&obj->dev->struct_mutex);
- sgt = obj->dev->driver->gem_prime_get_sg_table(obj);
- if (!IS_ERR_OR_NULL(sgt))
dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);- mutex_unlock(&obj->dev->struct_mutex);
- return sgt;
+}
+static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
struct sg_table *sgt, enum dma_data_direction dir)+{
- dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);
- sg_free_table(sgt);
- kfree(sgt);
+}
+static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) +{
- struct drm_gem_object *obj = dma_buf->priv;
- if (obj->export_dma_buf == dma_buf) {
/* drop the reference on the export fd holds */obj->export_dma_buf = NULL;drm_gem_object_unreference_unlocked(obj);- }
+}
+static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) +{
- struct drm_gem_object *obj = dma_buf->priv;
- struct drm_device *dev = obj->dev;
- return dev->driver->gem_prime_vmap(obj);
+}
+static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) +{
- struct drm_gem_object *obj = dma_buf->priv;
- struct drm_device *dev = obj->dev;
- dev->driver->gem_prime_vunmap(obj, vaddr);
+}
+static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
unsigned long page_num)+{
- return NULL;
+}
+static void drm_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
unsigned long page_num, void *addr)+{
+} +static void *drm_gem_dmabuf_kmap(struct dma_buf *dma_buf,
unsigned long page_num)+{
- return NULL;
+}
+static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
unsigned long page_num, void *addr)+{
+}
+static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
struct vm_area_struct *vma)+{
- return -EINVAL;
+}
+static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
- .map_dma_buf = drm_gem_map_dma_buf,
- .unmap_dma_buf = drm_gem_unmap_dma_buf,
- .release = drm_gem_dmabuf_release,
- .kmap = drm_gem_dmabuf_kmap,
- .kmap_atomic = drm_gem_dmabuf_kmap_atomic,
- .kunmap = drm_gem_dmabuf_kunmap,
- .kunmap_atomic = drm_gem_dmabuf_kunmap_atomic,
- .mmap = drm_gem_dmabuf_mmap,
- .vmap = drm_gem_dmabuf_vmap,
- .vunmap = drm_gem_dmabuf_vunmap,
+};
+/**
- DOC: PRIME Helpers
- Drivers can implement @gem_prime_export and @gem_prime_import in terms of
- simpler APIs by using the helper functions @drm_gem_prime_export and
- @drm_gem_prime_import. These functions implement dma-buf support in terms of
- five lower-level driver callbacks:
- Export callbacks:
- @gem_prime_pin (optional): prepare a GEM object for exporting
- @gem_prime_get_sg_table: provide a scatter/gather table of pinned pages
- @gem_prime_vmap: vmap a buffer exported by your driver
- @gem_prime_vunmap: vunmap a buffer exported by your driver
- Import callback:
- @gem_prime_import_sg_table (import): produce a GEM object from another
- driver's scatter/gather table
- */
+struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags)+{
- if (dev->driver->gem_prime_pin) {
int ret = dev->driver->gem_prime_pin(obj);if (ret)return ERR_PTR(ret);- }
- return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
0600);+} +EXPORT_SYMBOL(drm_gem_prime_export);
int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd) @@ -117,6 +249,58 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, } EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
+struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
struct dma_buf *dma_buf)+{
- struct dma_buf_attachment *attach;
- struct sg_table *sgt;
- struct drm_gem_object *obj;
- int ret;
- if (!dev->driver->gem_prime_import_sg_table)
return ERR_PTR(-EINVAL);- if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {
This here breaks self-import checks since it smashes all buffers from all drivers using these helpers into one set. Which means e.g. nouveau will happily import a buffer from radoen as it's own. The only reason afaics that shit didn't completely hit the fan is that i915 still has it's own buffer ops, and everyone seems to only care about sharing with i915.
So after a few months of subconscious pondering (well, just re-read the code to review Dave's patch) I now finally know why I totally didn't like that we move the vtable into the helpers ;-)
I think someone needs to fix this or we need to revert this patch here.
Cheers, Daniel
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_reference(obj);dma_buf_put(dma_buf);return obj;}- }
- attach = dma_buf_attach(dma_buf, dev->dev);
- if (IS_ERR(attach))
return ERR_PTR(PTR_ERR(attach));- sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
- if (IS_ERR_OR_NULL(sgt)) {
ret = PTR_ERR(sgt);goto fail_detach;- }
- obj = dev->driver->gem_prime_import_sg_table(dev, dma_buf->size, sgt);
- if (IS_ERR(obj)) {
ret = PTR_ERR(obj);goto fail_unmap;- }
- obj->import_attach = attach;
- return obj;
+fail_unmap:
- dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
+fail_detach:
- dma_buf_detach(dma_buf, attach);
- return ERR_PTR(ret);
+} +EXPORT_SYMBOL(drm_gem_prime_import);
int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle) { diff --git a/include/drm/drmP.h b/include/drm/drmP.h index fad21c9..2d4ca6f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -919,6 +919,14 @@ struct drm_driver { /* import dmabuf -> GEM */ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev, struct dma_buf *dma_buf);
/* low-level interface used by drm_gem_prime_{import,export} */
int (*gem_prime_pin)(struct drm_gem_object *obj);
struct sg_table *(*gem_prime_get_sg_table)(struct drm_gem_object *obj);
struct drm_gem_object *(*gem_prime_import_sg_table)(
struct drm_device *dev, size_t size,struct sg_table *sgt);void *(*gem_prime_vmap)(struct drm_gem_object *obj);
void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr);
/* vga arb irq handler */ void (*vgaarb_irq)(struct drm_device *dev, bool state);
@@ -1540,9 +1548,13 @@ extern int drm_clients_info(struct seq_file *m, void* data); extern int drm_gem_name_info(struct seq_file *m, void *data);
+extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags);extern int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd); +extern struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
struct dma_buf *dma_buf);extern int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle);
-- 1.8.1.1
On 04/12/13 07:58, Daniel Vetter wrote:
On Tue, Jan 15, 2013 at 12:47:42PM -0800, Aaron Plattner wrote:
Instead of reimplementing all of the dma_buf functionality in every driver, create helpers drm_prime_import and drm_prime_export that implement them in terms of new, lower-level hook functions:
gem_prime_pin: callback when a buffer is created, used to pin buffers into GTT gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for export gem_prime_import_sg_table: convert an sg_table into a drm_gem_object gem_prime_vmap, gem_prime_vunmap: map and unmap an object
These hooks are optional; drivers can opt in by using drm_gem_prime_import and drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of struct drm_driver.
v2:
- Drop .begin_cpu_access. None of the drivers this code replaces implemented it. Having it here was a leftover from when I was trying to include i915 in this rework.
- Use mutex_lock instead of mutex_lock_interruptible, as these three drivers did. This patch series shouldn't change that behavior.
- Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table. Rename struct sg_table* variables to 'sgt' for clarity.
- Update drm.tmpl for these new hooks.
v3:
- Pass the vaddr down to the driver. This lets drivers that just call vunmap on the pointer avoid having to store the pointer in their GEM private structures.
- Move documentation into a /** DOC */ comment in drm_prime.c and include it in drm.tmpl with a !P line. I tried to use !F lines to include documentation of the individual functions from drmP.h, but the docproc / kernel-doc scripts barf on that file, so hopefully this is good enough for now.
- apply refcount fix from commit be8a42ae60addd8b6092535c11b42d099d6470ec ("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner aplattner@nvidia.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Airlie airlied@linux.ie
Documentation/DocBook/drm.tmpl | 4 + drivers/gpu/drm/drm_prime.c | 186 ++++++++++++++++++++++++++++++++++++++++- include/drm/drmP.h | 12 +++ 3 files changed, 201 insertions(+), 1 deletion(-)
diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl index 4ee2304..ed40576 100644 --- a/Documentation/DocBook/drm.tmpl +++ b/Documentation/DocBook/drm.tmpl @@ -743,6 +743,10 @@ char *date;</synopsis> These two operations are mandatory for GEM drivers that support DRM PRIME. </para>
<sect4><title>DRM PRIME Helper Functions Reference</title>+!Pdrivers/gpu/drm/drm_prime.c PRIME Helpers
</sect4> </sect3> <sect3 id="drm-gem-objects-mapping"> <title>GEM Objects Mapping</title>diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7f12573..366910d 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -53,7 +53,8 @@
- Self-importing: if userspace is using PRIME as a replacement for flink
- then it will get a fd->handle request for a GEM object that it created.
- Drivers should detect this situation and return back the gem object
- from the dma-buf private.
- from the dma-buf private. Prime will do this automatically for drivers that
- use the drm_gem_prime_{import,export} helpers.
*/
struct drm_prime_member {
@@ -62,6 +63,137 @@ struct drm_prime_member { uint32_t handle; };
+static struct sg_table *drm_gem_map_dma_buf(struct dma_buf_attachment *attach,
enum dma_data_direction dir)+{
struct drm_gem_object *obj = attach->dmabuf->priv;struct sg_table *sgt;mutex_lock(&obj->dev->struct_mutex);sgt = obj->dev->driver->gem_prime_get_sg_table(obj);if (!IS_ERR_OR_NULL(sgt))dma_map_sg(attach->dev, sgt->sgl, sgt->nents, dir);mutex_unlock(&obj->dev->struct_mutex);return sgt;+}
+static void drm_gem_unmap_dma_buf(struct dma_buf_attachment *attach,
struct sg_table *sgt, enum dma_data_direction dir)+{
dma_unmap_sg(attach->dev, sgt->sgl, sgt->nents, dir);sg_free_table(sgt);kfree(sgt);+}
+static void drm_gem_dmabuf_release(struct dma_buf *dma_buf) +{
struct drm_gem_object *obj = dma_buf->priv;if (obj->export_dma_buf == dma_buf) {/* drop the reference on the export fd holds */obj->export_dma_buf = NULL;drm_gem_object_unreference_unlocked(obj);}+}
+static void *drm_gem_dmabuf_vmap(struct dma_buf *dma_buf) +{
struct drm_gem_object *obj = dma_buf->priv;struct drm_device *dev = obj->dev;return dev->driver->gem_prime_vmap(obj);+}
+static void drm_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr) +{
struct drm_gem_object *obj = dma_buf->priv;struct drm_device *dev = obj->dev;dev->driver->gem_prime_vunmap(obj, vaddr);+}
+static void *drm_gem_dmabuf_kmap_atomic(struct dma_buf *dma_buf,
unsigned long page_num)+{
return NULL;+}
+static void drm_gem_dmabuf_kunmap_atomic(struct dma_buf *dma_buf,
unsigned long page_num, void *addr)+{
+} +static void *drm_gem_dmabuf_kmap(struct dma_buf *dma_buf,
unsigned long page_num)+{
return NULL;+}
+static void drm_gem_dmabuf_kunmap(struct dma_buf *dma_buf,
unsigned long page_num, void *addr)+{
+}
+static int drm_gem_dmabuf_mmap(struct dma_buf *dma_buf,
struct vm_area_struct *vma)+{
return -EINVAL;+}
+static const struct dma_buf_ops drm_gem_prime_dmabuf_ops = {
.map_dma_buf = drm_gem_map_dma_buf,.unmap_dma_buf = drm_gem_unmap_dma_buf,.release = drm_gem_dmabuf_release,.kmap = drm_gem_dmabuf_kmap,.kmap_atomic = drm_gem_dmabuf_kmap_atomic,.kunmap = drm_gem_dmabuf_kunmap,.kunmap_atomic = drm_gem_dmabuf_kunmap_atomic,.mmap = drm_gem_dmabuf_mmap,.vmap = drm_gem_dmabuf_vmap,.vunmap = drm_gem_dmabuf_vunmap,+};
+/**
- DOC: PRIME Helpers
- Drivers can implement @gem_prime_export and @gem_prime_import in terms of
- simpler APIs by using the helper functions @drm_gem_prime_export and
- @drm_gem_prime_import. These functions implement dma-buf support in terms of
- five lower-level driver callbacks:
- Export callbacks:
- @gem_prime_pin (optional): prepare a GEM object for exporting
- @gem_prime_get_sg_table: provide a scatter/gather table of pinned pages
- @gem_prime_vmap: vmap a buffer exported by your driver
- @gem_prime_vunmap: vunmap a buffer exported by your driver
- Import callback:
- @gem_prime_import_sg_table (import): produce a GEM object from another
- driver's scatter/gather table
- */
+struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags)+{
if (dev->driver->gem_prime_pin) {int ret = dev->driver->gem_prime_pin(obj);if (ret)return ERR_PTR(ret);}return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,0600);+} +EXPORT_SYMBOL(drm_gem_prime_export);
- int drm_gem_prime_handle_to_fd(struct drm_device *dev, struct drm_file *file_priv, uint32_t handle, uint32_t flags, int *prime_fd)
@@ -117,6 +249,58 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, } EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
+struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
struct dma_buf *dma_buf)+{
struct dma_buf_attachment *attach;struct sg_table *sgt;struct drm_gem_object *obj;int ret;if (!dev->driver->gem_prime_import_sg_table)return ERR_PTR(-EINVAL);if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {This here breaks self-import checks since it smashes all buffers from all drivers using these helpers into one set. Which means e.g. nouveau will happily import a buffer from radoen as it's own. The only reason afaics that shit didn't completely hit the fan is that i915 still has it's own buffer ops, and everyone seems to only care about sharing with i915.
Doesn't the (obj->dev == dev) check below guard against that?
-- Aaron
So after a few months of subconscious pondering (well, just re-read the code to review Dave's patch) I now finally know why I totally didn't like that we move the vtable into the helpers ;-)
I think someone needs to fix this or we need to revert this patch here.
Cheers, Daniel
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_reference(obj);dma_buf_put(dma_buf);return obj;}}attach = dma_buf_attach(dma_buf, dev->dev);if (IS_ERR(attach))return ERR_PTR(PTR_ERR(attach));sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);if (IS_ERR_OR_NULL(sgt)) {ret = PTR_ERR(sgt);goto fail_detach;}obj = dev->driver->gem_prime_import_sg_table(dev, dma_buf->size, sgt);if (IS_ERR(obj)) {ret = PTR_ERR(obj);goto fail_unmap;}obj->import_attach = attach;return obj;+fail_unmap:
dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);+fail_detach:
dma_buf_detach(dma_buf, attach);return ERR_PTR(ret);+} +EXPORT_SYMBOL(drm_gem_prime_import);
- int drm_gem_prime_fd_to_handle(struct drm_device *dev, struct drm_file *file_priv, int prime_fd, uint32_t *handle) {
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index fad21c9..2d4ca6f 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -919,6 +919,14 @@ struct drm_driver { /* import dmabuf -> GEM */ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev, struct dma_buf *dma_buf);
/* low-level interface used by drm_gem_prime_{import,export} */int (*gem_prime_pin)(struct drm_gem_object *obj);struct sg_table *(*gem_prime_get_sg_table)(struct drm_gem_object *obj);struct drm_gem_object *(*gem_prime_import_sg_table)(struct drm_device *dev, size_t size,struct sg_table *sgt);void *(*gem_prime_vmap)(struct drm_gem_object *obj);void (*gem_prime_vunmap)(struct drm_gem_object *obj, void *vaddr); /* vga arb irq handler */ void (*vgaarb_irq)(struct drm_device *dev, bool state);@@ -1540,9 +1548,13 @@ extern int drm_clients_info(struct seq_file *m, void* data); extern int drm_gem_name_info(struct seq_file *m, void *data);
+extern struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags);+extern struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
struct dma_buf *dma_buf);-- 1.8.1.1
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Fri, Apr 12, 2013 at 5:13 PM, Aaron Plattner aplattner@nvidia.com wrote:
@@ -117,6 +249,58 @@ int drm_gem_prime_handle_to_fd(struct drm_device *dev, } EXPORT_SYMBOL(drm_gem_prime_handle_to_fd);
+struct drm_gem_object *drm_gem_prime_import(struct drm_device *dev,
struct dma_buf *dma_buf)+{
struct dma_buf_attachment *attach;struct sg_table *sgt;struct drm_gem_object *obj;int ret;if (!dev->driver->gem_prime_import_sg_table)return ERR_PTR(-EINVAL);if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {This here breaks self-import checks since it smashes all buffers from all drivers using these helpers into one set. Which means e.g. nouveau will happily import a buffer from radoen as it's own. The only reason afaics that shit didn't completely hit the fan is that i915 still has it's own buffer ops, and everyone seems to only care about sharing with i915.
Doesn't the (obj->dev == dev) check below guard against that?
Oh dear did I just make a big idiot out of myself ;-) Sorry for the fuss, you're right. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Aaron,
A bit late, but here's a small question.
On Tuesday 15 January 2013 12:47:42 Aaron Plattner wrote:
Instead of reimplementing all of the dma_buf functionality in every driver, create helpers drm_prime_import and drm_prime_export that implement them in terms of new, lower-level hook functions:
gem_prime_pin: callback when a buffer is created, used to pin buffers into GTT gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for export gem_prime_import_sg_table: convert an sg_table into a drm_gem_object gem_prime_vmap, gem_prime_vunmap: map and unmap an object
These hooks are optional; drivers can opt in by using drm_gem_prime_import and drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of struct drm_driver.
v2:
- Drop .begin_cpu_access. None of the drivers this code replaces
implemented it. Having it here was a leftover from when I was trying to include i915 in this rework.
- Use mutex_lock instead of mutex_lock_interruptible, as these three drivers
did. This patch series shouldn't change that behavior.
- Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table. Rename struct sg_table* variables to 'sgt' for clarity.
- Update drm.tmpl for these new hooks.
v3:
- Pass the vaddr down to the driver. This lets drivers that just call
vunmap on the pointer avoid having to store the pointer in their GEM private structures. - Move documentation into a /** DOC */ comment in drm_prime.c and include it in drm.tmpl with a !P line. I tried to use !F lines to include documentation of the individual functions from drmP.h, but the docproc / kernel-doc scripts barf on that file, so hopefully this is good enough for now.
- apply refcount fix from commit be8a42ae60addd8b6092535c11b42d099d6470ec ("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner aplattner@nvidia.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Airlie airlied@linux.ie
Documentation/DocBook/drm.tmpl | 4 + drivers/gpu/drm/drm_prime.c | 186 +++++++++++++++++++++++++++++++++++++- include/drm/drmP.h | 12 +++ 3 files changed, 201 insertions(+), 1 deletion(-)
[snip]
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7f12573..366910d 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c
[snip]
+/**
- DOC: PRIME Helpers
- Drivers can implement @gem_prime_export and @gem_prime_import in terms
of + * simpler APIs by using the helper functions @drm_gem_prime_export and
- @drm_gem_prime_import. These functions implement dma-buf support in
terms of + * five lower-level driver callbacks:
- Export callbacks:
- @gem_prime_pin (optional): prepare a GEM object for exporting
- @gem_prime_get_sg_table: provide a scatter/gather table of pinned
pages + *
- @gem_prime_vmap: vmap a buffer exported by your driver
- @gem_prime_vunmap: vunmap a buffer exported by your driver
- Import callback:
- @gem_prime_import_sg_table (import): produce a GEM object from
another + * driver's scatter/gather table
- */
+struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags)+{
- if (dev->driver->gem_prime_pin) {
int ret = dev->driver->gem_prime_pin(obj);if (ret)return ERR_PTR(ret);- }
- return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
0600);
Why do you use 0600 instead of the flags passed by the caller ?
+} +EXPORT_SYMBOL(drm_gem_prime_export);
On 06/18/2013 04:08 PM, Laurent Pinchart wrote:
Hi Aaron,
A bit late, but here's a small question.
On Tuesday 15 January 2013 12:47:42 Aaron Plattner wrote:
Instead of reimplementing all of the dma_buf functionality in every driver, create helpers drm_prime_import and drm_prime_export that implement them in terms of new, lower-level hook functions:
gem_prime_pin: callback when a buffer is created, used to pin buffers into GTT gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for export gem_prime_import_sg_table: convert an sg_table into a drm_gem_object gem_prime_vmap, gem_prime_vunmap: map and unmap an object
These hooks are optional; drivers can opt in by using drm_gem_prime_import and drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of struct drm_driver.
v2:
- Drop .begin_cpu_access. None of the drivers this code replaces
implemented it. Having it here was a leftover from when I was trying to include i915 in this rework.
- Use mutex_lock instead of mutex_lock_interruptible, as these three drivers
did. This patch series shouldn't change that behavior.
- Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table. Rename struct sg_table* variables to 'sgt' for clarity.
- Update drm.tmpl for these new hooks.
v3:
- Pass the vaddr down to the driver. This lets drivers that just call
vunmap on the pointer avoid having to store the pointer in their GEM private structures. - Move documentation into a /** DOC */ comment in drm_prime.c and include it in drm.tmpl with a !P line. I tried to use !F lines to include documentation of the individual functions from drmP.h, but the docproc / kernel-doc scripts barf on that file, so hopefully this is good enough for now.
- apply refcount fix from commit be8a42ae60addd8b6092535c11b42d099d6470ec ("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner aplattner@nvidia.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Airlie airlied@linux.ie
Documentation/DocBook/drm.tmpl | 4 + drivers/gpu/drm/drm_prime.c | 186 +++++++++++++++++++++++++++++++++++++- include/drm/drmP.h | 12 +++ 3 files changed, 201 insertions(+), 1 deletion(-)
[snip]
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7f12573..366910d 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c
[snip]
+/**
- DOC: PRIME Helpers
- Drivers can implement @gem_prime_export and @gem_prime_import in terms
of + * simpler APIs by using the helper functions @drm_gem_prime_export and
- @drm_gem_prime_import. These functions implement dma-buf support in
terms of + * five lower-level driver callbacks:
- Export callbacks:
- @gem_prime_pin (optional): prepare a GEM object for exporting
- @gem_prime_get_sg_table: provide a scatter/gather table of pinned
pages + *
- @gem_prime_vmap: vmap a buffer exported by your driver
- @gem_prime_vunmap: vunmap a buffer exported by your driver
- Import callback:
- @gem_prime_import_sg_table (import): produce a GEM object from
another + * driver's scatter/gather table
- */
+struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags)+{
- if (dev->driver->gem_prime_pin) {
int ret = dev->driver->gem_prime_pin(obj);if (ret)return ERR_PTR(ret);- }
- return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
0600);Why do you use 0600 instead of the flags passed by the caller ?
Because I copied & pasted it from i915_gem_prime_export prior to commit 5b42427fc38ecb9056c4e64deaff36d6d6ba1b67 and didn't notice until you pointed it out just now.
+} +EXPORT_SYMBOL(drm_gem_prime_export);
Hi Aaron,
On Tuesday 18 June 2013 16:28:15 Aaron Plattner wrote:
On 06/18/2013 04:08 PM, Laurent Pinchart wrote:
Hi Aaron,
A bit late, but here's a small question.
On Tuesday 15 January 2013 12:47:42 Aaron Plattner wrote:
Instead of reimplementing all of the dma_buf functionality in every driver, create helpers drm_prime_import and drm_prime_export that implement them in terms of new, lower-level hook functions: gem_prime_pin: callback when a buffer is created, used to pin buffers into GTT gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for export gem_prime_import_sg_table: convert an sg_table into a drm_gem_object gem_prime_vmap, gem_prime_vunmap: map and unmap an object
These hooks are optional; drivers can opt in by using drm_gem_prime_import and drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of struct drm_driver.
v2:
- Drop .begin_cpu_access. None of the drivers this code replaces
implemented it. Having it here was a leftover from when I was trying to include i915 in this rework.
- Use mutex_lock instead of mutex_lock_interruptible, as these three
drivers did. This patch series shouldn't change that behavior.
Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table.
Rename struct sg_table* variables to 'sgt' for clarity.
Update drm.tmpl for these new hooks.
v3:
- Pass the vaddr down to the driver. This lets drivers that just call
vunmap on the pointer avoid having to store the pointer in their GEM private structures. - Move documentation into a /** DOC */ comment in drm_prime.c and include it in drm.tmpl with a !P line. I tried to use !F lines to include documentation of the individual functions from drmP.h, but the docproc / kernel-doc scripts barf on that file, so hopefully this is good enough for now.
apply refcount fix from commit be8a42ae60addd8b6092535c11b42d099d6470ec
("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner aplattner@nvidia.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Airlie airlied@linux.ie
Documentation/DocBook/drm.tmpl | 4 + drivers/gpu/drm/drm_prime.c | 186 +++++++++++++++++++++++++++++++++++++- include/drm/drmP.h | 12 +++ 3 files changed, 201 insertions(+), 1 deletion(-)
[snip]
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7f12573..366910d 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c
[snip]
+/**
- DOC: PRIME Helpers
- Drivers can implement @gem_prime_export and @gem_prime_import in
terms of + * simpler APIs by using the helper functions @drm_gem_prime_export and
- @drm_gem_prime_import. These functions implement dma-buf support in
terms of + * five lower-level driver callbacks:
- Export callbacks:
- @gem_prime_pin (optional): prepare a GEM object for exporting
- @gem_prime_get_sg_table: provide a scatter/gather table of pinned
pages + *
- @gem_prime_vmap: vmap a buffer exported by your driver
- @gem_prime_vunmap: vunmap a buffer exported by your driver
- Import callback:
- @gem_prime_import_sg_table (import): produce a GEM object from
another + * driver's scatter/gather table
- */
+struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags)+{
- if (dev->driver->gem_prime_pin) {
int ret = dev->driver->gem_prime_pin(obj);if (ret)return ERR_PTR(ret);- }
- return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
0600);Why do you use 0600 instead of the flags passed by the caller ?
Because I copied & pasted it from i915_gem_prime_export prior to commit 5b42427fc38ecb9056c4e64deaff36d6d6ba1b67 and didn't notice until you pointed it out just now.
That's a pretty valid reason I guess :-) Should I submit a patch or are you already working on one ?
+} +EXPORT_SYMBOL(drm_gem_prime_export);
On 06/18/2013 04:37 PM, Laurent Pinchart wrote:
Hi Aaron,
On Tuesday 18 June 2013 16:28:15 Aaron Plattner wrote:
On 06/18/2013 04:08 PM, Laurent Pinchart wrote:
Hi Aaron,
A bit late, but here's a small question.
On Tuesday 15 January 2013 12:47:42 Aaron Plattner wrote:
Instead of reimplementing all of the dma_buf functionality in every driver, create helpers drm_prime_import and drm_prime_export that implement them in terms of new, lower-level hook functions: gem_prime_pin: callback when a buffer is created, used to pin buffers into GTT gem_prime_get_sg_table: convert a drm_gem_object to an sg_table for export gem_prime_import_sg_table: convert an sg_table into a drm_gem_object gem_prime_vmap, gem_prime_vunmap: map and unmap an object
These hooks are optional; drivers can opt in by using drm_gem_prime_import and drm_gem_prime_export as the .gem_prime_import and .gem_prime_export fields of struct drm_driver.
v2:
- Drop .begin_cpu_access. None of the drivers this code replaces
implemented it. Having it here was a leftover from when I was trying to include i915 in this rework.
- Use mutex_lock instead of mutex_lock_interruptible, as these three
drivers did. This patch series shouldn't change that behavior.
Rename helpers to gem_prime_get_sg_table and gem_prime_import_sg_table.
Rename struct sg_table* variables to 'sgt' for clarity.
Update drm.tmpl for these new hooks.
v3:
- Pass the vaddr down to the driver. This lets drivers that just call
vunmap on the pointer avoid having to store the pointer in their GEM private structures. - Move documentation into a /** DOC */ comment in drm_prime.c and include it in drm.tmpl with a !P line. I tried to use !F lines to include documentation of the individual functions from drmP.h, but the docproc / kernel-doc scripts barf on that file, so hopefully this is good enough for now.
apply refcount fix from commit be8a42ae60addd8b6092535c11b42d099d6470ec
("drm/prime: drop reference on imported dma-buf come from gem")
Signed-off-by: Aaron Plattner aplattner@nvidia.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Airlie airlied@linux.ie
Documentation/DocBook/drm.tmpl | 4 + drivers/gpu/drm/drm_prime.c | 186 +++++++++++++++++++++++++++++++++++++- include/drm/drmP.h | 12 +++ 3 files changed, 201 insertions(+), 1 deletion(-)
[snip]
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 7f12573..366910d 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c
[snip]
+/**
- DOC: PRIME Helpers
- Drivers can implement @gem_prime_export and @gem_prime_import in
terms of + * simpler APIs by using the helper functions @drm_gem_prime_export and
- @drm_gem_prime_import. These functions implement dma-buf support in
terms of + * five lower-level driver callbacks:
- Export callbacks:
- @gem_prime_pin (optional): prepare a GEM object for exporting
- @gem_prime_get_sg_table: provide a scatter/gather table of pinned
pages + *
- @gem_prime_vmap: vmap a buffer exported by your driver
- @gem_prime_vunmap: vunmap a buffer exported by your driver
- Import callback:
- @gem_prime_import_sg_table (import): produce a GEM object from
another + * driver's scatter/gather table
- */
+struct dma_buf *drm_gem_prime_export(struct drm_device *dev,
struct drm_gem_object *obj, int flags)+{
- if (dev->driver->gem_prime_pin) {
int ret = dev->driver->gem_prime_pin(obj);if (ret)return ERR_PTR(ret);- }
- return dma_buf_export(obj, &drm_gem_prime_dmabuf_ops, obj->size,
0600);Why do you use 0600 instead of the flags passed by the caller ?
Because I copied & pasted it from i915_gem_prime_export prior to commit 5b42427fc38ecb9056c4e64deaff36d6d6ba1b67 and didn't notice until you pointed it out just now.
That's a pretty valid reason I guess :-) Should I submit a patch or are you already working on one ?
:) Sorry!
I'm not working on this right now, but I can put it on my queue if you like. You'll probably be able to fix and test it sooner than I will, though.
+} +EXPORT_SYMBOL(drm_gem_prime_export);
Simplify the Nouveau prime implementation by using the default behavior provided by drm_gem_prime_import and drm_gem_prime_export.
v2: Rename functions to nouveau_gem_prime_get_sg_table and nouveau_gem_prime_import_sg_table.
Signed-off-by: Aaron Plattner aplattner@nvidia.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Airlie airlied@linux.ie --- drivers/gpu/drm/nouveau/nouveau_bo.h | 1 - drivers/gpu/drm/nouveau/nouveau_drm.c | 9 +- drivers/gpu/drm/nouveau/nouveau_gem.c | 2 - drivers/gpu/drm/nouveau/nouveau_gem.h | 10 +- drivers/gpu/drm/nouveau/nouveau_prime.c | 173 ++++---------------------------- 5 files changed, 34 insertions(+), 161 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.h b/drivers/gpu/drm/nouveau/nouveau_bo.h index 25ca379..836677a 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.h +++ b/drivers/gpu/drm/nouveau/nouveau_bo.h @@ -31,7 +31,6 @@ struct nouveau_bo { int pin_refcnt;
struct ttm_bo_kmap_obj dma_buf_vmap; - int vmapping_count; };
static inline struct nouveau_bo * diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 180a45e..5de1f9a 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -645,8 +645,13 @@ driver = {
.prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, - .gem_prime_export = nouveau_gem_prime_export, - .gem_prime_import = nouveau_gem_prime_import, + .gem_prime_export = drm_gem_prime_export, + .gem_prime_import = drm_gem_prime_import, + .gem_prime_pin = nouveau_gem_prime_pin, + .gem_prime_get_sg_table = nouveau_gem_prime_get_sg_table, + .gem_prime_import_sg_table = nouveau_gem_prime_import_sg_table, + .gem_prime_vmap = nouveau_gem_prime_vmap, + .gem_prime_vunmap = nouveau_gem_prime_vunmap,
.gem_init_object = nouveau_gem_object_new, .gem_free_object = nouveau_gem_object_del, diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c index 8bf695c..24e0aab 100644 --- a/drivers/gpu/drm/nouveau/nouveau_gem.c +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c @@ -24,8 +24,6 @@ * */
-#include <linux/dma-buf.h> - #include <subdev/fb.h>
#include "nouveau_drm.h" diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.h b/drivers/gpu/drm/nouveau/nouveau_gem.h index 5c10492..8d7a3f0 100644 --- a/drivers/gpu/drm/nouveau/nouveau_gem.h +++ b/drivers/gpu/drm/nouveau/nouveau_gem.h @@ -35,9 +35,11 @@ extern int nouveau_gem_ioctl_cpu_fini(struct drm_device *, void *, extern int nouveau_gem_ioctl_info(struct drm_device *, void *, struct drm_file *);
-extern struct dma_buf *nouveau_gem_prime_export(struct drm_device *dev, - struct drm_gem_object *obj, int flags); -extern struct drm_gem_object *nouveau_gem_prime_import(struct drm_device *dev, - struct dma_buf *dma_buf); +extern int nouveau_gem_prime_pin(struct drm_gem_object *); +extern struct sg_table *nouveau_gem_prime_get_sg_table(struct drm_gem_object *); +extern struct drm_gem_object *nouveau_gem_prime_import_sg_table( + struct drm_device *, size_t size, struct sg_table *); +extern void *nouveau_gem_prime_vmap(struct drm_gem_object *); +extern void nouveau_gem_prime_vunmap(struct drm_gem_object *, void *);
#endif diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c index b8e05ae..f53e108 100644 --- a/drivers/gpu/drm/nouveau/nouveau_prime.c +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c @@ -22,126 +22,42 @@ * Authors: Dave Airlie */
-#include <linux/dma-buf.h> - #include <drm/drmP.h>
#include "nouveau_drm.h" #include "nouveau_gem.h"
-static struct sg_table *nouveau_gem_map_dma_buf(struct dma_buf_attachment *attachment, - enum dma_data_direction dir) +struct sg_table *nouveau_gem_prime_get_sg_table(struct drm_gem_object *obj) { - struct nouveau_bo *nvbo = attachment->dmabuf->priv; - struct drm_device *dev = nvbo->gem->dev; + struct nouveau_bo *nvbo = nouveau_gem_object(obj); int npages = nvbo->bo.num_pages; - struct sg_table *sg; - int nents; - - mutex_lock(&dev->struct_mutex); - sg = drm_prime_pages_to_sg(nvbo->bo.ttm->pages, npages); - nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir); - mutex_unlock(&dev->struct_mutex); - return sg; -} - -static void nouveau_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, - struct sg_table *sg, enum dma_data_direction dir) -{ - dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); - sg_free_table(sg); - kfree(sg); -} - -static void nouveau_gem_dmabuf_release(struct dma_buf *dma_buf) -{ - struct nouveau_bo *nvbo = dma_buf->priv; - - if (nvbo->gem->export_dma_buf == dma_buf) { - nvbo->gem->export_dma_buf = NULL; - drm_gem_object_unreference_unlocked(nvbo->gem); - } -} - -static void *nouveau_gem_kmap_atomic(struct dma_buf *dma_buf, unsigned long page_num) -{ - return NULL; -} - -static void nouveau_gem_kunmap_atomic(struct dma_buf *dma_buf, unsigned long page_num, void *addr) -{
-} -static void *nouveau_gem_kmap(struct dma_buf *dma_buf, unsigned long page_num) -{ - return NULL; + return drm_prime_pages_to_sg(nvbo->bo.ttm->pages, npages); }
-static void nouveau_gem_kunmap(struct dma_buf *dma_buf, unsigned long page_num, void *addr) +void *nouveau_gem_prime_vmap(struct drm_gem_object *obj) { - -} - -static int nouveau_gem_prime_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma) -{ - return -EINVAL; -} - -static void *nouveau_gem_prime_vmap(struct dma_buf *dma_buf) -{ - struct nouveau_bo *nvbo = dma_buf->priv; - struct drm_device *dev = nvbo->gem->dev; + struct nouveau_bo *nvbo = nouveau_gem_object(obj); int ret;
- mutex_lock(&dev->struct_mutex); - if (nvbo->vmapping_count) { - nvbo->vmapping_count++; - goto out_unlock; - } - ret = ttm_bo_kmap(&nvbo->bo, 0, nvbo->bo.num_pages, &nvbo->dma_buf_vmap); - if (ret) { - mutex_unlock(&dev->struct_mutex); + if (ret) return ERR_PTR(ret); - } - nvbo->vmapping_count = 1; -out_unlock: - mutex_unlock(&dev->struct_mutex); + return nvbo->dma_buf_vmap.virtual; }
-static void nouveau_gem_prime_vunmap(struct dma_buf *dma_buf, void *vaddr) +void nouveau_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr) { - struct nouveau_bo *nvbo = dma_buf->priv; - struct drm_device *dev = nvbo->gem->dev; + struct nouveau_bo *nvbo = nouveau_gem_object(obj);
- mutex_lock(&dev->struct_mutex); - nvbo->vmapping_count--; - if (nvbo->vmapping_count == 0) { - ttm_bo_kunmap(&nvbo->dma_buf_vmap); - } - mutex_unlock(&dev->struct_mutex); + ttm_bo_kunmap(&nvbo->dma_buf_vmap); }
-static const struct dma_buf_ops nouveau_dmabuf_ops = { - .map_dma_buf = nouveau_gem_map_dma_buf, - .unmap_dma_buf = nouveau_gem_unmap_dma_buf, - .release = nouveau_gem_dmabuf_release, - .kmap = nouveau_gem_kmap, - .kmap_atomic = nouveau_gem_kmap_atomic, - .kunmap = nouveau_gem_kunmap, - .kunmap_atomic = nouveau_gem_kunmap_atomic, - .mmap = nouveau_gem_prime_mmap, - .vmap = nouveau_gem_prime_vmap, - .vunmap = nouveau_gem_prime_vunmap, -}; - -static int -nouveau_prime_new(struct drm_device *dev, - size_t size, - struct sg_table *sg, - struct nouveau_bo **pnvbo) +struct drm_gem_object *nouveau_gem_prime_import_sg_table(struct drm_device *dev, + size_t size, + struct sg_table *sg) { struct nouveau_bo *nvbo; u32 flags = 0; @@ -150,24 +66,22 @@ nouveau_prime_new(struct drm_device *dev, flags = TTM_PL_FLAG_TT;
ret = nouveau_bo_new(dev, size, 0, flags, 0, 0, - sg, pnvbo); + sg, &nvbo); if (ret) - return ret; - nvbo = *pnvbo; + return ERR_PTR(ret);
nvbo->valid_domains = NOUVEAU_GEM_DOMAIN_GART; nvbo->gem = drm_gem_object_alloc(dev, nvbo->bo.mem.size); if (!nvbo->gem) { - nouveau_bo_ref(NULL, pnvbo); - return -ENOMEM; + nouveau_bo_ref(NULL, &nvbo); + return ERR_PTR(-ENOMEM); }
nvbo->gem->driver_private = nvbo; - return 0; + return nvbo->gem; }
-struct dma_buf *nouveau_gem_prime_export(struct drm_device *dev, - struct drm_gem_object *obj, int flags) +int nouveau_gem_prime_pin(struct drm_gem_object *obj) { struct nouveau_bo *nvbo = nouveau_gem_object(obj); int ret = 0; @@ -175,52 +89,7 @@ struct dma_buf *nouveau_gem_prime_export(struct drm_device *dev, /* pin buffer into GTT */ ret = nouveau_bo_pin(nvbo, TTM_PL_FLAG_TT); if (ret) - return ERR_PTR(-EINVAL); - - return dma_buf_export(nvbo, &nouveau_dmabuf_ops, obj->size, flags); -} - -struct drm_gem_object *nouveau_gem_prime_import(struct drm_device *dev, - struct dma_buf *dma_buf) -{ - struct dma_buf_attachment *attach; - struct sg_table *sg; - struct nouveau_bo *nvbo; - int ret; - - if (dma_buf->ops == &nouveau_dmabuf_ops) { - nvbo = dma_buf->priv; - if (nvbo->gem) { - if (nvbo->gem->dev == dev) { - drm_gem_object_reference(nvbo->gem); - dma_buf_put(dma_buf); - return nvbo->gem; - } - } - } - /* need to attach */ - attach = dma_buf_attach(dma_buf, dev->dev); - if (IS_ERR(attach)) - return ERR_PTR(PTR_ERR(attach)); - - sg = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); - if (IS_ERR(sg)) { - ret = PTR_ERR(sg); - goto fail_detach; - } - - ret = nouveau_prime_new(dev, dma_buf->size, sg, &nvbo); - if (ret) - goto fail_unmap; - - nvbo->gem->import_attach = attach; - - return nvbo->gem; + return -EINVAL;
-fail_unmap: - dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL); -fail_detach: - dma_buf_detach(dma_buf, attach); - return ERR_PTR(ret); + return 0; } -
Simplify the Radeon prime implementation by using the default behavior provided by drm_gem_prime_import and drm_gem_prime_export.
v2: - Rename functions to radeon_gem_prime_get_sg_table and radeon_gem_prime_import_sg_table. - Delete the now-unused vmapping_count variable.
Signed-off-by: Aaron Plattner aplattner@nvidia.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: David Airlie airlied@linux.ie --- drivers/gpu/drm/radeon/radeon.h | 1 - drivers/gpu/drm/radeon/radeon_drv.c | 21 +++-- drivers/gpu/drm/radeon/radeon_prime.c | 170 +++++----------------------------- 3 files changed, 35 insertions(+), 157 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 34e5230..48bb80e 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -342,7 +342,6 @@ struct radeon_bo { struct drm_gem_object gem_base;
struct ttm_bo_kmap_obj dma_buf_vmap; - int vmapping_count; }; #define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, gem_base)
diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index dff6cf7..7a63817 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -117,11 +117,13 @@ int radeon_mode_dumb_create(struct drm_file *file_priv, int radeon_mode_dumb_destroy(struct drm_file *file_priv, struct drm_device *dev, uint32_t handle); -struct dma_buf *radeon_gem_prime_export(struct drm_device *dev, - struct drm_gem_object *obj, - int flags); -struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev, - struct dma_buf *dma_buf); +struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object *obj); +struct drm_gem_object *radeon_gem_prime_import_sg_table(struct drm_device *dev, + size_t size, + struct sg_table *sg); +int radeon_gem_prime_pin(struct drm_gem_object *obj); +void *radeon_gem_prime_vmap(struct drm_gem_object *obj); +void radeon_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr);
#if defined(CONFIG_DEBUG_FS) int radeon_debugfs_init(struct drm_minor *minor); @@ -396,8 +398,13 @@ 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 = radeon_gem_prime_export, - .gem_prime_import = radeon_gem_prime_import, + .gem_prime_export = drm_gem_prime_export, + .gem_prime_import = drm_gem_prime_import, + .gem_prime_pin = radeon_gem_prime_pin, + .gem_prime_get_sg_table = radeon_gem_prime_get_sg_table, + .gem_prime_import_sg_table = radeon_gem_prime_import_sg_table, + .gem_prime_vmap = radeon_gem_prime_vmap, + .gem_prime_vunmap = radeon_gem_prime_vunmap,
.name = DRIVER_NAME, .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c index 26c23bb..4940af7 100644 --- a/drivers/gpu/drm/radeon/radeon_prime.c +++ b/drivers/gpu/drm/radeon/radeon_prime.c @@ -28,199 +28,71 @@ #include "radeon.h" #include <drm/radeon_drm.h>
-#include <linux/dma-buf.h> - -static struct sg_table *radeon_gem_map_dma_buf(struct dma_buf_attachment *attachment, - enum dma_data_direction dir) +struct sg_table *radeon_gem_prime_get_sg_table(struct drm_gem_object *obj) { - struct radeon_bo *bo = attachment->dmabuf->priv; - struct drm_device *dev = bo->rdev->ddev; + struct radeon_bo *bo = gem_to_radeon_bo(obj); int npages = bo->tbo.num_pages; - struct sg_table *sg; - int nents; - - mutex_lock(&dev->struct_mutex); - sg = drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages); - nents = dma_map_sg(attachment->dev, sg->sgl, sg->nents, dir); - mutex_unlock(&dev->struct_mutex); - return sg; -} - -static void radeon_gem_unmap_dma_buf(struct dma_buf_attachment *attachment, - struct sg_table *sg, enum dma_data_direction dir) -{ - dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); - sg_free_table(sg); - kfree(sg); -} - -static void radeon_gem_dmabuf_release(struct dma_buf *dma_buf) -{ - struct radeon_bo *bo = dma_buf->priv; - - if (bo->gem_base.export_dma_buf == dma_buf) { - DRM_ERROR("unreference dmabuf %p\n", &bo->gem_base); - bo->gem_base.export_dma_buf = NULL; - drm_gem_object_unreference_unlocked(&bo->gem_base); - } -} - -static void *radeon_gem_kmap_atomic(struct dma_buf *dma_buf, unsigned long page_num) -{ - return NULL; -} - -static void radeon_gem_kunmap_atomic(struct dma_buf *dma_buf, unsigned long page_num, void *addr) -{ - -} -static void *radeon_gem_kmap(struct dma_buf *dma_buf, unsigned long page_num) -{ - return NULL; -} - -static void radeon_gem_kunmap(struct dma_buf *dma_buf, unsigned long page_num, void *addr) -{
+ return drm_prime_pages_to_sg(bo->tbo.ttm->pages, npages); }
-static int radeon_gem_prime_mmap(struct dma_buf *dma_buf, struct vm_area_struct *vma) +void *radeon_gem_prime_vmap(struct drm_gem_object *obj) { - return -EINVAL; -} - -static void *radeon_gem_prime_vmap(struct dma_buf *dma_buf) -{ - struct radeon_bo *bo = dma_buf->priv; - struct drm_device *dev = bo->rdev->ddev; + struct radeon_bo *bo = gem_to_radeon_bo(obj); int ret;
- mutex_lock(&dev->struct_mutex); - if (bo->vmapping_count) { - bo->vmapping_count++; - goto out_unlock; - } - ret = ttm_bo_kmap(&bo->tbo, 0, bo->tbo.num_pages, &bo->dma_buf_vmap); - if (ret) { - mutex_unlock(&dev->struct_mutex); + if (ret) return ERR_PTR(ret); - } - bo->vmapping_count = 1; -out_unlock: - mutex_unlock(&dev->struct_mutex); + return bo->dma_buf_vmap.virtual; }
-static void radeon_gem_prime_vunmap(struct dma_buf *dma_buf, void *vaddr) +void radeon_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr) { - struct radeon_bo *bo = dma_buf->priv; - struct drm_device *dev = bo->rdev->ddev; + struct radeon_bo *bo = gem_to_radeon_bo(obj);
- mutex_lock(&dev->struct_mutex); - bo->vmapping_count--; - if (bo->vmapping_count == 0) { - ttm_bo_kunmap(&bo->dma_buf_vmap); - } - mutex_unlock(&dev->struct_mutex); + ttm_bo_kunmap(&bo->dma_buf_vmap); } -const static struct dma_buf_ops radeon_dmabuf_ops = { - .map_dma_buf = radeon_gem_map_dma_buf, - .unmap_dma_buf = radeon_gem_unmap_dma_buf, - .release = radeon_gem_dmabuf_release, - .kmap = radeon_gem_kmap, - .kmap_atomic = radeon_gem_kmap_atomic, - .kunmap = radeon_gem_kunmap, - .kunmap_atomic = radeon_gem_kunmap_atomic, - .mmap = radeon_gem_prime_mmap, - .vmap = radeon_gem_prime_vmap, - .vunmap = radeon_gem_prime_vunmap, -}; - -static int radeon_prime_create(struct drm_device *dev, - size_t size, - struct sg_table *sg, - struct radeon_bo **pbo) + +struct drm_gem_object *radeon_gem_prime_import_sg_table(struct drm_device *dev, + size_t size, + struct sg_table *sg) { struct radeon_device *rdev = dev->dev_private; struct radeon_bo *bo; int ret;
ret = radeon_bo_create(rdev, size, PAGE_SIZE, false, - RADEON_GEM_DOMAIN_GTT, sg, pbo); + RADEON_GEM_DOMAIN_GTT, sg, &bo); if (ret) - return ret; - bo = *pbo; + return ERR_PTR(ret); bo->gem_base.driver_private = bo;
mutex_lock(&rdev->gem.mutex); list_add_tail(&bo->list, &rdev->gem.objects); mutex_unlock(&rdev->gem.mutex);
- return 0; + return &bo->gem_base; }
-struct dma_buf *radeon_gem_prime_export(struct drm_device *dev, - struct drm_gem_object *obj, - int flags) +int radeon_gem_prime_pin(struct drm_gem_object *obj) { struct radeon_bo *bo = gem_to_radeon_bo(obj); int ret = 0;
ret = radeon_bo_reserve(bo, false); if (unlikely(ret != 0)) - return ERR_PTR(ret); + return ret;
/* pin buffer into GTT */ ret = radeon_bo_pin(bo, RADEON_GEM_DOMAIN_GTT, NULL); if (ret) { radeon_bo_unreserve(bo); - return ERR_PTR(ret); + return ret; } radeon_bo_unreserve(bo); - return dma_buf_export(bo, &radeon_dmabuf_ops, obj->size, flags); -}
-struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev, - struct dma_buf *dma_buf) -{ - struct dma_buf_attachment *attach; - struct sg_table *sg; - struct radeon_bo *bo; - int ret; - - if (dma_buf->ops == &radeon_dmabuf_ops) { - bo = dma_buf->priv; - if (bo->gem_base.dev == dev) { - drm_gem_object_reference(&bo->gem_base); - dma_buf_put(dma_buf); - return &bo->gem_base; - } - } - - /* need to attach */ - attach = dma_buf_attach(dma_buf, dev->dev); - if (IS_ERR(attach)) - return ERR_CAST(attach); - - sg = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); - if (IS_ERR(sg)) { - ret = PTR_ERR(sg); - goto fail_detach; - } - - ret = radeon_prime_create(dev, dma_buf->size, sg, &bo); - if (ret) - goto fail_unmap; - - bo->gem_base.import_attach = attach; - - return &bo->gem_base; - -fail_unmap: - dma_buf_unmap_attachment(attach, sg, DMA_BIDIRECTIONAL); -fail_detach: - dma_buf_detach(dma_buf, attach); - return ERR_PTR(ret); + return 0; }
dri-devel@lists.freedesktop.org
-
Aaron Plattner -
Daniel Vetter -
Laurent Pinchart -
Maarten Lankhorst