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

2015-01-13 Thread Devin Heitmueller
> I couldn't reproduce what I was seeing when I did patch v2 series
> work. 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.

IIRC, the timer was set for 40ms, so if a complete video frame doesn't
arrive within that interval we generate a green frame.  It was never
really intended to have perfect clocking (i.e. 29.97 FPS), but is
really just there to prevent the tvtime user interface from blocking
indefinitely.

If you weren't seeing it in the V2 series, then I guess you fixed
whatever bug was present in V1.

> I am seeing tvtime hangs without the timeout. I am fine with this
> patch not going. It does make the code cleaner and also reduces
> buffer handling during streaming. However, there is a clear regression
> to tvtime.

Correct.  I think everybody agrees that the timer code is ugly and it
would be cleaner if it wasn't needed - except it clearly is needed to
prevent regressions in tvtime.

All that said, I'm quite excited to see the driver converted to VB2.  Nice job!

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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


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

2015-01-13 Thread Shuah Khan
On 01/12/2015 09:44 PM, Devin Heitmueller wrote:
> Hi Shuah,
> 
> On Mon, Jan 12, 2015 at 9:56 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 timer code paths in start/stop
>> streaming, and close interfaces. However, tvtime will hang
>> without the following tvtime change:
> 
> I'm confused.  When we last debated whether this patch would be
> accepted, the last message from Mauro said the following:
> 
>> That means that we'll need to keep holding such timeout code for
>> years, until all distros update to a new tvtime, of course assuming
>> that this is the only one application with such issue.
> 
> In other words, the timeout code has to stay in there since otherwise
> it will cause ABI breakage.  It's great that Hans has submitted a
> patch to improve tvtime, and over the next couple of years that patch
> might actually start to appear in distributions.  That unfortunately
> doesn't change the fact that everybody who updates their kernel (or
> has it updated for them as part of their distribution) will go from
> "works fine" to "completely broken".
> 
> The driver was working before the VB2 conversion, so if there is now
> instability then it's likely that some bug was introduced during the
> conversion to VB2.  Simply ripping out the timeout code seems like the
> wrong approach to addressing a regression likely caused by your own
> VB2 conversion patch.
> 

I couldn't reproduce what I was seeing when I did patch v2 series
work. 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.

I am seeing tvtime hangs without the timeout. I am fine with this
patch not going. It does make the code cleaner and also reduces
buffer handling during streaming. However, there is a clear regression
to tvtime.

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


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

2015-01-12 Thread Devin Heitmueller
Hi Shuah,

On Mon, Jan 12, 2015 at 9:56 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 timer code paths in start/stop
> streaming, and close interfaces. However, tvtime will hang
> without the following tvtime change:

I'm confused.  When we last debated whether this patch would be
accepted, the last message from Mauro said the following:

> That means that we'll need to keep holding such timeout code for
> years, until all distros update to a new tvtime, of course assuming
> that this is the only one application with such issue.

In other words, the timeout code has to stay in there since otherwise
it will cause ABI breakage.  It's great that Hans has submitted a
patch to improve tvtime, and over the next couple of years that patch
might actually start to appear in distributions.  That unfortunately
doesn't change the fact that everybody who updates their kernel (or
has it updated for them as part of their distribution) will go from
"works fine" to "completely broken".

The driver was working before the VB2 conversion, so if there is now
instability then it's likely that some bug was introduced during the
conversion to VB2.  Simply ripping out the timeout code seems like the
wrong approach to addressing a regression likely caused by your own
VB2 conversion patch.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
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 v3 3/3] media: au0828 remove video and vbi buffer timeout work-around

2015-01-12 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 timer code paths in start/stop
streaming, and close interfaces. However, tvtime will hang
without the following tvtime change:

'tvtime: don't block indefinitely waiting for frames' with

this change to remove timeout, if there is no valid video data.

Signed-off-by: Shuah Khan 
---
 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 e427913..08b1e96 100644
--- a/drivers/media/usb/au0828/au0828-video.c
+++ b/drivers/media/usb/au0828/au0828-video.c
@@ -592,15 +592,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) {
@@ -803,15 +794,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;
@@ -850,9 +835,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)
@@ -881,9 +863,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 = {
@@ -916,56 +895,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, buf);
-   }
-   vbi_get_nex