diff mbox

[v3,2/3] v4l: videobuf: Add support for V4L2_BUF_FLAG_ERROR

Message ID 1271849985-368-3-git-send-email-p.osciak@samsung.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Pawel Osciak April 21, 2010, 11:39 a.m. UTC 
  From: Hans Verkuil <hverkuil@xs4all.nl>

For recoverable stream errors dqbuf() now returns 0 and the error flag
is set instead of returning EIO.

Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
---
 drivers/media/video/videobuf-core.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

Comments

Laurent Pinchart April 22, 2010, 9:12 a.m. UTC | #1 
Hi Pawel,

On Wednesday 21 April 2010 13:39:44 Pawel Osciak wrote:
> From: Hans Verkuil <hverkuil@xs4all.nl>
> 
> For recoverable stream errors dqbuf() now returns 0 and the error flag
> is set instead of returning EIO.
> 
> Signed-off-by: Hans Verkuil <hverkuil@xs4all.nl>
> ---
>  drivers/media/video/videobuf-core.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/video/videobuf-core.c
> b/drivers/media/video/videobuf-core.c index 63d7043..1cf084f 100644
> --- a/drivers/media/video/videobuf-core.c
> +++ b/drivers/media/video/videobuf-core.c
> @@ -286,8 +286,10 @@ static void videobuf_status(struct videobuf_queue *q,
> struct v4l2_buffer *b, case VIDEOBUF_ACTIVE:
>  		b->flags |= V4L2_BUF_FLAG_QUEUED;
>  		break;
> -	case VIDEOBUF_DONE:
>  	case VIDEOBUF_ERROR:
> +		b->flags |= V4L2_BUF_FLAG_ERROR;
> +		/* fall through */
> +	case VIDEOBUF_DONE:
>  		b->flags |= V4L2_BUF_FLAG_DONE;
>  		break;
>  	case VIDEOBUF_NEEDS_INIT:
> @@ -668,6 +670,7 @@ int videobuf_dqbuf(struct videobuf_queue *q,
> 
>  	MAGIC_CHECK(q->int_ops->magic, MAGIC_QTYPE_OPS);
> 
> +	memset(b, 0, sizeof(*b));
>  	mutex_lock(&q->vb_lock);
> 
>  	retval = stream_next_buffer(q, &buf, nonblocking);
> @@ -679,23 +682,20 @@ int videobuf_dqbuf(struct videobuf_queue *q,
>  	switch (buf->state) {
>  	case VIDEOBUF_ERROR:
>  		dprintk(1, "dqbuf: state is error\n");
> -		retval = -EIO;
> -		CALL(q, sync, q, buf);
> -		buf->state = VIDEOBUF_IDLE;
>  		break;
>  	case VIDEOBUF_DONE:
>  		dprintk(1, "dqbuf: state is done\n");
> -		CALL(q, sync, q, buf);
> -		buf->state = VIDEOBUF_IDLE;
>  		break;
>  	default:
>  		dprintk(1, "dqbuf: state invalid\n");
>  		retval = -EINVAL;
>  		goto done;
>  	}
> -	list_del(&buf->stream);
> -	memset(b, 0, sizeof(*b));
> +	CALL(q, sync, q, buf);
>  	videobuf_status(q, b, buf, q->type);
> +	list_del(&buf->stream);
> +	buf->state = VIDEOBUF_IDLE;
> +	b->flags &= ~V4L2_BUF_FLAG_DONE;

We do you clear the done flag here ?

>  done:
>  	mutex_unlock(&q->vb_lock);
>  	return retval;
Pawel Osciak April 22, 2010, 9:35 a.m. UTC | #2 
Hi,

>Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>On Wednesday 21 April 2010 13:39:44 Pawel Osciak wrote:
>> @@ -679,23 +682,20 @@ int videobuf_dqbuf(struct videobuf_queue *q,
>>  	switch (buf->state) {
>>  	case VIDEOBUF_ERROR:
>>  		dprintk(1, "dqbuf: state is error\n");
>> -		retval = -EIO;
>> -		CALL(q, sync, q, buf);
>> -		buf->state = VIDEOBUF_IDLE;
>>  		break;
>>  	case VIDEOBUF_DONE:
>>  		dprintk(1, "dqbuf: state is done\n");
>> -		CALL(q, sync, q, buf);
>> -		buf->state = VIDEOBUF_IDLE;
>>  		break;
>>  	default:
>>  		dprintk(1, "dqbuf: state invalid\n");
>>  		retval = -EINVAL;
>>  		goto done;
>>  	}
>> -	list_del(&buf->stream);
>> -	memset(b, 0, sizeof(*b));
>> +	CALL(q, sync, q, buf);
>>  	videobuf_status(q, b, buf, q->type);
>> +	list_del(&buf->stream);
>> +	buf->state = VIDEOBUF_IDLE;
>> +	b->flags &= ~V4L2_BUF_FLAG_DONE;
>
>We do you clear the done flag here ?
>

The DONE flag is supposed to be cleared when dequeuing, but should
be set when querying:
 
"When this flag is set, the buffer is currently on the outgoing queue,
ready to be dequeued from the driver. Drivers set or clear this flag
when the VIDIOC_QUERYBUF  ioctl is called. After calling the VIDIOC_QBUF
or VIDIOC_DQBUF it is always cleared."

videobuf_status() is used for both QUERYBUF and DQBUF and making both
work properly is not very straightforward without losing
VIDEOBUF_DONE/VIDEOBUF_ERROR distinction (it becomes more clear when you
analyze both cases).

My previous patch was doing it the other way around, but Hans' version
seemed shorter and cleaner.

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center



--
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
Laurent Pinchart April 26, 2010, 11:41 a.m. UTC | #3 
Hi Pavel,

On Thursday 22 April 2010 11:35:35 Pawel Osciak wrote:
> >Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >On Wednesday 21 April 2010 13:39:44 Pawel Osciak wrote:
> >> @@ -679,23 +682,20 @@ int videobuf_dqbuf(struct videobuf_queue *q,
> >> 
> >>  	switch (buf->state) {
> >>  	
> >>  	case VIDEOBUF_ERROR:
> >>  		dprintk(1, "dqbuf: state is error\n");
> >> 
> >> -		retval = -EIO;
> >> -		CALL(q, sync, q, buf);
> >> -		buf->state = VIDEOBUF_IDLE;
> >> 
> >>  		break;
> >>  	
> >>  	case VIDEOBUF_DONE:
> >>  		dprintk(1, "dqbuf: state is done\n");
> >> 
> >> -		CALL(q, sync, q, buf);
> >> -		buf->state = VIDEOBUF_IDLE;
> >> 
> >>  		break;
> >>  	
> >>  	default:
> >>  		dprintk(1, "dqbuf: state invalid\n");
> >>  		retval = -EINVAL;
> >>  		goto done;
> >>  	
> >>  	}
> >> 
> >> -	list_del(&buf->stream);
> >> -	memset(b, 0, sizeof(*b));
> >> +	CALL(q, sync, q, buf);
> >> 
> >>  	videobuf_status(q, b, buf, q->type);
> >> 
> >> +	list_del(&buf->stream);
> >> +	buf->state = VIDEOBUF_IDLE;
> >> +	b->flags &= ~V4L2_BUF_FLAG_DONE;
> >
> >We do you clear the done flag here ?
> 
> The DONE flag is supposed to be cleared when dequeuing, but should
> be set when querying:
> 
> "When this flag is set, the buffer is currently on the outgoing queue,
> ready to be dequeued from the driver. Drivers set or clear this flag
> when the VIDIOC_QUERYBUF  ioctl is called. After calling the VIDIOC_QBUF
> or VIDIOC_DQBUF it is always cleared."
> 
> videobuf_status() is used for both QUERYBUF and DQBUF and making both
> work properly is not very straightforward without losing
> VIDEOBUF_DONE/VIDEOBUF_ERROR distinction (it becomes more clear when you
> analyze both cases).
> 
> My previous patch was doing it the other way around, but Hans' version
> seemed shorter and cleaner.

Thanks for pointing this out.

I have no more objection then,

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

Patch

diff --git a/drivers/media/video/videobuf-core.c b/drivers/media/video/videobuf-core.c
index 63d7043..1cf084f 100644
--- a/drivers/media/video/videobuf-core.c
+++ b/drivers/media/video/videobuf-core.c
@@ -286,8 +286,10 @@  static void videobuf_status(struct videobuf_queue *q, struct v4l2_buffer *b,
 	case VIDEOBUF_ACTIVE:
 		b->flags |= V4L2_BUF_FLAG_QUEUED;
 		break;
-	case VIDEOBUF_DONE:
 	case VIDEOBUF_ERROR:
+		b->flags |= V4L2_BUF_FLAG_ERROR;
+		/* fall through */
+	case VIDEOBUF_DONE:
 		b->flags |= V4L2_BUF_FLAG_DONE;
 		break;
 	case VIDEOBUF_NEEDS_INIT:
@@ -668,6 +670,7 @@  int videobuf_dqbuf(struct videobuf_queue *q,
 
 	MAGIC_CHECK(q->int_ops->magic, MAGIC_QTYPE_OPS);
 
+	memset(b, 0, sizeof(*b));
 	mutex_lock(&q->vb_lock);
 
 	retval = stream_next_buffer(q, &buf, nonblocking);
@@ -679,23 +682,20 @@  int videobuf_dqbuf(struct videobuf_queue *q,
 	switch (buf->state) {
 	case VIDEOBUF_ERROR:
 		dprintk(1, "dqbuf: state is error\n");
-		retval = -EIO;
-		CALL(q, sync, q, buf);
-		buf->state = VIDEOBUF_IDLE;
 		break;
 	case VIDEOBUF_DONE:
 		dprintk(1, "dqbuf: state is done\n");
-		CALL(q, sync, q, buf);
-		buf->state = VIDEOBUF_IDLE;
 		break;
 	default:
 		dprintk(1, "dqbuf: state invalid\n");
 		retval = -EINVAL;
 		goto done;
 	}
-	list_del(&buf->stream);
-	memset(b, 0, sizeof(*b));
+	CALL(q, sync, q, buf);
 	videobuf_status(q, b, buf, q->type);
+	list_del(&buf->stream);
+	buf->state = VIDEOBUF_IDLE;
+	b->flags &= ~V4L2_BUF_FLAG_DONE;
 done:
 	mutex_unlock(&q->vb_lock);
 	return retval;