[PATCH] dma-buf: heaps: Set VM_PFNMAP in mmap for system and cma heaps

[PATCH] dma-buf: heaps: Set VM_PFNMAP in mmap for system and cma heaps

[John Stultz]

Per discussion and patches here:
  https://lore.kernel.org/dri-devel/20210223105951.912577-1-daniel.vetter@ffwll.ch/

Daniel is planning on making VM_PFNMAP required on dmabufs.

Thus to avoid the warn_on noise, set the VM_PFNMAP in the
system and cma heap's mmap handler.

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Christian Koenig <christian.koenig@amd.com>
Cc: Sumit Semwal <sumit.semwal@linaro.org>
Cc: Liam Mark <lmark@codeaurora.org>
Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
Cc: Laura Abbott <labbott@kernel.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>
Cc: Hridya Valsaraju <hridya@google.com>
Cc: Suren Baghdasaryan <surenb@google.com>
Cc: Sandeep Patil <sspatil@google.com>
Cc: Daniel Mentz <danielmentz@google.com>
Cc: Ørjan Eide <orjan.eide@arm.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Ezequiel Garcia <ezequiel@collabora.com>
Cc: Simon Ser <contact@emersion.fr>
Cc: James Jones <jajones@nvidia.com>
Cc: linux-media@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/dma-buf/heaps/cma_heap.c    | 1 +
 drivers/dma-buf/heaps/system_heap.c | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
index 364fc2f3e499..34bc3987f942 100644
--- a/drivers/dma-buf/heaps/cma_heap.c
+++ b/drivers/dma-buf/heaps/cma_heap.c
@@ -185,6 +185,7 @@ static int cma_heap_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
 
 	vma->vm_ops = &dma_heap_vm_ops;
 	vma->vm_private_data = buffer;
+	vma->vm_flags |= VM_PFNMAP;
 
 	return 0;
 }
diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
index 3548b20cb98c..8995e3cbfcaf 100644
--- a/drivers/dma-buf/heaps/system_heap.c
+++ b/drivers/dma-buf/heaps/system_heap.c
@@ -228,8 +228,10 @@ static int system_heap_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
 			return ret;
 		addr += PAGE_SIZE;
 		if (addr >= vma->vm_end)
-			return 0;
+			break;
 	}
+
+	vma->vm_flags |= VM_PFNMAP;
 	return 0;
 }
 
-- 
2.25.1

Re: [PATCH] dma-buf: heaps: Set VM_PFNMAP in mmap for system and cma heaps

[Daniel Vetter]

On Fri, Feb 26, 2021 at 5:09 AM John Stultz <john.stultz@linaro.org> wrote:
>
> Per discussion and patches here:
>   https://lore.kernel.org/dri-devel/20210223105951.912577-1-daniel.vetter@ffwll.ch/
>
> Daniel is planning on making VM_PFNMAP required on dmabufs.
>
> Thus to avoid the warn_on noise, set the VM_PFNMAP in the
> system and cma heap's mmap handler.
>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> Cc: Christian Koenig <christian.koenig@amd.com>
> Cc: Sumit Semwal <sumit.semwal@linaro.org>
> Cc: Liam Mark <lmark@codeaurora.org>
> Cc: Chris Goldsworthy <cgoldswo@codeaurora.org>
> Cc: Laura Abbott <labbott@kernel.org>
> Cc: Brian Starkey <Brian.Starkey@arm.com>
> Cc: Hridya Valsaraju <hridya@google.com>
> Cc: Suren Baghdasaryan <surenb@google.com>
> Cc: Sandeep Patil <sspatil@google.com>
> Cc: Daniel Mentz <danielmentz@google.com>
> Cc: Ørjan Eide <orjan.eide@arm.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Ezequiel Garcia <ezequiel@collabora.com>
> Cc: Simon Ser <contact@emersion.fr>
> Cc: James Jones <jajones@nvidia.com>
> Cc: linux-media@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>

System heap uses remap_pfn_range so looks fine, but cma heap inserts
pages, and that's not fine for VM_PFNMAP. You need to use
vm_insert_pfn or remap_pfn_range or one of the related pfn mapping
functions for that too. I think this should splat when you run it.

Also note that remap_pfn_range updates the vma flags already correctly
for you, so you probably don't want to do that.

Also given that both deal with struct page there's a ton of divergence
between these two that doesn't make much sense. Maybe could even share
the code fully, aside from how you allocate the struct pages.
-Daniel

> ---
>  drivers/dma-buf/heaps/cma_heap.c    | 1 +
>  drivers/dma-buf/heaps/system_heap.c | 4 +++-
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma-buf/heaps/cma_heap.c b/drivers/dma-buf/heaps/cma_heap.c
> index 364fc2f3e499..34bc3987f942 100644
> --- a/drivers/dma-buf/heaps/cma_heap.c
> +++ b/drivers/dma-buf/heaps/cma_heap.c
> @@ -185,6 +185,7 @@ static int cma_heap_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
>
>         vma->vm_ops = &dma_heap_vm_ops;
>         vma->vm_private_data = buffer;
> +       vma->vm_flags |= VM_PFNMAP;
>
>         return 0;
>  }
> diff --git a/drivers/dma-buf/heaps/system_heap.c b/drivers/dma-buf/heaps/system_heap.c
> index 3548b20cb98c..8995e3cbfcaf 100644
> --- a/drivers/dma-buf/heaps/system_heap.c
> +++ b/drivers/dma-buf/heaps/system_heap.c
> @@ -228,8 +228,10 @@ static int system_heap_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma)
>                         return ret;
>                 addr += PAGE_SIZE;
>                 if (addr >= vma->vm_end)
> -                       return 0;
> +                       break;
>         }
> +
> +       vma->vm_flags |= VM_PFNMAP;
>         return 0;
>  }
>
> --
> 2.25.1
>


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

Re: [PATCH] dma-buf: heaps: Set VM_PFNMAP in mmap for system and cma heaps

[Christoph Hellwig]

On Fri, Feb 26, 2021 at 08:36:55AM +0100, Daniel Vetter wrote:
> Also given that both deal with struct page there's a ton of divergence
> between these two that doesn't make much sense. Maybe could even share
> the code fully, aside from how you allocate the struct pages.

I've been saying that since the code was first submitted.  Once pages
are allocated from CMA they should be treated not different from normal
pages.

Please take a look at how the DMA contigous allocator manages to share
all code for handling CMA vs alloc_pages pages.

Re: [PATCH] dma-buf: heaps: Set VM_PFNMAP in mmap for system and cma heaps

[John Stultz]

On Sat, Feb 27, 2021 at 1:44 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Fri, Feb 26, 2021 at 08:36:55AM +0100, Daniel Vetter wrote:
> > Also given that both deal with struct page there's a ton of divergence
> > between these two that doesn't make much sense. Maybe could even share
> > the code fully, aside from how you allocate the struct pages.
>
> I've been saying that since the code was first submitted.  Once pages
> are allocated from CMA they should be treated not different from normal
> pages.
>
> Please take a look at how the DMA contigous allocator manages to share
> all code for handling CMA vs alloc_pages pages.

I'll take a look at that! Thanks for the pointer!
-john