[1/1] libcamera: Put buffer back to V4L2BufferCache when VIDIOC_QBUF fails
diff mbox series

Message ID 20240912051409.3495486-2-chenghaoyang@google.com
State Accepted
Commit 835c1bf35ecf402b5fb13a8cd2ac56e9f22dc585
Headers show
Series
  • Recycle V4L2BufferCache when VIDIOC_QBUF fails
Related show

Commit Message

Cheng-Hao Yang Sept. 12, 2024, 5:09 a.m. UTC 
From: Han-Lin Chen <hanlinchen@chromium.org>

The patch puts buffer back to V4L2BufferCache when VIDIOC_QBUF fails
in V4L2VideoDevice. This is to avoid cache leaks and causing assert.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
---
 src/libcamera/v4l2_videodevice.cpp | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Kieran Bingham July 21, 2025, 7:13 p.m. UTC | #1 
Quoting Harvey Yang (2024-09-12 06:09:39)
> From: Han-Lin Chen <hanlinchen@chromium.org>
> 
> The patch puts buffer back to V4L2BufferCache when VIDIOC_QBUF fails
> in V4L2VideoDevice. This is to avoid cache leaks and causing assert.
> 

This seems reasonable - although could probably now be implemented in
less code with the new ScopeExitActions class which wasn't around when
this was posted I expect...

However - while we could do a version with ScopeExitActions - I think
this is still correct *AND* fixes the error paths in
V4L2VideoDevice::queueBuffer() which are clearly leaking buffer cache
entries.

So:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  src/libcamera/v4l2_videodevice.cpp | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index 76742e18..75a2cdb1 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -1626,11 +1626,15 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>          */
>         if (planes.size() < numV4l2Planes) {
>                 LOG(V4L2, Error) << "Frame buffer has too few planes";
> +               cache_->put(buf.index);
> +
>                 return -EINVAL;
>         }
>  
>         if (planes.size() != numV4l2Planes && !buffer->_d()->isContiguous()) {
>                 LOG(V4L2, Error) << "Device format requires contiguous buffer";
> +               cache_->put(buf.index);
> +
>                 return -EINVAL;
>         }
>  
> @@ -1673,6 +1677,8 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>                                 if (i != planes.size() - 1 && bytesused != length) {
>                                         LOG(V4L2, Error)
>                                                 << "Holes in multi-planar buffer not supported";
> +                                       cache_->put(buf.index);
> +
>                                         return -EINVAL;
>                                 }
>                         }
> @@ -1722,6 +1728,8 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
>                 LOG(V4L2, Error)
>                         << "Failed to queue buffer " << buf.index << ": "
>                         << strerror(-ret);
> +               cache_->put(buf.index);
> +
>                 return ret;
>         }
>  
> -- 
> 2.46.0.598.g6f2099f65c-goog
>
Kieran Bingham July 21, 2025, 7:29 p.m. UTC | #2 
Quoting Kieran Bingham (2025-07-21 20:13:40)
> Quoting Harvey Yang (2024-09-12 06:09:39)
> > From: Han-Lin Chen <hanlinchen@chromium.org>
> > 
> > The patch puts buffer back to V4L2BufferCache when VIDIOC_QBUF fails
> > in V4L2VideoDevice. This is to avoid cache leaks and causing assert.
> > 
> 
> This seems reasonable - although could probably now be implemented in
> less code with the new ScopeExitActions class which wasn't around when
> this was posted I expect...
> 
> However - while we could do a version with ScopeExitActions - I think
> this is still correct *AND* fixes the error paths in
> V4L2VideoDevice::queueBuffer() which are clearly leaking buffer cache
> entries.
> 
> So:
> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 

CI is green, so I'm very tempted to merge this already with the
following additional tag:


Fixes: cadae67e4578 ("libcamera: v4l2_videodevice: Add FrameBuffer interface")

Which is way back from 2019!



> 
> 
> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  src/libcamera/v4l2_videodevice.cpp | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> > index 76742e18..75a2cdb1 100644
> > --- a/src/libcamera/v4l2_videodevice.cpp
> > +++ b/src/libcamera/v4l2_videodevice.cpp
> > @@ -1626,11 +1626,15 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >          */
> >         if (planes.size() < numV4l2Planes) {
> >                 LOG(V4L2, Error) << "Frame buffer has too few planes";
> > +               cache_->put(buf.index);
> > +
> >                 return -EINVAL;
> >         }
> >  
> >         if (planes.size() != numV4l2Planes && !buffer->_d()->isContiguous()) {
> >                 LOG(V4L2, Error) << "Device format requires contiguous buffer";
> > +               cache_->put(buf.index);
> > +
> >                 return -EINVAL;
> >         }
> >  
> > @@ -1673,6 +1677,8 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >                                 if (i != planes.size() - 1 && bytesused != length) {
> >                                         LOG(V4L2, Error)
> >                                                 << "Holes in multi-planar buffer not supported";
> > +                                       cache_->put(buf.index);
> > +
> >                                         return -EINVAL;
> >                                 }
> >                         }
> > @@ -1722,6 +1728,8 @@ int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
> >                 LOG(V4L2, Error)
> >                         << "Failed to queue buffer " << buf.index << ": "
> >                         << strerror(-ret);
> > +               cache_->put(buf.index);
> > +
> >                 return ret;
> >         }
> >  
> > -- 
> > 2.46.0.598.g6f2099f65c-goog
> >

Patch
diff mbox series 

diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
index 76742e18..75a2cdb1 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -1626,11 +1626,15 @@  int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
 	 */
 	if (planes.size() < numV4l2Planes) {
 		LOG(V4L2, Error) << "Frame buffer has too few planes";
+		cache_->put(buf.index);
+
 		return -EINVAL;
 	}
 
 	if (planes.size() != numV4l2Planes && !buffer->_d()->isContiguous()) {
 		LOG(V4L2, Error) << "Device format requires contiguous buffer";
+		cache_->put(buf.index);
+
 		return -EINVAL;
 	}
 
@@ -1673,6 +1677,8 @@  int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
 				if (i != planes.size() - 1 && bytesused != length) {
 					LOG(V4L2, Error)
 						<< "Holes in multi-planar buffer not supported";
+					cache_->put(buf.index);
+
 					return -EINVAL;
 				}
 			}
@@ -1722,6 +1728,8 @@  int V4L2VideoDevice::queueBuffer(FrameBuffer *buffer)
 		LOG(V4L2, Error)
 			<< "Failed to queue buffer " << buf.index << ": "
 			<< strerror(-ret);
+		cache_->put(buf.index);
+
 		return ret;
 	}