| Message ID | 20190416134210.21097-4-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2019-04-16 15:42:06 +0200, Jacopo Mondi wrote: > Do not assume the freeBuffer() function can handle allocateBuffer() > method failures, as error handling and clean up should be performed > by allocateBuffer() method itself. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/camera.cpp | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 21caa24b90b5..2d0a80664214 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -650,7 +650,6 @@ int Camera::allocateBuffers() > int ret = pipe_->allocateBuffers(this, activeStreams_); > if (ret) { > LOG(Camera, Error) << "Failed to allocate buffers"; > - freeBuffers(); > return ret; > } > > -- > 2.21.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thank you for the patch. On Tue, Apr 16, 2019 at 03:42:06PM +0200, Jacopo Mondi wrote: > Do not assume the freeBuffer() function can handle allocateBuffer() > method failures, as error handling and clean up should be performed > by allocateBuffer() method itself. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I would however squash this with patch 2/7 as the freeBuffers() call is added there. > --- > src/libcamera/camera.cpp | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 21caa24b90b5..2d0a80664214 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -650,7 +650,6 @@ int Camera::allocateBuffers() > int ret = pipe_->allocateBuffers(this, activeStreams_); > if (ret) { > LOG(Camera, Error) << "Failed to allocate buffers"; > - freeBuffers(); > return ret; > } >
Hi Jacopo, On Wed, Apr 17, 2019 at 01:11:21AM +0300, Laurent Pinchart wrote: > On Tue, Apr 16, 2019 at 03:42:06PM +0200, Jacopo Mondi wrote: > > Do not assume the freeBuffer() function can handle allocateBuffer() > > method failures, as error handling and clean up should be performed > > by allocateBuffer() method itself. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I would however squash this with patch 2/7 as the freeBuffers() call is > added there. Actually, don't you also need to fix PipelineHandlerIPU3::allocateBuffers() to cleanup in case of error ? > > --- > > src/libcamera/camera.cpp | 1 - > > 1 file changed, 1 deletion(-) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 21caa24b90b5..2d0a80664214 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -650,7 +650,6 @@ int Camera::allocateBuffers() > > int ret = pipe_->allocateBuffers(this, activeStreams_); > > if (ret) { > > LOG(Camera, Error) << "Failed to allocate buffers"; > > - freeBuffers(); > > return ret; > > } > >
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 21caa24b90b5..2d0a80664214 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -650,7 +650,6 @@ int Camera::allocateBuffers() int ret = pipe_->allocateBuffers(this, activeStreams_); if (ret) { LOG(Camera, Error) << "Failed to allocate buffers"; - freeBuffers(); return ret; }
Do not assume the freeBuffer() function can handle allocateBuffer() method failures, as error handling and clean up should be performed by allocateBuffer() method itself. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/camera.cpp | 1 - 1 file changed, 1 deletion(-)