| Message ID | 20221113163535.884299-2-hch@lst.de (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [1/7] media: videobuf-dma-contig: use dma_mmap_coherent | expand |
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.
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);
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(-)