dqbuf in blocking mode

dqbuf in blocking mode

[Aguirre Rodriguez, Sergio Alberto]

Hi all,

I was wondering how acceptable is to requeue a buffer in a dqbuf call
if the videbuf_dqbuf returns error?

See, here's our current omap3 camera dqbuf function code:

static int vidioc_dqbuf(struct file *file, void *fh, struct v4l2_buffer *b)
{
	struct omap34xxcam_fh *ofh = fh;
	int rval;

videobuf_dqbuf_again:
	rval = videobuf_dqbuf(&ofh->vbq, b, file->f_flags & O_NONBLOCK);

	/*
	 * This is a hack. We don't want to show -EIO to the user
	 * space. Requeue the buffer and try again if we're not doing
	 * this in non-blocking mode.
	 */
	if (rval == -EIO) {
		videobuf_qbuf(&ofh->vbq, b);
		if (!(file->f_flags & O_NONBLOCK))
			goto videobuf_dqbuf_again;
		/*
		 * We don't have a videobuf_buffer now --- maybe next
		 * time...
		 */
		rval = -EAGAIN;
	}

	return rval;
}

Is anything wrong with doing this? Or perhaphs something better to do?

Regards,
Sergio

Re: dqbuf in blocking mode

[Laurent Pinchart]

Hi Sergio,

On Thursday 01 October 2009 13:56:19 Aguirre Rodriguez, Sergio Alberto wrote:
> Hi all,
> 
> I was wondering how acceptable is to requeue a buffer in a dqbuf call
> if the videbuf_dqbuf returns error?
> 
> See, here's our current omap3 camera dqbuf function code:
> 
> static int vidioc_dqbuf(struct file *file, void *fh, struct v4l2_buffer *b)
> {
> 	struct omap34xxcam_fh *ofh = fh;
> 	int rval;
> 
> videobuf_dqbuf_again:
> 	rval = videobuf_dqbuf(&ofh->vbq, b, file->f_flags & O_NONBLOCK);
> 
> 	/*
> 	 * This is a hack. We don't want to show -EIO to the user
> 	 * space. Requeue the buffer and try again if we're not doing
> 	 * this in non-blocking mode.
> 	 */

If I'm not mistaken videobuf_dqbuf() only returns -EIO if the buffer state is 
VIDEOBUF_ERROR. This is the direct result of either

- videobuf_queue_cancel() being called, or
- the device driver marking the buffer as erroneous because of a (possibly 
transient) device error

In the first case VIDIOC_DQBUF should in my opinion return with an error. In 
the second case things are not that clear. A transient error could be hidden 
from the application, or, if returned to the application through -EIO, 
shouldn't be treated as a fatal error. Non-transient errors should result in 
the application stopping video streaming.

Unfortunately there V4L2 API doesn't offer a way to find out if the error is 
transient or fatal:

"EIO		VIDIOC_DQBUF failed due to an internal error. Can also indicate 
temporary problems like signal loss. Note the driver might dequeue an (empty) 
buffer despite returning an error, or even stop capturing."

-EIO can mean many different things that need to be handled differently by 
applications. I especially hate the "the driver might dequeue an (empty) 
buffer despite returning an error".

Drivers should always or never dequeue a buffer when an error occurs, not 
sometimes. The problem is for the application to recognize the difference 
between a transient and a fatal error in a backward-compatible way.

> 	if (rval == -EIO) {
> 		videobuf_qbuf(&ofh->vbq, b);
> 		if (!(file->f_flags & O_NONBLOCK))
> 			goto videobuf_dqbuf_again;
> 		/*
> 		 * We don't have a videobuf_buffer now --- maybe next
> 		 * time...
> 		 */
> 		rval = -EAGAIN;
> 	}
> 
> 	return rval;
> }
> 
> Is anything wrong with doing this? Or perhaphs something better to do?

--
Regards,

Laurent Pinchart

RE: dqbuf in blocking mode

[Aguirre Rodriguez, Sergio Alberto]

Hi Laurent, 

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org 
> [mailto:linux-media-owner@vger.kernel.org] On Behalf Of 
> Laurent Pinchart
> Sent: Thursday, October 01, 2009 8:34 AM
> To: Aguirre Rodriguez, Sergio Alberto
> Cc: Linux Media Mailing List
> Subject: Re: dqbuf in blocking mode
> 
> Hi Sergio,
> 
> On Thursday 01 October 2009 13:56:19 Aguirre Rodriguez, 
> Sergio Alberto wrote:
> > Hi all,
> > 
> > I was wondering how acceptable is to requeue a buffer in a 
> dqbuf call
> > if the videbuf_dqbuf returns error?
> > 
> > See, here's our current omap3 camera dqbuf function code:
> > 
> > static int vidioc_dqbuf(struct file *file, void *fh, struct 
> v4l2_buffer *b)
> > {
> > 	struct omap34xxcam_fh *ofh = fh;
> > 	int rval;
> > 
> > videobuf_dqbuf_again:
> > 	rval = videobuf_dqbuf(&ofh->vbq, b, file->f_flags & O_NONBLOCK);
> > 
> > 	/*
> > 	 * This is a hack. We don't want to show -EIO to the user
> > 	 * space. Requeue the buffer and try again if we're not doing
> > 	 * this in non-blocking mode.
> > 	 */
> 
> If I'm not mistaken videobuf_dqbuf() only returns -EIO if the 
> buffer state is 
> VIDEOBUF_ERROR. This is the direct result of either
> 
> - videobuf_queue_cancel() being called, or
> - the device driver marking the buffer as erroneous because 
> of a (possibly 
> transient) device error
> 
> In the first case VIDIOC_DQBUF should in my opinion return 
> with an error. In 
> the second case things are not that clear. A transient error 
> could be hidden 
> from the application, or, if returned to the application 
> through -EIO, 
> shouldn't be treated as a fatal error. Non-transient errors 
> should result in 
> the application stopping video streaming.
> 
> Unfortunately there V4L2 API doesn't offer a way to find out 
> if the error is 
> transient or fatal:
> 
> "EIO		VIDIOC_DQBUF failed due to an internal error. 
> Can also indicate 
> temporary problems like signal loss. Note the driver might 
> dequeue an (empty) 
> buffer despite returning an error, or even stop capturing."
> 
> -EIO can mean many different things that need to be handled 
> differently by 
> applications. I especially hate the "the driver might dequeue 
> an (empty) 
> buffer despite returning an error".
> 
> Drivers should always or never dequeue a buffer when an error 
> occurs, not 
> sometimes. The problem is for the application to recognize 
> the difference 
> between a transient and a fatal error in a backward-compatible way.

I think there are 2 different problems we are talking about here.

Problem #1: Incomplete DQBUF communication from driver to userspace.
            (which is what you're talking about)

Problem #2: Should an internal requeue of a failed buffer be allowed?

About #1, I agree with all your points.

But about #2, I'm not sure what's your stand on it... Most probably you're
thinking that it is acceptable to lock the call _just_ if its a transient error,
therefore depending on solving problem #1 first.

What are your toughts on this?

Regards,
Sergio

> 
> > 	if (rval == -EIO) {
> > 		videobuf_qbuf(&ofh->vbq, b);
> > 		if (!(file->f_flags & O_NONBLOCK))
> > 			goto videobuf_dqbuf_again;
> > 		/*
> > 		 * We don't have a videobuf_buffer now --- maybe next
> > 		 * time...
> > 		 */
> > 		rval = -EAGAIN;
> > 	}
> > 
> > 	return rval;
> > }
> > 
> > Is anything wrong with doing this? Or perhaphs something 
> better to do?
> 
> --
> Regards,
> 
> Laurent Pinchart
> --
> 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
> 
> 

Re: dqbuf in blocking mode

[Sakari Ailus]

Laurent Pinchart wrote:
[clip]
> If I'm not mistaken videobuf_dqbuf() only returns -EIO if the buffer state is 
> VIDEOBUF_ERROR. This is the direct result of either
> 
> - videobuf_queue_cancel() being called, or
> - the device driver marking the buffer as erroneous because of a (possibly 
> transient) device error
> 
> In the first case VIDIOC_DQBUF should in my opinion return with an error. In 
> the second case things are not that clear. A transient error could be hidden 
> from the application, or, if returned to the application through -EIO, 
> shouldn't be treated as a fatal error. Non-transient errors should result in 
> the application stopping video streaming.
> 
> Unfortunately there V4L2 API doesn't offer a way to find out if the error is 
> transient or fatal:
> 
> "EIO		VIDIOC_DQBUF failed due to an internal error. Can also indicate 
> temporary problems like signal loss. Note the driver might dequeue an (empty) 
> buffer despite returning an error, or even stop capturing."
> 
> -EIO can mean many different things that need to be handled differently by 
> applications. I especially hate the "the driver might dequeue an (empty) 
> buffer despite returning an error".
> 
> Drivers should always or never dequeue a buffer when an error occurs, not 
> sometimes. The problem is for the application to recognize the difference 
> between a transient and a fatal error in a backward-compatible way.

The errors in this case are transient and for blocking mode IMO the 
safest way is to return the buffer only when there's one available. 
Which is what the driver is doing now.

What I'd probably change, however, is to move the handling to the ISP 
driver instead.

Regards,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com