| Message ID | 20221031114522.14215-1-naush@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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
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 >> >>
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(-)
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 >
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(-)
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 >