diff mbox series

[1/7] media: videobuf-dma-contig: use dma_mmap_coherent

Message ID 20221113163535.884299-2-hch@lst.de
State Accepted
Commit b3dc3f8e49577840dc8ac8a365c5b3da4edb10b8
Headers show
Series [1/7] media: videobuf-dma-contig: use dma_mmap_coherent | expand

Commit Message

Christoph Hellwig Nov. 13, 2022, 4:35 p.m. UTC
dma_alloc_coherent does not return a physical address, but a DMA address,
which might be remapped or have an offset.  Passing the DMA address to
vm_iomap_memory is thus broken.

Use the proper dma_mmap_coherent helper instead, and stop passing
__GFP_COMP to dma_alloc_coherent, as the memory management inside the
DMA allocator is hidden from the callers and does not require it.

With this the gfp_t argument to __videobuf_dc_alloc can be removed and
hard coded to GFP_KERNEL.

Fixes: a8f3c203e19b ("[media] videobuf-dma-contig: add cache support")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/media/v4l2-core/videobuf-dma-contig.c | 22 +++++++------------
 1 file changed, 8 insertions(+), 14 deletions(-)

Comments

Hans Verkuil Nov. 24, 2022, 2:10 p.m. UTC | #1
Hi Christoph,

On 13/11/2022 17:35, Christoph Hellwig wrote:
> dma_alloc_coherent does not return a physical address, but a DMA address,
> which might be remapped or have an offset.  Passing the DMA address to
> vm_iomap_memory is thus broken.
> 
> Use the proper dma_mmap_coherent helper instead, and stop passing
> __GFP_COMP to dma_alloc_coherent, as the memory management inside the
> DMA allocator is hidden from the callers and does not require it.
> 
> With this the gfp_t argument to __videobuf_dc_alloc can be removed and
> hard coded to GFP_KERNEL.
> 
> Fixes: a8f3c203e19b ("[media] videobuf-dma-contig: add cache support")
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/media/v4l2-core/videobuf-dma-contig.c | 22 +++++++------------

Very, very old code :-) Hopefully in the not-too-distant future we can kill off
the old videobuf framework. But for now:

Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

I assume you take this? If not, then let me know and I will pick it up for the media
subsystem.

Regards,

	Hans

>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
> index 52312ce2ba056..f2c4393595574 100644
> --- a/drivers/media/v4l2-core/videobuf-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
> @@ -36,12 +36,11 @@ struct videobuf_dma_contig_memory {
>  
>  static int __videobuf_dc_alloc(struct device *dev,
>  			       struct videobuf_dma_contig_memory *mem,
> -			       unsigned long size, gfp_t flags)
> +			       unsigned long size)
>  {
>  	mem->size = size;
> -	mem->vaddr = dma_alloc_coherent(dev, mem->size,
> -					&mem->dma_handle, flags);
> -
> +	mem->vaddr = dma_alloc_coherent(dev, mem->size, &mem->dma_handle,
> +					GFP_KERNEL);
>  	if (!mem->vaddr) {
>  		dev_err(dev, "memory alloc size %ld failed\n", mem->size);
>  		return -ENOMEM;
> @@ -258,8 +257,7 @@ static int __videobuf_iolock(struct videobuf_queue *q,
>  			return videobuf_dma_contig_user_get(mem, vb);
>  
>  		/* allocate memory for the read() method */
> -		if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(vb->size),
> -					GFP_KERNEL))
> +		if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(vb->size)))
>  			return -ENOMEM;
>  		break;
>  	case V4L2_MEMORY_OVERLAY:
> @@ -295,22 +293,18 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
>  	BUG_ON(!mem);
>  	MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
>  
> -	if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(buf->bsize),
> -				GFP_KERNEL | __GFP_COMP))
> +	if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(buf->bsize)))
>  		goto error;
>  
> -	/* Try to remap memory */
> -	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> -
>  	/* the "vm_pgoff" is just used in v4l2 to find the
>  	 * corresponding buffer data structure which is allocated
>  	 * earlier and it does not mean the offset from the physical
>  	 * buffer start address as usual. So set it to 0 to pass
> -	 * the sanity check in vm_iomap_memory().
> +	 * the sanity check in dma_mmap_coherent().
>  	 */
>  	vma->vm_pgoff = 0;
> -
> -	retval = vm_iomap_memory(vma, mem->dma_handle, mem->size);
> +	retval = dma_mmap_coherent(q->dev, vma, mem->vaddr, mem->dma_handle,
> +				   mem->size);
>  	if (retval) {
>  		dev_err(q->dev, "mmap: remap failed with error %d. ",
>  			retval);
Christoph Hellwig Nov. 29, 2022, 2 p.m. UTC | #2
On Thu, Nov 24, 2022 at 03:10:43PM +0100, Hans Verkuil wrote:
> Very, very old code :-) Hopefully in the not-too-distant future we can kill off
> the old videobuf framework.

That would be great for various reasons.

> But for now:
> 
> Acked-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> 
> I assume you take this? If not, then let me know and I will pick it up for the media
> subsystem.

I've actually picked it up a while ago.  So without a reabse I can't
add your formal ACK, but I hope Linus is fine with that.
diff mbox series

Patch

diff --git a/drivers/media/v4l2-core/videobuf-dma-contig.c b/drivers/media/v4l2-core/videobuf-dma-contig.c
index 52312ce2ba056..f2c4393595574 100644
--- a/drivers/media/v4l2-core/videobuf-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf-dma-contig.c
@@ -36,12 +36,11 @@  struct videobuf_dma_contig_memory {
 
 static int __videobuf_dc_alloc(struct device *dev,
 			       struct videobuf_dma_contig_memory *mem,
-			       unsigned long size, gfp_t flags)
+			       unsigned long size)
 {
 	mem->size = size;
-	mem->vaddr = dma_alloc_coherent(dev, mem->size,
-					&mem->dma_handle, flags);
-
+	mem->vaddr = dma_alloc_coherent(dev, mem->size, &mem->dma_handle,
+					GFP_KERNEL);
 	if (!mem->vaddr) {
 		dev_err(dev, "memory alloc size %ld failed\n", mem->size);
 		return -ENOMEM;
@@ -258,8 +257,7 @@  static int __videobuf_iolock(struct videobuf_queue *q,
 			return videobuf_dma_contig_user_get(mem, vb);
 
 		/* allocate memory for the read() method */
-		if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(vb->size),
-					GFP_KERNEL))
+		if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(vb->size)))
 			return -ENOMEM;
 		break;
 	case V4L2_MEMORY_OVERLAY:
@@ -295,22 +293,18 @@  static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 	BUG_ON(!mem);
 	MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
 
-	if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(buf->bsize),
-				GFP_KERNEL | __GFP_COMP))
+	if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(buf->bsize)))
 		goto error;
 
-	/* Try to remap memory */
-	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
-
 	/* the "vm_pgoff" is just used in v4l2 to find the
 	 * corresponding buffer data structure which is allocated
 	 * earlier and it does not mean the offset from the physical
 	 * buffer start address as usual. So set it to 0 to pass
-	 * the sanity check in vm_iomap_memory().
+	 * the sanity check in dma_mmap_coherent().
 	 */
 	vma->vm_pgoff = 0;
-
-	retval = vm_iomap_memory(vma, mem->dma_handle, mem->size);
+	retval = dma_mmap_coherent(q->dev, vma, mem->vaddr, mem->dma_handle,
+				   mem->size);
 	if (retval) {
 		dev_err(q->dev, "mmap: remap failed with error %d. ",
 			retval);