[libcamera-devel] [PATCH v5 0/7] Raspberry Pi AGC digital gain fixes

[Naushir Patuck]

Hi Laurent,

On Mon, 14 Nov 2022 at 14:45, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:

> Hi Naush,
>
> On Mon, Nov 14, 2022 at 02:17:04PM +0000, Naushir Patuck wrote:
> > On Mon, 14 Nov 2022 at 14:01, Laurent Pinchart wrote:
> > > On Mon, Nov 14, 2022 at 11:31:57AM +0000, Naushir Patuck via
> > > libcamera-devel wrote:
> > > > Hi,
> > > >
> > > > All patches now have their review tags in this series.  Unless there
> are no
> > > > other objections, it would be great if this could be merged.
> > >
> > > I still very much dislike the cookie in the delayed controls class. All
> > > other platforms are moving or will move to a frame context queue in the
> > > IPA module.
> > >
> > > If you want to go this way, I'll likely fork the DelayedControls class,
> > > and move your implementation to the RPi pipeline handler. This will
> > > likely mean a long term divergence in behaviour between Raspberry Pi
> and
> > > other platforms, which I don't think is a good idea.
> >
> > I don't think we are ready to make the jump to frame context queues just
> yet,
> > there is just way too much code to refactor.
> >
> > I'll rework this series to fork DelayedControls in RPi namespace with
> the cookie
> > change, and we can consider what to do longer-term.
>
> No no, I'm sorry if I didn't express my concern correctly, you don't
> have to fork it. I wanted to inform you I may fork it on top.
>

I'm happy to do the fork as part of this series - it's not much extra work
really, and will keep the cookie API change out of the core implementation.


> What concerns me more than code forks is divergences in behaviour. This
> of course only makes sense once the PFC behaviour will be well-defined,
> and I don't want to block this fix until PFC is complete. I however
> don't really see why your digital gain needs can't be handled with a
> queue of frame contexts on the IPA side.
>

I did originally look at the FC queue behavior to see if it could be used to
solve our digital gain bug, but could not directly use it.  This fix relies
on
providing the AGC with the state when it sent the shutter/gain changes, i.e.
accounting for the maximum sensor delay + any frame dropping handling.  I
didn't
see any handling of this type in the FC queue - of course, none of the users
of the class actually needed this functionality so no surprise. Hope that
makes
sense?

Regards,
Naush


>
> > > > On Tue, 8 Nov 2022 at 09:39, Naushir Patuck wrote:
> > > >
> > > > > Hi all,
> > > > >
> > > > > Any chance we could progress this one please.  Patches 3/4/6 need
> a second
> > > > > review.
> > > > >
> > > > > Many thanks,
> > > > > Naush
> > > > >
> > > > > PS: patchwork.libcamera.org seems to be a bit unhappy for a few
> days
> > > now.
> > > > > Might need a reboot!
> > > > >
> > > > >
> > > > > On Mon, 31 Oct 2022 at 11:45, Naushir Patuck <
> naush at raspberrypi.com>
> > > > > wrote:
> > > > >
> > > > >> Hi,
> > > > >>
> > > > >> In version 7:
> > > > >>
> > > > >> - For patch 2/7, the cookie must be provided in push() and
> reset().
> > > Updated the
> > > > >> rkisp1 and ipu3 pipeline handlers to provide frame numbers for the
> > > cookie value.
> > > > >> - For patch 3/4, add a test for skipped/dropped frames and cookie
> > > handling.
> > > > >> - Updated patch 6/7 to use the request sequence number for the
> > > context index
> > > > >> instead of using a separte sequence counter.
> > > > >>
> > > > >> Thanks,
> > > > >> Naush
> > > > >>
> > > > >> Naushir Patuck (7):
> > > > >>   delayed_controls: Template the ControlRingBuffer class
> > > > >>   delayed_controls: Add user cookie to DelayedControls
> > > > >>   tests: delayed_controls: Add cookie tests
> > > > >>   ipa: raspberrypi: Add RPiController::Metadata::mergeCopy
> > > > >>   ipa: raspberrypi: Use an array of RPiController::Metadata
> objects
> > > > >>   pipeline: ipa: raspberrypi: Use IPA cookies
> > > > >>   ipa: raspberrypi: agc: Fix digital gain calculation for manual
> mode
> > > > >>
> > > > >>  include/libcamera/internal/delayed_controls.h |  21 +--
> > > > >>  include/libcamera/ipa/raspberrypi.mojom       |   6 +-
> > > > >>  src/ipa/raspberrypi/controller/metadata.h     |  10 ++
> > > > >>  src/ipa/raspberrypi/controller/rpi/agc.cpp    |  10 +-
> > > > >>  src/ipa/raspberrypi/raspberrypi.cpp           | 104
> +++++++++------
> > > > >>  src/libcamera/delayed_controls.cpp            |  22 ++--
> > > > >>  src/libcamera/pipeline/ipu3/ipu3.cpp          |   9 +-
> > > > >>  .../pipeline/raspberrypi/raspberrypi.cpp      |  18 ++-
> > > > >>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      |   7 +-
> > > > >>  test/delayed_controls.cpp                     | 121
> > > ++++++++++++++++--
> > > > >>  10 files changed, 240 insertions(+), 88 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221114/1d4c9d70/attachment.htm>