[PATCH 0/3] Reduce rkisp1 flicker on first start

[Mikhail Rudenko]

On 2024-10-21 at 18:44 +03, Laurent Pinchart <laurent.pinchart at ideasonboard.com> wrote:

> On Mon, Oct 21, 2024 at 04:40:30PM +0100, Kieran Bingham wrote:
>> Quoting Mikhail Rudenko (2024-10-21 16:08:56)
>> > On 2024-10-20 at 18:33 +01, Kieran Bingham wrote:
>> > > Quoting Mikhail Rudenko (2024-10-20 14:39:45)
>> > >> On 2024-10-18 at 23:26 +01, Kieran Bingham wrote:
>> > >> > Quoting Kieran Bingham (2024-10-17 14:35:40)
>> > >> >> Quoting Mikhail Rudenko (2024-10-17 13:47:04)
>> > >> >> > The chart from camshark (see attachment).
>> > >> >> >
>> > >> >>
>> > >> >> That looks like a good result indeed!
>> > >> >>
>> > >> >> I'll test out the patches.
>> > >> >
>> > >> > I've just tried this out - and it seems to work well on the IMX283 I
>> > >> > have - but caused quite noticible AnalogueGain oscillations on the
>> > >> > IMX335 ... I would 'bet' that's not the fault of this series - but the
>> > >> > fact that we don't have per-sensor delayed-controls set up correctly
>> > >> > which I already know about - but I haven't seen that get fixed up yet.
>> > >>
>> > >> I did a few more tests myself (I use OV4689 btw), and also observed
>> > >> analogue gain oscillations on darker scenes when AGC tries to steer the
>> > >> gain above 1.0. It looks like the hardcoded gain delay value of 1 is
>> > >> wrong at least for my sensor. When I increased it to 2, gain
>> > >> oscillations were gone. I have also tweaked a few things here and there
>> > >> and will send v2 soon.
>> > >>
>> > >> > I think your series probably highlights the fault that already exists...
>> > >>
>> > >> I feel like we have reached the moment when we should switch from the
>> > >> delays hardcoded in the pipeline handler to sensor specific values like
>> > >> rpi does. I could try doing that, but in a separate series. What do you
>> > >> think?
>> > >
>> > > Oh - absolutely. If it's something you're willing to work on - I think
>> > > that would be a massive help. Indeed, I think that's a separate series.
>> >
>> > I'd like to give it a try. Am I right that adding
>> >
>> >     virtual void getDelays(int &exposureDelay, int &gainDelay, int &vblankDelay, int &hblankDelay)
>>
>> Yes, quite likely. I don't know what the plumbing will look like yet,
>> and maybe it would help to use something more structured than a series
>> of int references, but that's what's needed somewhere...
>
> A map of controls to delay values maybe.

The "series of int references" is what CamHelper currently does, so I
thought it could be a starting point. A map sounds more reasonable,
yes. I'll try to get an RFC ready this week.

>> > to CameraSensorHelper returning generic delays, then overriding it for
>> > sensors where we know the exact delays is a valid approach? Also, why do
>> > we have both CamHelper and CameraSensorHelper, with former being
>> > significantly more featureful, but all the IPAs except rpi using the
>> > latter?
>>
>> Because Raspberry Pi create their own IPA - which doesn't use the
>> CameraSensorHelpers - because they are not as featureful ... and all the
>> other IPAs can not use the Raspberry Pi helpers - because they are in
>> the Raspberry Pi IPA instead of common code ... and no one has yet tried
>> to align the libipa code until now.
>
> It's also because the RPi IPA predates CameraSensorHelper if I recall
> correctly.
>
>> Ultimately - Making sure that the helpers in libipa can be grown in a
>> way that Raspberry Pi can also use them will then mean that we only need
>> to have *one* location for all the common camera sensor specific
>> helpers.

I see, thanks. I'll stick to growing CameraSensorHelper as needed for
now.

--
Best regards,
Mikhail