Re: [PATCH v3 2/3] media: au0828 - convert to use videobuf2
Hi Shuah, Thanks for the patch. On Tue, Jan 13, 2015 at 2:56 AM, Shuah Khan shua...@osg.samsung.com wrote: Convert au0828 to use videobuf2. Tested with NTSC. Tested video and vbi devices with xawtv, tvtime, and vlc. Ran v4l2-compliance to ensure there are no regressions. video now has no failures and vbi has 3 fewer failures. video before: test VIDIOC_G_FMT: FAIL 3 failures Total: 72, Succeeded: 69, Failed: 3, Warnings: 0 Video after: Total: 72, Succeeded: 72, Failed: 0, Warnings: 18 vbi before: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL test VIDIOC_EXPBUF: FAIL test USERPTR: FAIL Total: 72, Succeeded: 66, Failed: 6, Warnings: 0 [Snip] - init_waitqueue_head(dma_q-wq); - /* submit urbs and enables IRQ */ for (i = 0; i dev-isoc_ctl.num_bufs; i++) { rc = usb_submit_urb(dev-isoc_ctl.urb[i], GFP_ATOMIC); @@ -308,16 +303,12 @@ static inline void buffer_filled(struct au0828_dev *dev, struct au0828_buffer *buf) { /* Advice that buffer was filled */ - au0828_isocdbg([%p/%d] wakeup\n, buf, buf-vb.i); - - buf-vb.state = VIDEOBUF_DONE; - buf-vb.field_count++; - v4l2_get_timestamp(buf-vb.ts); + au0828_isocdbg([%p/%d] wakeup\n, buf, buf-top_field); - dev-isoc_ctl.buf = NULL; - - list_del(buf-vb.queue); - wake_up(buf-vb.done); + buf-vb.v4l2_buf.sequence = dev-frame_count++; + buf-vb.v4l2_buf.field = V4L2_FIELD_INTERLACED; + v4l2_get_timestamp(buf-vb.v4l2_buf.timestamp); + vb2_buffer_done(buf-vb, VB2_BUF_STATE_DONE); } static inline void vbi_buffer_filled(struct au0828_dev *dev, @@ -325,16 +316,12 @@ static inline void vbi_buffer_filled(struct au0828_dev *dev, struct au0828_buffer *buf) { /* Advice that buffer was filled */ - au0828_isocdbg([%p/%d] wakeup\n, buf, buf-vb.i); - - buf-vb.state = VIDEOBUF_DONE; - buf-vb.field_count++; - v4l2_get_timestamp(buf-vb.ts); + au0828_isocdbg([%p/%d] wakeup\n, buf, buf-top_field); - dev-isoc_ctl.vbi_buf = NULL; - - list_del(buf-vb.queue); - wake_up(buf-vb.done); + buf-vb.v4l2_buf.sequence = dev-vbi_frame_count++; + buf-vb.v4l2_buf.field = V4L2_FIELD_INTERLACED; + v4l2_get_timestamp(buf-vb.v4l2_buf.timestamp); + vb2_buffer_done(buf-vb, VB2_BUF_STATE_DONE); } Why not just have one single buffer_filled function ? just check the queue type and assign the dev-isoc_ctl.buf/ dev-isoc_ctl.vbi_buf to NULL. /* @@ -353,8 +340,8 @@ static void au0828_copy_video(struct au0828_dev *dev, if (len == 0) return; - if (dma_q-pos + len buf-vb.size) - len = buf-vb.size - dma_q-pos; + if (dma_q-pos + len buf-length) + len = buf-length - dma_q-pos; startread = p; remain = len; @@ -372,11 +359,11 @@ static void au0828_copy_video(struct au0828_dev *dev, lencopy = bytesperline - currlinedone; lencopy = lencopy remain ? remain : lencopy; - if ((char *)startwrite + lencopy (char *)outp + buf-vb.size) { + if ((char *)startwrite + lencopy (char *)outp + buf-length) { au0828_isocdbg(Overflow of %zi bytes past buffer end (1)\n, ((char *)startwrite + lencopy) - - ((char *)outp + buf-vb.size)); - remain = (char *)outp + buf-vb.size - (char *)startwrite; + ((char *)outp + buf-length)); + remain = (char *)outp + buf-length - (char *)startwrite; lencopy = remain; } if (lencopy = 0) @@ -394,11 +381,11 @@ static void au0828_copy_video(struct au0828_dev *dev, lencopy = bytesperline; if ((char *)startwrite + lencopy (char *)outp + - buf-vb.size) { + buf-length) { au0828_isocdbg(Overflow %zi bytes past buf end (2)\n, ((char *)startwrite + lencopy) - - ((char *)outp + buf-vb.size)); - lencopy = remain = (char *)outp + buf-vb.size - + ((char *)outp + buf-length)); + lencopy = remain = (char *)outp + buf-length - (char *)startwrite; } if (lencopy = 0) @@ -434,7 +421,11 @@ static inline void get_next_buf(struct au0828_dmaqueue *dma_q, } /* Get the next buffer */ - *buf = list_entry(dma_q-active.next, struct au0828_buffer, vb.queue); + *buf = list_entry(dma_q-active.next, struct au0828_buffer, list); + /* Cleans up buffer - Useful for testing for frame/URB loss */ +
Re: [PATCH v3 2/3] media: au0828 - convert to use videobuf2
Hi Prabhakar, thanks for the review, please see in-line. On 01/22/2015 05:41 AM, Lad, Prabhakar wrote: Hi Shuah, Thanks for the patch. On Tue, Jan 13, 2015 at 2:56 AM, Shuah Khan shua...@osg.samsung.com wrote: Convert au0828 to use videobuf2. Tested with NTSC. Tested video and vbi devices with xawtv, tvtime, and vlc. Ran v4l2-compliance to ensure there are no regressions. video now has no failures and vbi has 3 fewer failures. video before: test VIDIOC_G_FMT: FAIL 3 failures Total: 72, Succeeded: 69, Failed: 3, Warnings: 0 Video after: Total: 72, Succeeded: 72, Failed: 0, Warnings: 18 vbi before: test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: FAIL test VIDIOC_EXPBUF: FAIL test USERPTR: FAIL Total: 72, Succeeded: 66, Failed: 6, Warnings: 0 [Snip] - init_waitqueue_head(dma_q-wq); - /* submit urbs and enables IRQ */ for (i = 0; i dev-isoc_ctl.num_bufs; i++) { rc = usb_submit_urb(dev-isoc_ctl.urb[i], GFP_ATOMIC); @@ -308,16 +303,12 @@ static inline void buffer_filled(struct au0828_dev *dev, struct au0828_buffer *buf) { /* Advice that buffer was filled */ - au0828_isocdbg([%p/%d] wakeup\n, buf, buf-vb.i); - - buf-vb.state = VIDEOBUF_DONE; - buf-vb.field_count++; - v4l2_get_timestamp(buf-vb.ts); + au0828_isocdbg([%p/%d] wakeup\n, buf, buf-top_field); - dev-isoc_ctl.buf = NULL; - - list_del(buf-vb.queue); - wake_up(buf-vb.done); + buf-vb.v4l2_buf.sequence = dev-frame_count++; + buf-vb.v4l2_buf.field = V4L2_FIELD_INTERLACED; + v4l2_get_timestamp(buf-vb.v4l2_buf.timestamp); + vb2_buffer_done(buf-vb, VB2_BUF_STATE_DONE); } static inline void vbi_buffer_filled(struct au0828_dev *dev, @@ -325,16 +316,12 @@ static inline void vbi_buffer_filled(struct au0828_dev *dev, struct au0828_buffer *buf) { /* Advice that buffer was filled */ - au0828_isocdbg([%p/%d] wakeup\n, buf, buf-vb.i); - - buf-vb.state = VIDEOBUF_DONE; - buf-vb.field_count++; - v4l2_get_timestamp(buf-vb.ts); + au0828_isocdbg([%p/%d] wakeup\n, buf, buf-top_field); - dev-isoc_ctl.vbi_buf = NULL; - - list_del(buf-vb.queue); - wake_up(buf-vb.done); + buf-vb.v4l2_buf.sequence = dev-vbi_frame_count++; + buf-vb.v4l2_buf.field = V4L2_FIELD_INTERLACED; + v4l2_get_timestamp(buf-vb.v4l2_buf.timestamp); + vb2_buffer_done(buf-vb, VB2_BUF_STATE_DONE); } Why not just have one single buffer_filled function ? just check the queue type and assign the dev-isoc_ctl.buf/ dev-isoc_ctl.vbi_buf to NULL. Yes. These two routines could be collapsed into a single. Is it okay if I made that change in a separate patch? /* @@ -353,8 +340,8 @@ static void au0828_copy_video(struct au0828_dev *dev, if (len == 0) return; - if (dma_q-pos + len buf-vb.size) - len = buf-vb.size - dma_q-pos; + if (dma_q-pos + len buf-length) + len = buf-length - dma_q-pos; startread = p; remain = len; @@ -372,11 +359,11 @@ static void au0828_copy_video(struct au0828_dev *dev, lencopy = bytesperline - currlinedone; lencopy = lencopy remain ? remain : lencopy; - if ((char *)startwrite + lencopy (char *)outp + buf-vb.size) { + if ((char *)startwrite + lencopy (char *)outp + buf-length) { au0828_isocdbg(Overflow of %zi bytes past buffer end (1)\n, ((char *)startwrite + lencopy) - - ((char *)outp + buf-vb.size)); - remain = (char *)outp + buf-vb.size - (char *)startwrite; + ((char *)outp + buf-length)); + remain = (char *)outp + buf-length - (char *)startwrite; lencopy = remain; } if (lencopy = 0) @@ -394,11 +381,11 @@ static void au0828_copy_video(struct au0828_dev *dev, lencopy = bytesperline; if ((char *)startwrite + lencopy (char *)outp + - buf-vb.size) { + buf-length) { au0828_isocdbg(Overflow %zi bytes past buf end (2)\n, ((char *)startwrite + lencopy) - - ((char *)outp + buf-vb.size)); - lencopy = remain = (char *)outp + buf-vb.size - + ((char *)outp + buf-length)); + lencopy = remain = (char *)outp + buf-length - (char *)startwrite; } if (lencopy = 0) @@ -434,7 +421,11 @@ static inline void get_next_buf(struct au0828_dmaqueue *dma_q, } /* Get the next buffer */ - *buf =
Re: [PATCH v3 2/3] media: au0828 - convert to use videobuf2
On 01/22/2015 01:47 PM, Lad, Prabhakar wrote: Hi Shuah, On Thu, Jan 22, 2015 at 4:16 PM, Shuah Khan shua...@osg.samsung.com wrote: Hi Prabhakar, [snip] + buf-vb.v4l2_buf.field = V4L2_FIELD_INTERLACED; + v4l2_get_timestamp(buf-vb.v4l2_buf.timestamp); + vb2_buffer_done(buf-vb, VB2_BUF_STATE_DONE); } Why not just have one single buffer_filled function ? just check the queue type and assign the dev-isoc_ctl.buf/ dev-isoc_ctl.vbi_buf to NULL. Yes. These two routines could be collapsed into a single. Is it okay if I made that change in a separate patch? hmm.. this can go as a separate patch. Thanks. Looked into this a bit more. It is definitely better done as a separate patch. /* @@ -353,8 +340,8 @@ static void au0828_copy_video(struct au0828_dev *dev, if (len == 0) return; - if (dma_q-pos + len buf-vb.size) - len = buf-vb.size - dma_q-pos; + if (dma_q-pos + len buf-length) + len = buf-length - dma_q-pos; startread = p; remain = len; @@ -372,11 +359,11 @@ static void au0828_copy_video(struct au0828_dev *dev, lencopy = bytesperline - currlinedone; lencopy = lencopy remain ? remain : lencopy; - if ((char *)startwrite + lencopy (char *)outp + buf-vb.size) { + if ((char *)startwrite + lencopy (char *)outp + buf-length) { au0828_isocdbg(Overflow of %zi bytes past buffer end (1)\n, ((char *)startwrite + lencopy) - - ((char *)outp + buf-vb.size)); - remain = (char *)outp + buf-vb.size - (char *)startwrite; + ((char *)outp + buf-length)); + remain = (char *)outp + buf-length - (char *)startwrite; lencopy = remain; } if (lencopy = 0) @@ -394,11 +381,11 @@ static void au0828_copy_video(struct au0828_dev *dev, lencopy = bytesperline; if ((char *)startwrite + lencopy (char *)outp + - buf-vb.size) { + buf-length) { au0828_isocdbg(Overflow %zi bytes past buf end (2)\n, ((char *)startwrite + lencopy) - - ((char *)outp + buf-vb.size)); - lencopy = remain = (char *)outp + buf-vb.size - + ((char *)outp + buf-length)); + lencopy = remain = (char *)outp + buf-length - (char *)startwrite; } if (lencopy = 0) @@ -434,7 +421,11 @@ static inline void get_next_buf(struct au0828_dmaqueue *dma_q, } /* Get the next buffer */ - *buf = list_entry(dma_q-active.next, struct au0828_buffer, vb.queue); + *buf = list_entry(dma_q-active.next, struct au0828_buffer, list); + /* Cleans up buffer - Useful for testing for frame/URB loss */ + list_del((*buf)-list); + dma_q-pos = 0; + (*buf)-vb_buf = (*buf)-mem; dev-isoc_ctl.buf = *buf; return; @@ -472,8 +463,8 @@ static void au0828_copy_vbi(struct au0828_dev *dev, bytesperline = dev-vbi_width; - if (dma_q-pos + len buf-vb.size) - len = buf-vb.size - dma_q-pos; + if (dma_q-pos + len buf-length) + len = buf-length - dma_q-pos; startread = p; startwrite = outp + (dma_q-pos / 2); @@ -496,7 +487,6 @@ static inline void vbi_get_next_buf(struct au0828_dmaqueue *dma_q, struct au0828_buffer **buf) { struct au0828_dev *dev = container_of(dma_q, struct au0828_dev, vbiq); - char *outp; if (list_empty(dma_q-active)) { au0828_isocdbg(No active queue to serve\n); @@ -506,13 +496,12 @@ static inline void vbi_get_next_buf(struct au0828_dmaqueue *dma_q, } /* Get the next buffer */ - *buf = list_entry(dma_q-active.next, struct au0828_buffer, vb.queue); + *buf = list_entry(dma_q-active.next, struct au0828_buffer, list); /* Cleans up buffer - Useful for testing for frame/URB loss */ - outp = videobuf_to_vmalloc((*buf)-vb); - memset(outp, 0x00, (*buf)-vb.size); - + list_del((*buf)-list); + dma_q-pos = 0; + (*buf)-vb_buf = (*buf)-mem; dev-isoc_ctl.vbi_buf = *buf; - return; } @@ -548,11 +537,11 @@ static inline int au0828_isoc_copy(struct au0828_dev *dev, struct urb *urb) buf = dev-isoc_ctl.buf; if (buf != NULL) - outp = videobuf_to_vmalloc(buf-vb); + outp = vb2_plane_vaddr(buf-vb, 0); vbi_buf = dev-isoc_ctl.vbi_buf; if (vbi_buf != NULL) - vbioutp = videobuf_to_vmalloc(vbi_buf-vb); + vbioutp =
Re: [PATCH v3 2/3] media: au0828 - convert to use videobuf2
- fh-type = type; - fh-dev = dev; - v4l2_fh_init(fh-fh, vdev); - filp-private_data = fh; + dprintk(1, + %s called std_set %d dev_state %d stream users %d users %d\n, + __func__, dev-std_set_in_tuner_core, dev-dev_state, + dev-streaming_users, dev-users); - if (mutex_lock_interruptible(dev-lock)) { - kfree(fh); + if (mutex_lock_interruptible(dev-lock)) return -ERESTARTSYS; + + ret = v4l2_fh_open(filp); + if (ret) { + au0828_isocdbg(%s: v4l2_fh_open() returned error %d\n, + __func__, ret); + mutex_unlock(dev-lock); + return ret; } + if (dev-users == 0) { you can use v4l2_fh_is_singular_file() and get rid of users member ? That won't work because the underlying resources are shared between /dev/videoX and /dev/vbiX device nodes. Hence if you were to move to v4l2_fh_is_singular_file(), the video device would get opened, the stream would get reset, the VBI device would get opened, and that would cause the analog stream to get enabled/reset *again*. 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 2/3] media: au0828 - convert to use videobuf2
On 01/22/2015 08:00 AM, Devin Heitmueller wrote: - fh-type = type; - fh-dev = dev; - v4l2_fh_init(fh-fh, vdev); - filp-private_data = fh; + dprintk(1, + %s called std_set %d dev_state %d stream users %d users %d\n, + __func__, dev-std_set_in_tuner_core, dev-dev_state, + dev-streaming_users, dev-users); - if (mutex_lock_interruptible(dev-lock)) { - kfree(fh); + if (mutex_lock_interruptible(dev-lock)) return -ERESTARTSYS; + + ret = v4l2_fh_open(filp); + if (ret) { + au0828_isocdbg(%s: v4l2_fh_open() returned error %d\n, + __func__, ret); + mutex_unlock(dev-lock); + return ret; } + if (dev-users == 0) { you can use v4l2_fh_is_singular_file() and get rid of users member ? That won't work because the underlying resources are shared between /dev/videoX and /dev/vbiX device nodes. Hence if you were to move to v4l2_fh_is_singular_file(), the video device would get opened, the stream would get reset, the VBI device would get opened, and that would cause the analog stream to get enabled/reset *again*. Thanks Devin for a detailed explanation. I did see this behavior when I was removed users and used v4l2_fh_is_singular_file() instead. I didn't understand that this is due to resource sharing between /dev/videoX and /dev/vbiX . 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 2/3] media: au0828 - convert to use videobuf2
Hi Shuah, On Thu, Jan 22, 2015 at 4:16 PM, Shuah Khan shua...@osg.samsung.com wrote: Hi Prabhakar, [snip] + buf-vb.v4l2_buf.field = V4L2_FIELD_INTERLACED; + v4l2_get_timestamp(buf-vb.v4l2_buf.timestamp); + vb2_buffer_done(buf-vb, VB2_BUF_STATE_DONE); } Why not just have one single buffer_filled function ? just check the queue type and assign the dev-isoc_ctl.buf/ dev-isoc_ctl.vbi_buf to NULL. Yes. These two routines could be collapsed into a single. Is it okay if I made that change in a separate patch? hmm.. this can go as a separate patch. /* @@ -353,8 +340,8 @@ static void au0828_copy_video(struct au0828_dev *dev, if (len == 0) return; - if (dma_q-pos + len buf-vb.size) - len = buf-vb.size - dma_q-pos; + if (dma_q-pos + len buf-length) + len = buf-length - dma_q-pos; startread = p; remain = len; @@ -372,11 +359,11 @@ static void au0828_copy_video(struct au0828_dev *dev, lencopy = bytesperline - currlinedone; lencopy = lencopy remain ? remain : lencopy; - if ((char *)startwrite + lencopy (char *)outp + buf-vb.size) { + if ((char *)startwrite + lencopy (char *)outp + buf-length) { au0828_isocdbg(Overflow of %zi bytes past buffer end (1)\n, ((char *)startwrite + lencopy) - - ((char *)outp + buf-vb.size)); - remain = (char *)outp + buf-vb.size - (char *)startwrite; + ((char *)outp + buf-length)); + remain = (char *)outp + buf-length - (char *)startwrite; lencopy = remain; } if (lencopy = 0) @@ -394,11 +381,11 @@ static void au0828_copy_video(struct au0828_dev *dev, lencopy = bytesperline; if ((char *)startwrite + lencopy (char *)outp + - buf-vb.size) { + buf-length) { au0828_isocdbg(Overflow %zi bytes past buf end (2)\n, ((char *)startwrite + lencopy) - - ((char *)outp + buf-vb.size)); - lencopy = remain = (char *)outp + buf-vb.size - + ((char *)outp + buf-length)); + lencopy = remain = (char *)outp + buf-length - (char *)startwrite; } if (lencopy = 0) @@ -434,7 +421,11 @@ static inline void get_next_buf(struct au0828_dmaqueue *dma_q, } /* Get the next buffer */ - *buf = list_entry(dma_q-active.next, struct au0828_buffer, vb.queue); + *buf = list_entry(dma_q-active.next, struct au0828_buffer, list); + /* Cleans up buffer - Useful for testing for frame/URB loss */ + list_del((*buf)-list); + dma_q-pos = 0; + (*buf)-vb_buf = (*buf)-mem; dev-isoc_ctl.buf = *buf; return; @@ -472,8 +463,8 @@ static void au0828_copy_vbi(struct au0828_dev *dev, bytesperline = dev-vbi_width; - if (dma_q-pos + len buf-vb.size) - len = buf-vb.size - dma_q-pos; + if (dma_q-pos + len buf-length) + len = buf-length - dma_q-pos; startread = p; startwrite = outp + (dma_q-pos / 2); @@ -496,7 +487,6 @@ static inline void vbi_get_next_buf(struct au0828_dmaqueue *dma_q, struct au0828_buffer **buf) { struct au0828_dev *dev = container_of(dma_q, struct au0828_dev, vbiq); - char *outp; if (list_empty(dma_q-active)) { au0828_isocdbg(No active queue to serve\n); @@ -506,13 +496,12 @@ static inline void vbi_get_next_buf(struct au0828_dmaqueue *dma_q, } /* Get the next buffer */ - *buf = list_entry(dma_q-active.next, struct au0828_buffer, vb.queue); + *buf = list_entry(dma_q-active.next, struct au0828_buffer, list); /* Cleans up buffer - Useful for testing for frame/URB loss */ - outp = videobuf_to_vmalloc((*buf)-vb); - memset(outp, 0x00, (*buf)-vb.size); - + list_del((*buf)-list); + dma_q-pos = 0; + (*buf)-vb_buf = (*buf)-mem; dev-isoc_ctl.vbi_buf = *buf; - return; } @@ -548,11 +537,11 @@ static inline int au0828_isoc_copy(struct au0828_dev *dev, struct urb *urb) buf = dev-isoc_ctl.buf; if (buf != NULL) - outp = videobuf_to_vmalloc(buf-vb); + outp = vb2_plane_vaddr(buf-vb, 0); vbi_buf = dev-isoc_ctl.vbi_buf; if (vbi_buf != NULL) - vbioutp = videobuf_to_vmalloc(vbi_buf-vb); + vbioutp = vb2_plane_vaddr(vbi_buf-vb, 0); for (i = 0; i urb-number_of_packets; i++) { int status =