[1/7] media: videobuf-dma-contig: use dma_mmap_coherent
Commit Message
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
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);
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.
@@ -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);