[PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[Shuah Khan]

When coherent DMA memory without struct page is shared, importer
fails to find the page and runs into kernel page fault when it
tries to dmabuf_ops_attach/map_sg/map_page the invalid page found
in the sg_table. Please see www.spinics.net/lists/stable/msg164204.html
for more information on this problem.

This solution allows coherent DMA memory without struct page to be
shared by providing a way for the exporter to tag the DMA buffer as
a special buffer without struct page association and passing the
information in sg_table to the importer. This information is used
in attach/map_sg to avoid cleaning D-cache and mapping.

The details of the change are:

Framework:
- Add a new dma_attrs field to struct scatterlist.
- Add a new DMA_ATTR_DEV_COHERENT_NOPAGE attribute to clearly identify
  Coherent memory without struct page.
- Add a new dma_check_dev_coherent() interface to check if memory is
  the device coherent area. There is no way to tell where the memory
  returned by dma_alloc_attrs() came from.

Exporter logic:
- Add logic to vb2_dc_alloc() to call dma_check_dev_coherent() and set
  DMA_ATTR_DEV_COHERENT_NOPAGE based the results of the check. This is
  done in the exporter context.
- Add logic to arm_dma_get_sgtable() to identify memory without struct
  page using DMA_ATTR_DEV_COHERENT_NOPAGE attribute. If this attr is
  set, arm_dma_get_sgtable() will set page as the cpu_addr and update
  dma_address and dma_attrs fields in struct scatterlist for this sgl.
  This is done in exporter context when buffer is exported. With this
  Note: This change is made on top of Russell King's patch that added
  !pfn_valid(pfn) check to arm_dma_get_sgtable() to error out on invalid
  pages. Coherent memory without struct page will trigger this error.

Importer logic:
- Add logic to vb2_dc_dmabuf_ops_attach() to identify memory without
  struct page using DMA_ATTR_DEV_COHERENT_NOPAGE attribute when it copies
  the sg_table from the exporter. It will copy dma_attrs and dma_address
  fields. With this logic, dmabuf_ops_attach will no longer trip on an
  invalid page.
- Add logic to arm_dma_map_sg() to avoid mapping the page when sg_table
  has DMA_ATTR_DEV_COHERENT_NOPAGE buffer.
- Add logic to arm_dma_unmap_sg() to do nothing for sg entries with
  DMA_ATTR_DEV_COHERENT_NOPAGE attribute.

Without this change the following use-case that runs into kernel
pagefault when importer tries to attach the exported buffer.

With this change it works: (what a relief after watching pagefaults for
weeks!!)

gst-launch-1.0 filesrc location=~/GH3_MOV_HD.mp4 ! qtdemux ! h264parse ! v4l2video4dec capture-io-mode=dmabuf ! v4l2video7convert output-io-mode=dmabuf-import ! kmssink force-modesetting=true

I am sending RFC patch to get feedback on the approach and see if I missed
anything.

Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
---
 arch/arm/mm/dma-mapping.c                      | 34 ++++++++++++++++++++++----
 drivers/base/dma-coherent.c                    | 25 +++++++++++++++++++
 drivers/media/v4l2-core/videobuf2-dma-contig.c |  6 +++++
 include/linux/dma-mapping.h                    |  8 ++++++
 include/linux/scatterlist.h                    |  1 +
 5 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 63eabb0..75c6692 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -939,13 +939,28 @@ int arm_dma_get_sgtable(struct device *dev, struct sg_table *sgt,
 		 void *cpu_addr, dma_addr_t handle, size_t size,
 		 unsigned long attrs)
 {
-	struct page *page = pfn_to_page(dma_to_pfn(dev, handle));
+	unsigned long pfn = dma_to_pfn(dev, handle);
+	struct page *page;
 	int ret;
 
+	/* If the PFN is not valid, we do not have a struct page. */
+	if (!pfn_valid(pfn)) {
+		/* If memory is from per-device coherent area, use cpu_addr */
+		if (attrs & DMA_ATTR_DEV_COHERENT_NOPAGE)
+			page = cpu_addr;
+		else
+			return -ENXIO;
+	} else
+		page = pfn_to_page(pfn);
+
 	ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
 	if (unlikely(ret))
 		return ret;
 
+	if (attrs & DMA_ATTR_DEV_COHERENT_NOPAGE)
+		sgt->sgl->dma_address = handle;
+
+	sgt->sgl->dma_attrs = attrs;
 	sg_set_page(sgt->sgl, page, PAGE_ALIGN(size), 0);
 	return 0;
 }
@@ -1080,10 +1095,17 @@ int arm_dma_map_sg(struct device *dev, struct scatterlist *sg, int nents,
 #ifdef CONFIG_NEED_SG_DMA_LENGTH
 		s->dma_length = s->length;
 #endif
-		s->dma_address = ops->map_page(dev, sg_page(s), s->offset,
+		/*
+		 * there is no struct page for this DMA buffer.
+		 * s->dma_address is the handle
+		 */
+		if (!(s->dma_attrs & DMA_ATTR_DEV_COHERENT_NOPAGE)) {
+			s->dma_address = ops->map_page(dev, sg_page(s),
+						s->offset,
 						s->length, dir, attrs);
-		if (dma_mapping_error(dev, s->dma_address))
-			goto bad_mapping;
+			if (dma_mapping_error(dev, s->dma_address))
+				goto bad_mapping;
+		}
 	}
 	return nents;
 
@@ -1112,7 +1134,9 @@ void arm_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
 	int i;
 
 	for_each_sg(sg, s, nents, i)
-		ops->unmap_page(dev, sg_dma_address(s), sg_dma_len(s), dir, attrs);
+		if (!(s->dma_attrs & DMA_ATTR_DEV_COHERENT_NOPAGE))
+			ops->unmap_page(dev, sg_dma_address(s),
+					sg_dma_len(s), dir, attrs);
 }
 
 /**
diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c
index 640a7e6..d08cf44 100644
--- a/drivers/base/dma-coherent.c
+++ b/drivers/base/dma-coherent.c
@@ -209,6 +209,31 @@ int dma_alloc_from_coherent(struct device *dev, ssize_t size,
 EXPORT_SYMBOL(dma_alloc_from_coherent);
 
 /**
+ * dma_check_dev_coherent() - checks if memory is from the device coherent area
+ *
+ * @dev:	device whose coherent area is checked to validate memory
+ * @dma_handle:	dma handle associated with the allocated memory
+ * @vaddr:	the virtual address to the allocated area.
+ *
+ * Returns true if memory does belong to the per-device cohrent area.
+ * false otherwise.
+ */
+bool dma_check_dev_coherent(struct device *dev, dma_addr_t dma_handle,
+				void *vaddr)
+{
+	struct dma_coherent_mem *mem = dev ? dev->dma_mem : NULL;
+
+	if (mem && vaddr >= mem->virt_base &&
+	    vaddr < (mem->virt_base + (mem->size << PAGE_SHIFT)) &&
+	    dma_handle >= mem->device_base &&
+	    dma_handle < (mem->device_base + (mem->size << PAGE_SHIFT)))
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL(dma_check_dev_coherent);
+
+/**
  * dma_release_from_coherent() - try to free the memory allocated from per-device coherent memory pool
  * @dev:	device from which the memory was allocated
  * @order:	the order of pages allocated
diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index fb6a177..f7caf2b 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -161,6 +161,9 @@ static void *vb2_dc_alloc(struct device *dev, unsigned long attrs,
 	if ((buf->attrs & DMA_ATTR_NO_KERNEL_MAPPING) == 0)
 		buf->vaddr = buf->cookie;
 
+	if (dma_check_dev_coherent(dev, buf->dma_addr, buf->cookie))
+		buf->attrs |= DMA_ATTR_DEV_COHERENT_NOPAGE;
+
 	/* Prevent the device from being released while the buffer is used */
 	buf->dev = get_device(dev);
 	buf->size = size;
@@ -248,6 +251,9 @@ static int vb2_dc_dmabuf_ops_attach(struct dma_buf *dbuf, struct device *dev,
 	rd = buf->sgt_base->sgl;
 	wr = sgt->sgl;
 	for (i = 0; i < sgt->orig_nents; ++i) {
+		if (rd->dma_attrs & DMA_ATTR_DEV_COHERENT_NOPAGE)
+			wr->dma_address = rd->dma_address;
+		wr->dma_attrs = rd->dma_attrs;
 		sg_set_page(wr, sg_page(rd), rd->length, rd->offset);
 		rd = sg_next(rd);
 		wr = sg_next(wr);
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 0977317..9f3ec53 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -70,6 +70,12 @@
 #define DMA_ATTR_PRIVILEGED		(1UL << 9)
 
 /*
+ * DMA_ATTR_DEV_COHERENT_NOPAGE: This is a hint to the DMA-mapping sub-system
+ * that this memory isn't backed by struct page.
+ */
+#define DMA_ATTR_DEV_COHERENT_NOPAGE	(1UL << 10)
+
+/*
  * A dma_addr_t can hold any valid DMA or bus address for the platform.
  * It can be given to a device to use as a DMA source or target.  A CPU cannot
  * reference a dma_addr_t directly because there may be translation between
@@ -160,6 +166,8 @@ static inline int is_device_dma_capable(struct device *dev)
  */
 int dma_alloc_from_coherent(struct device *dev, ssize_t size,
 				       dma_addr_t *dma_handle, void **ret);
+bool dma_check_dev_coherent(struct device *dev, dma_addr_t dma_handle,
+				void *vaddr);
 int dma_release_from_coherent(struct device *dev, int order, void *vaddr);
 
 int dma_mmap_from_coherent(struct device *dev, struct vm_area_struct *vma,
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index cb3c8fe..7da610b 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -18,6 +18,7 @@ struct scatterlist {
 #ifdef CONFIG_NEED_SG_DMA_LENGTH
 	unsigned int	dma_length;
 #endif
+	unsigned long	dma_attrs;
 };
 
 /*
-- 
2.7.4

Re: [PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[Russell King - ARM Linux]

On Wed, Apr 05, 2017 at 10:02:42AM -0600, Shuah Khan wrote:
> When coherent DMA memory without struct page is shared, importer
> fails to find the page and runs into kernel page fault when it
> tries to dmabuf_ops_attach/map_sg/map_page the invalid page found
> in the sg_table. Please see www.spinics.net/lists/stable/msg164204.html
> for more information on this problem.
> 
> This solution allows coherent DMA memory without struct page to be
> shared by providing a way for the exporter to tag the DMA buffer as
> a special buffer without struct page association and passing the
> information in sg_table to the importer. This information is used
> in attach/map_sg to avoid cleaning D-cache and mapping.
> 
> The details of the change are:
> 
> Framework:
> - Add a new dma_attrs field to struct scatterlist.
> - Add a new DMA_ATTR_DEV_COHERENT_NOPAGE attribute to clearly identify
>   Coherent memory without struct page.
> - Add a new dma_check_dev_coherent() interface to check if memory is
>   the device coherent area. There is no way to tell where the memory
>   returned by dma_alloc_attrs() came from.
> 
> Exporter logic:
> - Add logic to vb2_dc_alloc() to call dma_check_dev_coherent() and set
>   DMA_ATTR_DEV_COHERENT_NOPAGE based the results of the check. This is
>   done in the exporter context.
> - Add logic to arm_dma_get_sgtable() to identify memory without struct
>   page using DMA_ATTR_DEV_COHERENT_NOPAGE attribute. If this attr is
>   set, arm_dma_get_sgtable() will set page as the cpu_addr and update
>   dma_address and dma_attrs fields in struct scatterlist for this sgl.
>   This is done in exporter context when buffer is exported. With this

This sentence appears to just end...

I'm not convinced that coherent allocations should be setting the "page"
of a scatterlist to anything that isn't a real struct page or NULL.  It
is, after all, an error to look up the virtual address etc of the
scatterlist entry or kmap it when it isn't backed by a struct page.

I'm actually already passing non-page backed memory through the DMA API
in armada-drm, although not entirely correctly, and etnaviv handles it
fine:

        } else if (dobj->linear) {
                /* Single contiguous physical region - no struct page */
                if (sg_alloc_table(sgt, 1, GFP_KERNEL))
                        goto free_sgt;
                sg_dma_address(sgt->sgl) = dobj->dev_addr;
                sg_dma_len(sgt->sgl) = dobj->obj.size;

This is not quite correct, as it assumes (which is safe for it currently)
that the DMA address is the same on all devices.  On Dove, which is where
this is used, that is the case, but it's not true elsewhere.  Also note
that I'm avoid calling dma_map_sg() and dma_unmap_sg() - there's no iommus
to be considered.

I'd suggest that this follows the same pattern - setting the DMA address
(more appropriately for generic code) and the DMA length, while leaving
the virtual address members NULL/0.  However, there's also the
complication of setting up any IOMMUs that would be necessary.  I haven't
looked at that, or how it could work.

I also think this should be documented in the dmabuf API that it can
pass such scatterlists that are DMA-parameter only.

Lastly, I'd recommend that anything using this does _not_ provide
functional kmap/kmap_atomic support for these - kmap and kmap_atomic
are both out because there's no struct page anyway (and their use would
probably oops the kernel in this scenario.)  I avoided mmap support in
armada drm, but if there's a pressing reason and real use case for the
importer to mmap() the buffers in userspace, it's something I could be
convinced of.

What I'm quite certain of is that we do _not_ want to be passing
coherent memory allocations into the streaming DMA API, not even with
a special attribute.  The DMA API is about gaining coherent memory
(shared ownership of the buffer), or mapping system memory to a
specified device (which can imply unique ownership.)  Trying to mix
the two together muddies the separation that we have there, and makes
it harder to explain.  As can be seen from this patch, we'd end up
needing to add this special DMA_ATTR_DEV_COHERENT_NOPAGE everywhere,
which is added complexity on top of stuff that is not required for
this circumstance.

I can see why you're doing it, to avoid having to duplicate more of
the generic code in drm_prime, but I don't think plasting over this
problem in arch code by adding this special flag is a particularly
good way forward.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6733 bytes --]

Hi Shuah,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc5]
[cannot apply to next-20170405]
[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/Shuah-Khan/arm-dma-fix-sharing-of-coherent-DMA-memory-without-struct-page/20170406-114717
config: x86_64-randconfig-x009-201714 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

   In file included from include/linux/file.h:8:0,
                    from include/linux/dma-buf.h:27,
                    from drivers/media/v4l2-core/videobuf2-dma-contig.c:13:
   drivers/media/v4l2-core/videobuf2-dma-contig.c: In function 'vb2_dc_alloc':
>> drivers/media/v4l2-core/videobuf2-dma-contig.c:164:6: error: implicit declaration of function 'dma_check_dev_coherent' [-Werror=implicit-function-declaration]
     if (dma_check_dev_coherent(dev, buf->dma_addr, buf->cookie))
         ^
   include/linux/compiler.h:160:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> drivers/media/v4l2-core/videobuf2-dma-contig.c:164:2: note: in expansion of macro 'if'
     if (dma_check_dev_coherent(dev, buf->dma_addr, buf->cookie))
     ^~
   cc1: some warnings being treated as errors

vim +/dma_check_dev_coherent +164 drivers/media/v4l2-core/videobuf2-dma-contig.c

     7	 *
     8	 * This program is free software; you can redistribute it and/or modify
     9	 * it under the terms of the GNU General Public License as published by
    10	 * the Free Software Foundation.
    11	 */
    12	
  > 13	#include <linux/dma-buf.h>
    14	#include <linux/module.h>
    15	#include <linux/scatterlist.h>
    16	#include <linux/sched.h>
    17	#include <linux/slab.h>
    18	#include <linux/dma-mapping.h>
    19	
    20	#include <media/videobuf2-v4l2.h>
    21	#include <media/videobuf2-dma-contig.h>
    22	#include <media/videobuf2-memops.h>
    23	
    24	struct vb2_dc_buf {
    25		struct device			*dev;
    26		void				*vaddr;
    27		unsigned long			size;
    28		void				*cookie;
    29		dma_addr_t			dma_addr;
    30		unsigned long			attrs;
    31		enum dma_data_direction		dma_dir;
    32		struct sg_table			*dma_sgt;
    33		struct frame_vector		*vec;
    34	
    35		/* MMAP related */
    36		struct vb2_vmarea_handler	handler;
    37		atomic_t			refcount;
    38		struct sg_table			*sgt_base;
    39	
    40		/* DMABUF related */
    41		struct dma_buf_attachment	*db_attach;
    42	};
    43	
    44	/*********************************************/
    45	/*        scatterlist table functions        */
    46	/*********************************************/
    47	
    48	static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)
    49	{
    50		struct scatterlist *s;
    51		dma_addr_t expected = sg_dma_address(sgt->sgl);
    52		unsigned int i;
    53		unsigned long size = 0;
    54	
    55		for_each_sg(sgt->sgl, s, sgt->nents, i) {
    56			if (sg_dma_address(s) != expected)
    57				break;
    58			expected = sg_dma_address(s) + sg_dma_len(s);
    59			size += sg_dma_len(s);
    60		}
    61		return size;
    62	}
    63	
    64	/*********************************************/
    65	/*         callbacks for all buffers         */
    66	/*********************************************/
    67	
    68	static void *vb2_dc_cookie(void *buf_priv)
    69	{
    70		struct vb2_dc_buf *buf = buf_priv;
    71	
    72		return &buf->dma_addr;
    73	}
    74	
    75	static void *vb2_dc_vaddr(void *buf_priv)
    76	{
    77		struct vb2_dc_buf *buf = buf_priv;
    78	
    79		if (!buf->vaddr && buf->db_attach)
    80			buf->vaddr = dma_buf_vmap(buf->db_attach->dmabuf);
    81	
    82		return buf->vaddr;
    83	}
    84	
    85	static unsigned int vb2_dc_num_users(void *buf_priv)
    86	{
    87		struct vb2_dc_buf *buf = buf_priv;
    88	
    89		return atomic_read(&buf->refcount);
    90	}
    91	
    92	static void vb2_dc_prepare(void *buf_priv)
    93	{
    94		struct vb2_dc_buf *buf = buf_priv;
    95		struct sg_table *sgt = buf->dma_sgt;
    96	
    97		/* DMABUF exporter will flush the cache for us */
    98		if (!sgt || buf->db_attach)
    99			return;
   100	
   101		dma_sync_sg_for_device(buf->dev, sgt->sgl, sgt->orig_nents,
   102				       buf->dma_dir);
   103	}
   104	
   105	static void vb2_dc_finish(void *buf_priv)
   106	{
   107		struct vb2_dc_buf *buf = buf_priv;
   108		struct sg_table *sgt = buf->dma_sgt;
   109	
   110		/* DMABUF exporter will flush the cache for us */
   111		if (!sgt || buf->db_attach)
   112			return;
   113	
   114		dma_sync_sg_for_cpu(buf->dev, sgt->sgl, sgt->orig_nents, buf->dma_dir);
   115	}
   116	
   117	/*********************************************/
   118	/*        callbacks for MMAP buffers         */
   119	/*********************************************/
   120	
   121	static void vb2_dc_put(void *buf_priv)
   122	{
   123		struct vb2_dc_buf *buf = buf_priv;
   124	
   125		if (!atomic_dec_and_test(&buf->refcount))
   126			return;
   127	
   128		if (buf->sgt_base) {
   129			sg_free_table(buf->sgt_base);
   130			kfree(buf->sgt_base);
   131		}
   132		dma_free_attrs(buf->dev, buf->size, buf->cookie, buf->dma_addr,
   133			       buf->attrs);
   134		put_device(buf->dev);
   135		kfree(buf);
   136	}
   137	
   138	static void *vb2_dc_alloc(struct device *dev, unsigned long attrs,
   139				  unsigned long size, enum dma_data_direction dma_dir,
   140				  gfp_t gfp_flags)
   141	{
   142		struct vb2_dc_buf *buf;
   143	
   144		if (WARN_ON(!dev))
   145			return ERR_PTR(-EINVAL);
   146	
   147		buf = kzalloc(sizeof *buf, GFP_KERNEL);
   148		if (!buf)
   149			return ERR_PTR(-ENOMEM);
   150	
   151		if (attrs)
   152			buf->attrs = attrs;
   153		buf->cookie = dma_alloc_attrs(dev, size, &buf->dma_addr,
   154						GFP_KERNEL | gfp_flags, buf->attrs);
   155		if (!buf->cookie) {
   156			dev_err(dev, "dma_alloc_coherent of size %ld failed\n", size);
   157			kfree(buf);
   158			return ERR_PTR(-ENOMEM);
   159		}
   160	
   161		if ((buf->attrs & DMA_ATTR_NO_KERNEL_MAPPING) == 0)
   162			buf->vaddr = buf->cookie;
   163	
 > 164		if (dma_check_dev_coherent(dev, buf->dma_addr, buf->cookie))
   165			buf->attrs |= DMA_ATTR_DEV_COHERENT_NOPAGE;
   166	
   167		/* Prevent the device from being released while the buffer is used */

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27768 bytes --]

Re: [PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[Marek Szyprowski]

Hi Shuah,

On 2017-04-05 18:02, Shuah Khan wrote:
> When coherent DMA memory without struct page is shared, importer
> fails to find the page and runs into kernel page fault when it
> tries to dmabuf_ops_attach/map_sg/map_page the invalid page found
> in the sg_table. Please see www.spinics.net/lists/stable/msg164204.html
> for more information on this problem.
>
> This solution allows coherent DMA memory without struct page to be
> shared by providing a way for the exporter to tag the DMA buffer as
> a special buffer without struct page association and passing the
> information in sg_table to the importer. This information is used
> in attach/map_sg to avoid cleaning D-cache and mapping.
>
> The details of the change are:
>
> Framework:
> - Add a new dma_attrs field to struct scatterlist.
> - Add a new DMA_ATTR_DEV_COHERENT_NOPAGE attribute to clearly identify
>    Coherent memory without struct page.
> - Add a new dma_check_dev_coherent() interface to check if memory is
>    the device coherent area. There is no way to tell where the memory
>    returned by dma_alloc_attrs() came from.
>
> Exporter logic:
> - Add logic to vb2_dc_alloc() to call dma_check_dev_coherent() and set
>    DMA_ATTR_DEV_COHERENT_NOPAGE based the results of the check. This is
>    done in the exporter context.
> - Add logic to arm_dma_get_sgtable() to identify memory without struct
>    page using DMA_ATTR_DEV_COHERENT_NOPAGE attribute. If this attr is
>    set, arm_dma_get_sgtable() will set page as the cpu_addr and update
>    dma_address and dma_attrs fields in struct scatterlist for this sgl.
>    This is done in exporter context when buffer is exported. With this
>    Note: This change is made on top of Russell King's patch that added
>    !pfn_valid(pfn) check to arm_dma_get_sgtable() to error out on invalid
>    pages. Coherent memory without struct page will trigger this error.
>
> Importer logic:
> - Add logic to vb2_dc_dmabuf_ops_attach() to identify memory without
>    struct page using DMA_ATTR_DEV_COHERENT_NOPAGE attribute when it copies
>    the sg_table from the exporter. It will copy dma_attrs and dma_address
>    fields. With this logic, dmabuf_ops_attach will no longer trip on an
>    invalid page.
> - Add logic to arm_dma_map_sg() to avoid mapping the page when sg_table
>    has DMA_ATTR_DEV_COHERENT_NOPAGE buffer.
> - Add logic to arm_dma_unmap_sg() to do nothing for sg entries with
>    DMA_ATTR_DEV_COHERENT_NOPAGE attribute.
>
> Without this change the following use-case that runs into kernel
> pagefault when importer tries to attach the exported buffer.
>
> With this change it works: (what a relief after watching pagefaults for
> weeks!!)
>
> gst-launch-1.0 filesrc location=~/GH3_MOV_HD.mp4 ! qtdemux ! h264parse ! v4l2video4dec capture-io-mode=dmabuf ! v4l2video7convert output-io-mode=dmabuf-import ! kmssink force-modesetting=true
>
> I am sending RFC patch to get feedback on the approach and see if I missed
> anything.

Frankly, once You decided to hack around dma-buf and issues with coherent,
carved out memory, it might be a bit better to find the ultimate solution
instead of the another hack. Please note that it will still not allow to
share a buffer allocated from carved-out memory and a device, which is
behind IOMMU.

I thought a bit about this and the current shape of dma-buf code.

IMHO the proper way of solving all those issues would be to replace
dma-buf internal representation of the memory from struct scatter_list
to pfn array. This would really solve the problem of buffers which
cannot be properly represented by scatter lists/struct pages and would
even allow sharing buffers between all kinds of devices. Scatter-lists
are also quite over-engineered structures to represent a single buffer
(pfn array is a bit more compact representation). Also there is a lots
of buggy code which use scatter-list in a bit creative way (like
assuming that each page maps to a single scatter list entry for
example). The only missing piece, required for such change would be
extending DMA-mapping with dma_map_pfn() interface.

This would be however quite large task, especially taking into account
all current users of DMA-buf framework...

> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
> ---
>   arch/arm/mm/dma-mapping.c                      | 34 ++++++++++++++++++++++----
>   drivers/base/dma-coherent.c                    | 25 +++++++++++++++++++
>   drivers/media/v4l2-core/videobuf2-dma-contig.c |  6 +++++
>   include/linux/dma-mapping.h                    |  8 ++++++
>   include/linux/scatterlist.h                    |  1 +
>   5 files changed, 69 insertions(+), 5 deletions(-)
 > [...]

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[Shuah Khan]

On 04/05/2017 05:14 PM, Russell King - ARM Linux wrote:
> On Wed, Apr 05, 2017 at 10:02:42AM -0600, Shuah Khan wrote:
>> When coherent DMA memory without struct page is shared, importer
>> fails to find the page and runs into kernel page fault when it
>> tries to dmabuf_ops_attach/map_sg/map_page the invalid page found
>> in the sg_table. Please see www.spinics.net/lists/stable/msg164204.html
>> for more information on this problem.
>>
>> This solution allows coherent DMA memory without struct page to be
>> shared by providing a way for the exporter to tag the DMA buffer as
>> a special buffer without struct page association and passing the
>> information in sg_table to the importer. This information is used
>> in attach/map_sg to avoid cleaning D-cache and mapping.
>>
>> The details of the change are:
>>
>> Framework:
>> - Add a new dma_attrs field to struct scatterlist.
>> - Add a new DMA_ATTR_DEV_COHERENT_NOPAGE attribute to clearly identify
>>   Coherent memory without struct page.
>> - Add a new dma_check_dev_coherent() interface to check if memory is
>>   the device coherent area. There is no way to tell where the memory
>>   returned by dma_alloc_attrs() came from.
>>
>> Exporter logic:
>> - Add logic to vb2_dc_alloc() to call dma_check_dev_coherent() and set
>>   DMA_ATTR_DEV_COHERENT_NOPAGE based the results of the check. This is
>>   done in the exporter context.
>> - Add logic to arm_dma_get_sgtable() to identify memory without struct
>>   page using DMA_ATTR_DEV_COHERENT_NOPAGE attribute. If this attr is
>>   set, arm_dma_get_sgtable() will set page as the cpu_addr and update
>>   dma_address and dma_attrs fields in struct scatterlist for this sgl.
>>   This is done in exporter context when buffer is exported. With this
> 
> This sentence appears to just end...
> 
> I'm not convinced that coherent allocations should be setting the "page"
> of a scatterlist to anything that isn't a real struct page or NULL.  It
> is, after all, an error to look up the virtual address etc of the
> scatterlist entry or kmap it when it isn't backed by a struct page.
> 
> I'm actually already passing non-page backed memory through the DMA API
> in armada-drm, although not entirely correctly, and etnaviv handles it
> fine:
> 
>         } else if (dobj->linear) {
>                 /* Single contiguous physical region - no struct page */
>                 if (sg_alloc_table(sgt, 1, GFP_KERNEL))
>                         goto free_sgt;
>                 sg_dma_address(sgt->sgl) = dobj->dev_addr;
>                 sg_dma_len(sgt->sgl) = dobj->obj.size;
> 
> This is not quite correct, as it assumes (which is safe for it currently)
> that the DMA address is the same on all devices.  On Dove, which is where
> this is used, that is the case, but it's not true elsewhere.  Also note
> that I'm avoid calling dma_map_sg() and dma_unmap_sg() - there's no iommus
> to be considered.

I see. That is not the case for the drivers involved in my use-case. exynos
has iommu and this s5p-mfc exporting buffers to exynos-gsc use-case does
work when iommu is enabled.

> 
> I'd suggest that this follows the same pattern - setting the DMA address
> (more appropriately for generic code) and the DMA length, while leaving
> the virtual address members NULL/0.  However, there's also the
> complication of setting up any IOMMUs that would be necessary.  I haven't
> looked at that, or how it could work.
> 
> I also think this should be documented in the dmabuf API that it can
> pass such scatterlists that are DMA-parameter only.
> 
> Lastly, I'd recommend that anything using this does _not_ provide
> functional kmap/kmap_atomic support for these - kmap and kmap_atomic
> are both out because there's no struct page anyway (and their use would
> probably oops the kernel in this scenario.)  I avoided mmap support in
> armada drm, but if there's a pressing reason and real use case for the
> importer to mmap() the buffers in userspace, it's something I could be
> convinced of.
> 
> What I'm quite certain of is that we do _not_ want to be passing
> coherent memory allocations into the streaming DMA API, not even with
> a special attribute.  The DMA API is about gaining coherent memory
> (shared ownership of the buffer), or mapping system memory to a
> specified device (which can imply unique ownership.)  Trying to mix
> the two together muddies the separation that we have there, and makes
> it harder to explain.  As can be seen from this patch, we'd end up
> needing to add this special DMA_ATTR_DEV_COHERENT_NOPAGE everywhere,
> which is added complexity on top of stuff that is not required for
> this circumstance.

The ownership can be tricky as you mentioned. In this particular use-case,
there is a clear ownership definition because of the way v4l2 export/import
works and also the qbuf/dqbuf rules. However, there might be other use-cases
ownership isn't clearly established.

> 
> I can see why you're doing it, to avoid having to duplicate more of
> the generic code in drm_prime, but I don't think plasting over this
> problem in arch code by adding this special flag is a particularly
> good way forward.
> 

Right. I went with this approach to avoid duplicating the code. It does
come with the complexity of needing to check the attribute in a few
places.

With the current code, we still have the issue of pagefault. Your patch
that adds a check for invalid doesn't cover all cases.

My goal behind this patch is two fold. 1. Fix the pagefault with a
definitive test and 2. see if per-device coherent memory can be passed
through.

The first goal is still worth while. Would it be reasonable to use
dma_check_dev_coherent() to test for this case in arm_dma_get_sgtable()
or even from dma_get_sgtable_attrs() and fail early? This will avoid
false negatives with the invalid page test. If this sounds reasonable,
I can spin this work to do that instead.

thanks,
-- Shuah

Re: [PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[Shuah Khan]

On 04/06/2017 06:01 AM, Marek Szyprowski wrote:
> Hi Shuah,
> 
> On 2017-04-05 18:02, Shuah Khan wrote:
>> When coherent DMA memory without struct page is shared, importer
>> fails to find the page and runs into kernel page fault when it
>> tries to dmabuf_ops_attach/map_sg/map_page the invalid page found
>> in the sg_table. Please see www.spinics.net/lists/stable/msg164204.html
>> for more information on this problem.
>>
>> This solution allows coherent DMA memory without struct page to be
>> shared by providing a way for the exporter to tag the DMA buffer as
>> a special buffer without struct page association and passing the
>> information in sg_table to the importer. This information is used
>> in attach/map_sg to avoid cleaning D-cache and mapping.
>>
>> The details of the change are:
>>
>> Framework:
>> - Add a new dma_attrs field to struct scatterlist.
>> - Add a new DMA_ATTR_DEV_COHERENT_NOPAGE attribute to clearly identify
>>    Coherent memory without struct page.
>> - Add a new dma_check_dev_coherent() interface to check if memory is
>>    the device coherent area. There is no way to tell where the memory
>>    returned by dma_alloc_attrs() came from.
>>
>> Exporter logic:
>> - Add logic to vb2_dc_alloc() to call dma_check_dev_coherent() and set
>>    DMA_ATTR_DEV_COHERENT_NOPAGE based the results of the check. This is
>>    done in the exporter context.
>> - Add logic to arm_dma_get_sgtable() to identify memory without struct
>>    page using DMA_ATTR_DEV_COHERENT_NOPAGE attribute. If this attr is
>>    set, arm_dma_get_sgtable() will set page as the cpu_addr and update
>>    dma_address and dma_attrs fields in struct scatterlist for this sgl.
>>    This is done in exporter context when buffer is exported. With this
>>    Note: This change is made on top of Russell King's patch that added
>>    !pfn_valid(pfn) check to arm_dma_get_sgtable() to error out on invalid
>>    pages. Coherent memory without struct page will trigger this error.
>>
>> Importer logic:
>> - Add logic to vb2_dc_dmabuf_ops_attach() to identify memory without
>>    struct page using DMA_ATTR_DEV_COHERENT_NOPAGE attribute when it copies
>>    the sg_table from the exporter. It will copy dma_attrs and dma_address
>>    fields. With this logic, dmabuf_ops_attach will no longer trip on an
>>    invalid page.
>> - Add logic to arm_dma_map_sg() to avoid mapping the page when sg_table
>>    has DMA_ATTR_DEV_COHERENT_NOPAGE buffer.
>> - Add logic to arm_dma_unmap_sg() to do nothing for sg entries with
>>    DMA_ATTR_DEV_COHERENT_NOPAGE attribute.
>>
>> Without this change the following use-case that runs into kernel
>> pagefault when importer tries to attach the exported buffer.
>>
>> With this change it works: (what a relief after watching pagefaults for
>> weeks!!)
>>
>> gst-launch-1.0 filesrc location=~/GH3_MOV_HD.mp4 ! qtdemux ! h264parse ! v4l2video4dec capture-io-mode=dmabuf ! v4l2video7convert output-io-mode=dmabuf-import ! kmssink force-modesetting=true
>>
>> I am sending RFC patch to get feedback on the approach and see if I missed
>> anything.
> 
> Frankly, once You decided to hack around dma-buf and issues with coherent,
> carved out memory, it might be a bit better to find the ultimate solution
> instead of the another hack. Please note that it will still not allow to
> share a buffer allocated from carved-out memory and a device, which is
> behind IOMMU.

With your patch s5p-mfc patch series does address the problem for this
use-case for 4.12 onwards. However I am still concerned about prior
release and this pagefault is bad.

Invalid page test partially solves the problem. Would it helpful to
at least prevent the pagfault with a definitive test. Please see my
response to Russell. Let me know your thoughts on that.

> 
> I thought a bit about this and the current shape of dma-buf code.
> 
> IMHO the proper way of solving all those issues would be to replace
> dma-buf internal representation of the memory from struct scatter_list
> to pfn array. This would really solve the problem of buffers which
> cannot be properly represented by scatter lists/struct pages and would
> even allow sharing buffers between all kinds of devices. Scatter-lists
> are also quite over-engineered structures to represent a single buffer
> (pfn array is a bit more compact representation). Also there is a lots
> of buggy code which use scatter-list in a bit creative way (like
> assuming that each page maps to a single scatter list entry for
> example). The only missing piece, required for such change would be
> extending DMA-mapping with dma_map_pfn() interface.

I agree with you on scatterlists being clumsy. Changing over to pfn array
could simplify things. I am exploring a slightly different option that
might not require too many changes. I will respond with concrete ideas
later on this week.

> 
> This would be however quite large task, especially taking into account
> all current users of DMA-buf framework...

Yeah it will be a large task.

thanks,
-- Shuah

> 
>> Signed-off-by: Shuah Khan <shuahkh@osg.samsung.com>
>> ---
>>   arch/arm/mm/dma-mapping.c                      | 34 ++++++++++++++++++++++----
>>   drivers/base/dma-coherent.c                    | 25 +++++++++++++++++++
>>   drivers/media/v4l2-core/videobuf2-dma-contig.c |  6 +++++
>>   include/linux/dma-mapping.h                    |  8 ++++++
>>   include/linux/scatterlist.h                    |  1 +
>>   5 files changed, 69 insertions(+), 5 deletions(-)
>> [...]
> 
> Best regards

Re: [PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[Marek Szyprowski]

Hi Shuah,

On 2017-04-11 00:50, Shuah Khan wrote:
> On 04/06/2017 06:01 AM, Marek Szyprowski wrote:
>> On 2017-04-05 18:02, Shuah Khan wrote:
>>> When coherent DMA memory without struct page is shared, importer
>>> fails to find the page and runs into kernel page fault when it
>>> tries to dmabuf_ops_attach/map_sg/map_page the invalid page found
>>> in the sg_table. Please see www.spinics.net/lists/stable/msg164204.html
>>> for more information on this problem.
>>>
>>> This solution allows coherent DMA memory without struct page to be
>>> shared by providing a way for the exporter to tag the DMA buffer as
>>> a special buffer without struct page association and passing the
>>> information in sg_table to the importer. This information is used
>>> in attach/map_sg to avoid cleaning D-cache and mapping.
>>>
>>> The details of the change are:
>>>
>>> Framework:
>>> - Add a new dma_attrs field to struct scatterlist.
>>> - Add a new DMA_ATTR_DEV_COHERENT_NOPAGE attribute to clearly identify
>>>     Coherent memory without struct page.
>>> - Add a new dma_check_dev_coherent() interface to check if memory is
>>>     the device coherent area. There is no way to tell where the memory
>>>     returned by dma_alloc_attrs() came from.
>>>
>>> Exporter logic:
>>> - Add logic to vb2_dc_alloc() to call dma_check_dev_coherent() and set
>>>     DMA_ATTR_DEV_COHERENT_NOPAGE based the results of the check. This is
>>>     done in the exporter context.
>>> - Add logic to arm_dma_get_sgtable() to identify memory without struct
>>>     page using DMA_ATTR_DEV_COHERENT_NOPAGE attribute. If this attr is
>>>     set, arm_dma_get_sgtable() will set page as the cpu_addr and update
>>>     dma_address and dma_attrs fields in struct scatterlist for this sgl.
>>>     This is done in exporter context when buffer is exported. With this
>>>     Note: This change is made on top of Russell King's patch that added
>>>     !pfn_valid(pfn) check to arm_dma_get_sgtable() to error out on invalid
>>>     pages. Coherent memory without struct page will trigger this error.
>>>
>>> Importer logic:
>>> - Add logic to vb2_dc_dmabuf_ops_attach() to identify memory without
>>>     struct page using DMA_ATTR_DEV_COHERENT_NOPAGE attribute when it copies
>>>     the sg_table from the exporter. It will copy dma_attrs and dma_address
>>>     fields. With this logic, dmabuf_ops_attach will no longer trip on an
>>>     invalid page.
>>> - Add logic to arm_dma_map_sg() to avoid mapping the page when sg_table
>>>     has DMA_ATTR_DEV_COHERENT_NOPAGE buffer.
>>> - Add logic to arm_dma_unmap_sg() to do nothing for sg entries with
>>>     DMA_ATTR_DEV_COHERENT_NOPAGE attribute.
>>>
>>> Without this change the following use-case that runs into kernel
>>> pagefault when importer tries to attach the exported buffer.
>>>
>>> With this change it works: (what a relief after watching pagefaults for
>>> weeks!!)
>>>
>>> gst-launch-1.0 filesrc location=~/GH3_MOV_HD.mp4 ! qtdemux ! h264parse ! v4l2video4dec capture-io-mode=dmabuf ! v4l2video7convert output-io-mode=dmabuf-import ! kmssink force-modesetting=true
>>>
>>> I am sending RFC patch to get feedback on the approach and see if I missed
>>> anything.
>> Frankly, once You decided to hack around dma-buf and issues with coherent,
>> carved out memory, it might be a bit better to find the ultimate solution
>> instead of the another hack. Please note that it will still not allow to
>> share a buffer allocated from carved-out memory and a device, which is
>> behind IOMMU.
> With your patch s5p-mfc patch series does address the problem for this
> use-case for 4.12 onwards. However I am still concerned about prior
> release and this pagefault is bad.

Right. It should simply fail with error code instead of pagefault.

> Invalid page test partially solves the problem. Would it helpful to
> at least prevent the pagfault with a definitive test. Please see my
> response to Russell. Let me know your thoughts on that.
>
>> I thought a bit about this and the current shape of dma-buf code.
>>
>> IMHO the proper way of solving all those issues would be to replace
>> dma-buf internal representation of the memory from struct scatter_list
>> to pfn array. This would really solve the problem of buffers which
>> cannot be properly represented by scatter lists/struct pages and would
>> even allow sharing buffers between all kinds of devices. Scatter-lists
>> are also quite over-engineered structures to represent a single buffer
>> (pfn array is a bit more compact representation). Also there is a lots
>> of buggy code which use scatter-list in a bit creative way (like
>> assuming that each page maps to a single scatter list entry for
>> example). The only missing piece, required for such change would be
>> extending DMA-mapping with dma_map_pfn() interface.
> I agree with you on scatterlists being clumsy. Changing over to pfn array
> could simplify things. I am exploring a slightly different option that
> might not require too many changes. I will respond with concrete ideas
> later on this week.

It looks that a similar issue is being worked on, see the following thread:
https://lkml.org/lkml/2017/4/13/710

>> This would be however quite large task, especially taking into account
>> all current users of DMA-buf framework...
> Yeah it will be a large task.

Maybe once scatterlist are switched to pfns, changing dmabuf internal
memory representation to pfn array might be much easier.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[Russell King - ARM Linux]

On Fri, Apr 14, 2017 at 09:56:07AM +0200, Marek Szyprowski wrote:
> >>This would be however quite large task, especially taking into account
> >>all current users of DMA-buf framework...
> >Yeah it will be a large task.
> 
> Maybe once scatterlist are switched to pfns, changing dmabuf internal
> memory representation to pfn array might be much easier.

Switching to a PFN array won't work either as we have no cross-arch
way to translate PFNs to a DMA address and vice versa.  Yes, we have
them in ARM, but they are an _implementation detail_ of ARM's
DMA API support, they are not for use by drivers.

So, the very first problem that needs solving is this:

  How do we go from a coherent DMA allocation for device X to a set
  of DMA addresses for device Y.

Essentially, we need a way of remapping the DMA buffer for use with
another device, and returning a DMA address suitable for that device.
This could well mean that we need to deal with setting up an IOMMU
mapping.  My guess is that this needs to happen at the DMA coherent
API level - the DMA coherent API needs to be augmented with support
for this.  I'll call this "DMA coherent remap".

We then need to think about how to pass this through the dma-buf API.
dma_map_sg() is done by the exporter, who should know what kind of
memory is being exported.  The exporter can avoid calling dma_map_sg()
if it knows in advance that it is exporting DMA coherent memory.
Instead, the exporter can simply create a scatterlist with the DMA
address and DMA length prepopulated with the results of the DMA
coherent remap operation above.

What the scatterlist can't carry in this case is a set of valid
struct page pointers, and an importer must not walk the scatterlist
expecting to get at the virtual address parameters or struct page
pointers.

On the mmap() side of things, remember that DMA coherent allocations
may require special mapping into userspace, and which can only be
mapped by the DMA coherent mmap support.  kmap etc will also need to
be different.  So it probably makes sense for DMA coherent dma-buf
exports to use a completely separate set of dma_buf_ops from the
streaming version.

I think this is the easiest approach to solving the problem without
needing massive driver changes all over the kernel.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[Shuah Khan]

On 04/14/2017 03:46 AM, Russell King - ARM Linux wrote:
> On Fri, Apr 14, 2017 at 09:56:07AM +0200, Marek Szyprowski wrote:
>>>> This would be however quite large task, especially taking into account
>>>> all current users of DMA-buf framework...
>>> Yeah it will be a large task.
>>
>> Maybe once scatterlist are switched to pfns, changing dmabuf internal
>> memory representation to pfn array might be much easier.
> 
> Switching to a PFN array won't work either as we have no cross-arch
> way to translate PFNs to a DMA address and vice versa.  Yes, we have
> them in ARM, but they are an _implementation detail_ of ARM's
> DMA API support, they are not for use by drivers.
> 
> So, the very first problem that needs solving is this:
> 
>   How do we go from a coherent DMA allocation for device X to a set
>   of DMA addresses for device Y.
> 
> Essentially, we need a way of remapping the DMA buffer for use with
> another device, and returning a DMA address suitable for that device.
> This could well mean that we need to deal with setting up an IOMMU
> mapping.  My guess is that this needs to happen at the DMA coherent
> API level - the DMA coherent API needs to be augmented with support
> for this.  I'll call this "DMA coherent remap".
> 
> We then need to think about how to pass this through the dma-buf API.
> dma_map_sg() is done by the exporter, who should know what kind of
> memory is being exported.  The exporter can avoid calling dma_map_sg()
> if it knows in advance that it is exporting DMA coherent memory.
> Instead, the exporter can simply create a scatterlist with the DMA
> address and DMA length prepopulated with the results of the DMA
> coherent remap operation above.

The only way to conclusively say that it is coming from coherent area
is at the time it is getting allocated in dma_alloc_from_coherent().
Since dma_alloc_attrs() will go on to find memory from other areas if
dma_alloc_from_coherent() doesn't allocate memory.

dma_get_sgtable_attrs() is what is used by the exporter to create the
sg_table. One way to do this cleanly without needing to check buffer
type flags would be to add a set of sg_table ops: get_sgtable,
map_sg, and unmap_sg. Sounds like sg_table interfaces need to be in
dma_buf_ops level. More below.

> 
> What the scatterlist can't carry in this case is a set of valid
> struct page pointers, and an importer must not walk the scatterlist
> expecting to get at the virtual address parameters or struct page
> pointers.
> 
> On the mmap() side of things, remember that DMA coherent allocations
> may require special mapping into userspace, and which can only be
> mapped by the DMA coherent mmap support.  kmap etc will also need to
> be different.  So it probably makes sense for DMA coherent dma-buf
> exports to use a completely separate set of dma_buf_ops from the
> streaming version.

How about adding get_sgtable, map_sg, unmap_sg to dma_buf_ops. The right
ops need to be installed based on buffer type. As I mentioned before, we
don't know which memory we got until dma_alloc_from_coherent() finds
memory in dev->mem area. So how about using the dma_check_dev_coherent()
to determine which ops we need. These could be set based on buffer type.
vb2_dc_get_dmabuf() can do that.

I think this will work.

thanks,
-- Shuah

Re: [PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[Russell King - ARM Linux]

On Sun, Apr 16, 2017 at 07:10:21PM -0600, Shuah Khan wrote:
> On 04/14/2017 03:46 AM, Russell King - ARM Linux wrote:
> > On Fri, Apr 14, 2017 at 09:56:07AM +0200, Marek Szyprowski wrote:
> >>>> This would be however quite large task, especially taking into account
> >>>> all current users of DMA-buf framework...
> >>> Yeah it will be a large task.
> >>
> >> Maybe once scatterlist are switched to pfns, changing dmabuf internal
> >> memory representation to pfn array might be much easier.
> > 
> > Switching to a PFN array won't work either as we have no cross-arch
> > way to translate PFNs to a DMA address and vice versa.  Yes, we have
> > them in ARM, but they are an _implementation detail_ of ARM's
> > DMA API support, they are not for use by drivers.
> > 
> > So, the very first problem that needs solving is this:
> > 
> >   How do we go from a coherent DMA allocation for device X to a set
> >   of DMA addresses for device Y.
> > 
> > Essentially, we need a way of remapping the DMA buffer for use with
> > another device, and returning a DMA address suitable for that device.
> > This could well mean that we need to deal with setting up an IOMMU
> > mapping.  My guess is that this needs to happen at the DMA coherent
> > API level - the DMA coherent API needs to be augmented with support
> > for this.  I'll call this "DMA coherent remap".
> > 
> > We then need to think about how to pass this through the dma-buf API.
> > dma_map_sg() is done by the exporter, who should know what kind of
> > memory is being exported.  The exporter can avoid calling dma_map_sg()
> > if it knows in advance that it is exporting DMA coherent memory.
> > Instead, the exporter can simply create a scatterlist with the DMA
> > address and DMA length prepopulated with the results of the DMA
> > coherent remap operation above.
> 
> The only way to conclusively say that it is coming from coherent area
> is at the time it is getting allocated in dma_alloc_from_coherent().
> Since dma_alloc_attrs() will go on to find memory from other areas if
> dma_alloc_from_coherent() doesn't allocate memory.

Sorry, I disagree.

The only thing that matters is "did this memory come from
dma_alloc_coherent()".  It doesn't matter where dma_alloc_coherent()
ultimately got the memory from, it's memory from the coherent allocator
interface, and it should not be passed back into the streaming APIs.
It is, after all, DMA _coherent_ memory, passing it into the streaming
APIs which is for DMA _noncoherent_ memory is insane - the streaming APIs
can bring extra expensive cache flushes, which are not required for
DMA _coherent_ memory.

The exporter should know where it got the memory from.  It's really not
sane for anyone except the _original_ allocator to be exporting memory
through a DMA buffer - only the original allocator knows the properties
of that memory, and how to map it, whether that be for DMA, kmap or
mmap.

If a dmabuf is imported into a driver and then re-exported, the original
dmabuf should be what is re-exported, not some creation of the driver -
the re-exporting driver can't know what the properties of the memory
backing the dmabuf are, so anything else is just insane.

> How about adding get_sgtable, map_sg, unmap_sg to dma_buf_ops. The right
> ops need to be installed based on buffer type. As I mentioned before, we
> don't know which memory we got until dma_alloc_from_coherent() finds
> memory in dev->mem area. So how about using the dma_check_dev_coherent()
> to determine which ops we need. These could be set based on buffer type.
> vb2_dc_get_dmabuf() can do that.

Given my statement above, I don't believe any of that is necessary.  All
memory allocated from dma_alloc_coherent() is DMA coherent.  So, if
memory was obtained from dma_alloc_coherent() or similar, then it must
not be passed to the streaming DMA API.  It doesn't matter where it
ultimately came from.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] arm: dma: fix sharing of coherent DMA memory without struct page

[Shuah Khan]

Hi Russell, and Marek,

On 04/14/2017 03:46 AM, Russell King - ARM Linux wrote:
> On Fri, Apr 14, 2017 at 09:56:07AM +0200, Marek Szyprowski wrote:
>>>> This would be however quite large task, especially taking into account
>>>> all current users of DMA-buf framework...
>>> Yeah it will be a large task.
>>
>> Maybe once scatterlist are switched to pfns, changing dmabuf internal
>> memory representation to pfn array might be much easier.
> 
> Switching to a PFN array won't work either as we have no cross-arch
> way to translate PFNs to a DMA address and vice versa.  Yes, we have
> them in ARM, but they are an _implementation detail_ of ARM's
> DMA API support, they are not for use by drivers.
> 
> So, the very first problem that needs solving is this:
> 
>   How do we go from a coherent DMA allocation for device X to a set
>   of DMA addresses for device Y.
> 
> Essentially, we need a way of remapping the DMA buffer for use with
> another device, and returning a DMA address suitable for that device.
> This could well mean that we need to deal with setting up an IOMMU
> mapping.  My guess is that this needs to happen at the DMA coherent
> API level - the DMA coherent API needs to be augmented with support
> for this.  I'll call this "DMA coherent remap".
> 
> We then need to think about how to pass this through the dma-buf API.
> dma_map_sg() is done by the exporter, who should know what kind of
> memory is being exported.  The exporter can avoid calling dma_map_sg()
> if it knows in advance that it is exporting DMA coherent memory.
> Instead, the exporter can simply create a scatterlist with the DMA
> address and DMA length prepopulated with the results of the DMA
> coherent remap operation above.

As Russell pointed to armama-drm case, I looked at that closely.
armada-drm is creating sg_table and populating it with DMA-address in
its map_dma_buf ops and unmap_dma_buf ops handles the special case and
doesn't call dma_unmap_sg().

In the case of drm, gem_prime_map_dma_buf interfaces and the common
drm_gem_map_dma_buf() will need modification to not do dma_map_sg()
and create scatterlist with the DMA address and DMA length instead.
We have to get drm_gem_map_dma_buf() info. to have it not do dma_map_sg()
and create scatterlist.

Focusing on drm for now, looks like there are probably about 15 or so
map_dma_buf interfaces will need to handle coherent memory case.

> 
> What the scatterlist can't carry in this case is a set of valid
> struct page pointers, and an importer must not walk the scatterlist
> expecting to get at the virtual address parameters or struct page
> pointers.

Right - importers need handling to not walk the sg_list and handle
it differently. Is there a good example drm you can point me to for
this? aramda-drm seems to special case this in armada_gem_map_import()
if I am not mistaken.

> 
> On the mmap() side of things, remember that DMA coherent allocations
> may require special mapping into userspace, and which can only be
> mapped by the DMA coherent mmap support.  kmap etc will also need to
> be different.  So it probably makes sense for DMA coherent dma-buf
> exports to use a completely separate set of dma_buf_ops from the
> streaming version.
> 

I agree. It would make is easier and also limits the scope of changes.

> I think this is the easiest approach to solving the problem without
> needing massive driver changes all over the kernel.
> 

Anyway this is a quick note to say that I am looking into this and
haven't drooped it :)

thanks,
-- Shuah