[PATCH] vb2: Update buffer state flags after __vb2_dqbuf

[PATCH] vb2: Update buffer state flags after __vb2_dqbuf

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1278 bytes --]


Previously we where updating the buffer state using __fill_v4l2_buffer
before the state transition was completed through __vb2_dqbuf. This
would cause the V4L2_BUF_FLAG_DONE to be set, which would mean it still
queued. The spec says the dqbuf should clean the DONE flag, right not it
alway set it.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/v4l2-core/videobuf2-core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index f9059bb..ac5026a 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1943,14 +1943,15 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
 
 	call_vb_qop(vb, buf_finish, vb);
 
-	/* Fill buffer information for the userspace */
-	__fill_v4l2_buffer(vb, b);
 	/* Remove from videobuf queue */
 	list_del(&vb->queued_entry);
 	q->queued_count--;
 	/* go back to dequeued state */
 	__vb2_dqbuf(vb);
 
+	/* Fill buffer information for the userspace */
+	__fill_v4l2_buffer(vb, b);
+
 	dprintk(1, "dqbuf of buffer %d, with state %d\n",
 			vb->v4l2_buf.index, vb->state);
 
-- 
1.9.0



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH] vb2: Update buffer state flags after __vb2_dqbuf

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]

Hi Nicolas,

On Wednesday 16 April 2014 15:34:06 Nicolas Dufresne wrote:
> Previously we where updating the buffer state using __fill_v4l2_buffer
> before the state transition was completed through __vb2_dqbuf. This
> would cause the V4L2_BUF_FLAG_DONE to be set, which would mean it still
> queued. The spec says the dqbuf should clean the DONE flag, right not it
> alway set it.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

This looks fine to me, thanks for catching and fixing the problem.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
> b/drivers/media/v4l2-core/videobuf2-core.c index f9059bb..ac5026a 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1943,14 +1943,15 @@ static int vb2_internal_dqbuf(struct vb2_queue *q,
> struct v4l2_buffer *b, bool n
> 
>  	call_vb_qop(vb, buf_finish, vb);
> 
> -	/* Fill buffer information for the userspace */
> -	__fill_v4l2_buffer(vb, b);
>  	/* Remove from videobuf queue */
>  	list_del(&vb->queued_entry);
>  	q->queued_count--;
>  	/* go back to dequeued state */
>  	__vb2_dqbuf(vb);
> 
> +	/* Fill buffer information for the userspace */
> +	__fill_v4l2_buffer(vb, b);
> +
>  	dprintk(1, "dqbuf of buffer %d, with state %d\n",
>  			vb->v4l2_buf.index, vb->state);

-- 
Regards,

Laurent Pinchart

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH] vb2: Update buffer state flags after __vb2_dqbuf

[Hans Verkuil]

On 04/16/2014 09:34 PM, Nicolas Dufresne wrote:
> 
> Previously we where updating the buffer state using __fill_v4l2_buffer
> before the state transition was completed through __vb2_dqbuf. This
> would cause the V4L2_BUF_FLAG_DONE to be set, which would mean it still
> queued. The spec says the dqbuf should clean the DONE flag, right not it
> alway set it.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>

Nack: this will break the V4L2_BUF_FLAG_ERROR support.

I would recommend just clearing the DONE flag explicitly.

Looking at the code I believe the flags are also set incorrectly
after QBUF (and PREPARE_BUF might have problems as well).

This needs some careful checking and I need to add v4l2-compliance
checks for this.

I don't really have time to look into this at the moment, though.

Regards,

	Hans

> ---
>  drivers/media/v4l2-core/videobuf2-core.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index f9059bb..ac5026a 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1943,14 +1943,15 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n
>  
>  	call_vb_qop(vb, buf_finish, vb);
>  
> -	/* Fill buffer information for the userspace */
> -	__fill_v4l2_buffer(vb, b);
>  	/* Remove from videobuf queue */
>  	list_del(&vb->queued_entry);
>  	q->queued_count--;
>  	/* go back to dequeued state */
>  	__vb2_dqbuf(vb);
>  
> +	/* Fill buffer information for the userspace */
> +	__fill_v4l2_buffer(vb, b);
> +
>  	dprintk(1, "dqbuf of buffer %d, with state %d\n",
>  			vb->v4l2_buf.index, vb->state);
>  
>