[Xen-devel] [PATCH] dma-buf: add struct dma_buf_attach_info v2

Christian König posted 1 patch 4 years, 11 months ago
Failed in applying to current master (apply log)
drivers/dma-buf/dma-buf.c                       | 13 +++++++------
drivers/gpu/drm/armada/armada_gem.c             |  6 +++++-
drivers/gpu/drm/drm_prime.c                     |  6 +++++-
drivers/gpu/drm/i915/i915_gem_dmabuf.c          |  6 +++++-
drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c       |  6 +++++-
drivers/gpu/drm/tegra/gem.c                     |  6 +++++-
drivers/gpu/drm/udl/udl_dmabuf.c                |  6 +++++-
.../common/videobuf2/videobuf2-dma-contig.c     |  6 +++++-
.../media/common/videobuf2/videobuf2-dma-sg.c   |  6 +++++-
drivers/misc/fastrpc.c                          |  6 +++++-
drivers/staging/media/tegra-vde/tegra-vde.c     |  6 +++++-
drivers/xen/gntdev-dmabuf.c                     |  4 ++++
include/linux/dma-buf.h                         | 17 +++++++++++++++--
13 files changed, 76 insertions(+), 18 deletions(-)
[Xen-devel] [PATCH] dma-buf: add struct dma_buf_attach_info v2
Posted by Christian König 4 years, 11 months ago
Add a structure for the parameters of dma_buf_attach, this makes it much easier
to add new parameters later on.

v2: rebase cleanup and fix all new implementations as well

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/dma-buf/dma-buf.c                       | 13 +++++++------
 drivers/gpu/drm/armada/armada_gem.c             |  6 +++++-
 drivers/gpu/drm/drm_prime.c                     |  6 +++++-
 drivers/gpu/drm/i915/i915_gem_dmabuf.c          |  6 +++++-
 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c       |  6 +++++-
 drivers/gpu/drm/tegra/gem.c                     |  6 +++++-
 drivers/gpu/drm/udl/udl_dmabuf.c                |  6 +++++-
 .../common/videobuf2/videobuf2-dma-contig.c     |  6 +++++-
 .../media/common/videobuf2/videobuf2-dma-sg.c   |  6 +++++-
 drivers/misc/fastrpc.c                          |  6 +++++-
 drivers/staging/media/tegra-vde/tegra-vde.c     |  6 +++++-
 drivers/xen/gntdev-dmabuf.c                     |  4 ++++
 include/linux/dma-buf.h                         | 17 +++++++++++++++--
 13 files changed, 76 insertions(+), 18 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 3ae6c0c2cc02..e295e76a8c57 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -535,8 +535,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
 /**
  * dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
  * calls attach() of dma_buf_ops to allow device-specific attach functionality
- * @dmabuf:	[in]	buffer to attach device to.
- * @dev:	[in]	device to be attached.
+ * @info:	[in]	holds all the attach related information provided
+ *			by the importer. see &struct dma_buf_attach_info
+ *			for further details.
  *
  * Returns struct dma_buf_attachment pointer for this attachment. Attachments
  * must be cleaned up by calling dma_buf_detach().
@@ -550,20 +551,20 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
  * accessible to @dev, and cannot be moved to a more suitable place. This is
  * indicated with the error code -EBUSY.
  */
-struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
-					  struct device *dev)
+struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info)
 {
+	struct dma_buf *dmabuf = info->dmabuf;
 	struct dma_buf_attachment *attach;
 	int ret;
 
-	if (WARN_ON(!dmabuf || !dev))
+	if (WARN_ON(!dmabuf || !info->dev))
 		return ERR_PTR(-EINVAL);
 
 	attach = kzalloc(sizeof(*attach), GFP_KERNEL);
 	if (!attach)
 		return ERR_PTR(-ENOMEM);
 
-	attach->dev = dev;
+	attach->dev = info->dev;
 	attach->dmabuf = dmabuf;
 
 	mutex_lock(&dmabuf->lock);
diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
index 642d0e70d0f8..19c47821032f 100644
--- a/drivers/gpu/drm/armada/armada_gem.c
+++ b/drivers/gpu/drm/armada/armada_gem.c
@@ -501,6 +501,10 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj,
 struct drm_gem_object *
 armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev->dev,
+		.dmabuf = buf
+	};
 	struct dma_buf_attachment *attach;
 	struct armada_gem_object *dobj;
 
@@ -516,7 +520,7 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
 		}
 	}
 
-	attach = dma_buf_attach(buf, dev->dev);
+	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index dc079efb3b0f..1dd70fc095ee 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -710,6 +710,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
 					    struct dma_buf *dma_buf,
 					    struct device *attach_dev)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = attach_dev,
+		.dmabuf = dma_buf
+	};
 	struct dma_buf_attachment *attach;
 	struct sg_table *sgt;
 	struct drm_gem_object *obj;
@@ -730,7 +734,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
 	if (!dev->driver->gem_prime_import_sg_table)
 		return ERR_PTR(-EINVAL);
 
-	attach = dma_buf_attach(dma_buf, attach_dev);
+	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 5a101a9462d8..978054157c64 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -277,6 +277,10 @@ static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = {
 struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 					     struct dma_buf *dma_buf)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev->dev,
+		.dmabuf = dma_buf
+	};
 	struct dma_buf_attachment *attach;
 	struct drm_i915_gem_object *obj;
 	int ret;
@@ -295,7 +299,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
 	}
 
 	/* need to attach */
-	attach = dma_buf_attach(dma_buf, dev->dev);
+	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
index 0f8b597ccd10..38d06574b251 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
@@ -156,6 +156,10 @@ struct dma_buf *omap_gem_prime_export(struct drm_device *dev,
 struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
 					     struct dma_buf *dma_buf)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev->dev,
+		.dmabuf = dma_buf
+	};
 	struct dma_buf_attachment *attach;
 	struct drm_gem_object *obj;
 	struct sg_table *sgt;
@@ -173,7 +177,7 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
 		}
 	}
 
-	attach = dma_buf_attach(dma_buf, dev->dev);
+	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach))
 		return ERR_CAST(attach);
 
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
index 4f80100ff5f3..8e6b6c879add 100644
--- a/drivers/gpu/drm/tegra/gem.c
+++ b/drivers/gpu/drm/tegra/gem.c
@@ -332,6 +332,10 @@ struct tegra_bo *tegra_bo_create_with_handle(struct drm_file *file,
 static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
 					struct dma_buf *buf)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = drm->dev,
+		.dmabuf = buf
+	};
 	struct tegra_drm *tegra = drm->dev_private;
 	struct dma_buf_attachment *attach;
 	struct tegra_bo *bo;
@@ -341,7 +345,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
 	if (IS_ERR(bo))
 		return bo;
 
-	attach = dma_buf_attach(buf, drm->dev);
+	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach)) {
 		err = PTR_ERR(attach);
 		goto free;
diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c
index 556f62662aa9..86b928f9742f 100644
--- a/drivers/gpu/drm/udl/udl_dmabuf.c
+++ b/drivers/gpu/drm/udl/udl_dmabuf.c
@@ -226,6 +226,10 @@ static int udl_prime_create(struct drm_device *dev,
 struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
 				struct dma_buf *dma_buf)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev->dev,
+		.dmabuf = dma_buf
+	};
 	struct dma_buf_attachment *attach;
 	struct sg_table *sg;
 	struct udl_gem_object *uobj;
@@ -233,7 +237,7 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
 
 	/* need to attach */
 	get_device(dev->dev);
-	attach = dma_buf_attach(dma_buf, dev->dev);
+	attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(attach)) {
 		put_device(dev->dev);
 		return ERR_CAST(attach);
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
index 82389aead6ed..b2d844d45ea6 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
@@ -653,6 +653,10 @@ static void vb2_dc_detach_dmabuf(void *mem_priv)
 static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 	unsigned long size, enum dma_data_direction dma_dir)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev,
+		.dmabuf = dbuf
+	};
 	struct vb2_dc_buf *buf;
 	struct dma_buf_attachment *dba;
 
@@ -668,7 +672,7 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 
 	buf->dev = dev;
 	/* create attachment for the dmabuf with the user device */
-	dba = dma_buf_attach(dbuf, buf->dev);
+	dba = dma_buf_attach(&attach_info);
 	if (IS_ERR(dba)) {
 		pr_err("failed to attach dmabuf\n");
 		kfree(buf);
diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 270c3162fdcb..ddd5f36a8ec7 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -608,6 +608,10 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv)
 static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 	unsigned long size, enum dma_data_direction dma_dir)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev,
+		.dmabuf = dbuf
+	};
 	struct vb2_dma_sg_buf *buf;
 	struct dma_buf_attachment *dba;
 
@@ -623,7 +627,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
 
 	buf->dev = dev;
 	/* create attachment for the dmabuf with the user device */
-	dba = dma_buf_attach(dbuf, buf->dev);
+	dba = dma_buf_attach(&attach_info);
 	if (IS_ERR(dba)) {
 		pr_err("failed to attach dmabuf\n");
 		kfree(buf);
diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
index 39f832d27288..93d0aac05715 100644
--- a/drivers/misc/fastrpc.c
+++ b/drivers/misc/fastrpc.c
@@ -482,6 +482,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
 			      u64 len, struct fastrpc_map **ppmap)
 {
 	struct fastrpc_session_ctx *sess = fl->sctx;
+	struct dma_buf_attach_info attach_info;
 	struct fastrpc_map *map = NULL;
 	int err = 0;
 
@@ -501,7 +502,10 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
 		goto get_err;
 	}
 
-	map->attach = dma_buf_attach(map->buf, sess->dev);
+	memset(&attach_info, 0, sizeof(attach_info));
+	attach_info.dev = sess->dev;
+	attach_info.dmabuf = map->buf;
+	map->attach = dma_buf_attach(&attach_info);
 	if (IS_ERR(map->attach)) {
 		dev_err(sess->dev, "Failed to attach dmabuf\n");
 		err = PTR_ERR(map->attach);
diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c
index aa6c6bba961e..5a10c1facc27 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -568,6 +568,10 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
 				   size_t *size,
 				   enum dma_data_direction dma_dir)
 {
+	struct dma_buf_attach_info attach_info = {
+		.dev = dev,
+		.dmabuf = dmabuf
+	};
 	struct dma_buf_attachment *attachment;
 	struct dma_buf *dmabuf;
 	struct sg_table *sgt;
@@ -591,7 +595,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
 		return -EINVAL;
 	}
 
-	attachment = dma_buf_attach(dmabuf, dev);
+	attachment = dma_buf_attach(&attach_info);
 	if (IS_ERR(attachment)) {
 		dev_err(dev, "Failed to attach dmabuf\n");
 		err = PTR_ERR(attachment);
diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
index 2c4f324f8626..cacca830b482 100644
--- a/drivers/xen/gntdev-dmabuf.c
+++ b/drivers/xen/gntdev-dmabuf.c
@@ -608,6 +608,7 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
 		   int fd, int count, int domid)
 {
 	struct gntdev_dmabuf *gntdev_dmabuf, *ret;
+	struct dma_buf_attach_info attach_info;
 	struct dma_buf *dma_buf;
 	struct dma_buf_attachment *attach;
 	struct sg_table *sgt;
@@ -627,6 +628,9 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
 	gntdev_dmabuf->priv = priv;
 	gntdev_dmabuf->fd = fd;
 
+	memset(&attach_info, 0, sizeof(attach_info));
+	attach_info.dev = dev;
+	attach_info.dmabuf = dma_buf;
 	attach = dma_buf_attach(dma_buf, dev);
 	if (IS_ERR(attach)) {
 		ret = ERR_CAST(attach);
diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
index a0bd071466fc..b5b0f5e3f186 100644
--- a/include/linux/dma-buf.h
+++ b/include/linux/dma-buf.h
@@ -375,6 +375,19 @@ struct dma_buf_export_info {
 	struct dma_buf_export_info name = { .exp_name = KBUILD_MODNAME, \
 					 .owner = THIS_MODULE }
 
+/**
+ * struct dma_buf_attach_info - holds information needed to attach to a dma_buf
+ * @dmabuf:	the exported dma_buf
+ * @dev:	the device which wants to import the attachment
+ *
+ * This structure holds the information required to attach to a buffer. Used
+ * with dma_buf_attach() only.
+ */
+struct dma_buf_attach_info {
+	struct dma_buf *dmabuf;
+	struct device *dev;
+};
+
 /**
  * get_dma_buf - convenience wrapper for get_file.
  * @dmabuf:	[in]	pointer to dma_buf
@@ -389,8 +402,8 @@ static inline void get_dma_buf(struct dma_buf *dmabuf)
 	get_file(dmabuf->file);
 }
 
-struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
-							struct device *dev);
+struct dma_buf_attachment *
+dma_buf_attach(const struct dma_buf_attach_info *info);
 void dma_buf_detach(struct dma_buf *dmabuf,
 				struct dma_buf_attachment *dmabuf_attach);
 
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Intel-gfx] [PATCH] dma-buf: add struct dma_buf_attach_info v2
Posted by kbuild test robot 4 years, 11 months ago
Hi "Christian,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.1-rc7 next-20190429]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Christian-K-nig/dma-buf-add-struct-dma_buf_attach_info-v2/20190430-221017
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/staging/media/tegra-vde/tegra-vde.c: In function 'tegra_vde_attach_dmabuf':
>> drivers/staging/media/tegra-vde/tegra-vde.c:573:13: error: 'dmabuf' undeclared (first use in this function); did you mean 'dma_buf'?
      .dmabuf = dmabuf
                ^~~~~~
                dma_buf
   drivers/staging/media/tegra-vde/tegra-vde.c:573:13: note: each undeclared identifier is reported only once for each function it appears in

vim +573 drivers/staging/media/tegra-vde/tegra-vde.c

   559	
   560	static int tegra_vde_attach_dmabuf(struct device *dev,
   561					   int fd,
   562					   unsigned long offset,
   563					   size_t min_size,
   564					   size_t align_size,
   565					   struct dma_buf_attachment **a,
   566					   dma_addr_t *addr,
   567					   struct sg_table **s,
   568					   size_t *size,
   569					   enum dma_data_direction dma_dir)
   570	{
   571		struct dma_buf_attach_info attach_info = {
   572			.dev = dev,
 > 573			.dmabuf = dmabuf
   574		};
   575		struct dma_buf_attachment *attachment;
   576		struct dma_buf *dmabuf;
   577		struct sg_table *sgt;
   578		int err;
   579	
   580		dmabuf = dma_buf_get(fd);
   581		if (IS_ERR(dmabuf)) {
   582			dev_err(dev, "Invalid dmabuf FD\n");
   583			return PTR_ERR(dmabuf);
   584		}
   585	
   586		if (dmabuf->size & (align_size - 1)) {
   587			dev_err(dev, "Unaligned dmabuf 0x%zX, should be aligned to 0x%zX\n",
   588				dmabuf->size, align_size);
   589			return -EINVAL;
   590		}
   591	
   592		if ((u64)offset + min_size > dmabuf->size) {
   593			dev_err(dev, "Too small dmabuf size %zu @0x%lX, should be at least %zu\n",
   594				dmabuf->size, offset, min_size);
   595			return -EINVAL;
   596		}
   597	
   598		attachment = dma_buf_attach(&attach_info);
   599		if (IS_ERR(attachment)) {
   600			dev_err(dev, "Failed to attach dmabuf\n");
   601			err = PTR_ERR(attachment);
   602			goto err_put;
   603		}
   604	
   605		sgt = dma_buf_map_attachment(attachment, dma_dir);
   606		if (IS_ERR(sgt)) {
   607			dev_err(dev, "Failed to get dmabufs sg_table\n");
   608			err = PTR_ERR(sgt);
   609			goto err_detach;
   610		}
   611	
   612		if (sgt->nents != 1) {
   613			dev_err(dev, "Sparse DMA region is unsupported\n");
   614			err = -EINVAL;
   615			goto err_unmap;
   616		}
   617	
   618		*addr = sg_dma_address(sgt->sgl) + offset;
   619		*a = attachment;
   620		*s = sgt;
   621	
   622		if (size)
   623			*size = dmabuf->size - offset;
   624	
   625		return 0;
   626	
   627	err_unmap:
   628		dma_buf_unmap_attachment(attachment, sgt, dma_dir);
   629	err_detach:
   630		dma_buf_detach(dmabuf, attachment);
   631	err_put:
   632		dma_buf_put(dmabuf);
   633	
   634		return err;
   635	}
   636	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] dma-buf: add struct dma_buf_attach_info v2
Posted by Boris Ostrovsky 4 years, 11 months ago
On 4/30/19 7:10 AM, Christian König wrote:
> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
> index 2c4f324f8626..cacca830b482 100644
> --- a/drivers/xen/gntdev-dmabuf.c
> +++ b/drivers/xen/gntdev-dmabuf.c
> @@ -608,6 +608,7 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
>  		   int fd, int count, int domid)
>  {
>  	struct gntdev_dmabuf *gntdev_dmabuf, *ret;
> +	struct dma_buf_attach_info attach_info;
>  	struct dma_buf *dma_buf;
>  	struct dma_buf_attachment *attach;
>  	struct sg_table *sgt;
> @@ -627,6 +628,9 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
>  	gntdev_dmabuf->priv = priv;
>  	gntdev_dmabuf->fd = fd;
>  
> +	memset(&attach_info, 0, sizeof(attach_info));
> +	attach_info.dev = dev;
> +	attach_info.dmabuf = dma_buf;
>  	attach = dma_buf_attach(dma_buf, dev);
>  	if (IS_ERR(attach)) {
>  		ret = ERR_CAST(attach);

This won't build.

Did you mean

    attach = dma_buf_attach(&attach_info);

?

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] dma-buf: add struct dma_buf_attach_info v2
Posted by Russell King - ARM Linux admin 4 years, 11 months ago
On Tue, Apr 30, 2019 at 01:10:02PM +0200, Christian König wrote:
> Add a structure for the parameters of dma_buf_attach, this makes it much easier
> to add new parameters later on.

I don't understand this reasoning.  What are the "new parameters" that
are being proposed, and why do we need to put them into memory to pass
them across this interface?

If the intention is to make it easier to change the interface, passing
parameters in this manner mean that it's easy for the interface to
change and drivers not to notice the changes, since the compiler will
not warn (unless some member of the structure that the driver is using
gets removed, in which case it will error.)

Additions to the structure will go unnoticed by drivers - what if the
caller is expecting some different kind of behaviour, and the driver
ignores that new addition?

This doesn't seem to me like a good idea.

> 
> v2: rebase cleanup and fix all new implementations as well
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/dma-buf/dma-buf.c                       | 13 +++++++------
>  drivers/gpu/drm/armada/armada_gem.c             |  6 +++++-
>  drivers/gpu/drm/drm_prime.c                     |  6 +++++-
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c          |  6 +++++-
>  drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c       |  6 +++++-
>  drivers/gpu/drm/tegra/gem.c                     |  6 +++++-
>  drivers/gpu/drm/udl/udl_dmabuf.c                |  6 +++++-
>  .../common/videobuf2/videobuf2-dma-contig.c     |  6 +++++-
>  .../media/common/videobuf2/videobuf2-dma-sg.c   |  6 +++++-
>  drivers/misc/fastrpc.c                          |  6 +++++-
>  drivers/staging/media/tegra-vde/tegra-vde.c     |  6 +++++-
>  drivers/xen/gntdev-dmabuf.c                     |  4 ++++
>  include/linux/dma-buf.h                         | 17 +++++++++++++++--
>  13 files changed, 76 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 3ae6c0c2cc02..e295e76a8c57 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -535,8 +535,9 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
>  /**
>   * dma_buf_attach - Add the device to dma_buf's attachments list; optionally,
>   * calls attach() of dma_buf_ops to allow device-specific attach functionality
> - * @dmabuf:	[in]	buffer to attach device to.
> - * @dev:	[in]	device to be attached.
> + * @info:	[in]	holds all the attach related information provided
> + *			by the importer. see &struct dma_buf_attach_info
> + *			for further details.
>   *
>   * Returns struct dma_buf_attachment pointer for this attachment. Attachments
>   * must be cleaned up by calling dma_buf_detach().
> @@ -550,20 +551,20 @@ EXPORT_SYMBOL_GPL(dma_buf_put);
>   * accessible to @dev, and cannot be moved to a more suitable place. This is
>   * indicated with the error code -EBUSY.
>   */
> -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> -					  struct device *dev)
> +struct dma_buf_attachment *dma_buf_attach(const struct dma_buf_attach_info *info)
>  {
> +	struct dma_buf *dmabuf = info->dmabuf;
>  	struct dma_buf_attachment *attach;
>  	int ret;
>  
> -	if (WARN_ON(!dmabuf || !dev))
> +	if (WARN_ON(!dmabuf || !info->dev))
>  		return ERR_PTR(-EINVAL);
>  
>  	attach = kzalloc(sizeof(*attach), GFP_KERNEL);
>  	if (!attach)
>  		return ERR_PTR(-ENOMEM);
>  
> -	attach->dev = dev;
> +	attach->dev = info->dev;
>  	attach->dmabuf = dmabuf;
>  
>  	mutex_lock(&dmabuf->lock);
> diff --git a/drivers/gpu/drm/armada/armada_gem.c b/drivers/gpu/drm/armada/armada_gem.c
> index 642d0e70d0f8..19c47821032f 100644
> --- a/drivers/gpu/drm/armada/armada_gem.c
> +++ b/drivers/gpu/drm/armada/armada_gem.c
> @@ -501,6 +501,10 @@ armada_gem_prime_export(struct drm_device *dev, struct drm_gem_object *obj,
>  struct drm_gem_object *
>  armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev->dev,
> +		.dmabuf = buf
> +	};
>  	struct dma_buf_attachment *attach;
>  	struct armada_gem_object *dobj;
>  
> @@ -516,7 +520,7 @@ armada_gem_prime_import(struct drm_device *dev, struct dma_buf *buf)
>  		}
>  	}
>  
> -	attach = dma_buf_attach(buf, dev->dev);
> +	attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attach))
>  		return ERR_CAST(attach);
>  
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index dc079efb3b0f..1dd70fc095ee 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -710,6 +710,10 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>  					    struct dma_buf *dma_buf,
>  					    struct device *attach_dev)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = attach_dev,
> +		.dmabuf = dma_buf
> +	};
>  	struct dma_buf_attachment *attach;
>  	struct sg_table *sgt;
>  	struct drm_gem_object *obj;
> @@ -730,7 +734,7 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
>  	if (!dev->driver->gem_prime_import_sg_table)
>  		return ERR_PTR(-EINVAL);
>  
> -	attach = dma_buf_attach(dma_buf, attach_dev);
> +	attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attach))
>  		return ERR_CAST(attach);
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index 5a101a9462d8..978054157c64 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -277,6 +277,10 @@ static const struct drm_i915_gem_object_ops i915_gem_object_dmabuf_ops = {
>  struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>  					     struct dma_buf *dma_buf)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev->dev,
> +		.dmabuf = dma_buf
> +	};
>  	struct dma_buf_attachment *attach;
>  	struct drm_i915_gem_object *obj;
>  	int ret;
> @@ -295,7 +299,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>  	}
>  
>  	/* need to attach */
> -	attach = dma_buf_attach(dma_buf, dev->dev);
> +	attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attach))
>  		return ERR_CAST(attach);
>  
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> index 0f8b597ccd10..38d06574b251 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c
> @@ -156,6 +156,10 @@ struct dma_buf *omap_gem_prime_export(struct drm_device *dev,
>  struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
>  					     struct dma_buf *dma_buf)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev->dev,
> +		.dmabuf = dma_buf
> +	};
>  	struct dma_buf_attachment *attach;
>  	struct drm_gem_object *obj;
>  	struct sg_table *sgt;
> @@ -173,7 +177,7 @@ struct drm_gem_object *omap_gem_prime_import(struct drm_device *dev,
>  		}
>  	}
>  
> -	attach = dma_buf_attach(dma_buf, dev->dev);
> +	attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attach))
>  		return ERR_CAST(attach);
>  
> diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> index 4f80100ff5f3..8e6b6c879add 100644
> --- a/drivers/gpu/drm/tegra/gem.c
> +++ b/drivers/gpu/drm/tegra/gem.c
> @@ -332,6 +332,10 @@ struct tegra_bo *tegra_bo_create_with_handle(struct drm_file *file,
>  static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
>  					struct dma_buf *buf)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = drm->dev,
> +		.dmabuf = buf
> +	};
>  	struct tegra_drm *tegra = drm->dev_private;
>  	struct dma_buf_attachment *attach;
>  	struct tegra_bo *bo;
> @@ -341,7 +345,7 @@ static struct tegra_bo *tegra_bo_import(struct drm_device *drm,
>  	if (IS_ERR(bo))
>  		return bo;
>  
> -	attach = dma_buf_attach(buf, drm->dev);
> +	attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attach)) {
>  		err = PTR_ERR(attach);
>  		goto free;
> diff --git a/drivers/gpu/drm/udl/udl_dmabuf.c b/drivers/gpu/drm/udl/udl_dmabuf.c
> index 556f62662aa9..86b928f9742f 100644
> --- a/drivers/gpu/drm/udl/udl_dmabuf.c
> +++ b/drivers/gpu/drm/udl/udl_dmabuf.c
> @@ -226,6 +226,10 @@ static int udl_prime_create(struct drm_device *dev,
>  struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
>  				struct dma_buf *dma_buf)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev->dev,
> +		.dmabuf = dma_buf
> +	};
>  	struct dma_buf_attachment *attach;
>  	struct sg_table *sg;
>  	struct udl_gem_object *uobj;
> @@ -233,7 +237,7 @@ struct drm_gem_object *udl_gem_prime_import(struct drm_device *dev,
>  
>  	/* need to attach */
>  	get_device(dev->dev);
> -	attach = dma_buf_attach(dma_buf, dev->dev);
> +	attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attach)) {
>  		put_device(dev->dev);
>  		return ERR_CAST(attach);
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-contig.c b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> index 82389aead6ed..b2d844d45ea6 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-contig.c
> @@ -653,6 +653,10 @@ static void vb2_dc_detach_dmabuf(void *mem_priv)
>  static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  	unsigned long size, enum dma_data_direction dma_dir)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev,
> +		.dmabuf = dbuf
> +	};
>  	struct vb2_dc_buf *buf;
>  	struct dma_buf_attachment *dba;
>  
> @@ -668,7 +672,7 @@ static void *vb2_dc_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  
>  	buf->dev = dev;
>  	/* create attachment for the dmabuf with the user device */
> -	dba = dma_buf_attach(dbuf, buf->dev);
> +	dba = dma_buf_attach(&attach_info);
>  	if (IS_ERR(dba)) {
>  		pr_err("failed to attach dmabuf\n");
>  		kfree(buf);
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index 270c3162fdcb..ddd5f36a8ec7 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -608,6 +608,10 @@ static void vb2_dma_sg_detach_dmabuf(void *mem_priv)
>  static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  	unsigned long size, enum dma_data_direction dma_dir)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev,
> +		.dmabuf = dbuf
> +	};
>  	struct vb2_dma_sg_buf *buf;
>  	struct dma_buf_attachment *dba;
>  
> @@ -623,7 +627,7 @@ static void *vb2_dma_sg_attach_dmabuf(struct device *dev, struct dma_buf *dbuf,
>  
>  	buf->dev = dev;
>  	/* create attachment for the dmabuf with the user device */
> -	dba = dma_buf_attach(dbuf, buf->dev);
> +	dba = dma_buf_attach(&attach_info);
>  	if (IS_ERR(dba)) {
>  		pr_err("failed to attach dmabuf\n");
>  		kfree(buf);
> diff --git a/drivers/misc/fastrpc.c b/drivers/misc/fastrpc.c
> index 39f832d27288..93d0aac05715 100644
> --- a/drivers/misc/fastrpc.c
> +++ b/drivers/misc/fastrpc.c
> @@ -482,6 +482,7 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
>  			      u64 len, struct fastrpc_map **ppmap)
>  {
>  	struct fastrpc_session_ctx *sess = fl->sctx;
> +	struct dma_buf_attach_info attach_info;
>  	struct fastrpc_map *map = NULL;
>  	int err = 0;
>  
> @@ -501,7 +502,10 @@ static int fastrpc_map_create(struct fastrpc_user *fl, int fd,
>  		goto get_err;
>  	}
>  
> -	map->attach = dma_buf_attach(map->buf, sess->dev);
> +	memset(&attach_info, 0, sizeof(attach_info));
> +	attach_info.dev = sess->dev;
> +	attach_info.dmabuf = map->buf;
> +	map->attach = dma_buf_attach(&attach_info);
>  	if (IS_ERR(map->attach)) {
>  		dev_err(sess->dev, "Failed to attach dmabuf\n");
>  		err = PTR_ERR(map->attach);
> diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c
> index aa6c6bba961e..5a10c1facc27 100644
> --- a/drivers/staging/media/tegra-vde/tegra-vde.c
> +++ b/drivers/staging/media/tegra-vde/tegra-vde.c
> @@ -568,6 +568,10 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
>  				   size_t *size,
>  				   enum dma_data_direction dma_dir)
>  {
> +	struct dma_buf_attach_info attach_info = {
> +		.dev = dev,
> +		.dmabuf = dmabuf
> +	};
>  	struct dma_buf_attachment *attachment;
>  	struct dma_buf *dmabuf;
>  	struct sg_table *sgt;
> @@ -591,7 +595,7 @@ static int tegra_vde_attach_dmabuf(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> -	attachment = dma_buf_attach(dmabuf, dev);
> +	attachment = dma_buf_attach(&attach_info);
>  	if (IS_ERR(attachment)) {
>  		dev_err(dev, "Failed to attach dmabuf\n");
>  		err = PTR_ERR(attachment);
> diff --git a/drivers/xen/gntdev-dmabuf.c b/drivers/xen/gntdev-dmabuf.c
> index 2c4f324f8626..cacca830b482 100644
> --- a/drivers/xen/gntdev-dmabuf.c
> +++ b/drivers/xen/gntdev-dmabuf.c
> @@ -608,6 +608,7 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
>  		   int fd, int count, int domid)
>  {
>  	struct gntdev_dmabuf *gntdev_dmabuf, *ret;
> +	struct dma_buf_attach_info attach_info;
>  	struct dma_buf *dma_buf;
>  	struct dma_buf_attachment *attach;
>  	struct sg_table *sgt;
> @@ -627,6 +628,9 @@ dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
>  	gntdev_dmabuf->priv = priv;
>  	gntdev_dmabuf->fd = fd;
>  
> +	memset(&attach_info, 0, sizeof(attach_info));
> +	attach_info.dev = dev;
> +	attach_info.dmabuf = dma_buf;
>  	attach = dma_buf_attach(dma_buf, dev);
>  	if (IS_ERR(attach)) {
>  		ret = ERR_CAST(attach);
> diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h
> index a0bd071466fc..b5b0f5e3f186 100644
> --- a/include/linux/dma-buf.h
> +++ b/include/linux/dma-buf.h
> @@ -375,6 +375,19 @@ struct dma_buf_export_info {
>  	struct dma_buf_export_info name = { .exp_name = KBUILD_MODNAME, \
>  					 .owner = THIS_MODULE }
>  
> +/**
> + * struct dma_buf_attach_info - holds information needed to attach to a dma_buf
> + * @dmabuf:	the exported dma_buf
> + * @dev:	the device which wants to import the attachment
> + *
> + * This structure holds the information required to attach to a buffer. Used
> + * with dma_buf_attach() only.
> + */
> +struct dma_buf_attach_info {
> +	struct dma_buf *dmabuf;
> +	struct device *dev;
> +};
> +
>  /**
>   * get_dma_buf - convenience wrapper for get_file.
>   * @dmabuf:	[in]	pointer to dma_buf
> @@ -389,8 +402,8 @@ static inline void get_dma_buf(struct dma_buf *dmabuf)
>  	get_file(dmabuf->file);
>  }
>  
> -struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
> -							struct device *dev);
> +struct dma_buf_attachment *
> +dma_buf_attach(const struct dma_buf_attach_info *info);
>  void dma_buf_detach(struct dma_buf *dmabuf,
>  				struct dma_buf_attachment *dmabuf_attach);
>  
> -- 
> 2.17.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] dma-buf: add struct dma_buf_attach_info v2
Posted by Christian König 4 years, 11 months ago
Am 30.04.19 um 19:31 schrieb Russell King - ARM Linux admin:
> On Tue, Apr 30, 2019 at 01:10:02PM +0200, Christian König wrote:
>> Add a structure for the parameters of dma_buf_attach, this makes it much easier
>> to add new parameters later on.
> I don't understand this reasoning.  What are the "new parameters" that
> are being proposed, and why do we need to put them into memory to pass
> them across this interface?
>
> If the intention is to make it easier to change the interface, passing
> parameters in this manner mean that it's easy for the interface to
> change and drivers not to notice the changes, since the compiler will
> not warn (unless some member of the structure that the driver is using
> gets removed, in which case it will error.)
>
> Additions to the structure will go unnoticed by drivers - what if the
> caller is expecting some different kind of behaviour, and the driver
> ignores that new addition?

Well, exactly that's the intention here: That the drivers using this 
interface should be able to ignore the new additions for now as long as 
they are not going to use them.

The background is that we have multiple interface changes in the 
pipeline, and each step requires new optional parameters.

> This doesn't seem to me like a good idea.

Well, the obvious alternatives are:

a) Change all drivers to explicitly provide NULL/0 for the new parameters.

b) Use a wrapper, so that the function signature of dma_buf_attach stays 
the same.

Key point here is that I have an invalidation callback change, a P2P 
patch set and some locking changes which all require adding new 
parameters or flags. And at each step I would then start to change all 
drivers, adding some more NULL pointers or flags with 0 default value.

I'm actually perfectly fine going down any route, but this just seemed 
to me simplest and with the least risk of breaking anything. Opinions?

Thanks,
Christian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Intel-gfx] [PATCH] dma-buf: add struct dma_buf_attach_info v2
Posted by Daniel Vetter 4 years, 11 months ago
On Fri, May 03, 2019 at 02:05:47PM +0200, Christian König wrote:
> Am 30.04.19 um 19:31 schrieb Russell King - ARM Linux admin:
> > On Tue, Apr 30, 2019 at 01:10:02PM +0200, Christian König wrote:
> > > Add a structure for the parameters of dma_buf_attach, this makes it much easier
> > > to add new parameters later on.
> > I don't understand this reasoning.  What are the "new parameters" that
> > are being proposed, and why do we need to put them into memory to pass
> > them across this interface?
> > 
> > If the intention is to make it easier to change the interface, passing
> > parameters in this manner mean that it's easy for the interface to
> > change and drivers not to notice the changes, since the compiler will
> > not warn (unless some member of the structure that the driver is using
> > gets removed, in which case it will error.)
> > 
> > Additions to the structure will go unnoticed by drivers - what if the
> > caller is expecting some different kind of behaviour, and the driver
> > ignores that new addition?
> 
> Well, exactly that's the intention here: That the drivers using this
> interface should be able to ignore the new additions for now as long as they
> are not going to use them.
> 
> The background is that we have multiple interface changes in the pipeline,
> and each step requires new optional parameters.
> 
> > This doesn't seem to me like a good idea.
> 
> Well, the obvious alternatives are:
> 
> a) Change all drivers to explicitly provide NULL/0 for the new parameters.
> 
> b) Use a wrapper, so that the function signature of dma_buf_attach stays the
> same.
> 
> Key point here is that I have an invalidation callback change, a P2P patch
> set and some locking changes which all require adding new parameters or
> flags. And at each step I would then start to change all drivers, adding
> some more NULL pointers or flags with 0 default value.
> 
> I'm actually perfectly fine going down any route, but this just seemed to me
> simplest and with the least risk of breaking anything. Opinions?

I think given all our discussions and plans the argument object makes tons
of sense. Much easier to document well than a long list of parameters.
Maybe we should make it const, so it could work like an ops/func table and
we could store it as a pointer in the dma_buf_attachment?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Intel-gfx] [PATCH] dma-buf: add struct dma_buf_attach_info v2
Posted by Koenig, Christian 4 years, 11 months ago
Am 03.05.19 um 14:09 schrieb Daniel Vetter:
> [CAUTION: External Email]
>
> On Fri, May 03, 2019 at 02:05:47PM +0200, Christian König wrote:
>> Am 30.04.19 um 19:31 schrieb Russell King - ARM Linux admin:
>>> On Tue, Apr 30, 2019 at 01:10:02PM +0200, Christian König wrote:
>>>> Add a structure for the parameters of dma_buf_attach, this makes it much easier
>>>> to add new parameters later on.
>>> I don't understand this reasoning.  What are the "new parameters" that
>>> are being proposed, and why do we need to put them into memory to pass
>>> them across this interface?
>>>
>>> If the intention is to make it easier to change the interface, passing
>>> parameters in this manner mean that it's easy for the interface to
>>> change and drivers not to notice the changes, since the compiler will
>>> not warn (unless some member of the structure that the driver is using
>>> gets removed, in which case it will error.)
>>>
>>> Additions to the structure will go unnoticed by drivers - what if the
>>> caller is expecting some different kind of behaviour, and the driver
>>> ignores that new addition?
>> Well, exactly that's the intention here: That the drivers using this
>> interface should be able to ignore the new additions for now as long as they
>> are not going to use them.
>>
>> The background is that we have multiple interface changes in the pipeline,
>> and each step requires new optional parameters.
>>
>>> This doesn't seem to me like a good idea.
>> Well, the obvious alternatives are:
>>
>> a) Change all drivers to explicitly provide NULL/0 for the new parameters.
>>
>> b) Use a wrapper, so that the function signature of dma_buf_attach stays the
>> same.
>>
>> Key point here is that I have an invalidation callback change, a P2P patch
>> set and some locking changes which all require adding new parameters or
>> flags. And at each step I would then start to change all drivers, adding
>> some more NULL pointers or flags with 0 default value.
>>
>> I'm actually perfectly fine going down any route, but this just seemed to me
>> simplest and with the least risk of breaking anything. Opinions?
> I think given all our discussions and plans the argument object makes tons
> of sense. Much easier to document well than a long list of parameters.
> Maybe we should make it const, so it could work like an ops/func table and
> we could store it as a pointer in the dma_buf_attachment?

Yeah, the invalidation callback and P2P flags are constant. But the 
importer_priv field isn't.

We could do something like adding the importer_priv field as parameter 
and the other two as const structure.

Third alternative would be to throw out all the DRM abstraction and just 
embed the attachment structure in the buffer object and get completely 
rid of the importer_priv field (probably the cleanest alternative, but 
also the most work todo).

Christian.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Intel-gfx] [PATCH] dma-buf: add struct dma_buf_attach_info v2
Posted by kbuild test robot 4 years, 11 months ago
Hi "Christian,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.1-rc7 next-20190430]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Christian-K-nig/dma-buf-add-struct-dma_buf_attach_info-v2/20190430-221017
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/xen/gntdev-dmabuf.c:634:33: sparse: sparse: incorrect type in argument 1 (different base types) @@    expected struct dma_buf_attach_info const *info @@    got dma_buf_attach_info const *info @@
>> drivers/xen/gntdev-dmabuf.c:634:33: sparse:    expected struct dma_buf_attach_info const *info
>> drivers/xen/gntdev-dmabuf.c:634:33: sparse:    got struct dma_buf *[assigned] dma_buf
>> drivers/xen/gntdev-dmabuf.c:634:32: sparse: sparse: too many arguments for function dma_buf_attach

vim +634 drivers/xen/gntdev-dmabuf.c

bf8dc55b Oleksandr Andrushchenko 2018-07-20  605  
932d6562 Oleksandr Andrushchenko 2018-07-20  606  static struct gntdev_dmabuf *
932d6562 Oleksandr Andrushchenko 2018-07-20  607  dmabuf_imp_to_refs(struct gntdev_dmabuf_priv *priv, struct device *dev,
932d6562 Oleksandr Andrushchenko 2018-07-20  608  		   int fd, int count, int domid)
932d6562 Oleksandr Andrushchenko 2018-07-20  609  {
bf8dc55b Oleksandr Andrushchenko 2018-07-20  610  	struct gntdev_dmabuf *gntdev_dmabuf, *ret;
e648feab Christian König         2019-04-30  611  	struct dma_buf_attach_info attach_info;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  612  	struct dma_buf *dma_buf;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  613  	struct dma_buf_attachment *attach;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  614  	struct sg_table *sgt;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  615  	struct sg_page_iter sg_iter;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  616  	int i;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  617  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  618  	dma_buf = dma_buf_get(fd);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  619  	if (IS_ERR(dma_buf))
bf8dc55b Oleksandr Andrushchenko 2018-07-20  620  		return ERR_CAST(dma_buf);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  621  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  622  	gntdev_dmabuf = dmabuf_imp_alloc_storage(count);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  623  	if (IS_ERR(gntdev_dmabuf)) {
bf8dc55b Oleksandr Andrushchenko 2018-07-20  624  		ret = gntdev_dmabuf;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  625  		goto fail_put;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  626  	}
bf8dc55b Oleksandr Andrushchenko 2018-07-20  627  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  628  	gntdev_dmabuf->priv = priv;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  629  	gntdev_dmabuf->fd = fd;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  630  
e648feab Christian König         2019-04-30  631  	memset(&attach_info, 0, sizeof(attach_info));
e648feab Christian König         2019-04-30  632  	attach_info.dev = dev;
e648feab Christian König         2019-04-30  633  	attach_info.dmabuf = dma_buf;
bf8dc55b Oleksandr Andrushchenko 2018-07-20 @634  	attach = dma_buf_attach(dma_buf, dev);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  635  	if (IS_ERR(attach)) {
bf8dc55b Oleksandr Andrushchenko 2018-07-20  636  		ret = ERR_CAST(attach);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  637  		goto fail_free_obj;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  638  	}
bf8dc55b Oleksandr Andrushchenko 2018-07-20  639  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  640  	gntdev_dmabuf->u.imp.attach = attach;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  641  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  642  	sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  643  	if (IS_ERR(sgt)) {
bf8dc55b Oleksandr Andrushchenko 2018-07-20  644  		ret = ERR_CAST(sgt);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  645  		goto fail_detach;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  646  	}
bf8dc55b Oleksandr Andrushchenko 2018-07-20  647  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  648  	/* Check number of pages that imported buffer has. */
bf8dc55b Oleksandr Andrushchenko 2018-07-20  649  	if (attach->dmabuf->size != gntdev_dmabuf->nr_pages << PAGE_SHIFT) {
bf8dc55b Oleksandr Andrushchenko 2018-07-20  650  		ret = ERR_PTR(-EINVAL);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  651  		pr_debug("DMA buffer has %zu pages, user-space expects %d\n",
bf8dc55b Oleksandr Andrushchenko 2018-07-20  652  			 attach->dmabuf->size, gntdev_dmabuf->nr_pages);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  653  		goto fail_unmap;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  654  	}
bf8dc55b Oleksandr Andrushchenko 2018-07-20  655  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  656  	gntdev_dmabuf->u.imp.sgt = sgt;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  657  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  658  	/* Now convert sgt to array of pages and check for page validity. */
bf8dc55b Oleksandr Andrushchenko 2018-07-20  659  	i = 0;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  660  	for_each_sg_page(sgt->sgl, &sg_iter, sgt->nents, 0) {
bf8dc55b Oleksandr Andrushchenko 2018-07-20  661  		struct page *page = sg_page_iter_page(&sg_iter);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  662  		/*
bf8dc55b Oleksandr Andrushchenko 2018-07-20  663  		 * Check if page is valid: this can happen if we are given
bf8dc55b Oleksandr Andrushchenko 2018-07-20  664  		 * a page from VRAM or other resources which are not backed
bf8dc55b Oleksandr Andrushchenko 2018-07-20  665  		 * by a struct page.
bf8dc55b Oleksandr Andrushchenko 2018-07-20  666  		 */
bf8dc55b Oleksandr Andrushchenko 2018-07-20  667  		if (!pfn_valid(page_to_pfn(page))) {
bf8dc55b Oleksandr Andrushchenko 2018-07-20  668  			ret = ERR_PTR(-EINVAL);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  669  			goto fail_unmap;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  670  		}
bf8dc55b Oleksandr Andrushchenko 2018-07-20  671  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  672  		gntdev_dmabuf->pages[i++] = page;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  673  	}
bf8dc55b Oleksandr Andrushchenko 2018-07-20  674  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  675  	ret = ERR_PTR(dmabuf_imp_grant_foreign_access(gntdev_dmabuf->pages,
bf8dc55b Oleksandr Andrushchenko 2018-07-20  676  						      gntdev_dmabuf->u.imp.refs,
bf8dc55b Oleksandr Andrushchenko 2018-07-20  677  						      count, domid));
bf8dc55b Oleksandr Andrushchenko 2018-07-20  678  	if (IS_ERR(ret))
bf8dc55b Oleksandr Andrushchenko 2018-07-20  679  		goto fail_end_access;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  680  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  681  	pr_debug("Imported DMA buffer with fd %d\n", fd);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  682  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  683  	mutex_lock(&priv->lock);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  684  	list_add(&gntdev_dmabuf->next, &priv->imp_list);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  685  	mutex_unlock(&priv->lock);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  686  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  687  	return gntdev_dmabuf;
bf8dc55b Oleksandr Andrushchenko 2018-07-20  688  
bf8dc55b Oleksandr Andrushchenko 2018-07-20  689  fail_end_access:
bf8dc55b Oleksandr Andrushchenko 2018-07-20  690  	dmabuf_imp_end_foreign_access(gntdev_dmabuf->u.imp.refs, count);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  691  fail_unmap:
bf8dc55b Oleksandr Andrushchenko 2018-07-20  692  	dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  693  fail_detach:
bf8dc55b Oleksandr Andrushchenko 2018-07-20  694  	dma_buf_detach(dma_buf, attach);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  695  fail_free_obj:
bf8dc55b Oleksandr Andrushchenko 2018-07-20  696  	dmabuf_imp_free_storage(gntdev_dmabuf);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  697  fail_put:
bf8dc55b Oleksandr Andrushchenko 2018-07-20  698  	dma_buf_put(dma_buf);
bf8dc55b Oleksandr Andrushchenko 2018-07-20  699  	return ret;
932d6562 Oleksandr Andrushchenko 2018-07-20  700  }
932d6562 Oleksandr Andrushchenko 2018-07-20  701  

:::::: The code at line 634 was first introduced by commit
:::::: bf8dc55b135873d8bc58bb8fbc91c52f3a902eea xen/gntdev: Implement dma-buf import functionality

:::::: TO: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
:::::: CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel