Re: [REVIEWv3 PATCH 06/17] vb2: call buf_finish from __queue_cancel.

2014-03-03 Thread Laurent Pinchart
Hi Hans,

Thank you for the patch.

On Friday 28 February 2014 18:42:04 Hans Verkuil wrote:
 From: Hans Verkuil hans.verk...@cisco.com
 
 If a queue was canceled, then the buf_finish op was never called for the
 pending buffers. So add this call to queue_cancel. Before calling buf_finish
 set the buffer state to PREPARED, which is the correct state. That way the
 states DONE and ERROR will only be seen in buf_finish if streaming is in
 progress.
 
 Since buf_finish can now be called from non-streaming state we need to
 adapt the handful of drivers that actually need to know this.
 
 Signed-off-by: Hans Verkuil hans.verk...@cisco.com
 ---
  drivers/media/parport/bw-qcam.c  |  3 +++
  drivers/media/pci/sta2x11/sta2x11_vip.c  |  3 ++-
  drivers/media/usb/uvc/uvc_queue.c|  3 ++-
  drivers/media/v4l2-core/videobuf2-core.c | 14 --
  include/media/videobuf2-core.h   | 10 +-
  5 files changed, 28 insertions(+), 5 deletions(-)
 
 diff --git a/drivers/media/parport/bw-qcam.c
 b/drivers/media/parport/bw-qcam.c index 0166aef..8d69bfb 100644
 --- a/drivers/media/parport/bw-qcam.c
 +++ b/drivers/media/parport/bw-qcam.c
 @@ -674,6 +674,9 @@ static void buffer_finish(struct vb2_buffer *vb)
   int size = vb-vb2_queue-plane_sizes[0];
   int len;
 
 + if (!vb2_is_streaming(vb-vb2_queue))
 + return;
 +
   mutex_lock(qcam-lock);
   parport_claim_or_block(qcam-pdev);
 
 diff --git a/drivers/media/pci/sta2x11/sta2x11_vip.c
 b/drivers/media/pci/sta2x11/sta2x11_vip.c index e66556c..bb11443 100644
 --- a/drivers/media/pci/sta2x11/sta2x11_vip.c
 +++ b/drivers/media/pci/sta2x11/sta2x11_vip.c
 @@ -337,7 +337,8 @@ static void buffer_finish(struct vb2_buffer *vb)
   list_del_init(vip_buf-list);
   spin_unlock(vip-lock);
 
 - vip_active_buf_next(vip);
 + if (vb2_is_streaming(vb-vb2_queue))
 + vip_active_buf_next(vip);
  }
 
  static int start_streaming(struct vb2_queue *vq, unsigned int count)
 diff --git a/drivers/media/usb/uvc/uvc_queue.c
 b/drivers/media/usb/uvc/uvc_queue.c index cca2c6e..ab9f96e 100644
 --- a/drivers/media/usb/uvc/uvc_queue.c
 +++ b/drivers/media/usb/uvc/uvc_queue.c
 @@ -111,7 +111,8 @@ static void uvc_buffer_finish(struct vb2_buffer *vb)
   container_of(queue, struct uvc_streaming, queue);
   struct uvc_buffer *buf = container_of(vb, struct uvc_buffer, buf);
 
 - uvc_video_clock_update(stream, vb-v4l2_buf, buf);
 + if (vb-state == VB2_BUF_STATE_DONE)
 + uvc_video_clock_update(stream, vb-v4l2_buf, buf);
  }
 
  static void uvc_wait_prepare(struct vb2_queue *vq)
 diff --git a/drivers/media/v4l2-core/videobuf2-core.c
 b/drivers/media/v4l2-core/videobuf2-core.c index 59bfd85..6f84bcb 100644
 --- a/drivers/media/v4l2-core/videobuf2-core.c
 +++ b/drivers/media/v4l2-core/videobuf2-core.c
 @@ -1878,9 +1878,19 @@ static void __vb2_queue_cancel(struct vb2_queue *q)
 
   /*
* Reinitialize all buffers for next use.
 +  * Make sure to call buf_finish for any queued buffers. Normally
 +  * that's done in dqbuf, but that's not going to happen when we
 +  * cancel the whole queue.

I would also state that buf_finish can't simply be moved to __vb2_dqbuf as it 
needs to be called before __fill_v4l2_buffer in the DQBUF path. Otherwise 
someone might submit a patch to simplify that vb2 code later when we'll have 
forgotten about this, introducing a bug.

*/
 - for (i = 0; i  q-num_buffers; ++i)
 - __vb2_dqbuf(q-bufs[i]);
 + for (i = 0; i  q-num_buffers; ++i) {
 + struct vb2_buffer *vb = q-bufs[i];
 +
 + if (vb-state != VB2_BUF_STATE_DEQUEUED) {
 + vb-state = VB2_BUF_STATE_PREPARED;
 + call_vb_qop(vb, buf_finish, vb);
 + }
 + __vb2_dqbuf(vb);
 + }
  }
 
  static int vb2_internal_streamon(struct vb2_queue *q, enum v4l2_buf_type
 type) diff --git a/include/media/videobuf2-core.h
 b/include/media/videobuf2-core.h index f443ce0..3cb0bcf 100644
 --- a/include/media/videobuf2-core.h
 +++ b/include/media/videobuf2-core.h
 @@ -276,7 +276,15 @@ struct vb2_buffer {
   *   in driver; optional
   * @buf_finish:  called before every dequeue of the buffer back 
 to
   *   userspace; drivers may perform any operations required
 - *   before userspace accesses the buffer; optional
 + *   before userspace accesses the buffer; optional. The
 + *   buffer state can be one of the following: DONE and
 + *   ERROR occur while streaming is in progress, and the
 + *   PREPARED state occurs when the queue has been canceled
 + *   and all pending buffers are being returned to their
 + *   default DEQUEUED state. Typically you only have to do
 + *   something if the state is VB2_BUF_STATE_DONE, since in
 + 

Re: [REVIEWv3 PATCH 06/17] vb2: call buf_finish from __queue_cancel.

2014-03-02 Thread Sakari Ailus
On Fri, Feb 28, 2014 at 06:42:04PM +0100, Hans Verkuil wrote:
 From: Hans Verkuil hans.verk...@cisco.com
 
 If a queue was canceled, then the buf_finish op was never called for the
 pending buffers. So add this call to queue_cancel. Before calling buf_finish
 set the buffer state to PREPARED, which is the correct state. That way the
 states DONE and ERROR will only be seen in buf_finish if streaming is in
 progress.
 
 Since buf_finish can now be called from non-streaming state we need to
 adapt the handful of drivers that actually need to know this.
 
 Signed-off-by: Hans Verkuil hans.verk...@cisco.com

Acked-by: Sakari Ailus sakari.ai...@iki.fi

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
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: [REVIEWv3 PATCH 06/17] vb2: call buf_finish from __queue_cancel.

2014-03-02 Thread Sakari Ailus
On Mon, Mar 03, 2014 at 08:28:05AM +0200, Sakari Ailus wrote:
 On Fri, Feb 28, 2014 at 06:42:04PM +0100, Hans Verkuil wrote:
  From: Hans Verkuil hans.verk...@cisco.com
  
  If a queue was canceled, then the buf_finish op was never called for the
  pending buffers. So add this call to queue_cancel. Before calling buf_finish
  set the buffer state to PREPARED, which is the correct state. That way the
  states DONE and ERROR will only be seen in buf_finish if streaming is in
  progress.
  
  Since buf_finish can now be called from non-streaming state we need to
  adapt the handful of drivers that actually need to know this.
  
  Signed-off-by: Hans Verkuil hans.verk...@cisco.com
 
 Acked-by: Sakari Ailus sakari.ai...@iki.fi

You can replace this by:

Acked-by: Sakari Ailus sakari.ai...@linux.intel.com

-- 
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
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