[2/6] V4L2: OMAP: VOUT: dma map and unmap v4l2 buffers in qbuf and dqbuf

Message ID 1307458058-29030-3-git-send-email-amber@ti.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Amber Jain June 7, 2011, 2:47 p.m. UTC
  Add support to map the buffer using dma_map_single during qbuf which inturn
calls cache flush and unmap the same during dqbuf. This is done to prevent
the artifacts seen because of cache-coherency issues on OMAP4

Signed-off-by: Amber Jain <amber@ti.com>
---
 drivers/media/video/omap/omap_vout.c |   29 +++++++++++++++++++++++++++--
 1 files changed, 27 insertions(+), 2 deletions(-)
  

Comments

Hiremath, Vaibhav July 5, 2011, 7:04 p.m. UTC | #1
> -----Original Message-----
> From: JAIN, AMBER
> Sent: Tuesday, June 07, 2011 8:18 PM
> To: linux-media@vger.kernel.org
> Cc: Hiremath, Vaibhav; Semwal, Sumit; JAIN, AMBER
> Subject: [PATCH 2/6] V4L2: OMAP: VOUT: dma map and unmap v4l2 buffers in
> qbuf and dqbuf
> 
[Hiremath, Vaibhav] few minor comments below -

> Add support to map the buffer using dma_map_single during qbuf which
> inturn
> calls cache flush and unmap the same during dqbuf. This is done to prevent
> the artifacts seen because of cache-coherency issues on OMAP4
> 
> Signed-off-by: Amber Jain <amber@ti.com>
> ---
>  drivers/media/video/omap/omap_vout.c |   29 +++++++++++++++++++++++++++--
>  1 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/video/omap/omap_vout.c
> b/drivers/media/video/omap/omap_vout.c
> index 6fe7efa..435fe65 100644
> --- a/drivers/media/video/omap/omap_vout.c
> +++ b/drivers/media/video/omap/omap_vout.c
> @@ -37,6 +37,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/irq.h>
>  #include <linux/videodev2.h>
> +#include <linux/dma-mapping.h>
> 
>  #include <media/videobuf-dma-contig.h>
>  #include <media/v4l2-device.h>
> @@ -768,6 +769,17 @@ static int omap_vout_buffer_prepare(struct
> videobuf_queue *q,
>  		vout->queued_buf_addr[vb->i] = (u8 *)
>  			omap_vout_uservirt_to_phys(vb->baddr);
>  	} else {
> +		int addr, dma_addr;
[Hiremath, Vaibhav] Why is it "int"? It should be either u32 or unsigned long or dma_addr_t. Also you don't need type casting everywhere with this.

> +		unsigned long size;
> +
> +		addr = (unsigned long) vout->buf_virt_addr[vb->i];
> +		size = (unsigned long) vb->size;
> +
> +		dma_addr = dma_map_single(vout->vid_dev->v4l2_dev.dev, (void
> *) addr,
> +				(unsigned) size, DMA_TO_DEVICE);
[Hiremath, Vaibhav] Why type casting required here?

> +		if (dma_mapping_error(vout->vid_dev->v4l2_dev.dev, dma_addr))
> +			printk(KERN_ERR "dma_map_single failed\n");
[Hiremath, Vaibhav] Can this be changed to v4l2_err?

> +
>  		vout->queued_buf_addr[vb->i] = (u8 *)vout->buf_phy_addr[vb-
> >i];
>  	}
> 
> @@ -1549,15 +1561,28 @@ static int vidioc_dqbuf(struct file *file, void
> *fh, struct v4l2_buffer *b)
>  	struct omap_vout_device *vout = fh;
>  	struct videobuf_queue *q = &vout->vbq;
> 
> +	unsigned long size;
> +	u32 addr;
> +	struct videobuf_buffer *vb;
> +	int ret;
> +
[Hiremath, Vaibhav] Just for readability can you put them in order (lengthwise)?

> +	vb = q->bufs[b->index];
> +
>  	if (!vout->streaming)
>  		return -EINVAL;
> 
>  	if (file->f_flags & O_NONBLOCK)
>  		/* Call videobuf_dqbuf for non blocking mode */
> -		return videobuf_dqbuf(q, (struct v4l2_buffer *)b, 1);
> +		ret = videobuf_dqbuf(q, (struct v4l2_buffer *)b, 1);
>  	else
>  		/* Call videobuf_dqbuf for  blocking mode */
> -		return videobuf_dqbuf(q, (struct v4l2_buffer *)b, 0);
> +		ret = videobuf_dqbuf(q, (struct v4l2_buffer *)b, 0);
> +
> +	addr = (unsigned long) vout->buf_phy_addr[vb->i];
> +	size = (unsigned long) vb->size;
> +	dma_unmap_single(vout->vid_dev->v4l2_dev.dev,  addr,
> +				(unsigned) size, DMA_TO_DEVICE);
[Hiremath, Vaibhav] Type cast???

Thanks,
Vaibhav

> +	return ret;
>  }
> 
>  static int vidioc_streamon(struct file *file, void *fh, enum
> v4l2_buf_type i)
> --
> 1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  
Amber Jain July 6, 2011, 3:02 a.m. UTC | #2
> -----Original Message-----
> From: Hiremath, Vaibhav
> Sent: Wednesday, July 06, 2011 12:34 AM
> To: JAIN, AMBER; linux-media@vger.kernel.org
> Cc: Semwal, Sumit
> Subject: RE: [PATCH 2/6] V4L2: OMAP: VOUT: dma map and unmap v4l2 buffers
> in qbuf and dqbuf
> 
> 
> > -----Original Message-----
> > From: JAIN, AMBER
> > Sent: Tuesday, June 07, 2011 8:18 PM
> > To: linux-media@vger.kernel.org
> > Cc: Hiremath, Vaibhav; Semwal, Sumit; JAIN, AMBER
> > Subject: [PATCH 2/6] V4L2: OMAP: VOUT: dma map and unmap v4l2 buffers in
> > qbuf and dqbuf
> >
> [Hiremath, Vaibhav] few minor comments below -
> 
> > Add support to map the buffer using dma_map_single during qbuf which
> > inturn
> > calls cache flush and unmap the same during dqbuf. This is done to
> prevent
> > the artifacts seen because of cache-coherency issues on OMAP4
> >
> > Signed-off-by: Amber Jain <amber@ti.com>
> > ---
> >  drivers/media/video/omap/omap_vout.c |   29
> +++++++++++++++++++++++++++--
> >  1 files changed, 27 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/video/omap/omap_vout.c
> > b/drivers/media/video/omap/omap_vout.c
> > index 6fe7efa..435fe65 100644
> > --- a/drivers/media/video/omap/omap_vout.c
> > +++ b/drivers/media/video/omap/omap_vout.c
> > @@ -37,6 +37,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/irq.h>
> >  #include <linux/videodev2.h>
> > +#include <linux/dma-mapping.h>
> >
> >  #include <media/videobuf-dma-contig.h>
> >  #include <media/v4l2-device.h>
> > @@ -768,6 +769,17 @@ static int omap_vout_buffer_prepare(struct
> > videobuf_queue *q,
> >  		vout->queued_buf_addr[vb->i] = (u8 *)
> >  			omap_vout_uservirt_to_phys(vb->baddr);
> >  	} else {
> > +		int addr, dma_addr;
> [Hiremath, Vaibhav] Why is it "int"? It should be either u32 or unsigned
> long or dma_addr_t. Also you don't need type casting everywhere with this.

Ok, I will change it.

> 
> > +		unsigned long size;
> > +
> > +		addr = (unsigned long) vout->buf_virt_addr[vb->i];
> > +		size = (unsigned long) vb->size;
> > +
> > +		dma_addr = dma_map_single(vout->vid_dev->v4l2_dev.dev, (void
> > *) addr,
> > +				(unsigned) size, DMA_TO_DEVICE);
> [Hiremath, Vaibhav] Why type casting required here?
> 
> > +		if (dma_mapping_error(vout->vid_dev->v4l2_dev.dev, dma_addr))
> > +			printk(KERN_ERR "dma_map_single failed\n");
> [Hiremath, Vaibhav] Can this be changed to v4l2_err?

Ok, I will change it.

> 
> > +
> >  		vout->queued_buf_addr[vb->i] = (u8 *)vout->buf_phy_addr[vb-
> > >i];
> >  	}
> >
> > @@ -1549,15 +1561,28 @@ static int vidioc_dqbuf(struct file *file, void
> > *fh, struct v4l2_buffer *b)
> >  	struct omap_vout_device *vout = fh;
> >  	struct videobuf_queue *q = &vout->vbq;
> >
> > +	unsigned long size;
> > +	u32 addr;
> > +	struct videobuf_buffer *vb;
> > +	int ret;
> > +
> [Hiremath, Vaibhav] Just for readability can you put them in order
> (lengthwise)?
> 
> > +	vb = q->bufs[b->index];
> > +
> >  	if (!vout->streaming)
> >  		return -EINVAL;
> >
> >  	if (file->f_flags & O_NONBLOCK)
> >  		/* Call videobuf_dqbuf for non blocking mode */
> > -		return videobuf_dqbuf(q, (struct v4l2_buffer *)b, 1);
> > +		ret = videobuf_dqbuf(q, (struct v4l2_buffer *)b, 1);
> >  	else
> >  		/* Call videobuf_dqbuf for  blocking mode */
> > -		return videobuf_dqbuf(q, (struct v4l2_buffer *)b, 0);
> > +		ret = videobuf_dqbuf(q, (struct v4l2_buffer *)b, 0);
> > +
> > +	addr = (unsigned long) vout->buf_phy_addr[vb->i];
> > +	size = (unsigned long) vb->size;
> > +	dma_unmap_single(vout->vid_dev->v4l2_dev.dev,  addr,
> > +				(unsigned) size, DMA_TO_DEVICE);
> [Hiremath, Vaibhav] Type cast???
> 
> Thanks,
> Vaibhav
> 
> > +	return ret;
> >  }
> >
> >  static int vidioc_streamon(struct file *file, void *fh, enum
> > v4l2_buf_type i)
> > --
> > 1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Patch

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index 6fe7efa..435fe65 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -37,6 +37,7 @@ 
 #include <linux/platform_device.h>
 #include <linux/irq.h>
 #include <linux/videodev2.h>
+#include <linux/dma-mapping.h>
 
 #include <media/videobuf-dma-contig.h>
 #include <media/v4l2-device.h>
@@ -768,6 +769,17 @@  static int omap_vout_buffer_prepare(struct videobuf_queue *q,
 		vout->queued_buf_addr[vb->i] = (u8 *)
 			omap_vout_uservirt_to_phys(vb->baddr);
 	} else {
+		int addr, dma_addr;
+		unsigned long size;
+
+		addr = (unsigned long) vout->buf_virt_addr[vb->i];
+		size = (unsigned long) vb->size;
+
+		dma_addr = dma_map_single(vout->vid_dev->v4l2_dev.dev, (void *) addr,
+				(unsigned) size, DMA_TO_DEVICE);
+		if (dma_mapping_error(vout->vid_dev->v4l2_dev.dev, dma_addr))
+			printk(KERN_ERR "dma_map_single failed\n");
+
 		vout->queued_buf_addr[vb->i] = (u8 *)vout->buf_phy_addr[vb->i];
 	}
 
@@ -1549,15 +1561,28 @@  static int vidioc_dqbuf(struct file *file, void *fh, struct v4l2_buffer *b)
 	struct omap_vout_device *vout = fh;
 	struct videobuf_queue *q = &vout->vbq;
 
+	unsigned long size;
+	u32 addr;
+	struct videobuf_buffer *vb;
+	int ret;
+
+	vb = q->bufs[b->index];
+
 	if (!vout->streaming)
 		return -EINVAL;
 
 	if (file->f_flags & O_NONBLOCK)
 		/* Call videobuf_dqbuf for non blocking mode */
-		return videobuf_dqbuf(q, (struct v4l2_buffer *)b, 1);
+		ret = videobuf_dqbuf(q, (struct v4l2_buffer *)b, 1);
 	else
 		/* Call videobuf_dqbuf for  blocking mode */
-		return videobuf_dqbuf(q, (struct v4l2_buffer *)b, 0);
+		ret = videobuf_dqbuf(q, (struct v4l2_buffer *)b, 0);
+
+	addr = (unsigned long) vout->buf_phy_addr[vb->i];
+	size = (unsigned long) vb->size;
+	dma_unmap_single(vout->vid_dev->v4l2_dev.dev,  addr,
+				(unsigned) size, DMA_TO_DEVICE);
+	return ret;
 }
 
 static int vidioc_streamon(struct file *file, void *fh, enum v4l2_buf_type i)