[PATCH 0/5] [media]: OMAP_VOUT: Misc fixes and cleanup patches for 3.2

[PATCH 0/5] [media]: OMAP_VOUT: Misc fixes and cleanup patches for 3.2

[Archit Taneja]

This set includes patches which do the following:
- Fix crash if a we call dssdev->driver->update for a disabled panel.
- Fix the issue of not being able to request for a buffer which is larger than
  what we did the last time.
- Fix a small bug in omap_vout_isr()
- Remove some redundant code in omap_vout_isr()
- Add basic support for DSI panels

Note:
The patch "OMAP_VOUT: Fix check in reqbuf & mmap for buf_size allocation" had
been posted in the past, but wasn't commented on. So copied it here again.

These patches are based on Tomi's master branch(which is based on 3.1-rc6), and
a patches which affects omap_vout and has been posted recently on linux-media
(see "OMAPDSS/OMAP_VOUT: Fix incorrect OMAP3-alpha compatibility setting"). This
tree can be used as reference:

git@gitorious.org:~boddob/linux-omap-dss2/archit-dss2-clone.git 'for_omap_vout'

Archit Taneja (5):
  OMAP_VOUT: Fix check in reqbuf & mmap for buf_size allocation
  OMAP_VOUT: CLEANUP: Remove redundant code from omap_vout_isr
  OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr
  OMAP_VOUT: Add support for DSI panels
  OMAP_VOUT: Don't trigger updates in omap_vout_probe

 drivers/media/video/omap/omap_vout.c |  199 +++++++++++++++++-----------------
 1 files changed, 99 insertions(+), 100 deletions(-)

[PATCH 0/5] [media]: OMAP_VOUT: Misc fixes and cleanup patches for 3.2

[Archit Taneja]

This set includes patches which do the following:
- Fix crash if a we call dssdev->driver->update for a disabled panel.
- Fix the issue of not being able to request for a buffer which is larger than
  what we did the last time.
- Fix a small bug in omap_vout_isr()
- Remove some redundant code in omap_vout_isr()
- Add basic support for DSI panels

Note:
The patch "OMAP_VOUT: Fix check in reqbuf & mmap for buf_size allocation" had
been posted in the past, but wasn't commented on. So copied it here again.

These patches are based on Tomi's master branch(which is based on 3.1-rc6), and
a patches which affects omap_vout and has been posted recently on linux-media
(see "OMAPDSS/OMAP_VOUT: Fix incorrect OMAP3-alpha compatibility setting"). This
tree can be used as reference:

git@gitorious.org:~boddob/linux-omap-dss2/archit-dss2-clone.git 'for_omap_vout'

Archit Taneja (5):
  OMAP_VOUT: Fix check in reqbuf & mmap for buf_size allocation
  OMAP_VOUT: CLEANUP: Remove redundant code from omap_vout_isr
  OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr
  OMAP_VOUT: Add support for DSI panels
  OMAP_VOUT: Don't trigger updates in omap_vout_probe

 drivers/media/video/omap/omap_vout.c |  199 +++++++++++++++++-----------------
 1 files changed, 99 insertions(+), 100 deletions(-)

[PATCH 1/5] [media]: OMAP_VOUT: Fix check in reqbuf & mmap for buf_size allocation

[Archit Taneja]

The commit 383e4f69879d11c86ebdd38b3356f6d0690fb4cc makes reqbuf and mmap prevent
requesting a larger size buffer than what is allocated at kernel boot during
omap_vout_probe.

The requested size is compared with vout->buffer_size, this isn't correct as
vout->buffer_size is later set to the size requested in reqbuf. When the video
device is opened the next time, this check will prevent us to allocate a buffer
which is larger than what we requested the last time.

Don't use vout->buffer_size, always check with the parameters video1_bufsize
or video2_bufsize.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/media/video/omap/omap_vout.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index 95daf98..e14c82b 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -664,10 +664,14 @@ static int omap_vout_buffer_setup(struct videobuf_queue *q, unsigned int *count,
 	u32 phy_addr = 0, virt_addr = 0;
 	struct omap_vout_device *vout = q->priv_data;
 	struct omapvideo_info *ovid = &vout->vid_info;
+	int vid_max_buf_size;
 
 	if (!vout)
 		return -EINVAL;
 
+	vid_max_buf_size = vout->vid == OMAP_VIDEO1 ? video1_bufsize :
+		video2_bufsize;
+
 	if (V4L2_BUF_TYPE_VIDEO_OUTPUT != q->type)
 		return -EINVAL;
 
@@ -690,7 +694,7 @@ static int omap_vout_buffer_setup(struct videobuf_queue *q, unsigned int *count,
 		video1_numbuffers : video2_numbuffers;
 
 	/* Check the size of the buffer */
-	if (*size > vout->buffer_size) {
+	if (*size > vid_max_buf_size) {
 		v4l2_err(&vout->vid_dev->v4l2_dev,
 				"buffer allocation mismatch [%u] [%u]\n",
 				*size, vout->buffer_size);
@@ -865,6 +869,8 @@ static int omap_vout_mmap(struct file *file, struct vm_area_struct *vma)
 	unsigned long size = (vma->vm_end - vma->vm_start);
 	struct omap_vout_device *vout = file->private_data;
 	struct videobuf_queue *q = &vout->vbq;
+	int vid_max_buf_size = vout->vid == OMAP_VIDEO1 ? video1_bufsize :
+		video2_bufsize;
 
 	v4l2_dbg(1, debug, &vout->vid_dev->v4l2_dev,
 			" %s pgoff=0x%lx, start=0x%lx, end=0x%lx\n", __func__,
@@ -887,7 +893,7 @@ static int omap_vout_mmap(struct file *file, struct vm_area_struct *vma)
 		return -EINVAL;
 	}
 	/* Check the size of the buffer */
-	if (size > vout->buffer_size) {
+	if (size > vid_max_buf_size) {
 		v4l2_err(&vout->vid_dev->v4l2_dev,
 				"insufficient memory [%lu] [%u]\n",
 				size, vout->buffer_size);
-- 
1.7.1

[PATCH 1/5] [media]: OMAP_VOUT: Fix check in reqbuf & mmap for buf_size allocation

[Archit Taneja]

The commit 383e4f69879d11c86ebdd38b3356f6d0690fb4cc makes reqbuf and mmap prevent
requesting a larger size buffer than what is allocated at kernel boot during
omap_vout_probe.

The requested size is compared with vout->buffer_size, this isn't correct as
vout->buffer_size is later set to the size requested in reqbuf. When the video
device is opened the next time, this check will prevent us to allocate a buffer
which is larger than what we requested the last time.

Don't use vout->buffer_size, always check with the parameters video1_bufsize
or video2_bufsize.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/media/video/omap/omap_vout.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index 95daf98..e14c82b 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -664,10 +664,14 @@ static int omap_vout_buffer_setup(struct videobuf_queue *q, unsigned int *count,
 	u32 phy_addr = 0, virt_addr = 0;
 	struct omap_vout_device *vout = q->priv_data;
 	struct omapvideo_info *ovid = &vout->vid_info;
+	int vid_max_buf_size;
 
 	if (!vout)
 		return -EINVAL;
 
+	vid_max_buf_size = vout->vid == OMAP_VIDEO1 ? video1_bufsize :
+		video2_bufsize;
+
 	if (V4L2_BUF_TYPE_VIDEO_OUTPUT != q->type)
 		return -EINVAL;
 
@@ -690,7 +694,7 @@ static int omap_vout_buffer_setup(struct videobuf_queue *q, unsigned int *count,
 		video1_numbuffers : video2_numbuffers;
 
 	/* Check the size of the buffer */
-	if (*size > vout->buffer_size) {
+	if (*size > vid_max_buf_size) {
 		v4l2_err(&vout->vid_dev->v4l2_dev,
 				"buffer allocation mismatch [%u] [%u]\n",
 				*size, vout->buffer_size);
@@ -865,6 +869,8 @@ static int omap_vout_mmap(struct file *file, struct vm_area_struct *vma)
 	unsigned long size = (vma->vm_end - vma->vm_start);
 	struct omap_vout_device *vout = file->private_data;
 	struct videobuf_queue *q = &vout->vbq;
+	int vid_max_buf_size = vout->vid == OMAP_VIDEO1 ? video1_bufsize :
+		video2_bufsize;
 
 	v4l2_dbg(1, debug, &vout->vid_dev->v4l2_dev,
 			" %s pgoff=0x%lx, start=0x%lx, end=0x%lx\n", __func__,
@@ -887,7 +893,7 @@ static int omap_vout_mmap(struct file *file, struct vm_area_struct *vma)
 		return -EINVAL;
 	}
 	/* Check the size of the buffer */
-	if (size > vout->buffer_size) {
+	if (size > vid_max_buf_size) {
 		v4l2_err(&vout->vid_dev->v4l2_dev,
 				"insufficient memory [%lu] [%u]\n",
 				size, vout->buffer_size);
-- 
1.7.1

[PATCH 2/5] [media]: OMAP_VOUT: CLEANUP: Remove redundant code from omap_vout_isr

[Archit Taneja]

Currently, there is a lot of redundant code is between DPI and VENC panels, this
can be made common by moving out field/interlace specific code to a separate
function called omapvid_handle_interlace_display(). There is no functional
change made.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/media/video/omap/omap_vout.c |  172 ++++++++++++++++------------------
 1 files changed, 82 insertions(+), 90 deletions(-)

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index e14c82b..c5f2ea0 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -524,10 +524,50 @@ static int omapvid_apply_changes(struct omap_vout_device *vout)
 	return 0;
 }
 
+static int omapvid_handle_interlace_display(struct omap_vout_device *vout,
+		unsigned int irqstatus, struct timeval timevalue)
+{
+	u32 fid;
+
+	if (vout->first_int) {
+		vout->first_int = 0;
+		goto err;
+	}
+
+	if (irqstatus & DISPC_IRQ_EVSYNC_ODD)
+		fid = 1;
+	else if (irqstatus & DISPC_IRQ_EVSYNC_EVEN)
+		fid = 0;
+	else
+		goto err;
+
+	vout->field_id ^= 1;
+	if (fid != vout->field_id) {
+		/* reset field ID */
+		vout->field_id = 0;
+	} else if (0 == fid) {
+		if (vout->cur_frm == vout->next_frm)
+			goto err;
+
+		vout->cur_frm->ts = timevalue;
+		vout->cur_frm->state = VIDEOBUF_DONE;
+		wake_up_interruptible(&vout->cur_frm->done);
+		vout->cur_frm = vout->next_frm;
+	} else {
+		if (list_empty(&vout->dma_queue) ||
+				(vout->cur_frm != vout->next_frm))
+			goto err;
+	}
+
+	return vout->field_id;
+err:
+	return 0;
+}
+
 static void omap_vout_isr(void *arg, unsigned int irqstatus)
 {
-	int ret;
-	u32 addr, fid;
+	int ret, fid;
+	u32 addr;
 	struct omap_overlay *ovl;
 	struct timeval timevalue;
 	struct omapvideo_info *ovid;
@@ -548,107 +588,59 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus)
 	spin_lock(&vout->vbq_lock);
 	do_gettimeofday(&timevalue);
 
-	if (cur_display->type != OMAP_DISPLAY_TYPE_VENC) {
-		switch (cur_display->type) {
-		case OMAP_DISPLAY_TYPE_DPI:
-			if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
-				goto vout_isr_err;
-			break;
-		case OMAP_DISPLAY_TYPE_HDMI:
-			if (!(irqstatus & DISPC_IRQ_EVSYNC_EVEN))
-				goto vout_isr_err;
-			break;
-		default:
+	switch (cur_display->type) {
+	case OMAP_DISPLAY_TYPE_DPI:
+		if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
 			goto vout_isr_err;
-		}
-		if (!vout->first_int && (vout->cur_frm != vout->next_frm)) {
-			vout->cur_frm->ts = timevalue;
-			vout->cur_frm->state = VIDEOBUF_DONE;
-			wake_up_interruptible(&vout->cur_frm->done);
-			vout->cur_frm = vout->next_frm;
-		}
-		vout->first_int = 0;
-		if (list_empty(&vout->dma_queue))
+		break;
+	case OMAP_DISPLAY_TYPE_VENC:
+		fid = omapvid_handle_interlace_display(vout, irqstatus,
+				timevalue);
+		if (!fid)
 			goto vout_isr_err;
+		break;
+	case OMAP_DISPLAY_TYPE_HDMI:
+		if (!(irqstatus & DISPC_IRQ_EVSYNC_EVEN))
+			goto vout_isr_err;
+		break;
+	default:
+		goto vout_isr_err;
+	}
 
-		vout->next_frm = list_entry(vout->dma_queue.next,
-				struct videobuf_buffer, queue);
-		list_del(&vout->next_frm->queue);
-
-		vout->next_frm->state = VIDEOBUF_ACTIVE;
-
-		addr = (unsigned long) vout->queued_buf_addr[vout->next_frm->i]
-			+ vout->cropped_offset;
+	if (!vout->first_int && (vout->cur_frm != vout->next_frm)) {
+		vout->cur_frm->ts = timevalue;
+		vout->cur_frm->state = VIDEOBUF_DONE;
+		wake_up_interruptible(&vout->cur_frm->done);
+		vout->cur_frm = vout->next_frm;
+	}
 
-		/* First save the configuration in ovelray structure */
-		ret = omapvid_init(vout, addr);
-		if (ret)
-			printk(KERN_ERR VOUT_NAME
-				"failed to set overlay info\n");
-		/* Enable the pipeline and set the Go bit */
-		ret = omapvid_apply_changes(vout);
-		if (ret)
-			printk(KERN_ERR VOUT_NAME "failed to change mode\n");
-	} else {
+	vout->first_int = 0;
+	if (list_empty(&vout->dma_queue))
+		goto vout_isr_err;
 
-		if (vout->first_int) {
-			vout->first_int = 0;
-			goto vout_isr_err;
-		}
-		if (irqstatus & DISPC_IRQ_EVSYNC_ODD)
-			fid = 1;
-		else if (irqstatus & DISPC_IRQ_EVSYNC_EVEN)
-			fid = 0;
-		else
-			goto vout_isr_err;
+	vout->next_frm = list_entry(vout->dma_queue.next,
+			struct videobuf_buffer, queue);
+	list_del(&vout->next_frm->queue);
 
-		vout->field_id ^= 1;
-		if (fid != vout->field_id) {
-			if (0 == fid)
-				vout->field_id = fid;
+	vout->next_frm->state = VIDEOBUF_ACTIVE;
 
-			goto vout_isr_err;
-		}
-		if (0 == fid) {
-			if (vout->cur_frm == vout->next_frm)
-				goto vout_isr_err;
-
-			vout->cur_frm->ts = timevalue;
-			vout->cur_frm->state = VIDEOBUF_DONE;
-			wake_up_interruptible(&vout->cur_frm->done);
-			vout->cur_frm = vout->next_frm;
-		} else if (1 == fid) {
-			if (list_empty(&vout->dma_queue) ||
-					(vout->cur_frm != vout->next_frm))
-				goto vout_isr_err;
-
-			vout->next_frm = list_entry(vout->dma_queue.next,
-					struct videobuf_buffer, queue);
-			list_del(&vout->next_frm->queue);
-
-			vout->next_frm->state = VIDEOBUF_ACTIVE;
-			addr = (unsigned long)
-				vout->queued_buf_addr[vout->next_frm->i] +
-				vout->cropped_offset;
-			/* First save the configuration in ovelray structure */
-			ret = omapvid_init(vout, addr);
-			if (ret)
-				printk(KERN_ERR VOUT_NAME
-						"failed to set overlay info\n");
-			/* Enable the pipeline and set the Go bit */
-			ret = omapvid_apply_changes(vout);
-			if (ret)
-				printk(KERN_ERR VOUT_NAME
-						"failed to change mode\n");
-		}
+	addr = (unsigned long) vout->queued_buf_addr[vout->next_frm->i]
+		+ vout->cropped_offset;
 
-	}
+	/* First save the configuration in ovelray structure */
+	ret = omapvid_init(vout, addr);
+	if (ret)
+		printk(KERN_ERR VOUT_NAME
+			"failed to set overlay info\n");
+	/* Enable the pipeline and set the Go bit */
+	ret = omapvid_apply_changes(vout);
+	if (ret)
+		printk(KERN_ERR VOUT_NAME "failed to change mode\n");
 
 vout_isr_err:
 	spin_unlock(&vout->vbq_lock);
 }
 
-
 /* Video buffer call backs */
 
 /*
-- 
1.7.1

[PATCH 2/5] [media]: OMAP_VOUT: CLEANUP: Remove redundant code from omap_vout_isr

[Archit Taneja]

Currently, there is a lot of redundant code is between DPI and VENC panels, this
can be made common by moving out field/interlace specific code to a separate
function called omapvid_handle_interlace_display(). There is no functional
change made.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/media/video/omap/omap_vout.c |  172 ++++++++++++++++------------------
 1 files changed, 82 insertions(+), 90 deletions(-)

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index e14c82b..c5f2ea0 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -524,10 +524,50 @@ static int omapvid_apply_changes(struct omap_vout_device *vout)
 	return 0;
 }
 
+static int omapvid_handle_interlace_display(struct omap_vout_device *vout,
+		unsigned int irqstatus, struct timeval timevalue)
+{
+	u32 fid;
+
+	if (vout->first_int) {
+		vout->first_int = 0;
+		goto err;
+	}
+
+	if (irqstatus & DISPC_IRQ_EVSYNC_ODD)
+		fid = 1;
+	else if (irqstatus & DISPC_IRQ_EVSYNC_EVEN)
+		fid = 0;
+	else
+		goto err;
+
+	vout->field_id ^= 1;
+	if (fid != vout->field_id) {
+		/* reset field ID */
+		vout->field_id = 0;
+	} else if (0 == fid) {
+		if (vout->cur_frm == vout->next_frm)
+			goto err;
+
+		vout->cur_frm->ts = timevalue;
+		vout->cur_frm->state = VIDEOBUF_DONE;
+		wake_up_interruptible(&vout->cur_frm->done);
+		vout->cur_frm = vout->next_frm;
+	} else {
+		if (list_empty(&vout->dma_queue) ||
+				(vout->cur_frm != vout->next_frm))
+			goto err;
+	}
+
+	return vout->field_id;
+err:
+	return 0;
+}
+
 static void omap_vout_isr(void *arg, unsigned int irqstatus)
 {
-	int ret;
-	u32 addr, fid;
+	int ret, fid;
+	u32 addr;
 	struct omap_overlay *ovl;
 	struct timeval timevalue;
 	struct omapvideo_info *ovid;
@@ -548,107 +588,59 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus)
 	spin_lock(&vout->vbq_lock);
 	do_gettimeofday(&timevalue);
 
-	if (cur_display->type != OMAP_DISPLAY_TYPE_VENC) {
-		switch (cur_display->type) {
-		case OMAP_DISPLAY_TYPE_DPI:
-			if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
-				goto vout_isr_err;
-			break;
-		case OMAP_DISPLAY_TYPE_HDMI:
-			if (!(irqstatus & DISPC_IRQ_EVSYNC_EVEN))
-				goto vout_isr_err;
-			break;
-		default:
+	switch (cur_display->type) {
+	case OMAP_DISPLAY_TYPE_DPI:
+		if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
 			goto vout_isr_err;
-		}
-		if (!vout->first_int && (vout->cur_frm != vout->next_frm)) {
-			vout->cur_frm->ts = timevalue;
-			vout->cur_frm->state = VIDEOBUF_DONE;
-			wake_up_interruptible(&vout->cur_frm->done);
-			vout->cur_frm = vout->next_frm;
-		}
-		vout->first_int = 0;
-		if (list_empty(&vout->dma_queue))
+		break;
+	case OMAP_DISPLAY_TYPE_VENC:
+		fid = omapvid_handle_interlace_display(vout, irqstatus,
+				timevalue);
+		if (!fid)
 			goto vout_isr_err;
+		break;
+	case OMAP_DISPLAY_TYPE_HDMI:
+		if (!(irqstatus & DISPC_IRQ_EVSYNC_EVEN))
+			goto vout_isr_err;
+		break;
+	default:
+		goto vout_isr_err;
+	}
 
-		vout->next_frm = list_entry(vout->dma_queue.next,
-				struct videobuf_buffer, queue);
-		list_del(&vout->next_frm->queue);
-
-		vout->next_frm->state = VIDEOBUF_ACTIVE;
-
-		addr = (unsigned long) vout->queued_buf_addr[vout->next_frm->i]
-			+ vout->cropped_offset;
+	if (!vout->first_int && (vout->cur_frm != vout->next_frm)) {
+		vout->cur_frm->ts = timevalue;
+		vout->cur_frm->state = VIDEOBUF_DONE;
+		wake_up_interruptible(&vout->cur_frm->done);
+		vout->cur_frm = vout->next_frm;
+	}
 
-		/* First save the configuration in ovelray structure */
-		ret = omapvid_init(vout, addr);
-		if (ret)
-			printk(KERN_ERR VOUT_NAME
-				"failed to set overlay info\n");
-		/* Enable the pipeline and set the Go bit */
-		ret = omapvid_apply_changes(vout);
-		if (ret)
-			printk(KERN_ERR VOUT_NAME "failed to change mode\n");
-	} else {
+	vout->first_int = 0;
+	if (list_empty(&vout->dma_queue))
+		goto vout_isr_err;
 
-		if (vout->first_int) {
-			vout->first_int = 0;
-			goto vout_isr_err;
-		}
-		if (irqstatus & DISPC_IRQ_EVSYNC_ODD)
-			fid = 1;
-		else if (irqstatus & DISPC_IRQ_EVSYNC_EVEN)
-			fid = 0;
-		else
-			goto vout_isr_err;
+	vout->next_frm = list_entry(vout->dma_queue.next,
+			struct videobuf_buffer, queue);
+	list_del(&vout->next_frm->queue);
 
-		vout->field_id ^= 1;
-		if (fid != vout->field_id) {
-			if (0 == fid)
-				vout->field_id = fid;
+	vout->next_frm->state = VIDEOBUF_ACTIVE;
 
-			goto vout_isr_err;
-		}
-		if (0 == fid) {
-			if (vout->cur_frm == vout->next_frm)
-				goto vout_isr_err;
-
-			vout->cur_frm->ts = timevalue;
-			vout->cur_frm->state = VIDEOBUF_DONE;
-			wake_up_interruptible(&vout->cur_frm->done);
-			vout->cur_frm = vout->next_frm;
-		} else if (1 == fid) {
-			if (list_empty(&vout->dma_queue) ||
-					(vout->cur_frm != vout->next_frm))
-				goto vout_isr_err;
-
-			vout->next_frm = list_entry(vout->dma_queue.next,
-					struct videobuf_buffer, queue);
-			list_del(&vout->next_frm->queue);
-
-			vout->next_frm->state = VIDEOBUF_ACTIVE;
-			addr = (unsigned long)
-				vout->queued_buf_addr[vout->next_frm->i] +
-				vout->cropped_offset;
-			/* First save the configuration in ovelray structure */
-			ret = omapvid_init(vout, addr);
-			if (ret)
-				printk(KERN_ERR VOUT_NAME
-						"failed to set overlay info\n");
-			/* Enable the pipeline and set the Go bit */
-			ret = omapvid_apply_changes(vout);
-			if (ret)
-				printk(KERN_ERR VOUT_NAME
-						"failed to change mode\n");
-		}
+	addr = (unsigned long) vout->queued_buf_addr[vout->next_frm->i]
+		+ vout->cropped_offset;
 
-	}
+	/* First save the configuration in ovelray structure */
+	ret = omapvid_init(vout, addr);
+	if (ret)
+		printk(KERN_ERR VOUT_NAME
+			"failed to set overlay info\n");
+	/* Enable the pipeline and set the Go bit */
+	ret = omapvid_apply_changes(vout);
+	if (ret)
+		printk(KERN_ERR VOUT_NAME "failed to change mode\n");
 
 vout_isr_err:
 	spin_unlock(&vout->vbq_lock);
 }
 
-
 /* Video buffer call backs */
 
 /*
-- 
1.7.1

[PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Archit Taneja]

Currently, in omap_vout_isr(), if the panel type is DPI, and if we
get either VSYNC or VSYNC2 interrupts, we proceed ahead to set the
current buffers state to VIDEOBUF_DONE and prepare to display the
next frame in the queue.

On OMAP4, because we have 2 LCD managers, the panel type itself is not
sufficient to tell if we have received the correct irq, i.e, we shouldn't
proceed ahead if we get a VSYNC interrupt for LCD2 manager, or a VSYNC2
interrupt for LCD manager.

Fix this by correlating LCD manager to VSYNC interrupt and LCD2 manager
to VSYNC2 interrupt.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/media/video/omap/omap_vout.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index c5f2ea0..20638c3 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -566,8 +566,8 @@ err:
 
 static void omap_vout_isr(void *arg, unsigned int irqstatus)
 {
-	int ret, fid;
-	u32 addr;
+	int ret, fid, mgr_id;
+	u32 addr, irq;
 	struct omap_overlay *ovl;
 	struct timeval timevalue;
 	struct omapvideo_info *ovid;
@@ -583,6 +583,7 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus)
 	if (!ovl->manager || !ovl->manager->device)
 		return;
 
+	mgr_id = ovl->manager->id;
 	cur_display = ovl->manager->device;
 
 	spin_lock(&vout->vbq_lock);
@@ -590,7 +591,14 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus)
 
 	switch (cur_display->type) {
 	case OMAP_DISPLAY_TYPE_DPI:
-		if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
+		if (mgr_id == OMAP_DSS_CHANNEL_LCD)
+			irq = DISPC_IRQ_VSYNC;
+		else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
+			irq = DISPC_IRQ_VSYNC2;
+		else
+			goto vout_isr_err;
+
+		if (!(irqstatus & irq))
 			goto vout_isr_err;
 		break;
 	case OMAP_DISPLAY_TYPE_VENC:
-- 
1.7.1

[PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Archit Taneja]

Currently, in omap_vout_isr(), if the panel type is DPI, and if we
get either VSYNC or VSYNC2 interrupts, we proceed ahead to set the
current buffers state to VIDEOBUF_DONE and prepare to display the
next frame in the queue.

On OMAP4, because we have 2 LCD managers, the panel type itself is not
sufficient to tell if we have received the correct irq, i.e, we shouldn't
proceed ahead if we get a VSYNC interrupt for LCD2 manager, or a VSYNC2
interrupt for LCD manager.

Fix this by correlating LCD manager to VSYNC interrupt and LCD2 manager
to VSYNC2 interrupt.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/media/video/omap/omap_vout.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index c5f2ea0..20638c3 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -566,8 +566,8 @@ err:
 
 static void omap_vout_isr(void *arg, unsigned int irqstatus)
 {
-	int ret, fid;
-	u32 addr;
+	int ret, fid, mgr_id;
+	u32 addr, irq;
 	struct omap_overlay *ovl;
 	struct timeval timevalue;
 	struct omapvideo_info *ovid;
@@ -583,6 +583,7 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus)
 	if (!ovl->manager || !ovl->manager->device)
 		return;
 
+	mgr_id = ovl->manager->id;
 	cur_display = ovl->manager->device;
 
 	spin_lock(&vout->vbq_lock);
@@ -590,7 +591,14 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus)
 
 	switch (cur_display->type) {
 	case OMAP_DISPLAY_TYPE_DPI:
-		if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
+		if (mgr_id == OMAP_DSS_CHANNEL_LCD)
+			irq = DISPC_IRQ_VSYNC;
+		else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
+			irq = DISPC_IRQ_VSYNC2;
+		else
+			goto vout_isr_err;
+
+		if (!(irqstatus & irq))
 			goto vout_isr_err;
 		break;
 	case OMAP_DISPLAY_TYPE_VENC:
-- 
1.7.1

[PATCH 4/5] [media] OMAP_VOUT: Add support for DSI panels

[Archit Taneja]

Add support for DSI panels. DSI video mode panels will work directly. For
command mode panels, we will need to trigger updates regularly. This isn't done
by the omap_vout driver currently. It can still be supported if we connect a
framebuffer device to the panel and configure it in auto update mode.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/media/video/omap/omap_vout.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index 20638c3..51cf6c2 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -590,6 +590,7 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus)
 	do_gettimeofday(&timevalue);
 
 	switch (cur_display->type) {
+	case OMAP_DISPLAY_TYPE_DSI:
 	case OMAP_DISPLAY_TYPE_DPI:
 		if (mgr_id == OMAP_DSS_CHANNEL_LCD)
 			irq = DISPC_IRQ_VSYNC;
-- 
1.7.1

[PATCH 4/5] [media] OMAP_VOUT: Add support for DSI panels

[Archit Taneja]

Add support for DSI panels. DSI video mode panels will work directly. For
command mode panels, we will need to trigger updates regularly. This isn't done
by the omap_vout driver currently. It can still be supported if we connect a
framebuffer device to the panel and configure it in auto update mode.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/media/video/omap/omap_vout.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index 20638c3..51cf6c2 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -590,6 +590,7 @@ static void omap_vout_isr(void *arg, unsigned int irqstatus)
 	do_gettimeofday(&timevalue);
 
 	switch (cur_display->type) {
+	case OMAP_DISPLAY_TYPE_DSI:
 	case OMAP_DISPLAY_TYPE_DPI:
 		if (mgr_id == OMAP_DSS_CHANNEL_LCD)
 			irq = DISPC_IRQ_VSYNC;
-- 
1.7.1

[PATCH 5/5] [media]: OMAP_VOUT: Don't trigger updates in omap_vout_probe

[Archit Taneja]

Remove the code in omap_vout_probe() which calls display->driver->update() for
all the displays. This isn't correct because:

- An update in probe doesn't make sense, because we don't have any valid content
  to show at this time.
- Calling update for a panel which isn't enabled is not supported by DSS2. This
  leads to a crash at probe.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/media/video/omap/omap_vout.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index 51cf6c2..a9fcb1a 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -2220,14 +2220,6 @@ static int __init omap_vout_probe(struct platform_device *pdev)
 	if (ret)
 		goto probe_err2;
 
-	for (i = 0; i < vid_dev->num_displays; i++) {
-		struct omap_dss_device *display = vid_dev->displays[i];
-
-		if (display->driver->update)
-			display->driver->update(display, 0, 0,
-					display->panel.timings.x_res,
-					display->panel.timings.y_res);
-	}
 	return 0;
 
 probe_err2:
-- 
1.7.1

[PATCH 5/5] [media]: OMAP_VOUT: Don't trigger updates in omap_vout_probe

[Archit Taneja]

Remove the code in omap_vout_probe() which calls display->driver->update() for
all the displays. This isn't correct because:

- An update in probe doesn't make sense, because we don't have any valid content
  to show at this time.
- Calling update for a panel which isn't enabled is not supported by DSS2. This
  leads to a crash at probe.

Signed-off-by: Archit Taneja <archit@ti.com>
---
 drivers/media/video/omap/omap_vout.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/media/video/omap/omap_vout.c b/drivers/media/video/omap/omap_vout.c
index 51cf6c2..a9fcb1a 100644
--- a/drivers/media/video/omap/omap_vout.c
+++ b/drivers/media/video/omap/omap_vout.c
@@ -2220,14 +2220,6 @@ static int __init omap_vout_probe(struct platform_device *pdev)
 	if (ret)
 		goto probe_err2;
 
-	for (i = 0; i < vid_dev->num_displays; i++) {
-		struct omap_dss_device *display = vid_dev->displays[i];
-
-		if (display->driver->update)
-			display->driver->update(display, 0, 0,
-					display->panel.timings.x_res,
-					display->panel.timings.y_res);
-	}
 	return 0;
 
 probe_err2:
-- 
1.7.1

RE: [PATCH 1/5] [media]: OMAP_VOUT: Fix check in reqbuf & mmap for buf_size allocation

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Taneja, Archit
> Sent: Friday, September 16, 2011 3:30 PM
> To: Hiremath, Vaibhav
> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
> media@vger.kernel.org; Taneja, Archit
> Subject: [PATCH 1/5] [media]: OMAP_VOUT: Fix check in reqbuf & mmap for
> buf_size allocation
> 
> The commit 383e4f69879d11c86ebdd38b3356f6d0690fb4cc makes reqbuf and mmap
> prevent
> requesting a larger size buffer than what is allocated at kernel boot
> during
> omap_vout_probe.
> 
> The requested size is compared with vout->buffer_size, this isn't correct
> as
> vout->buffer_size is later set to the size requested in reqbuf. When the
> video
> device is opened the next time, this check will prevent us to allocate a
> buffer
> which is larger than what we requested the last time.
> 
> Don't use vout->buffer_size, always check with the parameters
> video1_bufsize
> or video2_bufsize.
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
>  drivers/media/video/omap/omap_vout.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/video/omap/omap_vout.c
> b/drivers/media/video/omap/omap_vout.c
> index 95daf98..e14c82b 100644
> --- a/drivers/media/video/omap/omap_vout.c
> +++ b/drivers/media/video/omap/omap_vout.c
> @@ -664,10 +664,14 @@ static int omap_vout_buffer_setup(struct
> videobuf_queue *q, unsigned int *count,
>  	u32 phy_addr = 0, virt_addr = 0;
>  	struct omap_vout_device *vout = q->priv_data;
>  	struct omapvideo_info *ovid = &vout->vid_info;
> +	int vid_max_buf_size;
> 
>  	if (!vout)
>  		return -EINVAL;
> 
> +	vid_max_buf_size = vout->vid == OMAP_VIDEO1 ? video1_bufsize :
> +		video2_bufsize;
> +
>  	if (V4L2_BUF_TYPE_VIDEO_OUTPUT != q->type)
>  		return -EINVAL;
> 
> @@ -690,7 +694,7 @@ static int omap_vout_buffer_setup(struct
> videobuf_queue *q, unsigned int *count,
>  		video1_numbuffers : video2_numbuffers;
> 
>  	/* Check the size of the buffer */
> -	if (*size > vout->buffer_size) {
> +	if (*size > vid_max_buf_size) {
Good catch !!!

>  		v4l2_err(&vout->vid_dev->v4l2_dev,
>  				"buffer allocation mismatch [%u] [%u]\n",
>  				*size, vout->buffer_size);
> @@ -865,6 +869,8 @@ static int omap_vout_mmap(struct file *file, struct
> vm_area_struct *vma)
>  	unsigned long size = (vma->vm_end - vma->vm_start);
>  	struct omap_vout_device *vout = file->private_data;
>  	struct videobuf_queue *q = &vout->vbq;
> +	int vid_max_buf_size = vout->vid == OMAP_VIDEO1 ? video1_bufsize :
> +		video2_bufsize;
> 
>  	v4l2_dbg(1, debug, &vout->vid_dev->v4l2_dev,
>  			" %s pgoff=0x%lx, start=0x%lx, end=0x%lx\n", __func__,
> @@ -887,7 +893,7 @@ static int omap_vout_mmap(struct file *file, struct
> vm_area_struct *vma)
>  		return -EINVAL;
>  	}
>  	/* Check the size of the buffer */
> -	if (size > vout->buffer_size) {
> +	if (size > vid_max_buf_size) {
Don't you think in case of mmap we should still check for the 
vout->buffer_size, since this is the size user has requested in req_buf.

Thanks,
Vaibhav


>  		v4l2_err(&vout->vid_dev->v4l2_dev,
>  				"insufficient memory [%lu] [%u]\n",
>  				size, vout->buffer_size);
> --
> 1.7.1

RE: [PATCH 2/5] [media]: OMAP_VOUT: CLEANUP: Remove redundant code from omap_vout_isr

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Taneja, Archit
> Sent: Friday, September 16, 2011 3:31 PM
> To: Hiremath, Vaibhav
> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
> media@vger.kernel.org; Taneja, Archit
> Subject: [PATCH 2/5] [media]: OMAP_VOUT: CLEANUP: Remove redundant code
> from omap_vout_isr
> 
> Currently, there is a lot of redundant code is between DPI and VENC panels,
> this
> can be made common by moving out field/interlace specific code to a
> separate
> function called omapvid_handle_interlace_display(). There is no functional
> change made.
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
>  drivers/media/video/omap/omap_vout.c |  172 ++++++++++++++++-------------
> -----
>  1 files changed, 82 insertions(+), 90 deletions(-)
> 
> diff --git a/drivers/media/video/omap/omap_vout.c
> b/drivers/media/video/omap/omap_vout.c
> index e14c82b..c5f2ea0 100644
> --- a/drivers/media/video/omap/omap_vout.c
> +++ b/drivers/media/video/omap/omap_vout.c
> @@ -524,10 +524,50 @@ static int omapvid_apply_changes(struct
> omap_vout_device *vout)
>  	return 0;
>  }
> 
> +static int omapvid_handle_interlace_display(struct omap_vout_device *vout,
> +		unsigned int irqstatus, struct timeval timevalue)
> +{
> +	u32 fid;
> +
> +	if (vout->first_int) {
> +		vout->first_int = 0;
> +		goto err;
> +	}
> +
> +	if (irqstatus & DISPC_IRQ_EVSYNC_ODD)
> +		fid = 1;
> +	else if (irqstatus & DISPC_IRQ_EVSYNC_EVEN)
> +		fid = 0;
> +	else
> +		goto err;
> +
> +	vout->field_id ^= 1;
> +	if (fid != vout->field_id) {
> +		/* reset field ID */
> +		vout->field_id = 0;
[Hiremath, Vaibhav] You should check whether fid == 0 before resetting it.

> +	} else if (0 == fid) {
[Hiremath, Vaibhav] This is not matching with the original code, probably I have to be more careful here. I need to spend more time here...


> +		if (vout->cur_frm == vout->next_frm)
> +			goto err;
> +
> +		vout->cur_frm->ts = timevalue;
> +		vout->cur_frm->state = VIDEOBUF_DONE;
> +		wake_up_interruptible(&vout->cur_frm->done);
> +		vout->cur_frm = vout->next_frm;
> +	} else {
> +		if (list_empty(&vout->dma_queue) ||
> +				(vout->cur_frm != vout->next_frm))
> +			goto err;
> +	}
> +
> +	return vout->field_id;
> +err:
> +	return 0;
> +}
> +
>  static void omap_vout_isr(void *arg, unsigned int irqstatus)
>  {
> -	int ret;
> -	u32 addr, fid;
> +	int ret, fid;
> +	u32 addr;
>  	struct omap_overlay *ovl;
>  	struct timeval timevalue;
>  	struct omapvideo_info *ovid;
> @@ -548,107 +588,59 @@ static void omap_vout_isr(void *arg, unsigned int
> irqstatus)
>  	spin_lock(&vout->vbq_lock);
>  	do_gettimeofday(&timevalue);
> 
> -	if (cur_display->type != OMAP_DISPLAY_TYPE_VENC) {
> -		switch (cur_display->type) {
> -		case OMAP_DISPLAY_TYPE_DPI:
> -			if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
> -				goto vout_isr_err;
> -			break;
> -		case OMAP_DISPLAY_TYPE_HDMI:
> -			if (!(irqstatus & DISPC_IRQ_EVSYNC_EVEN))
> -				goto vout_isr_err;
> -			break;
> -		default:
> +	switch (cur_display->type) {
> +	case OMAP_DISPLAY_TYPE_DPI:
> +		if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
>  			goto vout_isr_err;
> -		}
> -		if (!vout->first_int && (vout->cur_frm != vout->next_frm)) {
> -			vout->cur_frm->ts = timevalue;
> -			vout->cur_frm->state = VIDEOBUF_DONE;
> -			wake_up_interruptible(&vout->cur_frm->done);
> -			vout->cur_frm = vout->next_frm;
> -		}
> -		vout->first_int = 0;
> -		if (list_empty(&vout->dma_queue))
> +		break;
> +	case OMAP_DISPLAY_TYPE_VENC:
> +		fid = omapvid_handle_interlace_display(vout, irqstatus,
> +				timevalue);
> +		if (!fid)
>  			goto vout_isr_err;
[Hiremath, Vaibhav] 
Have you tested TV out functionality? 

> +		break;
> +	case OMAP_DISPLAY_TYPE_HDMI:
> +		if (!(irqstatus & DISPC_IRQ_EVSYNC_EVEN))
> +			goto vout_isr_err;
> +		break;
> +	default:
> +		goto vout_isr_err;
> +	}
> 
> -		vout->next_frm = list_entry(vout->dma_queue.next,
> -				struct videobuf_buffer, queue);
> -		list_del(&vout->next_frm->queue);
> -
> -		vout->next_frm->state = VIDEOBUF_ACTIVE;
> -
> -		addr = (unsigned long) vout->queued_buf_addr[vout->next_frm-
> >i]
> -			+ vout->cropped_offset;
> +	if (!vout->first_int && (vout->cur_frm != vout->next_frm)) {
> +		vout->cur_frm->ts = timevalue;
> +		vout->cur_frm->state = VIDEOBUF_DONE;
> +		wake_up_interruptible(&vout->cur_frm->done);
> +		vout->cur_frm = vout->next_frm;
> +	}
> 
> -		/* First save the configuration in ovelray structure */
> -		ret = omapvid_init(vout, addr);
> -		if (ret)
> -			printk(KERN_ERR VOUT_NAME
> -				"failed to set overlay info\n");
> -		/* Enable the pipeline and set the Go bit */
> -		ret = omapvid_apply_changes(vout);
> -		if (ret)
> -			printk(KERN_ERR VOUT_NAME "failed to change mode\n");
> -	} else {
> +	vout->first_int = 0;
> +	if (list_empty(&vout->dma_queue))
> +		goto vout_isr_err;
> 
> -		if (vout->first_int) {
> -			vout->first_int = 0;
> -			goto vout_isr_err;
> -		}
> -		if (irqstatus & DISPC_IRQ_EVSYNC_ODD)
> -			fid = 1;
> -		else if (irqstatus & DISPC_IRQ_EVSYNC_EVEN)
> -			fid = 0;
> -		else
> -			goto vout_isr_err;
> +	vout->next_frm = list_entry(vout->dma_queue.next,
> +			struct videobuf_buffer, queue);
> +	list_del(&vout->next_frm->queue);
> 
> -		vout->field_id ^= 1;
> -		if (fid != vout->field_id) {
> -			if (0 == fid)
> -				vout->field_id = fid;
> +	vout->next_frm->state = VIDEOBUF_ACTIVE;
> 
> -			goto vout_isr_err;
> -		}
> -		if (0 == fid) {
> -			if (vout->cur_frm == vout->next_frm)
> -				goto vout_isr_err;
> -
> -			vout->cur_frm->ts = timevalue;
> -			vout->cur_frm->state = VIDEOBUF_DONE;
> -			wake_up_interruptible(&vout->cur_frm->done);
> -			vout->cur_frm = vout->next_frm;
> -		} else if (1 == fid) {
> -			if (list_empty(&vout->dma_queue) ||
> -					(vout->cur_frm != vout->next_frm))
> -				goto vout_isr_err;
> -
> -			vout->next_frm = list_entry(vout->dma_queue.next,
> -					struct videobuf_buffer, queue);
> -			list_del(&vout->next_frm->queue);
> -
> -			vout->next_frm->state = VIDEOBUF_ACTIVE;
> -			addr = (unsigned long)
> -				vout->queued_buf_addr[vout->next_frm->i] +
> -				vout->cropped_offset;
> -			/* First save the configuration in ovelray structure */
> -			ret = omapvid_init(vout, addr);
> -			if (ret)
> -				printk(KERN_ERR VOUT_NAME
> -						"failed to set overlay info\n");
> -			/* Enable the pipeline and set the Go bit */
> -			ret = omapvid_apply_changes(vout);
> -			if (ret)
> -				printk(KERN_ERR VOUT_NAME
> -						"failed to change mode\n");
> -		}
> +	addr = (unsigned long) vout->queued_buf_addr[vout->next_frm->i]
> +		+ vout->cropped_offset;
> 
> -	}
> +	/* First save the configuration in ovelray structure */
> +	ret = omapvid_init(vout, addr);
> +	if (ret)
> +		printk(KERN_ERR VOUT_NAME
> +			"failed to set overlay info\n");
> +	/* Enable the pipeline and set the Go bit */
> +	ret = omapvid_apply_changes(vout);
> +	if (ret)
> +		printk(KERN_ERR VOUT_NAME "failed to change mode\n");
> 
>  vout_isr_err:
>  	spin_unlock(&vout->vbq_lock);
>  }
[Hiremath, Vaibhav] Overall this clean-up was required, thanks for working on this patch.

Thanks,
Vaibhav
> 
> -
>  /* Video buffer call backs */
> 
>  /*
> --
> 1.7.1

Re: [PATCH 2/5] [media]: OMAP_VOUT: CLEANUP: Remove redundant code from omap_vout_isr

[Archit Taneja]

Hi,

On Wednesday 21 September 2011 03:35 PM, Hiremath, Vaibhav wrote:
>
>> -----Original Message-----
>> From: Taneja, Archit
>> Sent: Friday, September 16, 2011 3:31 PM
>> To: Hiremath, Vaibhav
>> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
>> media@vger.kernel.org; Taneja, Archit
>> Subject: [PATCH 2/5] [media]: OMAP_VOUT: CLEANUP: Remove redundant code
>> from omap_vout_isr
>>
>> Currently, there is a lot of redundant code is between DPI and VENC panels,
>> this
>> can be made common by moving out field/interlace specific code to a
>> separate
>> function called omapvid_handle_interlace_display(). There is no functional
>> change made.
>>
>> Signed-off-by: Archit Taneja<archit@ti.com>
>> ---
>>   drivers/media/video/omap/omap_vout.c |  172 ++++++++++++++++-------------
>> -----
>>   1 files changed, 82 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/media/video/omap/omap_vout.c
>> b/drivers/media/video/omap/omap_vout.c
>> index e14c82b..c5f2ea0 100644
>> --- a/drivers/media/video/omap/omap_vout.c
>> +++ b/drivers/media/video/omap/omap_vout.c
>> @@ -524,10 +524,50 @@ static int omapvid_apply_changes(struct
>> omap_vout_device *vout)
>>   	return 0;
>>   }
>>
>> +static int omapvid_handle_interlace_display(struct omap_vout_device *vout,
>> +		unsigned int irqstatus, struct timeval timevalue)
>> +{
>> +	u32 fid;
>> +
>> +	if (vout->first_int) {
>> +		vout->first_int = 0;
>> +		goto err;
>> +	}
>> +
>> +	if (irqstatus&  DISPC_IRQ_EVSYNC_ODD)
>> +		fid = 1;
>> +	else if (irqstatus&  DISPC_IRQ_EVSYNC_EVEN)
>> +		fid = 0;
>> +	else
>> +		goto err;
>> +
>> +	vout->field_id ^= 1;
>> +	if (fid != vout->field_id) {
>> +		/* reset field ID */
>> +		vout->field_id = 0;
> [Hiremath, Vaibhav] You should check whether fid == 0 before resetting it.
>
>> +	} else if (0 == fid) {
> [Hiremath, Vaibhav] This is not matching with the original code, probably I have to be more careful here. I need to spend more time here...

If you do a dry run of it you'll see that it does the same thing 
functionally. If fid was 1, vout->field_id would have been 0 anyway.
So the check for fid == 0 looked a bit redundant to me. However, if you 
think that doing this makes the code less clear, we can surely keep this 
check.

>
>
>> +		if (vout->cur_frm == vout->next_frm)
>> +			goto err;
>> +
>> +		vout->cur_frm->ts = timevalue;
>> +		vout->cur_frm->state = VIDEOBUF_DONE;
>> +		wake_up_interruptible(&vout->cur_frm->done);
>> +		vout->cur_frm = vout->next_frm;
>> +	} else {
>> +		if (list_empty(&vout->dma_queue) ||
>> +				(vout->cur_frm != vout->next_frm))
>> +			goto err;
>> +	}
>> +
>> +	return vout->field_id;
>> +err:
>> +	return 0;
>> +}
>> +
>>   static void omap_vout_isr(void *arg, unsigned int irqstatus)
>>   {
>> -	int ret;
>> -	u32 addr, fid;
>> +	int ret, fid;
>> +	u32 addr;
>>   	struct omap_overlay *ovl;
>>   	struct timeval timevalue;
>>   	struct omapvideo_info *ovid;
>> @@ -548,107 +588,59 @@ static void omap_vout_isr(void *arg, unsigned int
>> irqstatus)
>>   	spin_lock(&vout->vbq_lock);
>>   	do_gettimeofday(&timevalue);
>>
>> -	if (cur_display->type != OMAP_DISPLAY_TYPE_VENC) {
>> -		switch (cur_display->type) {
>> -		case OMAP_DISPLAY_TYPE_DPI:
>> -			if (!(irqstatus&  (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
>> -				goto vout_isr_err;
>> -			break;
>> -		case OMAP_DISPLAY_TYPE_HDMI:
>> -			if (!(irqstatus&  DISPC_IRQ_EVSYNC_EVEN))
>> -				goto vout_isr_err;
>> -			break;
>> -		default:
>> +	switch (cur_display->type) {
>> +	case OMAP_DISPLAY_TYPE_DPI:
>> +		if (!(irqstatus&  (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
>>   			goto vout_isr_err;
>> -		}
>> -		if (!vout->first_int&&  (vout->cur_frm != vout->next_frm)) {
>> -			vout->cur_frm->ts = timevalue;
>> -			vout->cur_frm->state = VIDEOBUF_DONE;
>> -			wake_up_interruptible(&vout->cur_frm->done);
>> -			vout->cur_frm = vout->next_frm;
>> -		}
>> -		vout->first_int = 0;
>> -		if (list_empty(&vout->dma_queue))
>> +		break;
>> +	case OMAP_DISPLAY_TYPE_VENC:
>> +		fid = omapvid_handle_interlace_display(vout, irqstatus,
>> +				timevalue);
>> +		if (!fid)
>>   			goto vout_isr_err;
> [Hiremath, Vaibhav]
> Have you tested TV out functionality?

I haven't checked it yet to be totally honest. Its hard to find a VENC 
TV! I wanted to anyway get some kind of Ack from you before starting to 
test this. Since you also feel that this clean up is needed, I'll start 
testing this out :)

>
>> +		break;
>> +	case OMAP_DISPLAY_TYPE_HDMI:
>> +		if (!(irqstatus&  DISPC_IRQ_EVSYNC_EVEN))
>> +			goto vout_isr_err;
>> +		break;
>> +	default:
>> +		goto vout_isr_err;
>> +	}
>>
>> -		vout->next_frm = list_entry(vout->dma_queue.next,
>> -				struct videobuf_buffer, queue);
>> -		list_del(&vout->next_frm->queue);
>> -
>> -		vout->next_frm->state = VIDEOBUF_ACTIVE;
>> -
>> -		addr = (unsigned long) vout->queued_buf_addr[vout->next_frm-
>>> i]
>> -			+ vout->cropped_offset;
>> +	if (!vout->first_int&&  (vout->cur_frm != vout->next_frm)) {
>> +		vout->cur_frm->ts = timevalue;
>> +		vout->cur_frm->state = VIDEOBUF_DONE;
>> +		wake_up_interruptible(&vout->cur_frm->done);
>> +		vout->cur_frm = vout->next_frm;
>> +	}
>>
>> -		/* First save the configuration in ovelray structure */
>> -		ret = omapvid_init(vout, addr);
>> -		if (ret)
>> -			printk(KERN_ERR VOUT_NAME
>> -				"failed to set overlay info\n");
>> -		/* Enable the pipeline and set the Go bit */
>> -		ret = omapvid_apply_changes(vout);
>> -		if (ret)
>> -			printk(KERN_ERR VOUT_NAME "failed to change mode\n");
>> -	} else {
>> +	vout->first_int = 0;
>> +	if (list_empty(&vout->dma_queue))
>> +		goto vout_isr_err;
>>
>> -		if (vout->first_int) {
>> -			vout->first_int = 0;
>> -			goto vout_isr_err;
>> -		}
>> -		if (irqstatus&  DISPC_IRQ_EVSYNC_ODD)
>> -			fid = 1;
>> -		else if (irqstatus&  DISPC_IRQ_EVSYNC_EVEN)
>> -			fid = 0;
>> -		else
>> -			goto vout_isr_err;
>> +	vout->next_frm = list_entry(vout->dma_queue.next,
>> +			struct videobuf_buffer, queue);
>> +	list_del(&vout->next_frm->queue);
>>
>> -		vout->field_id ^= 1;
>> -		if (fid != vout->field_id) {
>> -			if (0 == fid)
>> -				vout->field_id = fid;
>> +	vout->next_frm->state = VIDEOBUF_ACTIVE;
>>
>> -			goto vout_isr_err;
>> -		}
>> -		if (0 == fid) {
>> -			if (vout->cur_frm == vout->next_frm)
>> -				goto vout_isr_err;
>> -
>> -			vout->cur_frm->ts = timevalue;
>> -			vout->cur_frm->state = VIDEOBUF_DONE;
>> -			wake_up_interruptible(&vout->cur_frm->done);
>> -			vout->cur_frm = vout->next_frm;
>> -		} else if (1 == fid) {
>> -			if (list_empty(&vout->dma_queue) ||
>> -					(vout->cur_frm != vout->next_frm))
>> -				goto vout_isr_err;
>> -
>> -			vout->next_frm = list_entry(vout->dma_queue.next,
>> -					struct videobuf_buffer, queue);
>> -			list_del(&vout->next_frm->queue);
>> -
>> -			vout->next_frm->state = VIDEOBUF_ACTIVE;
>> -			addr = (unsigned long)
>> -				vout->queued_buf_addr[vout->next_frm->i] +
>> -				vout->cropped_offset;
>> -			/* First save the configuration in ovelray structure */
>> -			ret = omapvid_init(vout, addr);
>> -			if (ret)
>> -				printk(KERN_ERR VOUT_NAME
>> -						"failed to set overlay info\n");
>> -			/* Enable the pipeline and set the Go bit */
>> -			ret = omapvid_apply_changes(vout);
>> -			if (ret)
>> -				printk(KERN_ERR VOUT_NAME
>> -						"failed to change mode\n");
>> -		}
>> +	addr = (unsigned long) vout->queued_buf_addr[vout->next_frm->i]
>> +		+ vout->cropped_offset;
>>
>> -	}
>> +	/* First save the configuration in ovelray structure */
>> +	ret = omapvid_init(vout, addr);
>> +	if (ret)
>> +		printk(KERN_ERR VOUT_NAME
>> +			"failed to set overlay info\n");
>> +	/* Enable the pipeline and set the Go bit */
>> +	ret = omapvid_apply_changes(vout);
>> +	if (ret)
>> +		printk(KERN_ERR VOUT_NAME "failed to change mode\n");
>>
>>   vout_isr_err:
>>   	spin_unlock(&vout->vbq_lock);
>>   }
> [Hiremath, Vaibhav] Overall this clean-up was required, thanks for working on this patch.

Thanks for the review!

Archit

>
> Thanks,
> Vaibhav
>>
>> -
>>   /* Video buffer call backs */
>>
>>   /*
>> --
>> 1.7.1
>
>

Re: [PATCH 1/5] [media]: OMAP_VOUT: Fix check in reqbuf & mmap for buf_size allocation

[Archit Taneja]

Hi,

On Wednesday 21 September 2011 02:10 PM, Hiremath, Vaibhav wrote:
>
>> -----Original Message-----
>> From: Taneja, Archit
>> Sent: Friday, September 16, 2011 3:30 PM
>> To: Hiremath, Vaibhav
>> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
>> media@vger.kernel.org; Taneja, Archit
>> Subject: [PATCH 1/5] [media]: OMAP_VOUT: Fix check in reqbuf&  mmap for
>> buf_size allocation
>>
>> The commit 383e4f69879d11c86ebdd38b3356f6d0690fb4cc makes reqbuf and mmap
>> prevent
>> requesting a larger size buffer than what is allocated at kernel boot
>> during
>> omap_vout_probe.
>>
>> The requested size is compared with vout->buffer_size, this isn't correct
>> as
>> vout->buffer_size is later set to the size requested in reqbuf. When the
>> video
>> device is opened the next time, this check will prevent us to allocate a
>> buffer
>> which is larger than what we requested the last time.
>>
>> Don't use vout->buffer_size, always check with the parameters
>> video1_bufsize
>> or video2_bufsize.
>>
>> Signed-off-by: Archit Taneja<archit@ti.com>
>> ---
>>   drivers/media/video/omap/omap_vout.c |   10 ++++++++--
>>   1 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/video/omap/omap_vout.c
>> b/drivers/media/video/omap/omap_vout.c
>> index 95daf98..e14c82b 100644
>> --- a/drivers/media/video/omap/omap_vout.c
>> +++ b/drivers/media/video/omap/omap_vout.c
>> @@ -664,10 +664,14 @@ static int omap_vout_buffer_setup(struct
>> videobuf_queue *q, unsigned int *count,
>>   	u32 phy_addr = 0, virt_addr = 0;
>>   	struct omap_vout_device *vout = q->priv_data;
>>   	struct omapvideo_info *ovid =&vout->vid_info;
>> +	int vid_max_buf_size;
>>
>>   	if (!vout)
>>   		return -EINVAL;
>>
>> +	vid_max_buf_size = vout->vid == OMAP_VIDEO1 ? video1_bufsize :
>> +		video2_bufsize;
>> +
>>   	if (V4L2_BUF_TYPE_VIDEO_OUTPUT != q->type)
>>   		return -EINVAL;
>>
>> @@ -690,7 +694,7 @@ static int omap_vout_buffer_setup(struct
>> videobuf_queue *q, unsigned int *count,
>>   		video1_numbuffers : video2_numbuffers;
>>
>>   	/* Check the size of the buffer */
>> -	if (*size>  vout->buffer_size) {
>> +	if (*size>  vid_max_buf_size) {
> Good catch !!!
>
>>   		v4l2_err(&vout->vid_dev->v4l2_dev,
>>   				"buffer allocation mismatch [%u] [%u]\n",
>>   				*size, vout->buffer_size);
>> @@ -865,6 +869,8 @@ static int omap_vout_mmap(struct file *file, struct
>> vm_area_struct *vma)
>>   	unsigned long size = (vma->vm_end - vma->vm_start);
>>   	struct omap_vout_device *vout = file->private_data;
>>   	struct videobuf_queue *q =&vout->vbq;
>> +	int vid_max_buf_size = vout->vid == OMAP_VIDEO1 ? video1_bufsize :
>> +		video2_bufsize;
>>
>>   	v4l2_dbg(1, debug,&vout->vid_dev->v4l2_dev,
>>   			" %s pgoff=0x%lx, start=0x%lx, end=0x%lx\n", __func__,
>> @@ -887,7 +893,7 @@ static int omap_vout_mmap(struct file *file, struct
>> vm_area_struct *vma)
>>   		return -EINVAL;
>>   	}
>>   	/* Check the size of the buffer */
>> -	if (size>  vout->buffer_size) {
>> +	if (size>  vid_max_buf_size) {
> Don't you think in case of mmap we should still check for the
> vout->buffer_size, since this is the size user has requested in req_buf.

Ah, you are right, the check for the maximum size should only be in the 
reqbuf path. vout->buffer_size would have been updated correctly at time 
of mmap. I'll change this back to vout->buffer_size.

Thanks,
Archit

>
> Thanks,
> Vaibhav
>
>
>>   		v4l2_err(&vout->vid_dev->v4l2_dev,
>>   				"insufficient memory [%lu] [%u]\n",
>>   				size, vout->buffer_size);
>> --
>> 1.7.1
>
>

RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Taneja, Archit
> Sent: Friday, September 16, 2011 3:31 PM
> To: Hiremath, Vaibhav
> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
> media@vger.kernel.org; Taneja, Archit
> Subject: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
> omap_vout_isr
> 
> Currently, in omap_vout_isr(), if the panel type is DPI, and if we
> get either VSYNC or VSYNC2 interrupts, we proceed ahead to set the
> current buffers state to VIDEOBUF_DONE and prepare to display the
> next frame in the queue.
> 
> On OMAP4, because we have 2 LCD managers, the panel type itself is not
> sufficient to tell if we have received the correct irq, i.e, we shouldn't
> proceed ahead if we get a VSYNC interrupt for LCD2 manager, or a VSYNC2
> interrupt for LCD manager.
> 
> Fix this by correlating LCD manager to VSYNC interrupt and LCD2 manager
> to VSYNC2 interrupt.
> 
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
>  drivers/media/video/omap/omap_vout.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/video/omap/omap_vout.c
> b/drivers/media/video/omap/omap_vout.c
> index c5f2ea0..20638c3 100644
> --- a/drivers/media/video/omap/omap_vout.c
> +++ b/drivers/media/video/omap/omap_vout.c
> @@ -566,8 +566,8 @@ err:
> 
>  static void omap_vout_isr(void *arg, unsigned int irqstatus)
>  {
> -	int ret, fid;
> -	u32 addr;
> +	int ret, fid, mgr_id;
> +	u32 addr, irq;
>  	struct omap_overlay *ovl;
>  	struct timeval timevalue;
>  	struct omapvideo_info *ovid;
> @@ -583,6 +583,7 @@ static void omap_vout_isr(void *arg, unsigned int
> irqstatus)
>  	if (!ovl->manager || !ovl->manager->device)
>  		return;
> 
> +	mgr_id = ovl->manager->id;
>  	cur_display = ovl->manager->device;
> 
>  	spin_lock(&vout->vbq_lock);
> @@ -590,7 +591,14 @@ static void omap_vout_isr(void *arg, unsigned int
> irqstatus)
> 
>  	switch (cur_display->type) {
>  	case OMAP_DISPLAY_TYPE_DPI:
> -		if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
> +		if (mgr_id == OMAP_DSS_CHANNEL_LCD)
> +			irq = DISPC_IRQ_VSYNC;
> +		else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
> +			irq = DISPC_IRQ_VSYNC2;
> +		else
> +			goto vout_isr_err;
> +
> +		if (!(irqstatus & irq))
>  			goto vout_isr_err;
>  		break;
I understand what this patch meant for, but I am more curious to know
What is value addition of this patch?

if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))

Vs

if (mgr_id == OMAP_DSS_CHANNEL_LCD)
	irq = DISPC_IRQ_VSYNC;
else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
	irq = DISPC_IRQ_VSYNC2;

Isn't this same, because we are not doing anything separate and special
for VSYNC and VSYNC2?

Thanks,
Vaibhav


>  	case OMAP_DISPLAY_TYPE_VENC:
> --
> 1.7.1

Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Archit Taneja]

Hi,

On Wednesday 21 September 2011 07:04 PM, Hiremath, Vaibhav wrote:
>> -----Original Message-----
>> From: Taneja, Archit
>> Sent: Friday, September 16, 2011 3:31 PM
>> To: Hiremath, Vaibhav
>> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
>> media@vger.kernel.org; Taneja, Archit
>> Subject: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
>> omap_vout_isr
>>
>> Currently, in omap_vout_isr(), if the panel type is DPI, and if we
>> get either VSYNC or VSYNC2 interrupts, we proceed ahead to set the
>> current buffers state to VIDEOBUF_DONE and prepare to display the
>> next frame in the queue.
>>
>> On OMAP4, because we have 2 LCD managers, the panel type itself is not
>> sufficient to tell if we have received the correct irq, i.e, we shouldn't
>> proceed ahead if we get a VSYNC interrupt for LCD2 manager, or a VSYNC2
>> interrupt for LCD manager.
>>
>> Fix this by correlating LCD manager to VSYNC interrupt and LCD2 manager
>> to VSYNC2 interrupt.
>>
>> Signed-off-by: Archit Taneja<archit@ti.com>
>> ---
>>   drivers/media/video/omap/omap_vout.c |   14 +++++++++++---
>>   1 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/video/omap/omap_vout.c
>> b/drivers/media/video/omap/omap_vout.c
>> index c5f2ea0..20638c3 100644
>> --- a/drivers/media/video/omap/omap_vout.c
>> +++ b/drivers/media/video/omap/omap_vout.c
>> @@ -566,8 +566,8 @@ err:
>>
>>   static void omap_vout_isr(void *arg, unsigned int irqstatus)
>>   {
>> -	int ret, fid;
>> -	u32 addr;
>> +	int ret, fid, mgr_id;
>> +	u32 addr, irq;
>>   	struct omap_overlay *ovl;
>>   	struct timeval timevalue;
>>   	struct omapvideo_info *ovid;
>> @@ -583,6 +583,7 @@ static void omap_vout_isr(void *arg, unsigned int
>> irqstatus)
>>   	if (!ovl->manager || !ovl->manager->device)
>>   		return;
>>
>> +	mgr_id = ovl->manager->id;
>>   	cur_display = ovl->manager->device;
>>
>>   	spin_lock(&vout->vbq_lock);
>> @@ -590,7 +591,14 @@ static void omap_vout_isr(void *arg, unsigned int
>> irqstatus)
>>
>>   	switch (cur_display->type) {
>>   	case OMAP_DISPLAY_TYPE_DPI:
>> -		if (!(irqstatus&  (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
>> +		if (mgr_id == OMAP_DSS_CHANNEL_LCD)
>> +			irq = DISPC_IRQ_VSYNC;
>> +		else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
>> +			irq = DISPC_IRQ_VSYNC2;
>> +		else
>> +			goto vout_isr_err;
>> +
>> +		if (!(irqstatus&  irq))
>>   			goto vout_isr_err;
>>   		break;
> I understand what this patch meant for, but I am more curious to know
> What is value addition of this patch?
>
> if (!(irqstatus&  (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
>
> Vs
>
> if (mgr_id == OMAP_DSS_CHANNEL_LCD)
> 	irq = DISPC_IRQ_VSYNC;
> else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
> 	irq = DISPC_IRQ_VSYNC2;
>
> Isn't this same, because we are not doing anything separate and special
> for VSYNC and VSYNC2?

Consider a use case where the primary LCD panel(connected to LCD 
manager) is configured at a 60 Hz refresh rate, and the secondary 
panel(connected to LCD2) refreshes at 30 Hz. Let the overlay 
configuration be something like this:

/dev/video1----->overlay1----->LCD manager----->Primary panel(60 Hz)


/dev/video2----->overlay2----->LCD2 manager----->Secondary Panel(30 Hz)


Now if we are doing streaming on both video1 and video2, since we deque 
on either VSYNC or VSYNC2, video2 buffers will deque faster than the 
panels refresh, which is incorrect. So for video2, we should only deque 
when we get a VSYNC2 interrupt. Thats why there is a need to correlate 
the interrupt and the overlay manager.

Regards,
Archit

>
> Thanks,
> Vaibhav
>
>
>>   	case OMAP_DISPLAY_TYPE_VENC:
>> --
>> 1.7.1
>
>

Re: [PATCH 2/5] [media]: OMAP_VOUT: CLEANUP: Remove redundant code from omap_vout_isr

[Archit Taneja]

Hi,

On Wednesday 21 September 2011 04:15 PM, Taneja, Archit wrote:
> Hi,
>
> On Wednesday 21 September 2011 03:35 PM, Hiremath, Vaibhav wrote:
>>
>>> -----Original Message-----
>>> From: Taneja, Archit
>>> Sent: Friday, September 16, 2011 3:31 PM
>>> To: Hiremath, Vaibhav
>>> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
>>> media@vger.kernel.org; Taneja, Archit
>>> Subject: [PATCH 2/5] [media]: OMAP_VOUT: CLEANUP: Remove redundant code
>>> from omap_vout_isr
>>>
>>> Currently, there is a lot of redundant code is between DPI and VENC panels,
>>> this
>>> can be made common by moving out field/interlace specific code to a
>>> separate
>>> function called omapvid_handle_interlace_display(). There is no functional
>>> change made.
>>>
>>> Signed-off-by: Archit Taneja<archit@ti.com>
>>> ---
>>>    drivers/media/video/omap/omap_vout.c |  172 ++++++++++++++++-------------
>>> -----


>> [Hiremath, Vaibhav]
>> Have you tested TV out functionality?
>
> I haven't checked it yet to be totally honest. Its hard to find a VENC
> TV! I wanted to anyway get some kind of Ack from you before starting to
> test this. Since you also feel that this clean up is needed, I'll start
> testing this out :)

I tested the TV out functionality. It works fine. I have left the extra 
fid == 0 check so that the code is more clear. Will post out the new 
patch soon.

Archit

RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Taneja, Archit
> Sent: Thursday, September 22, 2011 11:46 AM
> To: Hiremath, Vaibhav
> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
> media@vger.kernel.org
> Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
> omap_vout_isr
> 
> Hi,
> 
> On Wednesday 21 September 2011 07:04 PM, Hiremath, Vaibhav wrote:
> >> -----Original Message-----
> >> From: Taneja, Archit
> >> Sent: Friday, September 16, 2011 3:31 PM
> >> To: Hiremath, Vaibhav
> >> Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
> >> media@vger.kernel.org; Taneja, Archit
> >> Subject: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
> >> omap_vout_isr
> >>
> >> Currently, in omap_vout_isr(), if the panel type is DPI, and if we
> >> get either VSYNC or VSYNC2 interrupts, we proceed ahead to set the
> >> current buffers state to VIDEOBUF_DONE and prepare to display the
> >> next frame in the queue.
> >>
> >> On OMAP4, because we have 2 LCD managers, the panel type itself is not
> >> sufficient to tell if we have received the correct irq, i.e, we
> shouldn't
> >> proceed ahead if we get a VSYNC interrupt for LCD2 manager, or a VSYNC2
> >> interrupt for LCD manager.
> >>
> >> Fix this by correlating LCD manager to VSYNC interrupt and LCD2 manager
> >> to VSYNC2 interrupt.
> >>
> >> Signed-off-by: Archit Taneja<archit@ti.com>
> >> ---
> >>   drivers/media/video/omap/omap_vout.c |   14 +++++++++++---
> >>   1 files changed, 11 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/media/video/omap/omap_vout.c
> >> b/drivers/media/video/omap/omap_vout.c
> >> index c5f2ea0..20638c3 100644
> >> --- a/drivers/media/video/omap/omap_vout.c
> >> +++ b/drivers/media/video/omap/omap_vout.c
> >> @@ -566,8 +566,8 @@ err:
> >>
> >>   static void omap_vout_isr(void *arg, unsigned int irqstatus)
> >>   {
> >> -	int ret, fid;
> >> -	u32 addr;
> >> +	int ret, fid, mgr_id;
> >> +	u32 addr, irq;
> >>   	struct omap_overlay *ovl;
> >>   	struct timeval timevalue;
> >>   	struct omapvideo_info *ovid;
> >> @@ -583,6 +583,7 @@ static void omap_vout_isr(void *arg, unsigned int
> >> irqstatus)
> >>   	if (!ovl->manager || !ovl->manager->device)
> >>   		return;
> >>
> >> +	mgr_id = ovl->manager->id;
> >>   	cur_display = ovl->manager->device;
> >>
> >>   	spin_lock(&vout->vbq_lock);
> >> @@ -590,7 +591,14 @@ static void omap_vout_isr(void *arg, unsigned int
> >> irqstatus)
> >>
> >>   	switch (cur_display->type) {
> >>   	case OMAP_DISPLAY_TYPE_DPI:
> >> -		if (!(irqstatus&  (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
> >> +		if (mgr_id == OMAP_DSS_CHANNEL_LCD)
> >> +			irq = DISPC_IRQ_VSYNC;
> >> +		else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
> >> +			irq = DISPC_IRQ_VSYNC2;
> >> +		else
> >> +			goto vout_isr_err;
> >> +
> >> +		if (!(irqstatus&  irq))
> >>   			goto vout_isr_err;
> >>   		break;
> > I understand what this patch meant for, but I am more curious to know
> > What is value addition of this patch?
> >
> > if (!(irqstatus&  (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
> >
> > Vs
> >
> > if (mgr_id == OMAP_DSS_CHANNEL_LCD)
> > 	irq = DISPC_IRQ_VSYNC;
> > else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
> > 	irq = DISPC_IRQ_VSYNC2;
> >
> > Isn't this same, because we are not doing anything separate and special
> > for VSYNC and VSYNC2?
> 
> Consider a use case where the primary LCD panel(connected to LCD
> manager) is configured at a 60 Hz refresh rate, and the secondary
> panel(connected to LCD2) refreshes at 30 Hz. Let the overlay
> configuration be something like this:
> 
> /dev/video1----->overlay1----->LCD manager----->Primary panel(60 Hz)
> 
> 
> /dev/video2----->overlay2----->LCD2 manager----->Secondary Panel(30 Hz)
> 
> 
> Now if we are doing streaming on both video1 and video2, since we deque
> on either VSYNC or VSYNC2, video2 buffers will deque faster than the
> panels refresh, which is incorrect. So for video2, we should only deque
> when we get a VSYNC2 interrupt. Thats why there is a need to correlate
> the interrupt and the overlay manager.
> 

Archit,

I think I am still not understood or convinced with your description above,

The code snippet which we are referring here, does check whether the 
interrupt is either VSYNC or VSYNC2, else fall back to "vout_isr_err".

For me both are doing same thing, the original code is optimized way of implementation. Can you please review it again?

Thanks,
Vaibhav



> Regards,
> Archit
> 
> >
> > Thanks,
> > Vaibhav
> >
> >
> >>   	case OMAP_DISPLAY_TYPE_VENC:
> >> --
> >> 1.7.1
> >
> >

Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Semwal, Sumit]

Hi Vaibhav,
>>
>> On Mon, Sep 26, 2011 at 3:49 PM, Hiremath, Vaibhav <hvaibhav@ti.com> wrote:
>>>
>>> > -----Original Message-----
>>> > From: Taneja, Archit
>>> > Sent: Thursday, September 22, 2011 11:46 AM
>>> > To: Hiremath, Vaibhav
>>> > Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
>>> > media@vger.kernel.org
>>> > Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
>>> > omap_vout_isr
>>> >
>>> > Hi,
>
>   <snip>
>>>
>>> > >> -          if (!(irqstatus&  (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
>>> > >> +          if (mgr_id == OMAP_DSS_CHANNEL_LCD)
>>> > >> +                  irq = DISPC_IRQ_VSYNC;
>>> > >> +          else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
>>> > >> +                  irq = DISPC_IRQ_VSYNC2;
>>> > >> +          else
>>> > >> +                  goto vout_isr_err;
>>> > >> +
>>> > >> +          if (!(irqstatus&  irq))
>>> > >>                    goto vout_isr_err;
>>> > >>            break;
>>> > > I understand what this patch meant for, but I am more curious to know
>>> > > What is value addition of this patch?
>>> > >
>>> > > if (!(irqstatus&  (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
>>> > >
>>> > > Vs
>>> > >
>>> > > if (mgr_id == OMAP_DSS_CHANNEL_LCD)
>>> > >     irq = DISPC_IRQ_VSYNC;
>>> > > else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
>>> > >     irq = DISPC_IRQ_VSYNC2;
>>> > >
>>> > > Isn't this same, because we are not doing anything separate and special
>>> > > for VSYNC and VSYNC2?
>>> >
>>> > Consider a use case where the primary LCD panel(connected to LCD
>>> > manager) is configured at a 60 Hz refresh rate, and the secondary
>>> > panel(connected to LCD2) refreshes at 30 Hz. Let the overlay
>>> > configuration be something like this:
>>> >
>>> > /dev/video1----->overlay1----->LCD manager----->Primary panel(60 Hz)
>>> >
>>> >
>>> > /dev/video2----->overlay2----->LCD2 manager----->Secondary Panel(30 Hz)
>>> >
>>> >
>>> > Now if we are doing streaming on both video1 and video2, since we deque
>>> > on either VSYNC or VSYNC2, video2 buffers will deque faster than the
>>> > panels refresh, which is incorrect. So for video2, we should only deque
>>> > when we get a VSYNC2 interrupt. Thats why there is a need to correlate
>>> > the interrupt and the overlay manager.
>>> >
>>>
>>> Archit,
>>>
>>> I think I am still not understood or convinced with your description above,
>>>
>>> The code snippet which we are referring here, does check whether the
>>> interrupt is either VSYNC or VSYNC2, else fall back to "vout_isr_err".
>
>
I am not quite sure I understand what is the confusing part here -
below is my understanding; please correct me if you think otherwise.
As I understand, this patch creates a (missing) correlation between a
manager and the corresponding ISR. The earlier code would accept a
VSYNC2 for LCD1 manager, which is not the correct thing to do.
That is why the check of 'if ((mgr==LCD) && (IRQ==VSYNC))' kind of
thing is needed; Which part of this do you think the above patch
doesn't do? Or, do you think it is not needed / done correctly?
>>>
>>> For me both are doing same thing, the original code is optimized way of implementation. Can you please review it again?
>>>
>>> Thanks,
>>> Vaibhav
>
>
Thanks and best regards,
~Sumit.
>>>
>>> <snip>

Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Semwal, Sumit]

Hi Vaibhav,
>>
>> On Mon, Sep 26, 2011 at 3:49 PM, Hiremath, Vaibhav <hvaibhav@ti.com> wrote:
>>>
>>> > -----Original Message-----
>>> > From: Taneja, Archit
>>> > Sent: Thursday, September 22, 2011 11:46 AM
>>> > To: Hiremath, Vaibhav
>>> > Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit; linux-
>>> > media@vger.kernel.org
>>> > Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
>>> > omap_vout_isr
>>> >
>>> > Hi,
>
>   <snip>
>>>
>>> > >> -          if (!(irqstatus&  (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
>>> > >> +          if (mgr_id == OMAP_DSS_CHANNEL_LCD)
>>> > >> +                  irq = DISPC_IRQ_VSYNC;
>>> > >> +          else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
>>> > >> +                  irq = DISPC_IRQ_VSYNC2;
>>> > >> +          else
>>> > >> +                  goto vout_isr_err;
>>> > >> +
>>> > >> +          if (!(irqstatus&  irq))
>>> > >>                    goto vout_isr_err;
>>> > >>            break;
>>> > > I understand what this patch meant for, but I am more curious to know
>>> > > What is value addition of this patch?
>>> > >
>>> > > if (!(irqstatus&  (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
>>> > >
>>> > > Vs
>>> > >
>>> > > if (mgr_id == OMAP_DSS_CHANNEL_LCD)
>>> > >     irq = DISPC_IRQ_VSYNC;
>>> > > else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
>>> > >     irq = DISPC_IRQ_VSYNC2;
>>> > >
>>> > > Isn't this same, because we are not doing anything separate and special
>>> > > for VSYNC and VSYNC2?
>>> >
>>> > Consider a use case where the primary LCD panel(connected to LCD
>>> > manager) is configured at a 60 Hz refresh rate, and the secondary
>>> > panel(connected to LCD2) refreshes at 30 Hz. Let the overlay
>>> > configuration be something like this:
>>> >
>>> > /dev/video1----->overlay1----->LCD manager----->Primary panel(60 Hz)
>>> >
>>> >
>>> > /dev/video2----->overlay2----->LCD2 manager----->Secondary Panel(30 Hz)
>>> >
>>> >
>>> > Now if we are doing streaming on both video1 and video2, since we deque
>>> > on either VSYNC or VSYNC2, video2 buffers will deque faster than the
>>> > panels refresh, which is incorrect. So for video2, we should only deque
>>> > when we get a VSYNC2 interrupt. Thats why there is a need to correlate
>>> > the interrupt and the overlay manager.
>>> >
>>>
>>> Archit,
>>>
>>> I think I am still not understood or convinced with your description above,
>>>
>>> The code snippet which we are referring here, does check whether the
>>> interrupt is either VSYNC or VSYNC2, else fall back to "vout_isr_err".
>
>
I am not quite sure I understand what is the confusing part here -
below is my understanding; please correct me if you think otherwise.
As I understand, this patch creates a (missing) correlation between a
manager and the corresponding ISR. The earlier code would accept a
VSYNC2 for LCD1 manager, which is not the correct thing to do.
That is why the check of 'if ((mgr==LCD) && (IRQ==VSYNC))' kind of
thing is needed; Which part of this do you think the above patch
doesn't do? Or, do you think it is not needed / done correctly?
>>>
>>> For me both are doing same thing, the original code is optimized way of implementation. Can you please review it again?
>>>
>>> Thanks,
>>> Vaibhav
>
>
Thanks and best regards,
~Sumit.
>>>
>>> <snip>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Semwal, Sumit
> Sent: Tuesday, September 27, 2011 11:12 AM
> To: Hiremath, Vaibhav
> Cc: Taneja, Archit; Valkeinen, Tomi; linux-omap@vger.kernel.org; linux-
> media@vger.kernel.org
> Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
> omap_vout_isr
> 
> Hi Vaibhav,
> >>
> >> On Mon, Sep 26, 2011 at 3:49 PM, Hiremath, Vaibhav <hvaibhav@ti.com>
> wrote:
> >>>
> >>> > -----Original Message-----
> >>> > From: Taneja, Archit
> >>> > Sent: Thursday, September 22, 2011 11:46 AM
> >>> > To: Hiremath, Vaibhav
> >>> > Cc: Valkeinen, Tomi; linux-omap@vger.kernel.org; Semwal, Sumit;
> linux-
> >>> > media@vger.kernel.org
> >>> > Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling
> in
> >>> > omap_vout_isr
> >>> >
> >>> > Hi,
> >
> >   <snip>
> >>>
> >>> > >> -          if (!(irqstatus&  (DISPC_IRQ_VSYNC |
> DISPC_IRQ_VSYNC2)))
> >>> > >> +          if (mgr_id == OMAP_DSS_CHANNEL_LCD)
> >>> > >> +                  irq = DISPC_IRQ_VSYNC;
> >>> > >> +          else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
> >>> > >> +                  irq = DISPC_IRQ_VSYNC2;
> >>> > >> +          else
> >>> > >> +                  goto vout_isr_err;
> >>> > >> +
> >>> > >> +          if (!(irqstatus&  irq))
> >>> > >>                    goto vout_isr_err;
> >>> > >>            break;
> >>> > > I understand what this patch meant for, but I am more curious to
> know
> >>> > > What is value addition of this patch?
> >>> > >
> >>> > > if (!(irqstatus&  (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
> >>> > >
> >>> > > Vs
> >>> > >
> >>> > > if (mgr_id == OMAP_DSS_CHANNEL_LCD)
> >>> > >     irq = DISPC_IRQ_VSYNC;
> >>> > > else if (mgr_id == OMAP_DSS_CHANNEL_LCD2)
> >>> > >     irq = DISPC_IRQ_VSYNC2;
> >>> > >
> >>> > > Isn't this same, because we are not doing anything separate and
> special
> >>> > > for VSYNC and VSYNC2?
> >>> >
> >>> > Consider a use case where the primary LCD panel(connected to LCD
> >>> > manager) is configured at a 60 Hz refresh rate, and the secondary
> >>> > panel(connected to LCD2) refreshes at 30 Hz. Let the overlay
> >>> > configuration be something like this:
> >>> >
> >>> > /dev/video1----->overlay1----->LCD manager----->Primary panel(60 Hz)
> >>> >
> >>> >
> >>> > /dev/video2----->overlay2----->LCD2 manager----->Secondary Panel(30
> Hz)
> >>> >
> >>> >
> >>> > Now if we are doing streaming on both video1 and video2, since we
> deque
> >>> > on either VSYNC or VSYNC2, video2 buffers will deque faster than the
> >>> > panels refresh, which is incorrect. So for video2, we should only
> deque
> >>> > when we get a VSYNC2 interrupt. Thats why there is a need to
> correlate
> >>> > the interrupt and the overlay manager.
> >>> >
> >>>
> >>> Archit,
> >>>
> >>> I think I am still not understood or convinced with your description
> above,
> >>>
> >>> The code snippet which we are referring here, does check whether the
> >>> interrupt is either VSYNC or VSYNC2, else fall back to "vout_isr_err".
> >
> >
> I am not quite sure I understand what is the confusing part here -
> below is my understanding; please correct me if you think otherwise.
> As I understand, this patch creates a (missing) correlation between a
> manager and the corresponding ISR. The earlier code would accept a
> VSYNC2 for LCD1 manager, which is not the correct thing to do.
> That is why the check of 'if ((mgr==LCD) && (IRQ==VSYNC))' kind of
> thing is needed; Which part of this do you think the above patch
> doesn't do? Or, do you think it is not needed / done correctly?
Sumit,

Please look at the patch carefully, it does exactly same thing. I understand the use-case what Archit explained in the last email but in this patch context, the use-case change anything here in this patch. 

Can you review it carefully again?

Thanks,
Vaibhav
> >>>
> >>> For me both are doing same thing, the original code is optimized way
> of implementation. Can you please review it again?
> >>>
> >>> Thanks,
> >>> Vaibhav
> >
> >
> Thanks and best regards,
> ~Sumit.
> >>>
> >>> <snip>

RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Tomi Valkeinen]

On Tue, 2011-09-27 at 12:09 +0530, Hiremath, Vaibhav wrote:
> Please look at the patch carefully, it does exactly same thing. I
> understand the use-case what Archit explained in the last email but in
> this patch context, the use-case change anything here in this patch. 

With the current code, the ISR code will be ran for a panel connected to
LCD1 output when VSYNC for LCD2 happens.

After Archit's patch, this no longer happens.

I don't know what the ISR code does, so it may not cause any problems,
but it sure doesn't sound right running the code when a wrong interrupt
happens.

 Tomi

Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Semwal, Sumit]

Hi Vaibhav,

On Tue, Sep 27, 2011 at 12:09 PM, Hiremath, Vaibhav <hvaibhav@ti.com> wrote:
>> -----Original Message-----
>> From: Semwal, Sumit
>> Sent: Tuesday, September 27, 2011 11:12 AM
>> To: Hiremath, Vaibhav
>> Cc: Taneja, Archit; Valkeinen, Tomi; linux-omap@vger.kernel.org; linux-
>> media@vger.kernel.org
>> Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
>> omap_vout_isr
>>
>> Hi Vaibhav,
<snip>
>> >>> Archit,
>> >>>
>> >>> I think I am still not understood or convinced with your description
>> above,
>> >>>
>> >>> The code snippet which we are referring here, does check whether the
>> >>> interrupt is either VSYNC or VSYNC2, else fall back to "vout_isr_err".
>> >
>> >
>> I am not quite sure I understand what is the confusing part here -
>> below is my understanding; please correct me if you think otherwise.
>> As I understand, this patch creates a (missing) correlation between a
>> manager and the corresponding ISR. The earlier code would accept a
>> VSYNC2 for LCD1 manager, which is not the correct thing to do.
>> That is why the check of 'if ((mgr==LCD) && (IRQ==VSYNC))' kind of
>> thing is needed; Which part of this do you think the above patch
>> doesn't do? Or, do you think it is not needed / done correctly?
> Sumit,
>
> Please look at the patch carefully, it does exactly same thing. I understand the use-case what Archit explained in the last email but in this patch context, the use-case change anything here in this patch.
>
> Can you review it carefully again?
Thanks - I did review it carefully (the first time, and again), and
maybe it is something that you're able to see which I can't.

Could you please explain why you think

(1)
if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
    goto vout_isr_err;

is *exactly* the same as

(2)
 if (!((mgr==LCD) && (irqstatus & DISPC_IRQ_VSYNC)) || (mgr==LCD2) &&
(irqstatus & DISPC_IRQ_VSYNC2)) )
   goto vout_isr_err;
[which I understand is what Archit's patch does]

I am not able to see any correlation in (1) between mgr and irq,
whereas it is quite clear in (2).

Let me know if I missed something?

Best regards,
~Sumit.
>
> Thanks,
> Vaibhav
<snip>

Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Semwal, Sumit]

Hi Vaibhav,

On Tue, Sep 27, 2011 at 12:09 PM, Hiremath, Vaibhav <hvaibhav@ti.com> wrote:
>> -----Original Message-----
>> From: Semwal, Sumit
>> Sent: Tuesday, September 27, 2011 11:12 AM
>> To: Hiremath, Vaibhav
>> Cc: Taneja, Archit; Valkeinen, Tomi; linux-omap@vger.kernel.org; linux-
>> media@vger.kernel.org
>> Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
>> omap_vout_isr
>>
>> Hi Vaibhav,
<snip>
>> >>> Archit,
>> >>>
>> >>> I think I am still not understood or convinced with your description
>> above,
>> >>>
>> >>> The code snippet which we are referring here, does check whether the
>> >>> interrupt is either VSYNC or VSYNC2, else fall back to "vout_isr_err".
>> >
>> >
>> I am not quite sure I understand what is the confusing part here -
>> below is my understanding; please correct me if you think otherwise.
>> As I understand, this patch creates a (missing) correlation between a
>> manager and the corresponding ISR. The earlier code would accept a
>> VSYNC2 for LCD1 manager, which is not the correct thing to do.
>> That is why the check of 'if ((mgr==LCD) && (IRQ==VSYNC))' kind of
>> thing is needed; Which part of this do you think the above patch
>> doesn't do? Or, do you think it is not needed / done correctly?
> Sumit,
>
> Please look at the patch carefully, it does exactly same thing. I understand the use-case what Archit explained in the last email but in this patch context, the use-case change anything here in this patch.
>
> Can you review it carefully again?
Thanks - I did review it carefully (the first time, and again), and
maybe it is something that you're able to see which I can't.

Could you please explain why you think

(1)
if (!(irqstatus & (DISPC_IRQ_VSYNC | DISPC_IRQ_VSYNC2)))
    goto vout_isr_err;

is *exactly* the same as

(2)
 if (!((mgr==LCD) && (irqstatus & DISPC_IRQ_VSYNC)) || (mgr==LCD2) &&
(irqstatus & DISPC_IRQ_VSYNC2)) )
   goto vout_isr_err;
[which I understand is what Archit's patch does]

I am not able to see any correlation in (1) between mgr and irq,
whereas it is quite clear in (2).

Let me know if I missed something?

Best regards,
~Sumit.
>
> Thanks,
> Vaibhav
<snip>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Valkeinen, Tomi
> Sent: Tuesday, September 27, 2011 12:19 PM
> To: Hiremath, Vaibhav
> Cc: Semwal, Sumit; Taneja, Archit; linux-omap@vger.kernel.org; linux-
> media@vger.kernel.org
> Subject: RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
> omap_vout_isr
> 
> On Tue, 2011-09-27 at 12:09 +0530, Hiremath, Vaibhav wrote:
> > Please look at the patch carefully, it does exactly same thing. I
> > understand the use-case what Archit explained in the last email but in
> > this patch context, the use-case change anything here in this patch.
> 
> With the current code, the ISR code will be ran for a panel connected to
> LCD1 output when VSYNC for LCD2 happens.
> 
> After Archit's patch, this no longer happens.
> 
> I don't know what the ISR code does, so it may not cause any problems,
> but it sure doesn't sound right running the code when a wrong interrupt
> happens.
> 

If you look at the patch, the patch barely checks for the condition and
makes sure that the interrupt is either of VSYNC or VSYNC2, else return. Rest everything is same.

The right fix is in streamon api, where you mask the interrupt before
registering it.

Thanks,
Vaibhav

>  Tomi
> 

Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Archit Taneja]

On Tuesday 27 September 2011 12:24 PM, Hiremath, Vaibhav wrote:
>> -----Original Message-----
>> From: Valkeinen, Tomi
>> Sent: Tuesday, September 27, 2011 12:19 PM
>> To: Hiremath, Vaibhav
>> Cc: Semwal, Sumit; Taneja, Archit; linux-omap@vger.kernel.org; linux-
>> media@vger.kernel.org
>> Subject: RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
>> omap_vout_isr
>>
>> On Tue, 2011-09-27 at 12:09 +0530, Hiremath, Vaibhav wrote:
>>> Please look at the patch carefully, it does exactly same thing. I
>>> understand the use-case what Archit explained in the last email but in
>>> this patch context, the use-case change anything here in this patch.
>>
>> With the current code, the ISR code will be ran for a panel connected to
>> LCD1 output when VSYNC for LCD2 happens.
>>
>> After Archit's patch, this no longer happens.
>>
>> I don't know what the ISR code does, so it may not cause any problems,
>> but it sure doesn't sound right running the code when a wrong interrupt
>> happens.
>>
>
> If you look at the patch, the patch barely checks for the condition and
> makes sure that the interrupt is either of VSYNC or VSYNC2, else return. Rest everything is same.

It doesn't only make sure that the interrupt it one of them, it uses it 
later too in the check:

if (!(irqstatus & irq))
	goto vout_isr_err;
...
...

>
> The right fix is in streamon api, where you mask the interrupt before
> registering it.

If this is the right fix, we should have a purely selective method of 
selecting the interrupts. Even for OMAP3, we register interrupts for LCD 
and TV, and then check the interrupt in the handler using panel type. 
Now, since have 2 different interrupts for the same panel type, we have 
to further distinguish using the manager id.

Archit

>
> Thanks,
> Vaibhav
>
>>   Tomi
>>
>
>

RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Tomi Valkeinen]

On Tue, 2011-09-27 at 12:24 +0530, Hiremath, Vaibhav wrote:
> If you look at the patch, the patch barely checks for the condition
> and
> makes sure that the interrupt is either of VSYNC or VSYNC2, else
> return. Rest everything is same.

This is what the current code does, in clearer form and slightly pseudo
code:

if (irq == VSYNC || irq == VSYNC2) {
	do isr stuff
}

This is what it does after Archit's patch:

if ((lcd == LCD1 && irq == VSYNC) || (lcd == LCD2 && irq == VSYNC2)) {
	do isr stuff;
}

I see a clear difference there. Or am I missing something?

> The right fix is in streamon api, where you mask the interrupt before
> registering it.

I'm not familiar with v4l so I don't know what that means, but yes, it
would be better if it's possible to only register for the needed
interrupts.

But the ISR code is still needed. If you are using both LCDs, you will
get both interrupts and you need to check if the interrupt is for the
right output.

 Tomi

RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in omap_vout_isr

[Hiremath, Vaibhav]

> -----Original Message-----
> From: Taneja, Archit
> Sent: Tuesday, September 27, 2011 12:32 PM
> To: Hiremath, Vaibhav
> Cc: Valkeinen, Tomi; Semwal, Sumit; linux-omap@vger.kernel.org; linux-
> media@vger.kernel.org
> Subject: Re: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
> omap_vout_isr
> 
> On Tuesday 27 September 2011 12:24 PM, Hiremath, Vaibhav wrote:
> >> -----Original Message-----
> >> From: Valkeinen, Tomi
> >> Sent: Tuesday, September 27, 2011 12:19 PM
> >> To: Hiremath, Vaibhav
> >> Cc: Semwal, Sumit; Taneja, Archit; linux-omap@vger.kernel.org; linux-
> >> media@vger.kernel.org
> >> Subject: RE: [PATCH 3/5] [media]: OMAP_VOUT: Fix VSYNC IRQ handling in
> >> omap_vout_isr
> >>
> >> On Tue, 2011-09-27 at 12:09 +0530, Hiremath, Vaibhav wrote:
> >>> Please look at the patch carefully, it does exactly same thing. I
> >>> understand the use-case what Archit explained in the last email but in
> >>> this patch context, the use-case change anything here in this patch.
> >>
> >> With the current code, the ISR code will be ran for a panel connected
> to
> >> LCD1 output when VSYNC for LCD2 happens.
> >>
> >> After Archit's patch, this no longer happens.
> >>
> >> I don't know what the ISR code does, so it may not cause any problems,
> >> but it sure doesn't sound right running the code when a wrong interrupt
> >> happens.
> >>
> >
> > If you look at the patch, the patch barely checks for the condition and
> > makes sure that the interrupt is either of VSYNC or VSYNC2, else return.
> Rest everything is same.
> 
> It doesn't only make sure that the interrupt it one of them, it uses it
> later too in the check:
> 
> if (!(irqstatus & irq))
> 	goto vout_isr_err;
> ...
> ...
> 
> >
> > The right fix is in streamon api, where you mask the interrupt before
> > registering it.
> 
> If this is the right fix, we should have a purely selective method of
> selecting the interrupts. Even for OMAP3, we register interrupts for LCD
> and TV, and then check the interrupt in the handler using panel type.
> Now, since have 2 different interrupts for the same panel type, we have
> to further distinguish using the manager id.
> 
I have to agree here with you that we do not have this available now.

Also I had reviewed the patch again, and I think I now understand what usecase you are referring here. My bad, I concluded early on this....

Thanks,
Vaibhav

> Archit
> 
> >
> > Thanks,
> > Vaibhav
> >
> >>   Tomi
> >>
> >
> >