[libcamera-devel] [PATCH] pipeline: rpi: always initialize the embedded buffer in tryRunPipeline
Naushir Patuck
naush at raspberrypi.com
Wed Jan 24 10:32:42 CET 2024
On Wed, 24 Jan 2024 at 09:17, Naushir Patuck <naush at raspberrypi.com> wrote:
>
>
> 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.
>
And that points to the commit:
commit d17de86904f03f1d5a4d5d20af518e70c4758969
Author: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Date: Thu Jan 4 17:15:48 2024 +0200
utils: ipc: Update mojo
Update mojo from commit
9be4263648d7d1a04bb78be75df53f56449a5e3a "Updating trunk VERSION from
6225.0 to 6226.0"
from the Chromium repository.
The update-mojo.sh script was used for this update.
Bug: https://bugs.libcamera.org/show_bug.cgi?id=206
Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> 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/f15d0089/attachment.htm>
More information about the libcamera-devel
mailing list