[libcamera-devel,v2,15/17] v4l2: v4l2_camera_proxy: Don't allow streamon if no buffers have been requested

4095 diff mbox series

Make VIDIOC_STREAMON return -EINVAL if no buffers have been allocated
with reqbufs.

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

---
New in v2
- split from "Fix v4l2-compliance streaming tests"
---
 src/v4l2/v4l2_camera_proxy.cpp | 3 +++
 1 file changed, 3 insertions(+)

Hi Paul,

Thank you for the patch.

On Fri, Jun 19, 2020 at 02:41:21PM +0900, Paul Elder wrote:
> Make VIDIOC_STREAMON return -EINVAL if no buffers have been allocated
> with reqbufs.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> New in v2
> - split from "Fix v4l2-compliance streaming tests"
> ---
>  src/v4l2/v4l2_camera_proxy.cpp | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> index 8396f58..a7f987a 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -637,6 +637,9 @@ int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *cf, int *arg)
>  {
>  	LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << cf->efd();
>  
> +	if (bufferCount_ == 0)
> +		return -EINVAL;
> +

This should also be after the lock(), and I would move it after the
isRunning() test.

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

>  	if (vcam_->isRunning())
>  		return 0;
>

Hi Laurent,

Thank you for the review.

On Sat, Jun 20, 2020 at 05:26:40AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Fri, Jun 19, 2020 at 02:41:21PM +0900, Paul Elder wrote:
> > Make VIDIOC_STREAMON return -EINVAL if no buffers have been allocated
> > with reqbufs.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > New in v2
> > - split from "Fix v4l2-compliance streaming tests"
> > ---
> >  src/v4l2/v4l2_camera_proxy.cpp | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/src/v4l2/v4l2_camera_proxy.cpp b/src/v4l2/v4l2_camera_proxy.cpp
> > index 8396f58..a7f987a 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -637,6 +637,9 @@ int V4L2CameraProxy::vidioc_streamon(V4L2CameraFile *cf, int *arg)
> >  {
> >  	LOG(V4L2Compat, Debug) << "Servicing vidioc_streamon fd = " << cf->efd();
> >  
> > +	if (bufferCount_ == 0)
> > +		return -EINVAL;
> > +
> 
> This should also be after the lock(), and I would move it after the
> isRunning() test.

v4l2-compliance fails if it's before lock() (now hasOwnership()).

It does reqbufs 0 -> expect streamon EINVAL

So if it does hasOwnership() first, then it'll return EBUSY instead and
fail.

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  	if (vcam_->isRunning())
> >  		return 0;
> >  


Thanks,

Paul