[libcamera-devel] [PATCH] pipeline: rpi: always initialize the embedded buffer in tryRunPipeline

[Naushir Patuck]

On Mon, 22 Jan 2024 at 10:31, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> On Mon, Jan 22, 2024 at 10:18:44AM +0000, Naushir Patuck wrote:
> > On Mon, 22 Jan 2024 at 07:37, Laurent Pinchart via libcamera-devel wrote:
> > > On Sun, Jan 21, 2024 at 08:03:49AM -0500, Elias Naur wrote:
> > > > On Sat, 20 Jan 2024 at 23:07, Laurent Pinchart wrote:
> > > > > On Sat, Jan 20, 2024 at 04:14:49PM -0500, Elias Naur via
> libcamera-devel wrote:
> > > > > > Vc4CameraData::findMatchBuffers may return successfully without
> setting
> > > > > > the embedded buffer. Make sure to initialize the buffer and id
> to avoid
> > > > > > accessing garbage data.
> > > > >
> > > > > How so ? The function starts with
> > > > >
> > > > >         if (bayerQueue_.empty())
> > > > >                 return false;
> > > > >
> > > > >         /*
> > > > >          * Find the embedded data buffer with a matching timestamp
> to pass to
> > > > >          * the IPA. Any embedded buffers with a timestamp lower
> than the
> > > > >          * current bayer buffer will be removed and re-queued to
> the driver.
> > > > >          */
> > > > >         uint64_t ts =
> bayerQueue_.front().buffer->metadata().timestamp;
> > > > >         embeddedBuffer = nullptr;
> > > > >
> > > > > so I don't see how it can leave embeddedBuffer unset if it returns
> > > > > successfully.
> > > > >
> > > > > > Without this change, libcamera v0.2.0 usually crashes with an
> assertion
> > > > > > error:
> > > > > >
> > > > > >  ipa_base.cpp:397 assertion "it != buffers_.end()" failed in
> prepareIsp()
> > > > >
> > > > > I've seen this indeed, and will test your patch. Maybe it's due to
> the
> > > > > second change, where you set params.buffers.embedded to 0 ?
> > > >
> > > > Correct: the fix is the second change;
> > >
> > > Could you please update the commit message to reflect this ? I read it
> > > as indicating that embeddedBuffer could be null, while the issue is
> that
> > > params.buffers.embedded is not initialized.
> > >
> > > > the first change is defensive in case
> > > > findMatchBuffers for some reason doesn't set embeddedBuffer in case
> none
> > > > is found.
> > >
> > > I'm not sure I'd keep that change. I'll let Naush and David decide what
> > > they like best.
> >
> > I agree, let's just keep the change to initialising
>  params.buffers.embedded.
> > With this, the change itself looks reasonable so
> >
> > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
>
> Elias, would you be able to send a v2 ?
>
> > However... I could have sworn the mojom auto generated class file for
> > ipa::RPi::PrepareParams would initialize all member variables to 0
> (including
> > buffers.embedded) on default construction.  I'm away right now so I
> cannot
> > check this myself, but could anybody else check?  I'm somewhat surprised
> that
> > this has not been reported more widely...
>
> I checked, and the constructor doesn't initialize all fields to 0. I'm
> pretty surprised too to be honest.
>

Curiosity got the better of me.  In the
https://git.libcamera.org/libcamera/libcamera.git/ build tree,
the following bit of code gets generated in raspberrpyi_ipa_interface.h:

---
struct BufferIds
{
public:
#ifndef __DOXYGEN__
    BufferIds()
    {
    }

    BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats)
        : bayer(_bayer), embedded(_embedded), stats(_stats)
    {
    }
#endif

    uint32_t bayer;
    uint32_t embedded;
    uint32_t stats;
};
---

However, building from https://github.com/raspberrypi/libcamera/ generates
the following:

---
struct BufferIds
{
public:
#ifndef __DOXYGEN__
    BufferIds()
        : bayer(0), embedded(0), stats(0)
    {
    }

    BufferIds(uint32_t _bayer, uint32_t _embedded, uint32_t _stats)
        : bayer(_bayer), embedded(_embedded), stats(_stats)
    {
    }
#endif

    uint32_t bayer;
    uint32_t embedded;
    uint32_t stats;
};
---

So at some point in time, the fields were explicitly initialised to 0 and
things were working as expected.  The RPi tree definitely does not have any
changes to the mojom generation code, so I can't say what caused this
change, but it should hopefully be easy enough to bisect and find the root
case.

Regards,
Naush




>
> > > > > > Signed-off-by: Elias Naur <mail at eliasnaur.com>
> > > > > > ---
> > > > > >  src/libcamera/pipeline/rpi/vc4/vc4.cpp | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > > > > index 26102ea7..d76389f3 100644
> > > > > > --- a/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > > > > +++ b/src/libcamera/pipeline/rpi/vc4/vc4.cpp
> > > > > > @@ -906,7 +906,7 @@ void
> Vc4CameraData::setCameraTimeout(uint32_t maxFrameLengthMs)
> > > > > >
> > > > > >  void Vc4CameraData::tryRunPipeline()
> > > > > >  {
> > > > > > -     FrameBuffer *embeddedBuffer;
> > > > > > +     FrameBuffer *embeddedBuffer = nullptr;
> > > > > >       BayerFrame bayerFrame;
> > > > > >
> > > > > >       /* If any of our request or buffer queues are empty, we
> cannot proceed. */
> > > > > > @@ -945,6 +945,7 @@ void Vc4CameraData::tryRunPipeline()
> > > > > >       params.requestControls = request->controls();
> > > > > >       params.ipaContext = request->sequence();
> > > > > >       params.delayContext = bayerFrame.delayContext;
> > > > > > +     params.buffers.embedded = 0;
> > > > > >
> > > > > >       if (embeddedBuffer) {
> > > > > >               unsigned int embeddedId =
> unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20240124/d4b7e1ea/attachment.htm>