<div dir="ltr"><div dir="ltr">Hi Laurent,<div><br></div><div>David's away this week, so I'll respond on his behalf.</div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, 11 Jul 2021 at 21:34, 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">Hello,<br>
<br>
On Fri, Jul 09, 2021 at 04:04:22PM +0100, Kieran Bingham wrote:<br>
> On 07/07/2021 14:40, David Plowman wrote:<br>
> > The configuration (camera tuning) file used by the Raspberry Pi comes<br>
> > by default from the sensor name. However, we now allow this to be<br>
> > overridden by the LIBCAMERA_RPI_TUNING_FILE environment variable.<br>
> <br>
> Some how I had it in my head that this would be an override occurring in<br>
> the IPA itself, but I think this is ok too.<br>
<br>
I like it better in the pipeline handler, as the IPA module isolation<br>
mechanisms may limit the ability to pass environmen variables.<br>
<br>
> > Signed-off-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > ---<br>
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 14 ++++++++++++--<br>
> >  1 file changed, 12 insertions(+), 2 deletions(-)<br>
> > <br>
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > index 082eb1ee..a738770a 100644<br>
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp<br>
> > @@ -1232,8 +1232,18 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)<br>
> >     ipa_->setIspControls.connect(this, &RPiCameraData::setIspControls);<br>
> >     ipa_->setDelayedControls.connect(this, &RPiCameraData::setDelayedControls);<br>
> >  <br>
> > -   IPASettings settings(ipa_->configurationFile(sensor_->model() + ".json"),<br>
> > -                        sensor_->model());<br>
> > +   /*<br>
> > +    * The configuration (tuning file) is made from the sensor name unless<br>
> > +    * the environment variable overrides it.<br>
> > +    */<br>
> > +   std::string configurationFile;<br>
> > +   char const *configFromEnv = utils::secure_getenv("LIBCAMERA_RPI_TUNING_FILE");<br>
> <br>
> We likely need to document this somewhere...<br>
> <br>
> We have Documentation/environment_variables.rst<br>
> <br>
> But that could be a follow up patch<br>
> <br>
> > +   if (!configFromEnv || *configFromEnv == '\0')<br>
> > +           configurationFile = ipa_->configurationFile(sensor_->model() + ".json");<br>
> <br>
> I'm not a particular fan of the current ipa_->configurationFile<br>
> implementation, and I think I see that it should have changes in the future.<br>
> <br>
> If this were to be generalised, then the override should go there - but<br>
> given that this is quite specific to RPi needs right now - I think this<br>
> is ok.<br>
<br>
I'd like to see this being generalized indeed. We'll have to figure out<br>
how to specific a per-camera configuration file though.<br>
<br>
> However I could envisage that in the future this environment variable<br>
> gets deprecated, but I don't yet know what would replace it - so I<br>
> believe it's worth while to have this for now.<br>
<br>
That's the important question: David, if we later create a more generic<br>
mechanism to select a different IPA configuration file, will it be fine<br>
to drop support for LIBCAMERA_RPI_TUNING_FILE ?</blockquote><div><br></div><div>Yes, I think that should be fine.  I assume any new mechanism would look</div><div>like a string parameter passed into the pipeline handler directly through</div><div>the application, hence able to be set on the command line.</div><div><br></div><div>Regards,</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">If so,<br>
<br>
Reviewed-by: Laurent Pinchart <<a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>><br>
<br>
> Reviewed-by: Kieran Bingham <<a href="mailto:kieran.bingham@ideasonboard.com" target="_blank">kieran.bingham@ideasonboard.com</a>><br>
> <br>
> > +   else<br>
> > +           configurationFile = std::string(configFromEnv);<br>
> > +<br>
> > +   IPASettings settings(configurationFile, sensor_->model());<br>
> >  <br>
> >     return ipa_->init(settings, sensorConfig);<br>
> >  }<br>
<br>
-- <br>
Regards,<br>
<br>
Laurent Pinchart<br>
</blockquote></div></div>