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