Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
Hi Laurent! On 04/08/2011 02:53 PM, Laurent Pinchart wrote: > Hi Sylwester, > > On Thursday 07 April 2011 00:04:45 Sylwester Nawrocki wrote: >> On 04/06/2011 10:05 PM, Sylwester Nawrocki wrote: >>> On 03/07/2011 12:20 AM, Pawel Osciak wrote: On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart wrote: > On Monday 28 February 2011 16:44:38 Pawel Osciak wrote: >> Hi Laurent, >> A few questions from me below. I feel we need to talk about this >> change a bit more, it introduces some recovery and consistency >> problems, unless I'm missing something. >> >> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote: >>> videobuf2 expects drivers to check buffer in the buf_prepare >>> operation and to return success only if the buffer can queued >>> without any issue. >>> >>> For hot-pluggable devices, disconnection events need to be handled at >>> buf_queue time. Checking the disconnected flag and adding the buffer >>> to the driver queue need to be performed without releasing the >>> driver spinlock, otherwise race conditions can occur in which the >>> driver could still allow a buffer to be queued after the >>> disconnected flag has been set, resulting in a hang during the next >>> DQBUF operation. >>> >>> This problem can be solved either in the videobuf2 core or in the >>> device drivers. To avoid adding a spinlock to videobuf2, make >>> buf_queue return an int and handle buf_queue failures in videobuf2. >>> Drivers must not return an error in buf_queue if the condition >>> leading to the error can be caught in buf_prepare. >> >> ... >> >>> As buf_queue callback is called by vb2 only after start_streaming, >>> for a camera snapshot capture I needed to start a pipeline only from the >>> buf_queue handler level, i.e. subdev's video s_stream op was called from >>> within buf_queue. s_stream couldn't be done in the start_streaming >>> handler as at the time it is invoked there is always no buffer available >>> in the bridge H/W. >>> It's a consequence of how the vb2_streamon() is designed. >>> >>> Before, I used to simply call s_stream in start_streaming, only deferring >>> the actual bridge DMA enable till a buf_queue call, thus letting first >>> frames in the stream to be lost. This of course cannot be done in case >>> of single-frame capture. >>> >>> To make a long story short, it would be useful in my case to have the >>> ability to return error codes as per VIDIOC_STREAMON through buf_queue >>> in the driver (when the first buffer is queued). >>> At the moment mainly EPIPE comes to my mind. This error code has no >>> meaning in the API for QBUF though. Should the pipeline be started from >>> buf_queue >> >> Hmm, the pipeline validation could still be done in start_streaming() >> so we can return any EPIPE error from there directly and effectively >> in VIDIOC_STREAMON. > > That's correct, and that's what the OMAP3 ISP driver does. > >> So the only remaining errors are those related to I2C communication etc. >> when streaming is actually enabled in the subdev. > > buf_queue is called with a spinlock help, so you can't perform I2C > communication there. This sounds like a really simple requirement - configure the capture format, allocate and queue a single buffer and trigger the hardware to fill exactly that buffer. Yet the drivers are forced to perform some acrobatics to achieve that fairly simple task. Luckily videobuf2 does not enforce atomic context in buf_queue as Marek pointed out. And I know it from experience as I've experimented with S5P FIMC and M-5MOLS drivers for still JPEG capture, on top of your patch allowing to return errors from buf_queue btw :) VIDIOC_STREAMON has rather fuzzy meaning for me and I assume there has been an agreement that allowing drivers to return errors from buf_queue and performing blocking I/O from there is the expected way to work around the freedom the API gives to applications in regards to VIDIOC_STREAMON. But someone could prove me wrong here. I guess there are some issues with streaming over USB when buf_queue is called in a non atomic context? I'll have a closer look at your email explaining the USB disconnection handling. -- Regards, Sylwester Nawrocki -- 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: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
Hi Marek, On Friday 08 April 2011 15:09:02 Marek Szyprowski wrote: > On Friday, April 08, 2011 2:53 PM Laurent Pinchart wrote: [snip] > > buf_queue is called with a spinlock help, so you can't perform I2C > > communication there. > > In videobuf2 buf_queue() IS NOT called with any spinlock held. buf_queue > can call functions that require sleeping. This makes a lot of sense > especially for drivers that need to perform a lot of operations for > enabling/disabling hardware. Oops, my bad. > I remember we discussed your solution where you wanted to add a spinlock > for calling buf_queue. This case shows one more reason not go that way. :) Hehe. I totally agree with you that we should avoid locking wherever possible. We still have no solution for the disconnection problem though. > AFAIR buf_queue callback in old videobuf was called with spinlock held. That's correct, yes. > I agree that we definitely need more documentation for vb2 and clarification > what is allowed in each callback... -- Regards, Laurent Pinchart -- 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: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
Hello, On Friday, April 08, 2011 2:53 PM Laurent Pinchart wrote: > > ... > > > > > As buf_queue callback is called by vb2 only after start_streaming, > > > for a camera snapshot capture I needed to start a pipeline only from > the > > > buf_queue handler level, i.e. subdev's video s_stream op was called > from > > > within buf_queue. s_stream couldn't be done in the start_streaming > > > handler as at the time it is invoked there is always no buffer > available > > > in the bridge H/W. > > > It's a consequence of how the vb2_streamon() is designed. > > > > > > Before, I used to simply call s_stream in start_streaming, only > deferring > > > the actual bridge DMA enable till a buf_queue call, thus letting first > > > frames in the stream to be lost. This of course cannot be done in case > > > of single-frame capture. > > > > > > To make a long story short, it would be useful in my case to have the > > > ability to return error codes as per VIDIOC_STREAMON through buf_queue > > > in the driver (when the first buffer is queued). > > > At the moment mainly EPIPE comes to my mind. This error code has no > > > meaning in the API for QBUF though. Should the pipeline be started from > > > buf_queue > > > > Hmm, the pipeline validation could still be done in start_streaming() > > so we can return any EPIPE error from there directly and effectively > > in VIDIOC_STREAMON. > > That's correct, and that's what the OMAP3 ISP driver does. > > > So the only remaining errors are those related to I2C communication etc. > > when streaming is actually enabled in the subdev. > > buf_queue is called with a spinlock help, so you can't perform I2C > communication there. In videobuf2 buf_queue() IS NOT called with any spinlock held. buf_queue can call functions that require sleeping. This makes a lot of sense especially for drivers that need to perform a lot of operations for enabling/disabling hardware. I remember we discussed your solution where you wanted to add a spinlock for calling buf_queue. This case shows one more reason not go that way. :) AFAIR buf_queue callback in old videobuf was called with spinlock held. I agree that we definitely need more documentation for vb2 and clarification what is allowed in each callback... Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
Hi Sylwester, On Thursday 07 April 2011 00:04:45 Sylwester Nawrocki wrote: > On 04/06/2011 10:05 PM, Sylwester Nawrocki wrote: > > On 03/07/2011 12:20 AM, Pawel Osciak wrote: > >> On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart wrote: > >>> On Monday 28 February 2011 16:44:38 Pawel Osciak wrote: > Hi Laurent, > A few questions from me below. I feel we need to talk about this > change a bit more, it introduces some recovery and consistency > problems, unless I'm missing something. > > On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote: > > videobuf2 expects drivers to check buffer in the buf_prepare > > operation and to return success only if the buffer can queued > > without any issue. > > > > For hot-pluggable devices, disconnection events need to be handled at > > buf_queue time. Checking the disconnected flag and adding the buffer > > to the driver queue need to be performed without releasing the > > driver spinlock, otherwise race conditions can occur in which the > > driver could still allow a buffer to be queued after the > > disconnected flag has been set, resulting in a hang during the next > > DQBUF operation. > > > > This problem can be solved either in the videobuf2 core or in the > > device drivers. To avoid adding a spinlock to videobuf2, make > > buf_queue return an int and handle buf_queue failures in videobuf2. > > Drivers must not return an error in buf_queue if the condition > > leading to the error can be caught in buf_prepare. > > ... > > > As buf_queue callback is called by vb2 only after start_streaming, > > for a camera snapshot capture I needed to start a pipeline only from the > > buf_queue handler level, i.e. subdev's video s_stream op was called from > > within buf_queue. s_stream couldn't be done in the start_streaming > > handler as at the time it is invoked there is always no buffer available > > in the bridge H/W. > > It's a consequence of how the vb2_streamon() is designed. > > > > Before, I used to simply call s_stream in start_streaming, only deferring > > the actual bridge DMA enable till a buf_queue call, thus letting first > > frames in the stream to be lost. This of course cannot be done in case > > of single-frame capture. > > > > To make a long story short, it would be useful in my case to have the > > ability to return error codes as per VIDIOC_STREAMON through buf_queue > > in the driver (when the first buffer is queued). > > At the moment mainly EPIPE comes to my mind. This error code has no > > meaning in the API for QBUF though. Should the pipeline be started from > > buf_queue > > Hmm, the pipeline validation could still be done in start_streaming() > so we can return any EPIPE error from there directly and effectively > in VIDIOC_STREAMON. That's correct, and that's what the OMAP3 ISP driver does. > So the only remaining errors are those related to I2C communication etc. > when streaming is actually enabled in the subdev. buf_queue is called with a spinlock help, so you can't perform I2C communication there. > > the errors from buf_queue would be seen in userspace via VIDIOC_STREAMON > > and/or VIDIOC_QBUF. > > > > It should be also possible to signal any errors originating from the > > subdev when s_stream is called on it, perhaps by EIO ? -- Regards, Laurent Pinchart -- 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: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
On 04/06/2011 10:05 PM, Sylwester Nawrocki wrote: > Hi Pawel, > > On 03/07/2011 12:20 AM, Pawel Osciak wrote: >> Hi Laurent, >> >> On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart >>wrote: >>> Hi Pawel, >>> >>> On Monday 28 February 2011 16:44:38 Pawel Osciak wrote: Hi Laurent, A few questions from me below. I feel we need to talk about this change a bit more, it introduces some recovery and consistency problems, unless I'm missing something. On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote: > videobuf2 expects drivers to check buffer in the buf_prepare operation > and to return success only if the buffer can queued without any issue. > > For hot-pluggable devices, disconnection events need to be handled at > buf_queue time. Checking the disconnected flag and adding the buffer to > the driver queue need to be performed without releasing the driver > spinlock, otherwise race conditions can occur in which the driver could > still allow a buffer to be queued after the disconnected flag has been > set, resulting in a hang during the next DQBUF operation. > > This problem can be solved either in the videobuf2 core or in the device > drivers. To avoid adding a spinlock to videobuf2, make buf_queue return > an int and handle buf_queue failures in videobuf2. Drivers must not > return an error in buf_queue if the condition leading to the error can > be caught in buf_prepare. > ... > As buf_queue callback is called by vb2 only after start_streaming, > for a camera snapshot capture I needed to start a pipeline only from the > buf_queue handler level, i.e. subdev's video s_stream op was called from > within buf_queue. s_stream couldn't be done in the start_streaming handler > as at the time it is invoked there is always no buffer available in the > bridge H/W. > It's a consequence of how the vb2_streamon() is designed. > > Before, I used to simply call s_stream in start_streaming, only deferring > the actual bridge DMA enable till a buf_queue call, thus letting first frames > in the stream to be lost. This of course cannot be done in case of > single-frame > capture. > > To make a long story short, it would be useful in my case to have the ability > to return error codes as per VIDIOC_STREAMON through buf_queue in the driver > (when the first buffer is queued). > At the moment mainly EPIPE comes to my mind. This error code has no meaning > in the API for QBUF though. Should the pipeline be started from buf_queue Hmm, the pipeline validation could still be done in start_streaming() so we can return any EPIPE error from there directly and effectively in VIDIOC_STREAMON. So the only remaining errors are those related to I2C communication etc. when streaming is actually enabled in the subdev. > the errors from buf_queue would be seen in userspace via VIDIOC_STREAMON > and/or VIDIOC_QBUF. > > It should be also possible to signal any errors originating from the subdev > when s_stream is called on it, perhaps by EIO ? > ... -- Regards, Sylwester -- 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: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
Hi Pawel,
On 03/07/2011 12:20 AM, Pawel Osciak wrote:
> Hi Laurent,
>
> On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart
> wrote:
>> Hi Pawel,
>>
>> On Monday 28 February 2011 16:44:38 Pawel Osciak wrote:
>>> Hi Laurent,
>>> A few questions from me below. I feel we need to talk about this
>>> change a bit more, it introduces some recovery and consistency
>>> problems, unless I'm missing something.
>>>
>>> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote:
videobuf2 expects drivers to check buffer in the buf_prepare operation
and to return success only if the buffer can queued without any issue.
For hot-pluggable devices, disconnection events need to be handled at
buf_queue time. Checking the disconnected flag and adding the buffer to
the driver queue need to be performed without releasing the driver
spinlock, otherwise race conditions can occur in which the driver could
still allow a buffer to be queued after the disconnected flag has been
set, resulting in a hang during the next DQBUF operation.
This problem can be solved either in the videobuf2 core or in the device
drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
an int and handle buf_queue failures in videobuf2. Drivers must not
return an error in buf_queue if the condition leading to the error can
be caught in buf_prepare.
Signed-off-by: Laurent Pinchart
---
drivers/media/video/videobuf2-core.c | 32
++-- include/media/videobuf2-core.h |
2 +-
2 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/drivers/media/video/videobuf2-core.c
b/drivers/media/video/videobuf2-core.c index cc7ab0a..1d81536 100644
--- a/drivers/media/video/videobuf2-core.c
+++ b/drivers/media/video/videobuf2-core.c
@@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb,
struct v4l2_buffer *b) /**
* __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
*/
-static void __enqueue_in_driver(struct vb2_buffer *vb)
+static int __enqueue_in_driver(struct vb2_buffer *vb)
{
struct vb2_queue *q = vb->vb2_queue;
+ int ret;
vb->state = VB2_BUF_STATE_ACTIVE;
atomic_inc(&q->queued_count);
- q->ops->buf_queue(vb);
+ ret = q->ops->buf_queue(vb);
+ if (ret == 0)
+ return 0;
+
+ vb->state = VB2_BUF_STATE_ERROR;
+ atomic_dec(&q->queued_count);
+ wake_up_all(&q->done_wq);
+
+ return ret;
>>>
>>> Unless I am missing something, when this happens for an n-th buffer,
>>> we wake up all, but only one buffer will have the ERROR state, all the
>>> other will be in QUEUED state. This will mess up return values from
>>> dqbuf (if this happens on streamon) for other buffers, they will have
>>> their V4L2_BUF_FLAG_QUEUED set after dqbuf. Also, returning 0 from
>>> DQBUF and the V4L2_BUF_FLAG_ERROR for the failed buffer suggests that
>>> streaming may continue.
>>
>> Actually not quite, as the driver is expected to mark all buffers as
>> erroneous
>> and wake up userspace when the disconnection event is received. Subsequent
>> calls to VIDIOC_QBUF (or VIDIOC_STREAMON) need to return an error. I'm not
>> sure if we need to wake up userspace then, as applications shouldn't sleep on
>> VIDIOC_DQBUF or select() after VIDIOC_QBUF or VIDIOC_STREAMON returned an
>> error.
>>
>
> Ok, but what do you mean by driver marking them as erroneous? By
> issuing vb2_buffer_done with *_ERROR as parameter? Also, you meant
> that vb2 should be waking userspace, right? I believe we should aim
> for a solution that would require the driver to do as little as
> possible and move everything to vb2.
> vb2_dqbuf will return EINVAL and poll()/select() should fail because
> they check for streaming state. As long as the disconnection event
> (e.g. failed qbuf) disables streaming flag in vb2, we should be ok.
>
>>> So how do we recover from this disconnection event? What is the
>>> general idea? If buf_queue fails, can we restart from some point (i.e.
>>> reuse the buffers later) or do we have to clean up completely (i.e.
>>> deallocate, etc.)? Right now we are staying in an undefined state.
>>> If we cannot recover, we shouldn't be setting V4L2_BUF_FLAG_ERROR, but
>>> returning a stronger error instead and maybe clean up the rest, which
>>> is not waited for somehow. If we can recover on the other hand, we
>>> shouldn't be probably waking up all sleepers or at least giving them
>>> meaningful errors.
>>
>> I think a disconnection is pretty fatal. If the user unplugs the webcam,
>> there's not much that can be done anymore. Userspace needs to be woken up
>> with
>> all buffers marked as erroneous, and the next QBUF call needs to return an
>> error without queuing any buffer. We need to define the expe
Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
Hi Pawel,
On Monday 07 March 2011 00:20:36 Pawel Osciak wrote:
> On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart wrote:
> > On Monday 28 February 2011 16:44:38 Pawel Osciak wrote:
> >> Hi Laurent,
> >> A few questions from me below. I feel we need to talk about this
> >> change a bit more, it introduces some recovery and consistency
> >> problems, unless I'm missing something.
> >>
> >> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote:
> >> > videobuf2 expects drivers to check buffer in the buf_prepare operation
> >> > and to return success only if the buffer can queued without any issue.
> >> >
> >> > For hot-pluggable devices, disconnection events need to be handled at
> >> > buf_queue time. Checking the disconnected flag and adding the buffer
> >> > to the driver queue need to be performed without releasing the driver
> >> > spinlock, otherwise race conditions can occur in which the driver
> >> > could still allow a buffer to be queued after the disconnected flag
> >> > has been set, resulting in a hang during the next DQBUF operation.
> >> >
> >> > This problem can be solved either in the videobuf2 core or in the
> >> > device drivers. To avoid adding a spinlock to videobuf2, make
> >> > buf_queue return an int and handle buf_queue failures in videobuf2.
> >> > Drivers must not return an error in buf_queue if the condition
> >> > leading to the error can be caught in buf_prepare.
> >> >
> >> > Signed-off-by: Laurent Pinchart
> >> > ---
> >> > drivers/media/video/videobuf2-core.c | 32
> >> > ++-- include/media/videobuf2-core.h
> >> > | 2 +-
> >> > 2 files changed, 27 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/drivers/media/video/videobuf2-core.c
> >> > b/drivers/media/video/videobuf2-core.c index cc7ab0a..1d81536 100644
> >> > --- a/drivers/media/video/videobuf2-core.c
> >> > +++ b/drivers/media/video/videobuf2-core.c
> >> > @@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb,
> >> > struct v4l2_buffer *b) /**
> >> > * __enqueue_in_driver() - enqueue a vb2_buffer in driver for
> >> > processing */
> >> > -static void __enqueue_in_driver(struct vb2_buffer *vb)
> >> > +static int __enqueue_in_driver(struct vb2_buffer *vb)
> >> > {
> >> >struct vb2_queue *q = vb->vb2_queue;
> >> > + int ret;
> >> >
> >> >vb->state = VB2_BUF_STATE_ACTIVE;
> >> >atomic_inc(&q->queued_count);
> >> > - q->ops->buf_queue(vb);
> >> > + ret = q->ops->buf_queue(vb);
> >> > + if (ret == 0)
> >> > + return 0;
> >> > +
> >> > + vb->state = VB2_BUF_STATE_ERROR;
> >> > + atomic_dec(&q->queued_count);
> >> > + wake_up_all(&q->done_wq);
> >> > +
> >> > + return ret;
> >>
> >> Unless I am missing something, when this happens for an n-th buffer,
> >> we wake up all, but only one buffer will have the ERROR state, all the
> >> other will be in QUEUED state. This will mess up return values from
> >> dqbuf (if this happens on streamon) for other buffers, they will have
> >> their V4L2_BUF_FLAG_QUEUED set after dqbuf. Also, returning 0 from
> >> DQBUF and the V4L2_BUF_FLAG_ERROR for the failed buffer suggests that
> >> streaming may continue.
> >
> > Actually not quite, as the driver is expected to mark all buffers as
> > erroneous and wake up userspace when the disconnection event is
> > received. Subsequent calls to VIDIOC_QBUF (or VIDIOC_STREAMON) need to
> > return an error. I'm not sure if we need to wake up userspace then, as
> > applications shouldn't sleep on VIDIOC_DQBUF or select() after
> > VIDIOC_QBUF or VIDIOC_STREAMON returned an error.
>
> Ok, but what do you mean by driver marking them as erroneous? By
> issuing vb2_buffer_done with *_ERROR as parameter?
Yes.
> Also, you meant that vb2 should be waking userspace, right?
Yes. If an application is sleeping on a DQBUF or poll() call, it needs to be
woken up when the device is disconnected.
> I believe we should aim for a solution that would require the driver to do
> as little as possible and move everything to vb2.
I totally agree with this. All drivers should implement the same disconnect
behaviour, and the required locking is not trivial, so it should be moved to
vb2.
> vb2_dqbuf will return EINVAL and poll()/select() should fail because
> they check for streaming state. As long as the disconnection event
> (e.g. failed qbuf) disables streaming flag in vb2, we should be ok.
What I'm concerned about is race conditions. Please see below for
explanations.
> >> So how do we recover from this disconnection event? What is the
> >> general idea? If buf_queue fails, can we restart from some point (i.e.
> >> reuse the buffers later) or do we have to clean up completely (i.e.
> >> deallocate, etc.)? Right now we are staying in an undefined state.
> >> If we cannot recover, we shouldn't be setting V4L2_BUF_FLAG_ERROR, but
> >> returning a stronger error instead and maybe clean up the rest, which
> >> is not wai
Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
On Sun, 2011-03-06 at 15:20 -0800, Pawel Osciak wrote: > Hi Laurent, > > On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart > > I think a disconnection is pretty fatal. If the user unplugs the webcam, > > there's not much that can be done anymore. Userspace needs to be woken up > > with > > all buffers marked as erroneous, and the next QBUF call needs to return an > > error without queuing any buffer. We need to define the expected behaviour > > in > > the V4L2 spec, so that applications can rely on it and implement it > > properly. > > I would also like to handle this inside videobuf2 if possible (something > > like > > vb2_disconnect() ?) to ensure that all drivers behave correctly, but I'm not > > sure if that will be possible without messing locking up. > > > > I definitely agree that videbuf2 should handle as much as possible and > it shouldn't be left up to drivers. Although I'm not an expert in USB, > shouldn't a disconnection event cause a removal of the device node? Hi Pawel, I would think it should cause the device node to be unlink()-ed from the filesystem. However, even though the device node has been unlink()-ed from the filesystem, all the currently open file descriptors still exist and need to be intelligently handled by the driver until they are closed. > Could you explain how does that work for USB devices in general? If > not, we may need a more general state in vb2, something like "device > inoperable". Not only qbuf should fail then, but almost everything > should. And memory should be freed. I feel there will be the locking > problems as well. The USB layer or cdev should take a reference on the driver module so it can't be unloaded until all the open file descriptors are closed. The driver itself (and maybe videobuf2?) will need to reference count structures that must not be kfree()-ed if a file descriptor is still open. . > I definitely agree that we need to add this to the V4L2 spec. So could > we start from that point? Could we maybe start with a general flow and > expected behavior for a disconnection event? I guess we both have some > ideas in mind, but first I'd like to establish what will happen in > linux driver/USB core when a device is disconnected. > My understanding > is that the driver is removed and module is unloaded, but how does > that happen if we are in the middle of something? Could you give an > example of what happens after a disconnect a camera? A module cannot be unloaded, as long as anything has a reference to the module text using get_module(). If a file descriptor for the driver is still open, something should be holding a reference to the driver module text, so that it cannot be unloaded. In the case of an underlying device being disconnected, the driver has to do something sensible as long as file descriptors for that disconnected device remain open. In fixing up lirc_zilog, an IR device driver, I used locked reference counters to get() and put() pointers to objetcs for the underlying devices. I then had to modify all the driver code to sensibly handle the case when a get() of an object pointer to an underlying device came back as NULL. I suspect Laurent probably had to deal with similar issues already in his changes for v4l2_subdev's. Because it is all still fresh in my mind, what follow are (lengthy) details about how lirc_zilog handles the problem with a disconnect of an IR device. Hopefully it provides something useful for you... lirc_zilog can have the bridge driver (ivtv, cx18, pvrusb2, or hdpvr) removed out from under it, or have the underlying USB device disconnected (pvrusb2, hdpvr), while /dev/lircN device nodes are still open. Here is the sum of my rework to support that gracefully: http://git.linuxtv.org/awalls/media_tree.git?a=blob;f=drivers/staging/lirc/lirc_zilog.c;h=40195ef55e6dcfb4b10c8ff132eb81d551a253a8;hb=8a1f6484fd16ef990d588cc3b979416b2dca23bd It was more work than I expected. Pointers to instances of the following items in that driver are reference counted: struct IR // a Z8 IR device instance struct IR_tx// the Tx function instance of a Z8 instance struct IR_rx// the Rx function instance of a Z8 instance and the structres themselves maintain pointers to each other (which are not exempt from the reference counting) ++ v| struct IR <---+ | | | | | | +> struct IR_rx --+ | | | +--> struct IR_tx -+ The pointers to object instances are handed out like this on device probe; each reference given out is counted: a struct IRpointer is given to the struct IR_rx instance a struct IR_rx pointer is given to the struct IRinstance a struct IR_rx pointer is given to the RX i2c_client instance a struct IRpointer is given to the struct IR_tx in
Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
Hi Laurent,
On Tue, Mar 1, 2011 at 02:54, Laurent Pinchart
wrote:
> Hi Pawel,
>
> On Monday 28 February 2011 16:44:38 Pawel Osciak wrote:
>> Hi Laurent,
>> A few questions from me below. I feel we need to talk about this
>> change a bit more, it introduces some recovery and consistency
>> problems, unless I'm missing something.
>>
>> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote:
>> > videobuf2 expects drivers to check buffer in the buf_prepare operation
>> > and to return success only if the buffer can queued without any issue.
>> >
>> > For hot-pluggable devices, disconnection events need to be handled at
>> > buf_queue time. Checking the disconnected flag and adding the buffer to
>> > the driver queue need to be performed without releasing the driver
>> > spinlock, otherwise race conditions can occur in which the driver could
>> > still allow a buffer to be queued after the disconnected flag has been
>> > set, resulting in a hang during the next DQBUF operation.
>> >
>> > This problem can be solved either in the videobuf2 core or in the device
>> > drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
>> > an int and handle buf_queue failures in videobuf2. Drivers must not
>> > return an error in buf_queue if the condition leading to the error can
>> > be caught in buf_prepare.
>> >
>> > Signed-off-by: Laurent Pinchart
>> > ---
>> > drivers/media/video/videobuf2-core.c | 32
>> > ++-- include/media/videobuf2-core.h |
>> > 2 +-
>> > 2 files changed, 27 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/media/video/videobuf2-core.c
>> > b/drivers/media/video/videobuf2-core.c index cc7ab0a..1d81536 100644
>> > --- a/drivers/media/video/videobuf2-core.c
>> > +++ b/drivers/media/video/videobuf2-core.c
>> > @@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb,
>> > struct v4l2_buffer *b) /**
>> > * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
>> > */
>> > -static void __enqueue_in_driver(struct vb2_buffer *vb)
>> > +static int __enqueue_in_driver(struct vb2_buffer *vb)
>> > {
>> > struct vb2_queue *q = vb->vb2_queue;
>> > + int ret;
>> >
>> > vb->state = VB2_BUF_STATE_ACTIVE;
>> > atomic_inc(&q->queued_count);
>> > - q->ops->buf_queue(vb);
>> > + ret = q->ops->buf_queue(vb);
>> > + if (ret == 0)
>> > + return 0;
>> > +
>> > + vb->state = VB2_BUF_STATE_ERROR;
>> > + atomic_dec(&q->queued_count);
>> > + wake_up_all(&q->done_wq);
>> > +
>> > + return ret;
>>
>> Unless I am missing something, when this happens for an n-th buffer,
>> we wake up all, but only one buffer will have the ERROR state, all the
>> other will be in QUEUED state. This will mess up return values from
>> dqbuf (if this happens on streamon) for other buffers, they will have
>> their V4L2_BUF_FLAG_QUEUED set after dqbuf. Also, returning 0 from
>> DQBUF and the V4L2_BUF_FLAG_ERROR for the failed buffer suggests that
>> streaming may continue.
>
> Actually not quite, as the driver is expected to mark all buffers as erroneous
> and wake up userspace when the disconnection event is received. Subsequent
> calls to VIDIOC_QBUF (or VIDIOC_STREAMON) need to return an error. I'm not
> sure if we need to wake up userspace then, as applications shouldn't sleep on
> VIDIOC_DQBUF or select() after VIDIOC_QBUF or VIDIOC_STREAMON returned an
> error.
>
Ok, but what do you mean by driver marking them as erroneous? By
issuing vb2_buffer_done with *_ERROR as parameter? Also, you meant
that vb2 should be waking userspace, right? I believe we should aim
for a solution that would require the driver to do as little as
possible and move everything to vb2.
vb2_dqbuf will return EINVAL and poll()/select() should fail because
they check for streaming state. As long as the disconnection event
(e.g. failed qbuf) disables streaming flag in vb2, we should be ok.
>> So how do we recover from this disconnection event? What is the
>> general idea? If buf_queue fails, can we restart from some point (i.e.
>> reuse the buffers later) or do we have to clean up completely (i.e.
>> deallocate, etc.)? Right now we are staying in an undefined state.
>> If we cannot recover, we shouldn't be setting V4L2_BUF_FLAG_ERROR, but
>> returning a stronger error instead and maybe clean up the rest, which
>> is not waited for somehow. If we can recover on the other hand, we
>> shouldn't be probably waking up all sleepers or at least giving them
>> meaningful errors.
>
> I think a disconnection is pretty fatal. If the user unplugs the webcam,
> there's not much that can be done anymore. Userspace needs to be woken up with
> all buffers marked as erroneous, and the next QBUF call needs to return an
> error without queuing any buffer. We need to define the expected behaviour in
> the V4L2 spec, so that applications can rely on it and implement it properly.
> I would also like to handle this inside
Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
Hi Pawel,
On Monday 28 February 2011 16:44:38 Pawel Osciak wrote:
> Hi Laurent,
> A few questions from me below. I feel we need to talk about this
> change a bit more, it introduces some recovery and consistency
> problems, unless I'm missing something.
>
> On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart wrote:
> > videobuf2 expects drivers to check buffer in the buf_prepare operation
> > and to return success only if the buffer can queued without any issue.
> >
> > For hot-pluggable devices, disconnection events need to be handled at
> > buf_queue time. Checking the disconnected flag and adding the buffer to
> > the driver queue need to be performed without releasing the driver
> > spinlock, otherwise race conditions can occur in which the driver could
> > still allow a buffer to be queued after the disconnected flag has been
> > set, resulting in a hang during the next DQBUF operation.
> >
> > This problem can be solved either in the videobuf2 core or in the device
> > drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
> > an int and handle buf_queue failures in videobuf2. Drivers must not
> > return an error in buf_queue if the condition leading to the error can
> > be caught in buf_prepare.
> >
> > Signed-off-by: Laurent Pinchart
> > ---
> > drivers/media/video/videobuf2-core.c | 32
> > ++-- include/media/videobuf2-core.h |
> >2 +-
> > 2 files changed, 27 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/media/video/videobuf2-core.c
> > b/drivers/media/video/videobuf2-core.c index cc7ab0a..1d81536 100644
> > --- a/drivers/media/video/videobuf2-core.c
> > +++ b/drivers/media/video/videobuf2-core.c
> > @@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb,
> > struct v4l2_buffer *b) /**
> > * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
> > */
> > -static void __enqueue_in_driver(struct vb2_buffer *vb)
> > +static int __enqueue_in_driver(struct vb2_buffer *vb)
> > {
> >struct vb2_queue *q = vb->vb2_queue;
> > + int ret;
> >
> >vb->state = VB2_BUF_STATE_ACTIVE;
> >atomic_inc(&q->queued_count);
> > - q->ops->buf_queue(vb);
> > + ret = q->ops->buf_queue(vb);
> > + if (ret == 0)
> > + return 0;
> > +
> > + vb->state = VB2_BUF_STATE_ERROR;
> > + atomic_dec(&q->queued_count);
> > + wake_up_all(&q->done_wq);
> > +
> > + return ret;
>
> Unless I am missing something, when this happens for an n-th buffer,
> we wake up all, but only one buffer will have the ERROR state, all the
> other will be in QUEUED state. This will mess up return values from
> dqbuf (if this happens on streamon) for other buffers, they will have
> their V4L2_BUF_FLAG_QUEUED set after dqbuf. Also, returning 0 from
> DQBUF and the V4L2_BUF_FLAG_ERROR for the failed buffer suggests that
> streaming may continue.
Actually not quite, as the driver is expected to mark all buffers as erroneous
and wake up userspace when the disconnection event is received. Subsequent
calls to VIDIOC_QBUF (or VIDIOC_STREAMON) need to return an error. I'm not
sure if we need to wake up userspace then, as applications shouldn't sleep on
VIDIOC_DQBUF or select() after VIDIOC_QBUF or VIDIOC_STREAMON returned an
error.
> So how do we recover from this disconnection event? What is the
> general idea? If buf_queue fails, can we restart from some point (i.e.
> reuse the buffers later) or do we have to clean up completely (i.e.
> deallocate, etc.)? Right now we are staying in an undefined state.
> If we cannot recover, we shouldn't be setting V4L2_BUF_FLAG_ERROR, but
> returning a stronger error instead and maybe clean up the rest, which
> is not waited for somehow. If we can recover on the other hand, we
> shouldn't be probably waking up all sleepers or at least giving them
> meaningful errors.
I think a disconnection is pretty fatal. If the user unplugs the webcam,
there's not much that can be done anymore. Userspace needs to be woken up with
all buffers marked as erroneous, and the next QBUF call needs to return an
error without queuing any buffer. We need to define the expected behaviour in
the V4L2 spec, so that applications can rely on it and implement it properly.
I would also like to handle this inside videobuf2 if possible (something like
vb2_disconnect() ?) to ensure that all drivers behave correctly, but I'm not
sure if that will be possible without messing locking up.
> > }
> > /**
> > @@ -890,8 +899,13 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer
> > *b) * If already streaming, give the buffer to driver for processing. *
> > If not, the buffer will be given to driver on next streamon. */
> > - if (q->streaming)
> > - __enqueue_in_driver(vb);
> > + if (q->streaming) {
> > + ret = __enqueue_in_driver(vb);
> > + if (ret < 0) {
> > + dprintk(1, "qbuf: buffer q
Re: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
Hi Laurent,
A few questions from me below. I feel we need to talk about this
change a bit more, it introduces some recovery and consistency
problems, unless I'm missing something.
On Sun, Feb 27, 2011 at 10:12, Laurent Pinchart
wrote:
> videobuf2 expects drivers to check buffer in the buf_prepare operation
> and to return success only if the buffer can queued without any issue.
>
> For hot-pluggable devices, disconnection events need to be handled at
> buf_queue time. Checking the disconnected flag and adding the buffer to
> the driver queue need to be performed without releasing the driver
> spinlock, otherwise race conditions can occur in which the driver could
> still allow a buffer to be queued after the disconnected flag has been
> set, resulting in a hang during the next DQBUF operation.
>
> This problem can be solved either in the videobuf2 core or in the device
> drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
> an int and handle buf_queue failures in videobuf2. Drivers must not
> return an error in buf_queue if the condition leading to the error can
> be caught in buf_prepare.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/media/video/videobuf2-core.c | 32 ++--
> include/media/videobuf2-core.h | 2 +-
> 2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/video/videobuf2-core.c
> b/drivers/media/video/videobuf2-core.c
> index cc7ab0a..1d81536 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb, struct
> v4l2_buffer *b)
> /**
> * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
> */
> -static void __enqueue_in_driver(struct vb2_buffer *vb)
> +static int __enqueue_in_driver(struct vb2_buffer *vb)
> {
> struct vb2_queue *q = vb->vb2_queue;
> + int ret;
>
> vb->state = VB2_BUF_STATE_ACTIVE;
> atomic_inc(&q->queued_count);
> - q->ops->buf_queue(vb);
> + ret = q->ops->buf_queue(vb);
> + if (ret == 0)
> + return 0;
> +
> + vb->state = VB2_BUF_STATE_ERROR;
> + atomic_dec(&q->queued_count);
> + wake_up_all(&q->done_wq);
> +
> + return ret;
Unless I am missing something, when this happens for an n-th buffer,
we wake up all, but only one buffer will have the ERROR state, all the
other will be in QUEUED state. This will mess up return values from
dqbuf (if this happens on streamon) for other buffers, they will have
their V4L2_BUF_FLAG_QUEUED set after dqbuf. Also, returning 0 from
DQBUF and the V4L2_BUF_FLAG_ERROR for the failed buffer suggests that
streaming may continue.
So how do we recover from this disconnection event? What is the
general idea? If buf_queue fails, can we restart from some point (i.e.
reuse the buffers later) or do we have to clean up completely (i.e.
deallocate, etc.)? Right now we are staying in an undefined state.
If we cannot recover, we shouldn't be setting V4L2_BUF_FLAG_ERROR, but
returning a stronger error instead and maybe clean up the rest, which
is not waited for somehow. If we can recover on the other hand, we
shouldn't be probably waking up all sleepers or at least giving them
meaningful errors.
> }
> /**
> @@ -890,8 +899,13 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> * If already streaming, give the buffer to driver for processing.
> * If not, the buffer will be given to driver on next streamon.
> */
> - if (q->streaming)
> - __enqueue_in_driver(vb);
> + if (q->streaming) {
> + ret = __enqueue_in_driver(vb);
> + if (ret < 0) {
> + dprintk(1, "qbuf: buffer queue failed\n");
> + return ret;
> + }
> + }
>
What errors can be allowed to be returned from driver here? EIO? Also,
isn't returning an error here to userspace suggesting that qbuf didn't
happen? But it actually did happen, we put the buffer onto vb2 list
and set it state to QUEUED. From the point of view of vb2, the buffer
is on its queue, but the userspace may not think so.
> dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index);
> return 0;
> @@ -1101,6 +1115,7 @@ EXPORT_SYMBOL_GPL(vb2_dqbuf);
> int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
> {
> struct vb2_buffer *vb;
> + int ret;
>
> if (q->fileio) {
> dprintk(1, "streamon: file io in progress\n");
> @@ -1139,8 +1154,13 @@ int vb2_streamon(struct vb2_queue *q, enum
> v4l2_buf_type type)
> * If any buffers were queued before streamon,
> * we can now pass them to driver for processing.
> */
> - list_for_each_entry(vb, &q->queued_list, queued_entry)
> - __enqueue_in_driver(vb);
> + list_for_each_entry(vb, &q->queued_list, queued_entry) {
> + ret =
RE: [RFC/PATCH 1/2] v4l: videobuf2: Handle buf_queue errors
Hello,
On Sunday, February 27, 2011 7:13 PM Laurent Pinchart wrote:
> videobuf2 expects drivers to check buffer in the buf_prepare operation
> and to return success only if the buffer can queued without any issue.
>
> For hot-pluggable devices, disconnection events need to be handled at
> buf_queue time. Checking the disconnected flag and adding the buffer to
> the driver queue need to be performed without releasing the driver
> spinlock, otherwise race conditions can occur in which the driver could
> still allow a buffer to be queued after the disconnected flag has been
> set, resulting in a hang during the next DQBUF operation.
>
> This problem can be solved either in the videobuf2 core or in the device
> drivers. To avoid adding a spinlock to videobuf2, make buf_queue return
> an int and handle buf_queue failures in videobuf2. Drivers must not
> return an error in buf_queue if the condition leading to the error can
> be caught in buf_prepare.
>
> Signed-off-by: Laurent Pinchart
Acked-by: Marek Szyprowski
We discussed how to solve the hot-plug issue and this is the solution I prefer.
> ---
> drivers/media/video/videobuf2-core.c | 32 ++--
> include/media/videobuf2-core.h |2 +-
> 2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/video/videobuf2-core.c
> b/drivers/media/video/videobuf2-core.c
> index cc7ab0a..1d81536 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -798,13 +798,22 @@ static int __qbuf_mmap(struct vb2_buffer *vb, struct
> v4l2_buffer *b)
> /**
> * __enqueue_in_driver() - enqueue a vb2_buffer in driver for processing
> */
> -static void __enqueue_in_driver(struct vb2_buffer *vb)
> +static int __enqueue_in_driver(struct vb2_buffer *vb)
> {
> struct vb2_queue *q = vb->vb2_queue;
> + int ret;
>
> vb->state = VB2_BUF_STATE_ACTIVE;
> atomic_inc(&q->queued_count);
> - q->ops->buf_queue(vb);
> + ret = q->ops->buf_queue(vb);
> + if (ret == 0)
> + return 0;
> +
> + vb->state = VB2_BUF_STATE_ERROR;
> + atomic_dec(&q->queued_count);
> + wake_up_all(&q->done_wq);
> +
> + return ret;
> }
>
> /**
> @@ -890,8 +899,13 @@ int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
>* If already streaming, give the buffer to driver for processing.
>* If not, the buffer will be given to driver on next streamon.
>*/
> - if (q->streaming)
> - __enqueue_in_driver(vb);
> + if (q->streaming) {
> + ret = __enqueue_in_driver(vb);
> + if (ret < 0) {
> + dprintk(1, "qbuf: buffer queue failed\n");
> + return ret;
> + }
> + }
>
> dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index);
> return 0;
> @@ -1101,6 +1115,7 @@ EXPORT_SYMBOL_GPL(vb2_dqbuf);
> int vb2_streamon(struct vb2_queue *q, enum v4l2_buf_type type)
> {
> struct vb2_buffer *vb;
> + int ret;
>
> if (q->fileio) {
> dprintk(1, "streamon: file io in progress\n");
> @@ -1139,8 +1154,13 @@ int vb2_streamon(struct vb2_queue *q, enum
> v4l2_buf_type type)
>* If any buffers were queued before streamon,
>* we can now pass them to driver for processing.
>*/
> - list_for_each_entry(vb, &q->queued_list, queued_entry)
> - __enqueue_in_driver(vb);
> + list_for_each_entry(vb, &q->queued_list, queued_entry) {
> + ret = __enqueue_in_driver(vb);
> + if (ret < 0) {
> + dprintk(1, "streamon: buffer queue failed\n");
> + return ret;
> + }
> + }
>
> dprintk(3, "Streamon successful\n");
> return 0;
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 597efe6..3a92f75 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -225,7 +225,7 @@ struct vb2_ops {
> int (*start_streaming)(struct vb2_queue *q);
> int (*stop_streaming)(struct vb2_queue *q);
>
> - void (*buf_queue)(struct vb2_buffer *vb);
> + int (*buf_queue)(struct vb2_buffer *vb);
> };
>
> /**
> --
> 1.7.3.4
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
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
