[libcamera-devel,v5,0/7] Raspberry Pi AGC digital gain fixes
mbox series

Message ID 20221031114522.14215-1-naush@raspberrypi.com
Headers show
Series
  • Raspberry Pi AGC digital gain fixes
Related show

Message

Naushir Patuck Oct. 31, 2022, 11:45 a.m. UTC
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(-)

Comments

Kieran Bingham Nov. 8, 2022, 5:22 p.m. UTC | #1
Quoting Laurent Pinchart via libcamera-devel (2022-11-08 16:55:48)
> Hi Naush,
> 
> On Tue, Nov 08, 2022 at 09:39:05AM +0000, Naushir Patuck via libcamera-devel wrote:
> > Hi all,
> > 
> > Any chance we could progress this one please.  Patches 3/4/6 need a second
> > review.
> 
> I'm catching up on patch review after my vacation, this is on my list.
> The list is long though :-S
> 
> > PS: patchwork.libcamera.org seems to be a bit unhappy for a few days now.
> > Might need a reboot!
> > 

Argh ... rebooting!
--
Kieran


> > 
> > On Mon, 31 Oct 2022 at 11:45, Naushir Patuck <naush@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
Naushir Patuck Nov. 14, 2022, 11:31 a.m. UTC | #2
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.

Regards,
Naush


On Tue, 8 Nov 2022 at 09:39, Naushir Patuck <naush@raspberrypi.com> 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@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(-)
>>
>> --
>> 2.25.1
>>
>>
Laurent Pinchart Nov. 14, 2022, 2:01 p.m. UTC | #3
Hi Naush,

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.

> 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@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(-)
Naushir Patuck Nov. 14, 2022, 2:17 p.m. UTC | #4
Hi Laurent,


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

> Hi Naush,
>
> 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.

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@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
>
Laurent Pinchart Nov. 14, 2022, 2:45 p.m. UTC | #5
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@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(-)
Naushir Patuck Nov. 14, 2022, 3:05 p.m. UTC | #6
Hi Laurent,

On Mon, 14 Nov 2022 at 14:45, Laurent Pinchart <
laurent.pinchart@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@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
>