[RFC,v1,26/27] v4l2: v4l2_camera: Always clear pending requests
diff mbox series

Message ID 20260618123844.656396-27-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • Misc. changes before request-buffer split
Related show

Commit Message

Barnabás Pőcze June 18, 2026, 12:38 p.m. UTC
Clear the list of pending requests regardless whether the camera is running
or not. The list should only contain entries when `!isRunning_`, in which
case it was not cleared previously.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/v4l2/v4l2_camera.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jacopo Mondi June 22, 2026, 1:57 p.m. UTC | #1
On Thu, Jun 18, 2026 at 02:38:43PM +0200, Barnabás Pőcze wrote:
> Clear the list of pending requests regardless whether the camera is running
> or not. The list should only contain entries when `!isRunning_`, in which
> case it was not cleared previously.


Isn't this meant to mimic the behaviour of STREAMOFF ?

https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html
If VIDIOC_STREAMON fails then any already queued buffers will remain queued.

>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/v4l2/v4l2_camera.cpp | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> index e72b73a22a..8c7de8e92b 100644
> --- a/src/v4l2/v4l2_camera.cpp
> +++ b/src/v4l2/v4l2_camera.cpp
> @@ -226,6 +226,8 @@ int V4L2Camera::streamOn()
>
>  int V4L2Camera::streamOff()
>  {
> +	pendingRequests_.clear();
> +
>  	if (!isRunning_) {
>  		for (std::unique_ptr<Request> &req : requestPool_)
>  			req->reuse();
> @@ -233,8 +235,6 @@ int V4L2Camera::streamOff()
>  		return 0;
>  	}
>
> -	pendingRequests_.clear();
> -
>  	int ret = camera_->stop();
>  	if (ret < 0)
>  		return ret == -EACCES ? -EBUSY : ret;
> --
> 2.54.0
>
Barnabás Pőcze June 22, 2026, 2:06 p.m. UTC | #2
2026. 06. 22. 15:57 keltezéssel, Jacopo Mondi írta:
> On Thu, Jun 18, 2026 at 02:38:43PM +0200, Barnabás Pőcze wrote:
>> Clear the list of pending requests regardless whether the camera is running
>> or not. The list should only contain entries when `!isRunning_`, in which
>> case it was not cleared previously.
> 
> 
> Isn't this meant to mimic the behaviour of STREAMOFF ?
> 
> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html
> If VIDIOC_STREAMON fails then any already queued buffers will remain queued.

But `VIDIOC_STREAMOFF` should always clear the pending buffers, no?

   If buffers have been queued with ioctl VIDIOC_QBUF, VIDIOC_DQBUF and VIDIOC_STREAMOFF is
   called without ever having called VIDIOC_STREAMON, then those queued buffers will also be
   removed from the incoming queue and all are returned to the same state as after calling
   ioctl VIDIOC_REQBUFS and can be restarted accordingly.

And `V4L2CameraProxy::vidioc_streamoff()` seems to match this because it removes `V4L2_BUF_FLAG_QUEUED`
from all buffers. But that means the pending list must be cleared as well in my understanding.

> 
>>
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   src/v4l2/v4l2_camera.cpp | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
>> index e72b73a22a..8c7de8e92b 100644
>> --- a/src/v4l2/v4l2_camera.cpp
>> +++ b/src/v4l2/v4l2_camera.cpp
>> @@ -226,6 +226,8 @@ int V4L2Camera::streamOn()
>>
>>   int V4L2Camera::streamOff()
>>   {
>> +	pendingRequests_.clear();
>> +
>>   	if (!isRunning_) {
>>   		for (std::unique_ptr<Request> &req : requestPool_)
>>   			req->reuse();
>> @@ -233,8 +235,6 @@ int V4L2Camera::streamOff()
>>   		return 0;
>>   	}
>>
>> -	pendingRequests_.clear();
>> -
>>   	int ret = camera_->stop();
>>   	if (ret < 0)
>>   		return ret == -EACCES ? -EBUSY : ret;
>> --
>> 2.54.0
>>
Jacopo Mondi June 22, 2026, 2:22 p.m. UTC | #3
Hi Barnabás

On Mon, Jun 22, 2026 at 04:06:05PM +0200, Barnabás Pőcze wrote:
> 2026. 06. 22. 15:57 keltezéssel, Jacopo Mondi írta:
> > On Thu, Jun 18, 2026 at 02:38:43PM +0200, Barnabás Pőcze wrote:
> > > Clear the list of pending requests regardless whether the camera is running
> > > or not. The list should only contain entries when `!isRunning_`, in which
> > > case it was not cleared previously.
> >
> >
> > Isn't this meant to mimic the behaviour of STREAMOFF ?
> >
> > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html
> > If VIDIOC_STREAMON fails then any already queued buffers will remain queued.
>
> But `VIDIOC_STREAMOFF` should always clear the pending buffers, no?
>
>   If buffers have been queued with ioctl VIDIOC_QBUF, VIDIOC_DQBUF and VIDIOC_STREAMOFF is
>   called without ever having called VIDIOC_STREAMON, then those queued buffers will also be
>   removed from the incoming queue and all are returned to the same state as after calling
>   ioctl VIDIOC_REQBUFS and can be restarted accordingly.
>
> And `V4L2CameraProxy::vidioc_streamoff()` seems to match this because it removes `V4L2_BUF_FLAG_QUEUED`
> from all buffers. But that means the pending list must be cleared as well in my understanding.

As  V4L2Camera::streamOn() consumes all pendingRequests_ then if the
list is still populate at  V4L2Camera::streamOff() does it mean we
never got through a STREAMON ?

If that's the case then yes, we should return all pending request.

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Let's maybe ask Paul as well as the author of this part of the
codebase.

>
> >
> > >
> > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > ---
> > >   src/v4l2/v4l2_camera.cpp | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > index e72b73a22a..8c7de8e92b 100644
> > > --- a/src/v4l2/v4l2_camera.cpp
> > > +++ b/src/v4l2/v4l2_camera.cpp
> > > @@ -226,6 +226,8 @@ int V4L2Camera::streamOn()
> > >
> > >   int V4L2Camera::streamOff()
> > >   {
> > > +	pendingRequests_.clear();
> > > +
> > >   	if (!isRunning_) {
> > >   		for (std::unique_ptr<Request> &req : requestPool_)
> > >   			req->reuse();
> > > @@ -233,8 +235,6 @@ int V4L2Camera::streamOff()
> > >   		return 0;
> > >   	}
> > >
> > > -	pendingRequests_.clear();
> > > -
> > >   	int ret = camera_->stop();
> > >   	if (ret < 0)
> > >   		return ret == -EACCES ? -EBUSY : ret;
> > > --
> > > 2.54.0
> > >
>
Laurent Pinchart June 23, 2026, 11:33 p.m. UTC | #4
On Mon, Jun 22, 2026 at 04:22:28PM +0200, Jacopo Mondi wrote:
> On Mon, Jun 22, 2026 at 04:06:05PM +0200, Barnabás Pőcze wrote:
> > 2026. 06. 22. 15:57 keltezéssel, Jacopo Mondi írta:
> > > On Thu, Jun 18, 2026 at 02:38:43PM +0200, Barnabás Pőcze wrote:
> > > > Clear the list of pending requests regardless whether the camera is running
> > > > or not. The list should only contain entries when `!isRunning_`, in which
> > > > case it was not cleared previously.
> > >
> > > Isn't this meant to mimic the behaviour of STREAMOFF ?
> > >
> > > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html
> > > If VIDIOC_STREAMON fails then any already queued buffers will remain queued.
> >
> > But `VIDIOC_STREAMOFF` should always clear the pending buffers, no?
> >
> >   If buffers have been queued with ioctl VIDIOC_QBUF, VIDIOC_DQBUF and VIDIOC_STREAMOFF is
> >   called without ever having called VIDIOC_STREAMON, then those queued buffers will also be
> >   removed from the incoming queue and all are returned to the same state as after calling
> >   ioctl VIDIOC_REQBUFS and can be restarted accordingly.
> >
> > And `V4L2CameraProxy::vidioc_streamoff()` seems to match this because it removes `V4L2_BUF_FLAG_QUEUED`
> > from all buffers. But that means the pending list must be cleared as well in my understanding.
> 
> As  V4L2Camera::streamOn() consumes all pendingRequests_ then if the
> list is still populate at  V4L2Camera::streamOff() does it mean we
> never got through a STREAMON ?
> 
> If that's the case then yes, we should return all pending request.

In V4L2, if buffers have been queued, and STREAMON hasn't been called,
STREAMOFF will dequeue all the queued buffers.

V4L2CameraProxy::vidioc_qbuf() sets the V4L2_BUF_FLAG_QUEUED flag on the
buffer being queued, and V4L2CameraProxy::vidioc_streamoff() clears that
flag from all buffers, regardless of if the camera is running.
V4L2CameraProxy implements the V4L2 behaviour properly.

In V4L2Camera, when not running, qbuf() takes a request from the pool,
adds the buffer to the request, and pushes the request to the
pendingRequests_ list. streamOff() should undo that. It already calls
Request::reuse() on all requests to remove the associated buffers, and
should clear the pendingRequests_ list as done in this patch.

The change seems good to me, but the commit message should be expanded
to explain the rationale better. With that,

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

Also, the v4l2-compat unit test should catch this
issue. v4l2-compliance already tests an ioctl sequence that should
trigger the issue, in testMmap():

	// Test queuing buffers...
	for (unsigned i = 0; i < q.g_buffers(); i++) {
		buffer buf(q);

		fail_on_test(buf.querybuf(node, i));
		fail_on_test(buf.qbuf(node));
		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_REQUEST_FD);
		fail_on_test(buf.g_request_fd());
	}
	// calling STREAMOFF...
	fail_on_test(node->streamoff(q.g_type()));
	// and now we should be able to queue those buffers again since
	// STREAMOFF should return them back to the dequeued state.
	for (unsigned i = 0; i < q.g_buffers(); i++) {
		buffer buf(q);

		fail_on_test(buf.querybuf(node, i));
		fail_on_test(buf.qbuf(node));
		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
	}

As far as I understand, this should result in the request being added
twice to pendingRequests_, which should then cause
V4L2Camera::streamOn() to return an error.

Barnabás, could you check this ?

> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> Let's maybe ask Paul as well as the author of this part of the
> codebase.
> 
> > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > > ---
> > > >   src/v4l2/v4l2_camera.cpp | 4 ++--
> > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > > index e72b73a22a..8c7de8e92b 100644
> > > > --- a/src/v4l2/v4l2_camera.cpp
> > > > +++ b/src/v4l2/v4l2_camera.cpp
> > > > @@ -226,6 +226,8 @@ int V4L2Camera::streamOn()
> > > >
> > > >   int V4L2Camera::streamOff()
> > > >   {
> > > > +	pendingRequests_.clear();
> > > > +
> > > >   	if (!isRunning_) {
> > > >   		for (std::unique_ptr<Request> &req : requestPool_)
> > > >   			req->reuse();
> > > > @@ -233,8 +235,6 @@ int V4L2Camera::streamOff()
> > > >   		return 0;
> > > >   	}
> > > >
> > > > -	pendingRequests_.clear();
> > > > -
> > > >   	int ret = camera_->stop();
> > > >   	if (ret < 0)
> > > >   		return ret == -EACCES ? -EBUSY : ret;
Barnabás Pőcze June 24, 2026, 8:17 a.m. UTC | #5
2026. 06. 24. 1:33 keltezéssel, Laurent Pinchart írta:
> On Mon, Jun 22, 2026 at 04:22:28PM +0200, Jacopo Mondi wrote:
>> On Mon, Jun 22, 2026 at 04:06:05PM +0200, Barnabás Pőcze wrote:
>>> 2026. 06. 22. 15:57 keltezéssel, Jacopo Mondi írta:
>>>> On Thu, Jun 18, 2026 at 02:38:43PM +0200, Barnabás Pőcze wrote:
>>>>> Clear the list of pending requests regardless whether the camera is running
>>>>> or not. The list should only contain entries when `!isRunning_`, in which
>>>>> case it was not cleared previously.
>>>>
>>>> Isn't this meant to mimic the behaviour of STREAMOFF ?
>>>>
>>>> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html
>>>> If VIDIOC_STREAMON fails then any already queued buffers will remain queued.
>>>
>>> But `VIDIOC_STREAMOFF` should always clear the pending buffers, no?
>>>
>>>    If buffers have been queued with ioctl VIDIOC_QBUF, VIDIOC_DQBUF and VIDIOC_STREAMOFF is
>>>    called without ever having called VIDIOC_STREAMON, then those queued buffers will also be
>>>    removed from the incoming queue and all are returned to the same state as after calling
>>>    ioctl VIDIOC_REQBUFS and can be restarted accordingly.
>>>
>>> And `V4L2CameraProxy::vidioc_streamoff()` seems to match this because it removes `V4L2_BUF_FLAG_QUEUED`
>>> from all buffers. But that means the pending list must be cleared as well in my understanding.
>>
>> As  V4L2Camera::streamOn() consumes all pendingRequests_ then if the
>> list is still populate at  V4L2Camera::streamOff() does it mean we
>> never got through a STREAMON ?
>>
>> If that's the case then yes, we should return all pending request.
> 
> In V4L2, if buffers have been queued, and STREAMON hasn't been called,
> STREAMOFF will dequeue all the queued buffers.
> 
> V4L2CameraProxy::vidioc_qbuf() sets the V4L2_BUF_FLAG_QUEUED flag on the
> buffer being queued, and V4L2CameraProxy::vidioc_streamoff() clears that
> flag from all buffers, regardless of if the camera is running.
> V4L2CameraProxy implements the V4L2 behaviour properly.
> 
> In V4L2Camera, when not running, qbuf() takes a request from the pool,
> adds the buffer to the request, and pushes the request to the
> pendingRequests_ list. streamOff() should undo that. It already calls
> Request::reuse() on all requests to remove the associated buffers, and
> should clear the pendingRequests_ list as done in this patch.
> 
> The change seems good to me, but the commit message should be expanded
> to explain the rationale better. With that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Also, the v4l2-compat unit test should catch this
> issue. v4l2-compliance already tests an ioctl sequence that should
> trigger the issue, in testMmap():
> 
> 	// Test queuing buffers...
> 	for (unsigned i = 0; i < q.g_buffers(); i++) {
> 		buffer buf(q);
> 
> 		fail_on_test(buf.querybuf(node, i));
> 		fail_on_test(buf.qbuf(node));
> 		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
> 		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_REQUEST_FD);
> 		fail_on_test(buf.g_request_fd());
> 	}
> 	// calling STREAMOFF...
> 	fail_on_test(node->streamoff(q.g_type()));
> 	// and now we should be able to queue those buffers again since
> 	// STREAMOFF should return them back to the dequeued state.
> 	for (unsigned i = 0; i < q.g_buffers(); i++) {
> 		buffer buf(q);
> 
> 		fail_on_test(buf.querybuf(node, i));
> 		fail_on_test(buf.qbuf(node));
> 		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
> 	}
> 
> As far as I understand, this should result in the request being added
> twice to pendingRequests_, which should then cause
> V4L2Camera::streamOn() to return an error.
> 
> Barnabás, could you check this ?

As far as I can tell, the above test won't trigger any issues because it
does not start the camera afterwards but requests buffers again, which
calls `V4L2Camera::freeBuffers()`, which will clear `V4L2Camera::pendingRequests_`.

It would have to do STREAMON after the above loop. I haven't checked
if there is a test that does that.


> 
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>
>> Let's maybe ask Paul as well as the author of this part of the
>> codebase.
>>
>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>>> ---
>>>>>    src/v4l2/v4l2_camera.cpp | 4 ++--
>>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
>>>>> index e72b73a22a..8c7de8e92b 100644
>>>>> --- a/src/v4l2/v4l2_camera.cpp
>>>>> +++ b/src/v4l2/v4l2_camera.cpp
>>>>> @@ -226,6 +226,8 @@ int V4L2Camera::streamOn()
>>>>>
>>>>>    int V4L2Camera::streamOff()
>>>>>    {
>>>>> +	pendingRequests_.clear();
>>>>> +
>>>>>    	if (!isRunning_) {
>>>>>    		for (std::unique_ptr<Request> &req : requestPool_)
>>>>>    			req->reuse();
>>>>> @@ -233,8 +235,6 @@ int V4L2Camera::streamOff()
>>>>>    		return 0;
>>>>>    	}
>>>>>
>>>>> -	pendingRequests_.clear();
>>>>> -
>>>>>    	int ret = camera_->stop();
>>>>>    	if (ret < 0)
>>>>>    		return ret == -EACCES ? -EBUSY : ret;
> 
> --
> Regards,
> 
> Laurent Pinchart
Paul Elder June 24, 2026, 8:32 a.m. UTC | #6
Quoting Laurent Pinchart (2026-06-24 08:33:07)
> On Mon, Jun 22, 2026 at 04:22:28PM +0200, Jacopo Mondi wrote:
> > On Mon, Jun 22, 2026 at 04:06:05PM +0200, Barnabás Pőcze wrote:
> > > 2026. 06. 22. 15:57 keltezéssel, Jacopo Mondi írta:
> > > > On Thu, Jun 18, 2026 at 02:38:43PM +0200, Barnabás Pőcze wrote:
> > > > > Clear the list of pending requests regardless whether the camera is running
> > > > > or not. The list should only contain entries when `!isRunning_`, in which
> > > > > case it was not cleared previously.
> > > >
> > > > Isn't this meant to mimic the behaviour of STREAMOFF ?
> > > >
> > > > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html
> > > > If VIDIOC_STREAMON fails then any already queued buffers will remain queued.
> > >
> > > But `VIDIOC_STREAMOFF` should always clear the pending buffers, no?
> > >
> > >   If buffers have been queued with ioctl VIDIOC_QBUF, VIDIOC_DQBUF and VIDIOC_STREAMOFF is
> > >   called without ever having called VIDIOC_STREAMON, then those queued buffers will also be
> > >   removed from the incoming queue and all are returned to the same state as after calling
> > >   ioctl VIDIOC_REQBUFS and can be restarted accordingly.
> > >
> > > And `V4L2CameraProxy::vidioc_streamoff()` seems to match this because it removes `V4L2_BUF_FLAG_QUEUED`
> > > from all buffers. But that means the pending list must be cleared as well in my understanding.
> > 
> > As  V4L2Camera::streamOn() consumes all pendingRequests_ then if the
> > list is still populate at  V4L2Camera::streamOff() does it mean we
> > never got through a STREAMON ?
> > 
> > If that's the case then yes, we should return all pending request.
> 
> In V4L2, if buffers have been queued, and STREAMON hasn't been called,
> STREAMOFF will dequeue all the queued buffers.
> 
> V4L2CameraProxy::vidioc_qbuf() sets the V4L2_BUF_FLAG_QUEUED flag on the
> buffer being queued, and V4L2CameraProxy::vidioc_streamoff() clears that
> flag from all buffers, regardless of if the camera is running.
> V4L2CameraProxy implements the V4L2 behaviour properly.
> 
> In V4L2Camera, when not running, qbuf() takes a request from the pool,
> adds the buffer to the request, and pushes the request to the
> pendingRequests_ list. streamOff() should undo that. It already calls
> Request::reuse() on all requests to remove the associated buffers, and
> should clear the pendingRequests_ list as done in this patch.

Yes.

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> 
> The change seems good to me, but the commit message should be expanded
> to explain the rationale better. With that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Also, the v4l2-compat unit test should catch this
> issue. v4l2-compliance already tests an ioctl sequence that should
> trigger the issue, in testMmap():
> 
>         // Test queuing buffers...
>         for (unsigned i = 0; i < q.g_buffers(); i++) {
>                 buffer buf(q);
> 
>                 fail_on_test(buf.querybuf(node, i));
>                 fail_on_test(buf.qbuf(node));
>                 fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
>                 fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_REQUEST_FD);
>                 fail_on_test(buf.g_request_fd());
>         }
>         // calling STREAMOFF...
>         fail_on_test(node->streamoff(q.g_type()));
>         // and now we should be able to queue those buffers again since
>         // STREAMOFF should return them back to the dequeued state.
>         for (unsigned i = 0; i < q.g_buffers(); i++) {
>                 buffer buf(q);
> 
>                 fail_on_test(buf.querybuf(node, i));
>                 fail_on_test(buf.qbuf(node));
>                 fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
>         }
> 
> As far as I understand, this should result in the request being added
> twice to pendingRequests_, which should then cause
> V4L2Camera::streamOn() to return an error.
> 
> Barnabás, could you check this ?
> 
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > 
> > Let's maybe ask Paul as well as the author of this part of the
> > codebase.
> > 
> > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > > > ---
> > > > >   src/v4l2/v4l2_camera.cpp | 4 ++--
> > > > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> > > > > index e72b73a22a..8c7de8e92b 100644
> > > > > --- a/src/v4l2/v4l2_camera.cpp
> > > > > +++ b/src/v4l2/v4l2_camera.cpp
> > > > > @@ -226,6 +226,8 @@ int V4L2Camera::streamOn()
> > > > >
> > > > >   int V4L2Camera::streamOff()
> > > > >   {
> > > > > +       pendingRequests_.clear();
> > > > > +
> > > > >         if (!isRunning_) {
> > > > >                 for (std::unique_ptr<Request> &req : requestPool_)
> > > > >                         req->reuse();
> > > > > @@ -233,8 +235,6 @@ int V4L2Camera::streamOff()
> > > > >                 return 0;
> > > > >         }
> > > > >
> > > > > -       pendingRequests_.clear();
> > > > > -
> > > > >         int ret = camera_->stop();
> > > > >         if (ret < 0)
> > > > >                 return ret == -EACCES ? -EBUSY : ret;
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Laurent Pinchart June 24, 2026, 9:26 a.m. UTC | #7
On Wed, Jun 24, 2026 at 10:17:40AM +0200, Barnabás Pőcze wrote:
> 2026. 06. 24. 1:33 keltezéssel, Laurent Pinchart írta:
> > On Mon, Jun 22, 2026 at 04:22:28PM +0200, Jacopo Mondi wrote:
> >> On Mon, Jun 22, 2026 at 04:06:05PM +0200, Barnabás Pőcze wrote:
> >>> 2026. 06. 22. 15:57 keltezéssel, Jacopo Mondi írta:
> >>>> On Thu, Jun 18, 2026 at 02:38:43PM +0200, Barnabás Pőcze wrote:
> >>>>> Clear the list of pending requests regardless whether the camera is running
> >>>>> or not. The list should only contain entries when `!isRunning_`, in which
> >>>>> case it was not cleared previously.
> >>>>
> >>>> Isn't this meant to mimic the behaviour of STREAMOFF ?
> >>>>
> >>>> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html
> >>>> If VIDIOC_STREAMON fails then any already queued buffers will remain queued.
> >>>
> >>> But `VIDIOC_STREAMOFF` should always clear the pending buffers, no?
> >>>
> >>>    If buffers have been queued with ioctl VIDIOC_QBUF, VIDIOC_DQBUF and VIDIOC_STREAMOFF is
> >>>    called without ever having called VIDIOC_STREAMON, then those queued buffers will also be
> >>>    removed from the incoming queue and all are returned to the same state as after calling
> >>>    ioctl VIDIOC_REQBUFS and can be restarted accordingly.
> >>>
> >>> And `V4L2CameraProxy::vidioc_streamoff()` seems to match this because it removes `V4L2_BUF_FLAG_QUEUED`
> >>> from all buffers. But that means the pending list must be cleared as well in my understanding.
> >>
> >> As  V4L2Camera::streamOn() consumes all pendingRequests_ then if the
> >> list is still populate at  V4L2Camera::streamOff() does it mean we
> >> never got through a STREAMON ?
> >>
> >> If that's the case then yes, we should return all pending request.
> > 
> > In V4L2, if buffers have been queued, and STREAMON hasn't been called,
> > STREAMOFF will dequeue all the queued buffers.
> > 
> > V4L2CameraProxy::vidioc_qbuf() sets the V4L2_BUF_FLAG_QUEUED flag on the
> > buffer being queued, and V4L2CameraProxy::vidioc_streamoff() clears that
> > flag from all buffers, regardless of if the camera is running.
> > V4L2CameraProxy implements the V4L2 behaviour properly.
> > 
> > In V4L2Camera, when not running, qbuf() takes a request from the pool,
> > adds the buffer to the request, and pushes the request to the
> > pendingRequests_ list. streamOff() should undo that. It already calls
> > Request::reuse() on all requests to remove the associated buffers, and
> > should clear the pendingRequests_ list as done in this patch.
> > 
> > The change seems good to me, but the commit message should be expanded
> > to explain the rationale better. With that,
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > Also, the v4l2-compat unit test should catch this
> > issue. v4l2-compliance already tests an ioctl sequence that should
> > trigger the issue, in testMmap():
> > 
> > 	// Test queuing buffers...
> > 	for (unsigned i = 0; i < q.g_buffers(); i++) {
> > 		buffer buf(q);
> > 
> > 		fail_on_test(buf.querybuf(node, i));
> > 		fail_on_test(buf.qbuf(node));
> > 		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
> > 		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_REQUEST_FD);
> > 		fail_on_test(buf.g_request_fd());
> > 	}
> > 	// calling STREAMOFF...
> > 	fail_on_test(node->streamoff(q.g_type()));
> > 	// and now we should be able to queue those buffers again since
> > 	// STREAMOFF should return them back to the dequeued state.
> > 	for (unsigned i = 0; i < q.g_buffers(); i++) {
> > 		buffer buf(q);
> > 
> > 		fail_on_test(buf.querybuf(node, i));
> > 		fail_on_test(buf.qbuf(node));
> > 		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
> > 	}
> > 
> > As far as I understand, this should result in the request being added
> > twice to pendingRequests_, which should then cause
> > V4L2Camera::streamOn() to return an error.
> > 
> > Barnabás, could you check this ?
> 
> As far as I can tell, the above test won't trigger any issues because it
> does not start the camera afterwards but requests buffers again, which
> calls `V4L2Camera::freeBuffers()`, which will clear `V4L2Camera::pendingRequests_`.
> 
> It would have to do STREAMON after the above loop. I haven't checked
> if there is a test that does that.

A right, that's probably what's happening.

Upstreaming a new test for this to v4l2-compliance is not
straightforward as the code is very convoluted (at least in my opinion),
but hacking it to test this scenario should be quick. Would you have
time to give it a go, to verify that it triggers as expected without
this patch, and that this patch fixes the issue ?

> >> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >>
> >> Let's maybe ask Paul as well as the author of this part of the
> >> codebase.
> >>
> >>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> >>>>> ---
> >>>>>    src/v4l2/v4l2_camera.cpp | 4 ++--
> >>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
> >>>>> index e72b73a22a..8c7de8e92b 100644
> >>>>> --- a/src/v4l2/v4l2_camera.cpp
> >>>>> +++ b/src/v4l2/v4l2_camera.cpp
> >>>>> @@ -226,6 +226,8 @@ int V4L2Camera::streamOn()
> >>>>>
> >>>>>    int V4L2Camera::streamOff()
> >>>>>    {
> >>>>> +	pendingRequests_.clear();
> >>>>> +
> >>>>>    	if (!isRunning_) {
> >>>>>    		for (std::unique_ptr<Request> &req : requestPool_)
> >>>>>    			req->reuse();
> >>>>> @@ -233,8 +235,6 @@ int V4L2Camera::streamOff()
> >>>>>    		return 0;
> >>>>>    	}
> >>>>>
> >>>>> -	pendingRequests_.clear();
> >>>>> -
> >>>>>    	int ret = camera_->stop();
> >>>>>    	if (ret < 0)
> >>>>>    		return ret == -EACCES ? -EBUSY : ret;
Barnabás Pőcze June 24, 2026, 12:07 p.m. UTC | #8
2026. 06. 24. 11:26 keltezéssel, Laurent Pinchart írta:
> On Wed, Jun 24, 2026 at 10:17:40AM +0200, Barnabás Pőcze wrote:
>> 2026. 06. 24. 1:33 keltezéssel, Laurent Pinchart írta:
>>> On Mon, Jun 22, 2026 at 04:22:28PM +0200, Jacopo Mondi wrote:
>>>> On Mon, Jun 22, 2026 at 04:06:05PM +0200, Barnabás Pőcze wrote:
>>>>> 2026. 06. 22. 15:57 keltezéssel, Jacopo Mondi írta:
>>>>>> On Thu, Jun 18, 2026 at 02:38:43PM +0200, Barnabás Pőcze wrote:
>>>>>>> Clear the list of pending requests regardless whether the camera is running
>>>>>>> or not. The list should only contain entries when `!isRunning_`, in which
>>>>>>> case it was not cleared previously.
>>>>>>
>>>>>> Isn't this meant to mimic the behaviour of STREAMOFF ?
>>>>>>
>>>>>> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html
>>>>>> If VIDIOC_STREAMON fails then any already queued buffers will remain queued.
>>>>>
>>>>> But `VIDIOC_STREAMOFF` should always clear the pending buffers, no?
>>>>>
>>>>>     If buffers have been queued with ioctl VIDIOC_QBUF, VIDIOC_DQBUF and VIDIOC_STREAMOFF is
>>>>>     called without ever having called VIDIOC_STREAMON, then those queued buffers will also be
>>>>>     removed from the incoming queue and all are returned to the same state as after calling
>>>>>     ioctl VIDIOC_REQBUFS and can be restarted accordingly.
>>>>>
>>>>> And `V4L2CameraProxy::vidioc_streamoff()` seems to match this because it removes `V4L2_BUF_FLAG_QUEUED`
>>>>> from all buffers. But that means the pending list must be cleared as well in my understanding.
>>>>
>>>> As  V4L2Camera::streamOn() consumes all pendingRequests_ then if the
>>>> list is still populate at  V4L2Camera::streamOff() does it mean we
>>>> never got through a STREAMON ?
>>>>
>>>> If that's the case then yes, we should return all pending request.
>>>
>>> In V4L2, if buffers have been queued, and STREAMON hasn't been called,
>>> STREAMOFF will dequeue all the queued buffers.
>>>
>>> V4L2CameraProxy::vidioc_qbuf() sets the V4L2_BUF_FLAG_QUEUED flag on the
>>> buffer being queued, and V4L2CameraProxy::vidioc_streamoff() clears that
>>> flag from all buffers, regardless of if the camera is running.
>>> V4L2CameraProxy implements the V4L2 behaviour properly.
>>>
>>> In V4L2Camera, when not running, qbuf() takes a request from the pool,
>>> adds the buffer to the request, and pushes the request to the
>>> pendingRequests_ list. streamOff() should undo that. It already calls
>>> Request::reuse() on all requests to remove the associated buffers, and
>>> should clear the pendingRequests_ list as done in this patch.
>>>
>>> The change seems good to me, but the commit message should be expanded
>>> to explain the rationale better. With that,
>>>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>
>>> Also, the v4l2-compat unit test should catch this
>>> issue. v4l2-compliance already tests an ioctl sequence that should
>>> trigger the issue, in testMmap():
>>>
>>> 	// Test queuing buffers...
>>> 	for (unsigned i = 0; i < q.g_buffers(); i++) {
>>> 		buffer buf(q);
>>>
>>> 		fail_on_test(buf.querybuf(node, i));
>>> 		fail_on_test(buf.qbuf(node));
>>> 		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
>>> 		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_REQUEST_FD);
>>> 		fail_on_test(buf.g_request_fd());
>>> 	}
>>> 	// calling STREAMOFF...
>>> 	fail_on_test(node->streamoff(q.g_type()));
>>> 	// and now we should be able to queue those buffers again since
>>> 	// STREAMOFF should return them back to the dequeued state.
>>> 	for (unsigned i = 0; i < q.g_buffers(); i++) {
>>> 		buffer buf(q);
>>>
>>> 		fail_on_test(buf.querybuf(node, i));
>>> 		fail_on_test(buf.qbuf(node));
>>> 		fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
>>> 	}
>>>
>>> As far as I understand, this should result in the request being added
>>> twice to pendingRequests_, which should then cause
>>> V4L2Camera::streamOn() to return an error.
>>>
>>> Barnabás, could you check this ?
>>
>> As far as I can tell, the above test won't trigger any issues because it
>> does not start the camera afterwards but requests buffers again, which
>> calls `V4L2Camera::freeBuffers()`, which will clear `V4L2Camera::pendingRequests_`.
>>
>> It would have to do STREAMON after the above loop. I haven't checked
>> if there is a test that does that.
> 
> A right, that's probably what's happening.
> 
> Upstreaming a new test for this to v4l2-compliance is not
> straightforward as the code is very convoluted (at least in my opinion),
> but hacking it to test this scenario should be quick. Would you have
> time to give it a go, to verify that it triggers as expected without
> this patch, and that this patch fixes the issue ?

After this change:

diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index 7e08668b..815b41d3 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -1612,6 +1612,8 @@ int testMmap(struct node *node, struct node *node_m2m_cap, unsigned frame_count,
                 }
                 // calling STREAMOFF...
                 fail_on_test(node->streamoff(q.g_type()));
+               fail_on_test(node->streamon(q.g_type()));
+               fail_on_test(node->streamoff(q.g_type()));
                 // and now we should be able to queue those buffers again since
                 // STREAMOFF should return them back to the dequeued state.
                 for (unsigned i = 0; i < q.g_buffers(); i++) {

Things fail:

Streaming ioctls:
	test read/write: OK (Not Supported)
[18:56:37.944208500] [380834]  INFO Camera camera.cpp:1295 configuring streams: (0) 1280x720-MJPEG/Rec709/Rec709/Rec601/Limited
[18:56:37.973089374] [380834]  WARN V4L2Compat v4l2_camera_proxy.cpp:216 Camera does not support FrameDurationLimits
[18:56:38.022487040] [380834] ERROR Camera camera.cpp:1446 Request contains no buffers
		fail: v4l2-test-buffers.cpp(1615): node->streamon(q.g_type())
	test MMAP (no poll, REQBUFS): FAIL
		fail: v4l2-test-buffers.cpp(1577): can_stream
	test MMAP (select, REQBUFS): FAIL
		fail: v4l2-test-buffers.cpp(1577): can_stream
	test MMAP (epoll, REQBUFS): FAIL
		fail: v4l2-test-buffers.cpp(1577): can_stream
	test MMAP (no poll, CREATE_BUFS): FAIL
		fail: v4l2-test-buffers.cpp(1577): can_stream
	test MMAP (select, CREATE_BUFS): FAIL
		fail: v4l2-test-buffers.cpp(1577): can_stream
	test MMAP (epoll, CREATE_BUFS): FAIL
	test USERPTR (no poll): OK (Not Supported)
	test USERPTR (select): OK (Not Supported)
	test DMABUF (no poll): OK (Not Supported)
	test DMABUF (select): OK (Not Supported)

Now I have inadvertently also discovered another failure case with the following change:

diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp
index 7e08668b..2206024c 100644
--- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
+++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
@@ -1621,6 +1621,8 @@ int testMmap(struct node *node, struct node *node_m2m_cap, unsigned frame_count,
                         fail_on_test(buf.qbuf(node));
                         fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
                 }
+               fail_on_test(node->streamon(q.g_type()));
+               fail_on_test(node->streamoff(q.g_type()));
                 // Now request buffers again, freeing the old buffers.
                 // Good check for whether all the internal vb2 calls are in
                 // balance.

This change seems to fix the first case.


> 
>>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>>
>>>> Let's maybe ask Paul as well as the author of this part of the
>>>> codebase.
>>>>
>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>>>>> ---
>>>>>>>     src/v4l2/v4l2_camera.cpp | 4 ++--
>>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
>>>>>>> index e72b73a22a..8c7de8e92b 100644
>>>>>>> --- a/src/v4l2/v4l2_camera.cpp
>>>>>>> +++ b/src/v4l2/v4l2_camera.cpp
>>>>>>> @@ -226,6 +226,8 @@ int V4L2Camera::streamOn()
>>>>>>>
>>>>>>>     int V4L2Camera::streamOff()
>>>>>>>     {
>>>>>>> +	pendingRequests_.clear();
>>>>>>> +
>>>>>>>     	if (!isRunning_) {
>>>>>>>     		for (std::unique_ptr<Request> &req : requestPool_)
>>>>>>>     			req->reuse();
>>>>>>> @@ -233,8 +235,6 @@ int V4L2Camera::streamOff()
>>>>>>>     		return 0;
>>>>>>>     	}
>>>>>>>
>>>>>>> -	pendingRequests_.clear();
>>>>>>> -
>>>>>>>     	int ret = camera_->stop();
>>>>>>>     	if (ret < 0)
>>>>>>>     		return ret == -EACCES ? -EBUSY : ret;
>
Barnabás Pőcze June 24, 2026, 12:09 p.m. UTC | #9
2026. 06. 24. 14:07 keltezéssel, Barnabás Pőcze írta:
> 2026. 06. 24. 11:26 keltezéssel, Laurent Pinchart írta:
>> On Wed, Jun 24, 2026 at 10:17:40AM +0200, Barnabás Pőcze wrote:
>>> 2026. 06. 24. 1:33 keltezéssel, Laurent Pinchart írta:
>>>> On Mon, Jun 22, 2026 at 04:22:28PM +0200, Jacopo Mondi wrote:
>>>>> On Mon, Jun 22, 2026 at 04:06:05PM +0200, Barnabás Pőcze wrote:
>>>>>> 2026. 06. 22. 15:57 keltezéssel, Jacopo Mondi írta:
>>>>>>> On Thu, Jun 18, 2026 at 02:38:43PM +0200, Barnabás Pőcze wrote:
>>>>>>>> Clear the list of pending requests regardless whether the camera is running
>>>>>>>> or not. The list should only contain entries when `!isRunning_`, in which
>>>>>>>> case it was not cleared previously.
>>>>>>>
>>>>>>> Isn't this meant to mimic the behaviour of STREAMOFF ?
>>>>>>>
>>>>>>> https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/vidioc-streamon.html
>>>>>>> If VIDIOC_STREAMON fails then any already queued buffers will remain queued.
>>>>>>
>>>>>> But `VIDIOC_STREAMOFF` should always clear the pending buffers, no?
>>>>>>
>>>>>>     If buffers have been queued with ioctl VIDIOC_QBUF, VIDIOC_DQBUF and VIDIOC_STREAMOFF is
>>>>>>     called without ever having called VIDIOC_STREAMON, then those queued buffers will also be
>>>>>>     removed from the incoming queue and all are returned to the same state as after calling
>>>>>>     ioctl VIDIOC_REQBUFS and can be restarted accordingly.
>>>>>>
>>>>>> And `V4L2CameraProxy::vidioc_streamoff()` seems to match this because it removes `V4L2_BUF_FLAG_QUEUED`
>>>>>> from all buffers. But that means the pending list must be cleared as well in my understanding.
>>>>>
>>>>> As  V4L2Camera::streamOn() consumes all pendingRequests_ then if the
>>>>> list is still populate at  V4L2Camera::streamOff() does it mean we
>>>>> never got through a STREAMON ?
>>>>>
>>>>> If that's the case then yes, we should return all pending request.
>>>>
>>>> In V4L2, if buffers have been queued, and STREAMON hasn't been called,
>>>> STREAMOFF will dequeue all the queued buffers.
>>>>
>>>> V4L2CameraProxy::vidioc_qbuf() sets the V4L2_BUF_FLAG_QUEUED flag on the
>>>> buffer being queued, and V4L2CameraProxy::vidioc_streamoff() clears that
>>>> flag from all buffers, regardless of if the camera is running.
>>>> V4L2CameraProxy implements the V4L2 behaviour properly.
>>>>
>>>> In V4L2Camera, when not running, qbuf() takes a request from the pool,
>>>> adds the buffer to the request, and pushes the request to the
>>>> pendingRequests_ list. streamOff() should undo that. It already calls
>>>> Request::reuse() on all requests to remove the associated buffers, and
>>>> should clear the pendingRequests_ list as done in this patch.
>>>>
>>>> The change seems good to me, but the commit message should be expanded
>>>> to explain the rationale better. With that,
>>>>
>>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>
>>>> Also, the v4l2-compat unit test should catch this
>>>> issue. v4l2-compliance already tests an ioctl sequence that should
>>>> trigger the issue, in testMmap():
>>>>
>>>>     // Test queuing buffers...
>>>>     for (unsigned i = 0; i < q.g_buffers(); i++) {
>>>>         buffer buf(q);
>>>>
>>>>         fail_on_test(buf.querybuf(node, i));
>>>>         fail_on_test(buf.qbuf(node));
>>>>         fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
>>>>         fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_REQUEST_FD);
>>>>         fail_on_test(buf.g_request_fd());
>>>>     }
>>>>     // calling STREAMOFF...
>>>>     fail_on_test(node->streamoff(q.g_type()));
>>>>     // and now we should be able to queue those buffers again since
>>>>     // STREAMOFF should return them back to the dequeued state.
>>>>     for (unsigned i = 0; i < q.g_buffers(); i++) {
>>>>         buffer buf(q);
>>>>
>>>>         fail_on_test(buf.querybuf(node, i));
>>>>         fail_on_test(buf.qbuf(node));
>>>>         fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
>>>>     }
>>>>
>>>> As far as I understand, this should result in the request being added
>>>> twice to pendingRequests_, which should then cause
>>>> V4L2Camera::streamOn() to return an error.
>>>>
>>>> Barnabás, could you check this ?
>>>
>>> As far as I can tell, the above test won't trigger any issues because it
>>> does not start the camera afterwards but requests buffers again, which
>>> calls `V4L2Camera::freeBuffers()`, which will clear `V4L2Camera::pendingRequests_`.
>>>
>>> It would have to do STREAMON after the above loop. I haven't checked
>>> if there is a test that does that.
>>
>> A right, that's probably what's happening.
>>
>> Upstreaming a new test for this to v4l2-compliance is not
>> straightforward as the code is very convoluted (at least in my opinion),
>> but hacking it to test this scenario should be quick. Would you have
>> time to give it a go, to verify that it triggers as expected without
>> this patch, and that this patch fixes the issue ?
> 
> After this change:
> 
> diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> index 7e08668b..815b41d3 100644
> --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> @@ -1612,6 +1612,8 @@ int testMmap(struct node *node, struct node *node_m2m_cap, unsigned frame_count,
>                  }
>                  // calling STREAMOFF...
>                  fail_on_test(node->streamoff(q.g_type()));
> +               fail_on_test(node->streamon(q.g_type()));
> +               fail_on_test(node->streamoff(q.g_type()));
>                  // and now we should be able to queue those buffers again since
>                  // STREAMOFF should return them back to the dequeued state.
>                  for (unsigned i = 0; i < q.g_buffers(); i++) {
> 
> Things fail:
> 
> Streaming ioctls:
>      test read/write: OK (Not Supported)
> [18:56:37.944208500] [380834]  INFO Camera camera.cpp:1295 configuring streams: (0) 1280x720-MJPEG/Rec709/Rec709/Rec601/Limited
> [18:56:37.973089374] [380834]  WARN V4L2Compat v4l2_camera_proxy.cpp:216 Camera does not support FrameDurationLimits
> [18:56:38.022487040] [380834] ERROR Camera camera.cpp:1446 Request contains no buffers
>          fail: v4l2-test-buffers.cpp(1615): node->streamon(q.g_type())
>      test MMAP (no poll, REQBUFS): FAIL
>          fail: v4l2-test-buffers.cpp(1577): can_stream
>      test MMAP (select, REQBUFS): FAIL
>          fail: v4l2-test-buffers.cpp(1577): can_stream
>      test MMAP (epoll, REQBUFS): FAIL
>          fail: v4l2-test-buffers.cpp(1577): can_stream
>      test MMAP (no poll, CREATE_BUFS): FAIL
>          fail: v4l2-test-buffers.cpp(1577): can_stream
>      test MMAP (select, CREATE_BUFS): FAIL
>          fail: v4l2-test-buffers.cpp(1577): can_stream
>      test MMAP (epoll, CREATE_BUFS): FAIL
>      test USERPTR (no poll): OK (Not Supported)
>      test USERPTR (select): OK (Not Supported)
>      test DMABUF (no poll): OK (Not Supported)
>      test DMABUF (select): OK (Not Supported)
> 
> Now I have inadvertently also discovered another failure case with the following change:
> 
> diff --git a/utils/v4l2-compliance/v4l2-test-buffers.cpp b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> index 7e08668b..2206024c 100644
> --- a/utils/v4l2-compliance/v4l2-test-buffers.cpp
> +++ b/utils/v4l2-compliance/v4l2-test-buffers.cpp
> @@ -1621,6 +1621,8 @@ int testMmap(struct node *node, struct node *node_m2m_cap, unsigned frame_count,
>                          fail_on_test(buf.qbuf(node));
>                          fail_on_test(buf.g_flags() & V4L2_BUF_FLAG_DONE);
>                  }
> +               fail_on_test(node->streamon(q.g_type()));
> +               fail_on_test(node->streamoff(q.g_type()));
>                  // Now request buffers again, freeing the old buffers.
>                  // Good check for whether all the internal vb2 calls are in
>                  // balance.
> 
> This change seems to fix the first case.

Maybe I spoke too soon, it seems to fix both.


> 
> 
>>
>>>>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>>>>
>>>>> Let's maybe ask Paul as well as the author of this part of the
>>>>> codebase.
>>>>>
>>>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>>>>>>> ---
>>>>>>>>     src/v4l2/v4l2_camera.cpp | 4 ++--
>>>>>>>>     1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
>>>>>>>> index e72b73a22a..8c7de8e92b 100644
>>>>>>>> --- a/src/v4l2/v4l2_camera.cpp
>>>>>>>> +++ b/src/v4l2/v4l2_camera.cpp
>>>>>>>> @@ -226,6 +226,8 @@ int V4L2Camera::streamOn()
>>>>>>>>
>>>>>>>>     int V4L2Camera::streamOff()
>>>>>>>>     {
>>>>>>>> +    pendingRequests_.clear();
>>>>>>>> +
>>>>>>>>         if (!isRunning_) {
>>>>>>>>             for (std::unique_ptr<Request> &req : requestPool_)
>>>>>>>>                 req->reuse();
>>>>>>>> @@ -233,8 +235,6 @@ int V4L2Camera::streamOff()
>>>>>>>>             return 0;
>>>>>>>>         }
>>>>>>>>
>>>>>>>> -    pendingRequests_.clear();
>>>>>>>> -
>>>>>>>>         int ret = camera_->stop();
>>>>>>>>         if (ret < 0)
>>>>>>>>             return ret == -EACCES ? -EBUSY : ret;
>>
>

Patch
diff mbox series

diff --git a/src/v4l2/v4l2_camera.cpp b/src/v4l2/v4l2_camera.cpp
index e72b73a22a..8c7de8e92b 100644
--- a/src/v4l2/v4l2_camera.cpp
+++ b/src/v4l2/v4l2_camera.cpp
@@ -226,6 +226,8 @@  int V4L2Camera::streamOn()
 
 int V4L2Camera::streamOff()
 {
+	pendingRequests_.clear();
+
 	if (!isRunning_) {
 		for (std::unique_ptr<Request> &req : requestPool_)
 			req->reuse();
@@ -233,8 +235,6 @@  int V4L2Camera::streamOff()
 		return 0;
 	}
 
-	pendingRequests_.clear();
-
 	int ret = camera_->stop();
 	if (ret < 0)
 		return ret == -EACCES ? -EBUSY : ret;