<div dir="ltr"><div dir="ltr">Hi Laurent,</div><div dir="ltr"><br></div><div dir="ltr">(cc David)<br><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, 5 Oct 2022 at 11:07, 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 Wed, Oct 05, 2022 at 10:50:19AM +0100, Naushir Patuck wrote:<br>
> On Tue, 4 Oct 2022 at 20:43, Laurent Pinchart wrote:<br>
> > On Mon, Oct 03, 2022 at 10:55:56AM +0100, Naushir Patuck wrote:<br>
> > > Append "_mono" to the sensor name when generating the tuning filename for<br>
> > > monochrome sensor variants. So the new naming convention is as follows:<br>
> > ><br>
> > > <sensor_name>.json - Standard colour sensor variant<br>
> > > <sensor_name>_mono.json - Monochrom sensor variant<br>
> > ><br>
> > > Rename the existing imx296.json file to imx296_mono.json as this tuning file<br>
> > > is based on the monochrome variant of the IMX296.<br>
> ><br>
> > We'll still have to figure out how to identify camera module, but that's<br>
> > fine, we don't have to solve all issues in one go.<br>
> ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > ---<br>
> > > .../data/{imx296.json => imx296_mono.json} | 0<br>
> > > src/ipa/raspberrypi/data/meson.build | 2 +-<br>
> > > .../pipeline/raspberrypi/raspberrypi.cpp | 20 +++++++++++++++----<br>
> > > 3 files changed, 17 insertions(+), 5 deletions(-)<br>
> > > rename src/ipa/raspberrypi/data/{imx296.json => imx296_mono.json} (100%)<br>
> > ><br>
> > > diff --git a/src/ipa/raspberrypi/data/imx296.json b/src/ipa/raspberrypi/data/imx296_mono.json<br>
> > > similarity index 100%<br>
> > > rename from src/ipa/raspberrypi/data/imx296.json<br>
> > > rename to src/ipa/raspberrypi/data/imx296_mono.json<br>
> > > diff --git a/src/ipa/raspberrypi/data/meson.build b/src/ipa/raspberrypi/data/meson.build<br>
> > > index 211811cfa915..465a7a17ce6f 100644<br>
> > > --- a/src/ipa/raspberrypi/data/meson.build<br>
> > > +++ b/src/ipa/raspberrypi/data/meson.build<br>
> > > @@ -4,7 +4,7 @@ conf_files = files([<br>
> > > 'imx219.json',<br>
> > > 'imx219_noir.json',<br>
> > > 'imx290.json',<br>
> > > - 'imx296.json',<br>
> > > + 'imx296_mono.json',<br>
> > > 'imx378.json',<br>
> > > 'imx477.json',<br>
> > > 'imx477_noir.json',<br>
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > index dcd81650c32d..39a1c798df2e 100644<br>
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > > @@ -67,6 +67,14 @@ SensorFormats populateSensorFormats(std::unique_ptr<CameraSensor> &sensor)<br>
> > > return formats;<br>
> > > }<br>
> > ><br>
> > > +bool isMonoSensor(std::unique_ptr<CameraSensor> &sensor)<br>
> > > +{<br>
> > > + unsigned int mbusCode = sensor->mbusCodes()[0];<br>
> > > + const BayerFormat &bayer = BayerFormat::fromMbusCode(mbusCode);<br>
> > > +<br>
> > > + return bayer.order == BayerFormat::Order::MONO;<br>
> > > +}<br>
> > > +<br>
> > > PixelFormat mbusCodeToPixelFormat(unsigned int mbus_code,<br>
> > > BayerFormat::Packing packingReq)<br>
> > > {<br>
> > > @@ -1551,12 +1559,16 @@ int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)<br>
> > > */<br>
> > > std::string configurationFile;<br>
> > > char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE");<br>
> > > - if (!configFromEnv || *configFromEnv == '\0')<br>
> > > - configurationFile = ipa_->configurationFile(sensor_->model() + ".json");<br>
> > > - else<br>
> > > + std::string model = sensor_->model();<br>
> > > + if (!configFromEnv || *configFromEnv == '\0') {<br>
> > > + if (isMonoSensor(sensor_))<br>
> > > + model += "_mono";<br>
> > > + configurationFile = ipa_->configurationFile(model + ".json");<br>
> > > + } else {<br>
> > > configurationFile = std::string(configFromEnv);<br>
> > > + }<br>
> > ><br>
> > > - IPASettings settings(configurationFile, sensor_->model());<br>
> > > + IPASettings settings(configurationFile, model);<br>
> ><br>
> > The model will be suffixed by "_mono" only when the<br>
> > LIBCAMERA_RPI_TUNING_FILE environment variable isn't set. Is that on<br>
> > purpose ?<br>
> <br>
> Yes, this is intentional, Jacopo queried the same thing :)<br>
> <br>
> If a user specifies the LIBCAMERA_RPI_TUNING_FILE env argument,<br>
> we expect them to provide the exact filename that must be used including<br>
> path and all. This user provided filename may have not resemblance to the<br>
> sensor model either. So it will never be modified with variant suffixes.<br>
<br>
For the configuration file name, that's fine, but note how the model is<br>
passed as the second argument to the IPASettings constructor. It's<br>
currently used by the IPA module to instantiate the camera sensor<br>
helper. If a custom configuration file is specified, the sensor model<br>
won't get the "_mono" suffix. It should work fine for now I think, as<br>
the helper matching logic performs a partial string match, but it seems<br>
a bit inconsistent. Shouldn't the IPA module get the "_mono" model name,<br>
even if a custom tuning file is used ?<br></blockquote><div><br></div><div>Sorry, I misunderstood your point!</div><div><br></div><div>Yes, you are right, sensor model will not get the _mono suffix in the</div><div>env variable case. But by luck, the cam helper does a partial match</div><div>and it all works.</div><div><br></div><div>IMHO sensor model should not get the _mono suffix as the model</div><div>string is only needed to match the CamHelper instance. Apart from</div><div>the config file itself, the IPA does not need to know about sensor variant</div><div>differences.</div><div><br></div><div>What do you and David think?</div><div><br></div><div>Naush</div><div><br></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>
> > ><br>
> > > return ipa_->init(settings, result);<br>
> > > }<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>