| Message ID | 20220520124919.2873559-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, On Fri, May 20, 2022 at 01:49:19PM +0100, Naushir Patuck via libcamera-devel wrote: > The freeBuffers() cleanup code calls into the IPA to unmap and free shared > buffers. However, this function could be called before the IPA has opened (via > registerCamera()), causing a segmentation fault. Fix this by guarding against > calling the IPA if it has not been opened. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 2636acb758b7..26cd4e5f2b99 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1484,10 +1484,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer > > void RPiCameraData::freeBuffers() > { > - /* Copy the buffer ids from the unordered_set to a vector to pass to the IPA. */ > - std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end()); > - ipa_->unmapBuffers(ipaBuffers); > - ipaBuffers_.clear(); > + if (ipa_) { > + /* Copy the buffer ids from the unordered_set to a vector to pass to the IPA. */ > + std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end()); > + ipa_->unmapBuffers(ipaBuffers); > + ipaBuffers_.clear(); > + } > > for (auto const stream : streams_) > stream->releaseBuffers(); > -- > 2.34.1 >
Hi Naush, Thank you for the patch. On Fri, May 20, 2022 at 01:49:19PM +0100, Naushir Patuck via libcamera-devel wrote: > The freeBuffers() cleanup code calls into the IPA to unmap and free shared > buffers. However, this function could be called before the IPA has opened (via > registerCamera()), causing a segmentation fault. Fix this by guarding against > calling the IPA if it has not been opened. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 2636acb758b7..26cd4e5f2b99 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -1484,10 +1484,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer > > void RPiCameraData::freeBuffers() > { > - /* Copy the buffer ids from the unordered_set to a vector to pass to the IPA. */ > - std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end()); > - ipa_->unmapBuffers(ipaBuffers); > - ipaBuffers_.clear(); > + if (ipa_) { > + /* Copy the buffer ids from the unordered_set to a vector to pass to the IPA. */ > + std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end()); I'll wrap this when applying. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + ipa_->unmapBuffers(ipaBuffers); > + ipaBuffers_.clear(); > + } > > for (auto const stream : streams_) > stream->releaseBuffers();
Quoting Laurent Pinchart via libcamera-devel (2022-05-22 18:34:15) > Hi Naush, > > Thank you for the patch. > > On Fri, May 20, 2022 at 01:49:19PM +0100, Naushir Patuck via libcamera-devel wrote: > > The freeBuffers() cleanup code calls into the IPA to unmap and free shared > > buffers. However, this function could be called before the IPA has opened (via > > registerCamera()), causing a segmentation fault. Fix this by guarding against > > calling the IPA if it has not been opened. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 2636acb758b7..26cd4e5f2b99 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -1484,10 +1484,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer > > > > void RPiCameraData::freeBuffers() > > { > > - /* Copy the buffer ids from the unordered_set to a vector to pass to the IPA. */ > > - std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end()); > > - ipa_->unmapBuffers(ipaBuffers); > > - ipaBuffers_.clear(); > > + if (ipa_) { > > + /* Copy the buffer ids from the unordered_set to a vector to pass to the IPA. */ > > + std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end()); > > I'll wrap this when applying. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Can this be handled more generically by the interface layer between the IPA? (Doesn't need to be done for this patch), but it seems like something that could occur in other places (/pipelines) too. -- Kieran > > > + ipa_->unmapBuffers(ipaBuffers); > > + ipaBuffers_.clear(); > > + } > > > > for (auto const stream : streams_) > > stream->releaseBuffers(); > > -- > Regards, > > Laurent Pinchart
Hi Kieran, Thank you for the review! On Mon, 23 May 2022 at 08:31, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Laurent Pinchart via libcamera-devel (2022-05-22 18:34:15) > > Hi Naush, > > > > Thank you for the patch. > > > > On Fri, May 20, 2022 at 01:49:19PM +0100, Naushir Patuck via > libcamera-devel wrote: > > > The freeBuffers() cleanup code calls into the IPA to unmap and free > shared > > > buffers. However, this function could be called before the IPA has > opened (via > > > registerCamera()), causing a segmentation fault. Fix this by guarding > against > > > calling the IPA if it has not been opened. > > > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > > --- > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 2636acb758b7..26cd4e5f2b99 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -1484,10 +1484,12 @@ void PipelineHandlerRPi::mapBuffers(Camera > *camera, const RPi::BufferMap &buffer > > > > > > void RPiCameraData::freeBuffers() > > > { > > > - /* Copy the buffer ids from the unordered_set to a vector to > pass to the IPA. */ > > > - std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(), > ipaBuffers_.end()); > > > - ipa_->unmapBuffers(ipaBuffers); > > > - ipaBuffers_.clear(); > > > + if (ipa_) { > > > + /* Copy the buffer ids from the unordered_set to a > vector to pass to the IPA. */ > > > + std::vector<unsigned int> > ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end()); > > > > I'll wrap this when applying. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Can this be handled more generically by the interface layer between the > IPA? (Doesn't need to be done for this patch), but it seems like > something that could occur in other places (/pipelines) too. > In this particular instance, I don't think so. The ipa_ variable itself is uninitialised at this point, so the IPA interface doesn't even exist yet :) I'm sure in other cases, errors like this could be handled in the interface layer. Regards, Naush > > -- > Kieran > > > > > > + ipa_->unmapBuffers(ipaBuffers); > > > + ipaBuffers_.clear(); > > > + } > > > > > > for (auto const stream : streams_) > > > stream->releaseBuffers(); > > > > -- > > Regards, > > > > Laurent Pinchart >
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 2636acb758b7..26cd4e5f2b99 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -1484,10 +1484,12 @@ void PipelineHandlerRPi::mapBuffers(Camera *camera, const RPi::BufferMap &buffer void RPiCameraData::freeBuffers() { - /* Copy the buffer ids from the unordered_set to a vector to pass to the IPA. */ - std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end()); - ipa_->unmapBuffers(ipaBuffers); - ipaBuffers_.clear(); + if (ipa_) { + /* Copy the buffer ids from the unordered_set to a vector to pass to the IPA. */ + std::vector<unsigned int> ipaBuffers(ipaBuffers_.begin(), ipaBuffers_.end()); + ipa_->unmapBuffers(ipaBuffers); + ipaBuffers_.clear(); + } for (auto const stream : streams_) stream->releaseBuffers();
The freeBuffers() cleanup code calls into the IPA to unmap and free shared buffers. However, this function could be called before the IPA has opened (via registerCamera()), causing a segmentation fault. Fix this by guarding against calling the IPA if it has not been opened. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)