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

[Laurent Pinchart]

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.

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.

> > > 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