[libcamera-devel,v2,16/17] v4l2: v4l2_camera_proxy: Don't allow buffers to be freed if still mmaped

4096 diff mbox series

In VIDIOC_REQBUFS with count = 0, if the buffers are still mmaped, they
should not be allowed to be freed. Add a check for this.

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:22PM +0900, Paul Elder wrote:
> In VIDIOC_REQBUFS with count = 0, if the buffers are still mmaped, they
> should not be allowed to be freed. Add a check for this.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

Is that required by v4l2-compliance ? V4L2 supports buffer orphaning
since v5.0.

> ---
> 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 a7f987a..2eb2fcc 100644
> --- a/src/v4l2/v4l2_camera_proxy.cpp
> +++ b/src/v4l2/v4l2_camera_proxy.cpp
> @@ -467,6 +467,9 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *cf, struct v4l2_requestbuffe
>  	memset(arg->reserved, 0, sizeof(arg->reserved));
>  
>  	if (arg->count == 0) {
> +		if (!mmaps_.empty())
> +			return -EBUSY;
> +
>  		if (vcam_->isRunning()) {
>  			for (struct v4l2_buffer &buf : buffers_)
>  				buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);

Hi Laurent,

Thank you for the review.

On Sat, Jun 20, 2020 at 05:27:43AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Fri, Jun 19, 2020 at 02:41:22PM +0900, Paul Elder wrote:
> > In VIDIOC_REQBUFS with count = 0, if the buffers are still mmaped, they
> > should not be allowed to be freed. Add a check for this.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> Is that required by v4l2-compliance ? V4L2 supports buffer orphaning
> since v5.0.

It's not required, but it's warned.

I think I'll keep it.

> > ---
> > 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 a7f987a..2eb2fcc 100644
> > --- a/src/v4l2/v4l2_camera_proxy.cpp
> > +++ b/src/v4l2/v4l2_camera_proxy.cpp
> > @@ -467,6 +467,9 @@ int V4L2CameraProxy::vidioc_reqbufs(V4L2CameraFile *cf, struct v4l2_requestbuffe
> >  	memset(arg->reserved, 0, sizeof(arg->reserved));
> >  
> >  	if (arg->count == 0) {
> > +		if (!mmaps_.empty())
> > +			return -EBUSY;
> > +
> >  		if (vcam_->isRunning()) {
> >  			for (struct v4l2_buffer &buf : buffers_)
> >  				buf.flags &= ~(V4L2_BUF_FLAG_QUEUED | V4L2_BUF_FLAG_DONE);
> 


Thanks,

Paul