[libcamera-devel] [PATCH v5 08/10] ipa: rkisp1: Add AF controls to the RkISP1 IPA

[Jacopo Mondi]

Hi Kieran

On Fri, Mar 24, 2023 at 03:47:53PM +0000, Kieran Bingham wrote:
>
>
> On 24/03/2023 15:42, Jacopo Mondi via libcamera-devel wrote:
> > Hi daniel
> >
> > On Fri, Mar 24, 2023 at 03:29:06PM +0100, Daniel Semkowicz via libcamera-devel wrote:
> > > Add controls supported by the AF algorithm to the list of controls
> > > supported by the RkISP1 IPA. This exposes the AF controls to the user
> > > and allows controlling the AF algorithm using the top level API.
> > >
> > > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > > ---
> > >   src/ipa/rkisp1/rkisp1.cpp | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > >
> > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > index cd1fbae3..4b30844f 100644
> > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > @@ -101,10 +101,16 @@ namespace {
> > >   /* List of controls handled by the RkISP1 IPA */
> > >   const ControlInfoMap::Map rkisp1Controls{
> > >   	{ &controls::AeEnable, ControlInfo(false, true) },
> > > +	{ &controls::AfMetering, ControlInfo(controls::AfMeteringValues) },
> > > +	{ &controls::AfMode, ControlInfo(controls::AfModeValues) },
> >
> > AfPauseDeferred is not supported
> >
>
> What happens if there's no VCM though - Should we only expose the controls

You're very right, I overlooked that.

> if a VCM is identified? (Though that requires reworking the creation of this
> rkisp1Controls list to be handled by the algorithms)

I'm not sure it needs to be done by algorithms, but the IPA should
certainly be made aware of the lens presence at init() time.

To be honest I would be fine with what RPi does in this regard. have a
look at the usage of lensPresent_ in their IPA module init() function.
This looks like the less invasive change.

I presume this could even be made better by passing the lens controls'
from the PH to the IPA at init() time so that the AF ControlInfo
elements could be initialized with the proper limits and not with
default values as it happens here and on RPi.

>
> --
> Kieran
>
>
> > Otherwise
> > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> >
> > > +	{ &controls::AfPause, ControlInfo(controls::AfPauseValues) },
> > > +	{ &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) },
> > > +	{ &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> >
> > We should -really- move to instantiate control limits directly based
> > on the sensor configuration...
> >
> > >   	{ &controls::AwbEnable, ControlInfo(false, true) },
> > >   	{ &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) },
> > >   	{ &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) },
> > >   	{ &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) },
> > > +	{ &controls::LensPosition, ControlInfo(0.0f, 2147483647.0f) },
> > >   	{ &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) },
> > >   	{ &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) },
> > >   	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
> > > --
> > > 2.39.2
> > >