| Message ID | 1435927676-24559-1-git-send-email-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 07/03/2015 02:47 PM, Sakari Ailus wrote: > Buffers can be returned back to videobuf2 in driver's streamon handler. In > this case vb2_buffer_done() with buffer state VB2_BUF_STATE_QUEUED will > cause the driver's buf_queue vb2 operation to be called, queueing the same > buffer again only to be returned to videobuf2 using vb2_buffer_done() and so > on. > > Add a new buffer state VB2_BUF_STATE_REQUEUEING which, when used as the It's spelled as requeuing (no e). The verb is 'to queue', but the -ing form is queuing. Check the dictionary: http://dictionary.reference.com/browse/queuing > state argument to vb2_buffer_done(), will result in buffers queued to the > driver. Using VB2_BUF_STATE_QUEUED will leave the buffer to videobuf2, as it > was before "[media] vb2: allow requeuing buffers while streaming". > > Fixes: ce0eff016f72 ("[media] vb2: allow requeuing buffers while streaming") > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: stable@vger.kernel.org # for v4.1 > --- > since v1: > > - Instead of relying on q->start_streaming_called and q->streaming, add a > new buffer state VB2_BUF_STATE_REQUEUEING, as suggested by Hans. The > cobalt driver will need the new flag as it returns the buffer back to the > driver's queue, the rest will continue using VB2_BUF_STATE_QUEUED. > > drivers/media/pci/cobalt/cobalt-irq.c | 2 +- > drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++---- > include/media/videobuf2-core.h | 2 ++ > 3 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/pci/cobalt/cobalt-irq.c b/drivers/media/pci/cobalt/cobalt-irq.c > index e18f49e..2687cb0 100644 > --- a/drivers/media/pci/cobalt/cobalt-irq.c > +++ b/drivers/media/pci/cobalt/cobalt-irq.c > @@ -134,7 +134,7 @@ done: > also know about dropped frames. */ > cb->vb.v4l2_buf.sequence = s->sequence++; > vb2_buffer_done(&cb->vb, (skip || s->unstable_frame) ? > - VB2_BUF_STATE_QUEUED : VB2_BUF_STATE_DONE); > + VB2_BUF_STATE_REQUEUEING : VB2_BUF_STATE_DONE); > } > > irqreturn_t cobalt_irq_handler(int irq, void *dev_id) > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 1a096a6..ca8c041 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -1182,7 +1182,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > > if (WARN_ON(state != VB2_BUF_STATE_DONE && > state != VB2_BUF_STATE_ERROR && > - state != VB2_BUF_STATE_QUEUED)) > + state != VB2_BUF_STATE_QUEUED && > + state != VB2_BUF_STATE_REQUEUEING)) > state = VB2_BUF_STATE_ERROR; > > #ifdef CONFIG_VIDEO_ADV_DEBUG > @@ -1199,15 +1200,21 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > for (plane = 0; plane < vb->num_planes; ++plane) > call_void_memop(vb, finish, vb->planes[plane].mem_priv); > > - /* Add the buffer to the done buffers list */ > spin_lock_irqsave(&q->done_lock, flags); > - vb->state = state; > - if (state != VB2_BUF_STATE_QUEUED) > + if (state == VB2_BUF_STATE_QUEUED || > + state == VB2_BUF_STATE_REQUEUEING) { > + vb->state = VB2_BUF_STATE_QUEUED; > + } else { > + /* Add the buffer to the done buffers list */ > list_add_tail(&vb->done_entry, &q->done_list); > + vb->state = state; > + } > atomic_dec(&q->owned_by_drv_count); > spin_unlock_irqrestore(&q->done_lock, flags); > > if (state == VB2_BUF_STATE_QUEUED) { > + return; > + } else if (state == VB2_BUF_STATE_REQUEUEING) { No 'else' is needed here since the 'if' just returns. Regards, Hans > if (q->start_streaming_called) > __enqueue_in_driver(vb); > return; > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 22a44c2..c192e1b 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -139,6 +139,7 @@ enum vb2_io_modes { > * @VB2_BUF_STATE_PREPARING: buffer is being prepared in videobuf > * @VB2_BUF_STATE_PREPARED: buffer prepared in videobuf and by the driver > * @VB2_BUF_STATE_QUEUED: buffer queued in videobuf, but not in driver > + * @VB2_BUF_STATE_REQUEUEING: re-queue a buffer to the driver > * @VB2_BUF_STATE_ACTIVE: buffer queued in driver and possibly used > * in a hardware operation > * @VB2_BUF_STATE_DONE: buffer returned from driver to videobuf, but > @@ -152,6 +153,7 @@ enum vb2_buffer_state { > VB2_BUF_STATE_PREPARING, > VB2_BUF_STATE_PREPARED, > VB2_BUF_STATE_QUEUED, > + VB2_BUF_STATE_REQUEUEING, > VB2_BUF_STATE_ACTIVE, > VB2_BUF_STATE_DONE, > VB2_BUF_STATE_ERROR, > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Hans Verkuil wrote: > On 07/03/2015 02:47 PM, Sakari Ailus wrote: >> Buffers can be returned back to videobuf2 in driver's streamon handler. In >> this case vb2_buffer_done() with buffer state VB2_BUF_STATE_QUEUED will >> cause the driver's buf_queue vb2 operation to be called, queueing the same >> buffer again only to be returned to videobuf2 using vb2_buffer_done() and so >> on. >> >> Add a new buffer state VB2_BUF_STATE_REQUEUEING which, when used as the > > It's spelled as requeuing (no e). The verb is 'to queue', but the -ing form is > queuing. Check the dictionary: http://dictionary.reference.com/browse/queuing My dictionary disagrees with yours. :-) http://dictionary.cambridge.org/dictionary/british/queue?q=queueing > >> state argument to vb2_buffer_done(), will result in buffers queued to the >> driver. Using VB2_BUF_STATE_QUEUED will leave the buffer to videobuf2, as it >> was before "[media] vb2: allow requeuing buffers while streaming". >> >> Fixes: ce0eff016f72 ("[media] vb2: allow requeuing buffers while streaming") >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> Cc: stable@vger.kernel.org # for v4.1 >> --- >> since v1: >> >> - Instead of relying on q->start_streaming_called and q->streaming, add a >> new buffer state VB2_BUF_STATE_REQUEUEING, as suggested by Hans. The >> cobalt driver will need the new flag as it returns the buffer back to the >> driver's queue, the rest will continue using VB2_BUF_STATE_QUEUED. >> >> drivers/media/pci/cobalt/cobalt-irq.c | 2 +- >> drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++---- >> include/media/videobuf2-core.h | 2 ++ >> 3 files changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/media/pci/cobalt/cobalt-irq.c b/drivers/media/pci/cobalt/cobalt-irq.c >> index e18f49e..2687cb0 100644 >> --- a/drivers/media/pci/cobalt/cobalt-irq.c >> +++ b/drivers/media/pci/cobalt/cobalt-irq.c >> @@ -134,7 +134,7 @@ done: >> also know about dropped frames. */ >> cb->vb.v4l2_buf.sequence = s->sequence++; >> vb2_buffer_done(&cb->vb, (skip || s->unstable_frame) ? >> - VB2_BUF_STATE_QUEUED : VB2_BUF_STATE_DONE); >> + VB2_BUF_STATE_REQUEUEING : VB2_BUF_STATE_DONE); >> } >> >> irqreturn_t cobalt_irq_handler(int irq, void *dev_id) >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c >> index 1a096a6..ca8c041 100644 >> --- a/drivers/media/v4l2-core/videobuf2-core.c >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >> @@ -1182,7 +1182,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) >> >> if (WARN_ON(state != VB2_BUF_STATE_DONE && >> state != VB2_BUF_STATE_ERROR && >> - state != VB2_BUF_STATE_QUEUED)) >> + state != VB2_BUF_STATE_QUEUED && >> + state != VB2_BUF_STATE_REQUEUEING)) >> state = VB2_BUF_STATE_ERROR; >> >> #ifdef CONFIG_VIDEO_ADV_DEBUG >> @@ -1199,15 +1200,21 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) >> for (plane = 0; plane < vb->num_planes; ++plane) >> call_void_memop(vb, finish, vb->planes[plane].mem_priv); >> >> - /* Add the buffer to the done buffers list */ >> spin_lock_irqsave(&q->done_lock, flags); >> - vb->state = state; >> - if (state != VB2_BUF_STATE_QUEUED) >> + if (state == VB2_BUF_STATE_QUEUED || >> + state == VB2_BUF_STATE_REQUEUEING) { >> + vb->state = VB2_BUF_STATE_QUEUED; >> + } else { >> + /* Add the buffer to the done buffers list */ >> list_add_tail(&vb->done_entry, &q->done_list); >> + vb->state = state; >> + } >> atomic_dec(&q->owned_by_drv_count); >> spin_unlock_irqrestore(&q->done_lock, flags); >> >> if (state == VB2_BUF_STATE_QUEUED) { >> + return; >> + } else if (state == VB2_BUF_STATE_REQUEUEING) { > > No 'else' is needed here since the 'if' just returns. Will fix.
On 07/03/2015 03:28 PM, Sakari Ailus wrote: > Hi Hans, > > Hans Verkuil wrote: >> On 07/03/2015 02:47 PM, Sakari Ailus wrote: >>> Buffers can be returned back to videobuf2 in driver's streamon handler. In >>> this case vb2_buffer_done() with buffer state VB2_BUF_STATE_QUEUED will >>> cause the driver's buf_queue vb2 operation to be called, queueing the same >>> buffer again only to be returned to videobuf2 using vb2_buffer_done() and so >>> on. >>> >>> Add a new buffer state VB2_BUF_STATE_REQUEUEING which, when used as the >> >> It's spelled as requeuing (no e). The verb is 'to queue', but the -ing form is >> queuing. Check the dictionary: http://dictionary.reference.com/browse/queuing > > My dictionary disagrees with yours. :-) > > http://dictionary.cambridge.org/dictionary/british/queue?q=queueing $ git grep -i queueing|wc 655 5660 54709 $ git grep -i queuing|wc 650 5623 55249 That's not helpful either... On the other hand: $ git grep -i queuing drivers/media/|wc 19 200 1846 $ git grep -i queueing drivers/media/|wc 2 25 203 Within drivers/media there seems to be a clear preference for queuing :-) But yes, both spellings are OK, but for me queueing has way too many vowels! And interestingly, my spellchecker thinks queueing is wrong. > >> >>> state argument to vb2_buffer_done(), will result in buffers queued to the >>> driver. Using VB2_BUF_STATE_QUEUED will leave the buffer to videobuf2, as it >>> was before "[media] vb2: allow requeuing buffers while streaming". >>> >>> Fixes: ce0eff016f72 ("[media] vb2: allow requeuing buffers while streaming") >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >>> Cc: stable@vger.kernel.org # for v4.1 >>> --- >>> since v1: >>> >>> - Instead of relying on q->start_streaming_called and q->streaming, add a >>> new buffer state VB2_BUF_STATE_REQUEUEING, as suggested by Hans. The >>> cobalt driver will need the new flag as it returns the buffer back to the >>> driver's queue, the rest will continue using VB2_BUF_STATE_QUEUED. >>> >>> drivers/media/pci/cobalt/cobalt-irq.c | 2 +- >>> drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++---- >>> include/media/videobuf2-core.h | 2 ++ >>> 3 files changed, 14 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/media/pci/cobalt/cobalt-irq.c b/drivers/media/pci/cobalt/cobalt-irq.c >>> index e18f49e..2687cb0 100644 >>> --- a/drivers/media/pci/cobalt/cobalt-irq.c >>> +++ b/drivers/media/pci/cobalt/cobalt-irq.c >>> @@ -134,7 +134,7 @@ done: >>> also know about dropped frames. */ >>> cb->vb.v4l2_buf.sequence = s->sequence++; >>> vb2_buffer_done(&cb->vb, (skip || s->unstable_frame) ? >>> - VB2_BUF_STATE_QUEUED : VB2_BUF_STATE_DONE); >>> + VB2_BUF_STATE_REQUEUEING : VB2_BUF_STATE_DONE); >>> } >>> >>> irqreturn_t cobalt_irq_handler(int irq, void *dev_id) >>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c >>> index 1a096a6..ca8c041 100644 >>> --- a/drivers/media/v4l2-core/videobuf2-core.c >>> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>> @@ -1182,7 +1182,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) >>> >>> if (WARN_ON(state != VB2_BUF_STATE_DONE && >>> state != VB2_BUF_STATE_ERROR && >>> - state != VB2_BUF_STATE_QUEUED)) >>> + state != VB2_BUF_STATE_QUEUED && >>> + state != VB2_BUF_STATE_REQUEUEING)) >>> state = VB2_BUF_STATE_ERROR; >>> >>> #ifdef CONFIG_VIDEO_ADV_DEBUG >>> @@ -1199,15 +1200,21 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) >>> for (plane = 0; plane < vb->num_planes; ++plane) >>> call_void_memop(vb, finish, vb->planes[plane].mem_priv); >>> >>> - /* Add the buffer to the done buffers list */ >>> spin_lock_irqsave(&q->done_lock, flags); >>> - vb->state = state; >>> - if (state != VB2_BUF_STATE_QUEUED) >>> + if (state == VB2_BUF_STATE_QUEUED || >>> + state == VB2_BUF_STATE_REQUEUEING) { >>> + vb->state = VB2_BUF_STATE_QUEUED; >>> + } else { >>> + /* Add the buffer to the done buffers list */ >>> list_add_tail(&vb->done_entry, &q->done_list); >>> + vb->state = state; >>> + } >>> atomic_dec(&q->owned_by_drv_count); >>> spin_unlock_irqrestore(&q->done_lock, flags); >>> >>> if (state == VB2_BUF_STATE_QUEUED) { >>> + return; >>> + } else if (state == VB2_BUF_STATE_REQUEUEING) { >> >> No 'else' is needed here since the 'if' just returns. > > Will fix. > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Hans Verkuil wrote: > On 07/03/2015 03:28 PM, Sakari Ailus wrote: >> Hi Hans, >> >> Hans Verkuil wrote: >>> On 07/03/2015 02:47 PM, Sakari Ailus wrote: >>>> Buffers can be returned back to videobuf2 in driver's streamon handler. In >>>> this case vb2_buffer_done() with buffer state VB2_BUF_STATE_QUEUED will >>>> cause the driver's buf_queue vb2 operation to be called, queueing the same >>>> buffer again only to be returned to videobuf2 using vb2_buffer_done() and so >>>> on. >>>> >>>> Add a new buffer state VB2_BUF_STATE_REQUEUEING which, when used as the >>> >>> It's spelled as requeuing (no e). The verb is 'to queue', but the -ing form is >>> queuing. Check the dictionary: http://dictionary.reference.com/browse/queuing >> >> My dictionary disagrees with yours. :-) >> >> http://dictionary.cambridge.org/dictionary/british/queue?q=queueing > > $ git grep -i queueing|wc > 655 5660 54709 > $ git grep -i queuing|wc > 650 5623 55249 > > That's not helpful either... > > On the other hand: > > $ git grep -i queuing drivers/media/|wc > 19 200 1846 > $ git grep -i queueing drivers/media/|wc > 2 25 203 > > Within drivers/media there seems to be a clear preference for queuing :-) The rest of the kernel apparently prefers "queueing" with a slight margin, if you don't consider V4L2. And who do you think might have added those lines containing "queuing" in V4L2? :-D The matter was discussed long time ago and my understanding was in case of multiple possible spellings both should be allowed. I'll post v3, replacing the if's at the end by a single switch. I think it's cleaner that way.
diff --git a/drivers/media/pci/cobalt/cobalt-irq.c b/drivers/media/pci/cobalt/cobalt-irq.c index e18f49e..2687cb0 100644 --- a/drivers/media/pci/cobalt/cobalt-irq.c +++ b/drivers/media/pci/cobalt/cobalt-irq.c @@ -134,7 +134,7 @@ done: also know about dropped frames. */ cb->vb.v4l2_buf.sequence = s->sequence++; vb2_buffer_done(&cb->vb, (skip || s->unstable_frame) ? - VB2_BUF_STATE_QUEUED : VB2_BUF_STATE_DONE); + VB2_BUF_STATE_REQUEUEING : VB2_BUF_STATE_DONE); } irqreturn_t cobalt_irq_handler(int irq, void *dev_id) diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index 1a096a6..ca8c041 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -1182,7 +1182,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) if (WARN_ON(state != VB2_BUF_STATE_DONE && state != VB2_BUF_STATE_ERROR && - state != VB2_BUF_STATE_QUEUED)) + state != VB2_BUF_STATE_QUEUED && + state != VB2_BUF_STATE_REQUEUEING)) state = VB2_BUF_STATE_ERROR; #ifdef CONFIG_VIDEO_ADV_DEBUG @@ -1199,15 +1200,21 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) for (plane = 0; plane < vb->num_planes; ++plane) call_void_memop(vb, finish, vb->planes[plane].mem_priv); - /* Add the buffer to the done buffers list */ spin_lock_irqsave(&q->done_lock, flags); - vb->state = state; - if (state != VB2_BUF_STATE_QUEUED) + if (state == VB2_BUF_STATE_QUEUED || + state == VB2_BUF_STATE_REQUEUEING) { + vb->state = VB2_BUF_STATE_QUEUED; + } else { + /* Add the buffer to the done buffers list */ list_add_tail(&vb->done_entry, &q->done_list); + vb->state = state; + } atomic_dec(&q->owned_by_drv_count); spin_unlock_irqrestore(&q->done_lock, flags); if (state == VB2_BUF_STATE_QUEUED) { + return; + } else if (state == VB2_BUF_STATE_REQUEUEING) { if (q->start_streaming_called) __enqueue_in_driver(vb); return; diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 22a44c2..c192e1b 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -139,6 +139,7 @@ enum vb2_io_modes { * @VB2_BUF_STATE_PREPARING: buffer is being prepared in videobuf * @VB2_BUF_STATE_PREPARED: buffer prepared in videobuf and by the driver * @VB2_BUF_STATE_QUEUED: buffer queued in videobuf, but not in driver + * @VB2_BUF_STATE_REQUEUEING: re-queue a buffer to the driver * @VB2_BUF_STATE_ACTIVE: buffer queued in driver and possibly used * in a hardware operation * @VB2_BUF_STATE_DONE: buffer returned from driver to videobuf, but @@ -152,6 +153,7 @@ enum vb2_buffer_state { VB2_BUF_STATE_PREPARING, VB2_BUF_STATE_PREPARED, VB2_BUF_STATE_QUEUED, + VB2_BUF_STATE_REQUEUEING, VB2_BUF_STATE_ACTIVE, VB2_BUF_STATE_DONE, VB2_BUF_STATE_ERROR,
Buffers can be returned back to videobuf2 in driver's streamon handler. In this case vb2_buffer_done() with buffer state VB2_BUF_STATE_QUEUED will cause the driver's buf_queue vb2 operation to be called, queueing the same buffer again only to be returned to videobuf2 using vb2_buffer_done() and so on. Add a new buffer state VB2_BUF_STATE_REQUEUEING which, when used as the state argument to vb2_buffer_done(), will result in buffers queued to the driver. Using VB2_BUF_STATE_QUEUED will leave the buffer to videobuf2, as it was before "[media] vb2: allow requeuing buffers while streaming". Fixes: ce0eff016f72 ("[media] vb2: allow requeuing buffers while streaming") Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: stable@vger.kernel.org # for v4.1 --- since v1: - Instead of relying on q->start_streaming_called and q->streaming, add a new buffer state VB2_BUF_STATE_REQUEUEING, as suggested by Hans. The cobalt driver will need the new flag as it returns the buffer back to the driver's queue, the rest will continue using VB2_BUF_STATE_QUEUED. drivers/media/pci/cobalt/cobalt-irq.c | 2 +- drivers/media/v4l2-core/videobuf2-core.c | 15 +++++++++++---- include/media/videobuf2-core.h | 2 ++ 3 files changed, 14 insertions(+), 5 deletions(-)