Re: [PATCH v2 3/3] media: au0828 remove video and vbi buffer timeout work-around

2015-01-12 Thread Hans Verkuil 
On 12/18/2014 05:20 PM, Shuah Khan wrote:
 au0828 does video and vbi buffer timeout handling to prevent
 applications such as tvtime from hanging by ensuring that the
 video frames continue to be delivered even when the ITU-656
 input isn't receiving any data. This work-around is complex
 as it introduces set and clear tier code paths in start/stop
 streaming, and close interfaces. After the vb2 conversion, the
 timeout handling is introducing instability as well as feeding
 too many blank green screens, resulting in degraded video quality.

Why would this result in degraded video quality? And which instability
exactly?

 Without this timeout handling, both xawtv, and tvtime are working
 well with good quality video.

Erm, tvtime without the recent 'tvtime: don't block indefinitely waiting
for frames' patch will not work well with au0828 without the timeout code
if there is no valid video data.

This should at minimum be mentioned in the commit log.

Especially if this fixes other issues as you mentioned in the log I am
all for it. But others will probably object to this.

Regards,

Hans

 
 Signed-off-by: Shuah Khan shua...@osg.samsung.com
 ---
  drivers/media/usb/au0828/au0828-video.c | 103 
 +---
  drivers/media/usb/au0828/au0828.h   |   5 --
  2 files changed, 1 insertion(+), 107 deletions(-)
 
 diff --git a/drivers/media/usb/au0828/au0828-video.c 
 b/drivers/media/usb/au0828/au0828-video.c
 index ef49b2e..3bdf132 100644
 --- a/drivers/media/usb/au0828/au0828-video.c
 +++ b/drivers/media/usb/au0828/au0828-video.c
 @@ -593,15 +593,6 @@ static inline int au0828_isoc_copy(struct au0828_dev 
 *dev, struct urb *urb)
   outp = NULL;
   else
   outp = vb2_plane_vaddr(buf-vb, 0);
 -
 - /* As long as isoc traffic is arriving, keep
 -resetting the timer */
 - if (dev-vid_timeout_running)
 - mod_timer(dev-vid_timeout,
 -   jiffies + (HZ / 10));
 - if (dev-vbi_timeout_running)
 - mod_timer(dev-vbi_timeout,
 -   jiffies + (HZ / 10));
   }
  
   if (buf != NULL) {
 @@ -804,15 +795,9 @@ int au0828_start_analog_streaming(struct vb2_queue *vq, 
 unsigned int count)
   return rc;
   }
  
 - if (vq-type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 + if (vq-type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
   v4l2_device_call_all(dev-v4l2_dev, 0, video,
   s_stream, 1);
 - dev-vid_timeout_running = 1;
 - mod_timer(dev-vid_timeout, jiffies + (HZ / 10));
 - } else if (vq-type == V4L2_BUF_TYPE_VBI_CAPTURE) {
 - dev-vbi_timeout_running = 1;
 - mod_timer(dev-vbi_timeout, jiffies + (HZ / 10));
 - }
   }
   dev-streaming_users++;
   return rc;
 @@ -851,9 +836,6 @@ static void au0828_stop_streaming(struct vb2_queue *vq)
   (AUVI_INPUT(i).audio_setup)(dev, 0);
   }
   spin_unlock_irqrestore(dev-slock, flags);
 -
 - dev-vid_timeout_running = 0;
 - del_timer_sync(dev-vid_timeout);
  }
  
  void au0828_stop_vbi_streaming(struct vb2_queue *vq)
 @@ -882,9 +864,6 @@ void au0828_stop_vbi_streaming(struct vb2_queue *vq)
   vb2_buffer_done(buf-vb, VB2_BUF_STATE_ERROR);
   }
   spin_unlock_irqrestore(dev-slock, flags);
 -
 - dev-vbi_timeout_running = 0;
 - del_timer_sync(dev-vbi_timeout);
  }
  
  static struct vb2_ops au0828_video_qops = {
 @@ -917,56 +896,6 @@ void au0828_analog_unregister(struct au0828_dev *dev)
   mutex_unlock(au0828_sysfs_lock);
  }
  
 -/* This function ensures that video frames continue to be delivered even if
 -   the ITU-656 input isn't receiving any data (thereby preventing 
 applications
 -   such as tvtime from hanging) */
 -static void au0828_vid_buffer_timeout(unsigned long data)
 -{
 - struct au0828_dev *dev = (struct au0828_dev *) data;
 - struct au0828_dmaqueue *dma_q = dev-vidq;
 - struct au0828_buffer *buf;
 - unsigned char *vid_data;
 - unsigned long flags = 0;
 -
 - spin_lock_irqsave(dev-slock, flags);
 -
 - buf = dev-isoc_ctl.buf;
 - if (buf != NULL) {
 - vid_data = vb2_plane_vaddr(buf-vb, 0);
 - memset(vid_data, 0x00, buf-length); /* Blank green frame */
 - buffer_filled(dev, dma_q, buf);
 - }
 - get_next_buf(dma_q, buf);
 -
 - if (dev-vid_timeout_running == 1)
 - mod_timer(dev-vid_timeout, jiffies + (HZ / 10));
 -
 - spin_unlock_irqrestore(dev-slock, flags);
 -}
 -
 -static void

Re: [PATCH v2 3/3] media: au0828 remove video and vbi buffer timeout work-around

2015-01-12 Thread Shuah Khan 
On 01/12/2015 07:10 AM, Hans Verkuil wrote:
 On 12/18/2014 05:20 PM, Shuah Khan wrote:
 au0828 does video and vbi buffer timeout handling to prevent
 applications such as tvtime from hanging by ensuring that the
 video frames continue to be delivered even when the ITU-656
 input isn't receiving any data. This work-around is complex
 as it introduces set and clear tier code paths in start/stop
 streaming, and close interfaces. After the vb2 conversion, the
 timeout handling is introducing instability as well as feeding
 too many blank green screens, resulting in degraded video quality.
 
 Why would this result in degraded video quality? And which instability
 exactly?

What I noticed was that I was seeing a few too many green screens
and I had to re-tune xawtv when the timeout code is in place. My
thinking was that this timeout handling could be introducing blank
green frames when there is no need. However, I can't reproduce the
problem on 3.19-rc4 base which is what I am using to test the changes
to the patch series. Hence, I am not positive if the timeout code
indeed was doing anything bad.

 
 Without this timeout handling, both xawtv, and tvtime are working
 well with good quality video.

I am seeing tvtime hangs.

 
 Erm, tvtime without the recent 'tvtime: don't block indefinitely waiting
 for frames' patch will not work well with au0828 without the timeout code
 if there is no valid video data.
 
 This should at minimum be mentioned in the commit log.

I will resend the patch with the updated commit log knowing full well
that it might not be accepted.

I do have to re-cut the patch after the changes to address your
comments on the vb2 conversion patch. It applies, with fuzz, so
I decided to re-cut the patch.

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shua...@osg.samsung.com | (970) 217-8978
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] media: au0828 remove video and vbi buffer timeout work-around

2014-12-18 Thread Shuah Khan 
au0828 does video and vbi buffer timeout handling to prevent
applications such as tvtime from hanging by ensuring that the
video frames continue to be delivered even when the ITU-656
input isn't receiving any data. This work-around is complex
as it introduces set and clear tier code paths in start/stop
streaming, and close interfaces. After the vb2 conversion, the
timeout handling is introducing instability as well as feeding
too many blank green screens, resulting in degraded video quality.
Without this timeout handling, both xawtv, and tvtime are working
well with good quality video.

Signed-off-by: Shuah Khan shua...@osg.samsung.com
---
 drivers/media/usb/au0828/au0828-video.c | 103 +---
 drivers/media/usb/au0828/au0828.h   |   5 --
 2 files changed, 1 insertion(+), 107 deletions(-)

diff --git a/drivers/media/usb/au0828/au0828-video.c 
b/drivers/media/usb/au0828/au0828-video.c
index ef49b2e..3bdf132 100644
--- a/drivers/media/usb/au0828/au0828-video.c
+++ b/drivers/media/usb/au0828/au0828-video.c
@@ -593,15 +593,6 @@ static inline int au0828_isoc_copy(struct au0828_dev *dev, 
struct urb *urb)
outp = NULL;
else
outp = vb2_plane_vaddr(buf-vb, 0);
-
-   /* As long as isoc traffic is arriving, keep
-  resetting the timer */
-   if (dev-vid_timeout_running)
-   mod_timer(dev-vid_timeout,
- jiffies + (HZ / 10));
-   if (dev-vbi_timeout_running)
-   mod_timer(dev-vbi_timeout,
- jiffies + (HZ / 10));
}
 
if (buf != NULL) {
@@ -804,15 +795,9 @@ int au0828_start_analog_streaming(struct vb2_queue *vq, 
unsigned int count)
return rc;
}
 
-   if (vq-type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
+   if (vq-type == V4L2_BUF_TYPE_VIDEO_CAPTURE)
v4l2_device_call_all(dev-v4l2_dev, 0, video,
s_stream, 1);
-   dev-vid_timeout_running = 1;
-   mod_timer(dev-vid_timeout, jiffies + (HZ / 10));
-   } else if (vq-type == V4L2_BUF_TYPE_VBI_CAPTURE) {
-   dev-vbi_timeout_running = 1;
-   mod_timer(dev-vbi_timeout, jiffies + (HZ / 10));
-   }
}
dev-streaming_users++;
return rc;
@@ -851,9 +836,6 @@ static void au0828_stop_streaming(struct vb2_queue *vq)
(AUVI_INPUT(i).audio_setup)(dev, 0);
}
spin_unlock_irqrestore(dev-slock, flags);
-
-   dev-vid_timeout_running = 0;
-   del_timer_sync(dev-vid_timeout);
 }
 
 void au0828_stop_vbi_streaming(struct vb2_queue *vq)
@@ -882,9 +864,6 @@ void au0828_stop_vbi_streaming(struct vb2_queue *vq)
vb2_buffer_done(buf-vb, VB2_BUF_STATE_ERROR);
}
spin_unlock_irqrestore(dev-slock, flags);
-
-   dev-vbi_timeout_running = 0;
-   del_timer_sync(dev-vbi_timeout);
 }
 
 static struct vb2_ops au0828_video_qops = {
@@ -917,56 +896,6 @@ void au0828_analog_unregister(struct au0828_dev *dev)
mutex_unlock(au0828_sysfs_lock);
 }
 
-/* This function ensures that video frames continue to be delivered even if
-   the ITU-656 input isn't receiving any data (thereby preventing applications
-   such as tvtime from hanging) */
-static void au0828_vid_buffer_timeout(unsigned long data)
-{
-   struct au0828_dev *dev = (struct au0828_dev *) data;
-   struct au0828_dmaqueue *dma_q = dev-vidq;
-   struct au0828_buffer *buf;
-   unsigned char *vid_data;
-   unsigned long flags = 0;
-
-   spin_lock_irqsave(dev-slock, flags);
-
-   buf = dev-isoc_ctl.buf;
-   if (buf != NULL) {
-   vid_data = vb2_plane_vaddr(buf-vb, 0);
-   memset(vid_data, 0x00, buf-length); /* Blank green frame */
-   buffer_filled(dev, dma_q, buf);
-   }
-   get_next_buf(dma_q, buf);
-
-   if (dev-vid_timeout_running == 1)
-   mod_timer(dev-vid_timeout, jiffies + (HZ / 10));
-
-   spin_unlock_irqrestore(dev-slock, flags);
-}
-
-static void au0828_vbi_buffer_timeout(unsigned long data)
-{
-   struct au0828_dev *dev = (struct au0828_dev *) data;
-   struct au0828_dmaqueue *dma_q = dev-vbiq;
-   struct au0828_buffer *buf;
-   unsigned char *vbi_data;
-   unsigned long flags = 0;
-
-   spin_lock_irqsave(dev-slock, flags);
-
-   buf = dev-isoc_ctl.vbi_buf;
-   if (buf != NULL) {
-   vbi_data = vb2_plane_vaddr(buf-vb, 0);
-   memset(vbi_data, 0x00, buf-length);
-   vbi_buffer_filled(dev, dma_q,