<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 14 Nov 2022 at 14:45, Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com">laurent.pinchart@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush,<br>
<br>
On Mon, Nov 14, 2022 at 02:17:04PM +0000, Naushir Patuck wrote:<br>
> On Mon, 14 Nov 2022 at 14:01, Laurent Pinchart wrote:<br>
> > On Mon, Nov 14, 2022 at 11:31:57AM +0000, Naushir Patuck via<br>
> > libcamera-devel wrote:<br>
> > > Hi,<br>
> > ><br>
> > > All patches now have their review tags in this series. Unless there are no<br>
> > > other objections, it would be great if this could be merged.<br>
> ><br>
> > I still very much dislike the cookie in the delayed controls class. All<br>
> > other platforms are moving or will move to a frame context queue in the<br>
> > IPA module.<br>
> ><br>
> > If you want to go this way, I'll likely fork the DelayedControls class,<br>
> > and move your implementation to the RPi pipeline handler. This will<br>
> > likely mean a long term divergence in behaviour between Raspberry Pi and<br>
> > other platforms, which I don't think is a good idea.<br>
> <br>
> I don't think we are ready to make the jump to frame context queues just yet,<br>
> there is just way too much code to refactor.<br>
> <br>
> I'll rework this series to fork DelayedControls in RPi namespace with the cookie<br>
> change, and we can consider what to do longer-term.<br>
<br>
No no, I'm sorry if I didn't express my concern correctly, you don't<br>
have to fork it. I wanted to inform you I may fork it on top.<br></blockquote><div><br></div><div>I'm happy to do the fork as part of this series - it's not much extra work<br>really, and will keep the cookie API change out of the core implementation.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
What concerns me more than code forks is divergences in behaviour. This<br>
of course only makes sense once the PFC behaviour will be well-defined,<br>
and I don't want to block this fix until PFC is complete. I however<br>
don't really see why your digital gain needs can't be handled with a<br>
queue of frame contexts on the IPA side.<br></blockquote><div> </div><div>I did originally look at the FC queue behavior to see if it could be used to<br>solve our digital gain bug, but could not directly use it. This fix relies on<br>providing the AGC with the state when it sent the shutter/gain changes, i.e.<br>accounting for the maximum sensor delay + any frame dropping handling. I didn't<br>see any handling of this type in the FC queue - of course, none of the users<br>of the class actually needed this functionality so no surprise. Hope that makes<br></div><div>sense?</div><div><br></div><div>Regards,</div><div>Naush</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> > > On Tue, 8 Nov 2022 at 09:39, Naushir Patuck wrote:<br>
> > ><br>
> > > > Hi all,<br>
> > > ><br>
> > > > Any chance we could progress this one please. Patches 3/4/6 need a second<br>
> > > > review.<br>
> > > ><br>
> > > > Many thanks,<br>
> > > > Naush<br>
> > > ><br>
> > > > PS: <a href="http://patchwork.libcamera.org" rel="noreferrer" target="_blank">patchwork.libcamera.org</a> seems to be a bit unhappy for a few days<br>
> > now.<br>
> > > > Might need a reboot!<br>
> > > ><br>
> > > ><br>
> > > > On Mon, 31 Oct 2022 at 11:45, Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > > wrote:<br>
> > > ><br>
> > > >> Hi,<br>
> > > >><br>
> > > >> In version 7:<br>
> > > >><br>
> > > >> - For patch 2/7, the cookie must be provided in push() and reset().<br>
> > Updated the<br>
> > > >> rkisp1 and ipu3 pipeline handlers to provide frame numbers for the<br>
> > cookie value.<br>
> > > >> - For patch 3/4, add a test for skipped/dropped frames and cookie<br>
> > handling.<br>
> > > >> - Updated patch 6/7 to use the request sequence number for the<br>
> > context index<br>
> > > >> instead of using a separte sequence counter.<br>
> > > >><br>
> > > >> Thanks,<br>
> > > >> Naush<br>
> > > >><br>
> > > >> Naushir Patuck (7):<br>
> > > >> delayed_controls: Template the ControlRingBuffer class<br>
> > > >> delayed_controls: Add user cookie to DelayedControls<br>
> > > >> tests: delayed_controls: Add cookie tests<br>
> > > >> ipa: raspberrypi: Add RPiController::Metadata::mergeCopy<br>
> > > >> ipa: raspberrypi: Use an array of RPiController::Metadata objects<br>
> > > >> pipeline: ipa: raspberrypi: Use IPA cookies<br>
> > > >> ipa: raspberrypi: agc: Fix digital gain calculation for manual mode<br>
> > > >><br>
> > > >> include/libcamera/internal/delayed_controls.h | 21 +--<br>
> > > >> include/libcamera/ipa/raspberrypi.mojom | 6 +-<br>
> > > >> src/ipa/raspberrypi/controller/metadata.h | 10 ++<br>
> > > >> src/ipa/raspberrypi/controller/rpi/agc.cpp | 10 +-<br>
> > > >> src/ipa/raspberrypi/raspberrypi.cpp | 104 +++++++++------<br>
> > > >> src/libcamera/delayed_controls.cpp | 22 ++--<br>
> > > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +-<br>
> > > >> .../pipeline/raspberrypi/raspberrypi.cpp | 18 ++-<br>
> > > >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +-<br>
> > > >> test/delayed_controls.cpp | 121<br>
> > ++++++++++++++++--<br>
> > > >> 10 files changed, 240 insertions(+), 88 deletions(-)<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>