[libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch correct value for sensor's modeSensitivity
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Apr 13 11:39:44 CEST 2022
Quoting Kieran Bingham (2022-02-09 13:17:42)
> Quoting David Plowman (2022-02-09 13:00:57)
> > Hi Kieran
> >
> > On Wed, 9 Feb 2022 at 12:27, Kieran Bingham
> > <kieran.bingham at ideasonboard.com> wrote:
> > >
> > > Quoting David Plowman (2022-01-04 13:33:05)
> > > > Hi again
> > > >
> > > > On Tue, 4 Jan 2022 at 11:13, Kieran Bingham
> > > > <kieran.bingham at ideasonboard.com> wrote:
> > > > >
> > > > > Quoting David Plowman (2021-09-22 14:29:15)
> > > > > > These changes retrieve the correct value for sensitivity of the mode
> > > > > > selected for the sensor. This value is known to the CamHelper which
> > > > > > passes it across to the pipeline handler so that it can be set
> > > > > > correctly in the CameraConfiguration.
> > > > > >
> > > > > > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > > > > > ---
> > > > > > include/libcamera/ipa/raspberrypi.mojom | 7 ++++++-
> > > > > > src/ipa/raspberrypi/raspberrypi.cpp | 7 +++++--
> > > > > > src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 +++++++---
> > > > > > 3 files changed, 18 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> > > > > > index e453d46c..a92a76f8 100644
> > > > > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > > > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > > > > @@ -38,6 +38,10 @@ struct IPAConfig {
> > > > > > libcamera.FileDescriptor lsTableHandle;
> > > > > > };
> > > > > >
> > > > > > +struct IPAConfigResult {
> > > > > > + float modeSensitivity;
> > > > > > +};
> > > > > > +
> > > > > > struct StartConfig {
> > > > > > libcamera.ControlList controls;
> > > > > > int32 dropFrameCount;
> > > > > > @@ -57,6 +61,7 @@ interface IPARPiInterface {
> > > > > > * \param[in] entityControls Controls provided by the pipeline entities
> > > > > > * \param[in] ipaConfig Pipeline-handler-specific configuration data
> > > > > > * \param[out] controls Controls to apply by the pipeline entity
> > > > > > + * \param[out] result Other results that the pipeline handler may require
> > > > > > *
> > > > > > * This function shall be called when the camera is configured to inform
> > > > > > * the IPA of the camera's streams and the sensor settings.
> > > > > > @@ -71,7 +76,7 @@ interface IPARPiInterface {
> > > > > > map<uint32, libcamera.IPAStream> streamConfig,
> > > > > > map<uint32, libcamera.ControlInfoMap> entityControls,
> > > > > > IPAConfig ipaConfig)
> > > > > > - => (int32 ret, libcamera.ControlList controls);
> > > > > > + => (int32 ret, libcamera.ControlList controls, IPAConfigResult result);
> > > > > >
> > > > > > /**
> > > > > > * \fn mapBuffers()
> > > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > index 047123ab..796e6d15 100644
> > > > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > > > @@ -97,7 +97,7 @@ public:
> > > > > > const std::map<unsigned int, IPAStream> &streamConfig,
> > > > > > const std::map<unsigned int, ControlInfoMap> &entityControls,
> > > > > > const ipa::RPi::IPAConfig &data,
> > > > > > - ControlList *controls) override;
> > > > > > + ControlList *controls, ipa::RPi::IPAConfigResult *result) override;
> > > > > > void mapBuffers(const std::vector<IPABuffer> &buffers) override;
> > > > > > void unmapBuffers(const std::vector<unsigned int> &ids) override;
> > > > > > void signalStatReady(const uint32_t bufferId) override;
> > > > > > @@ -337,7 +337,7 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > > > > > [[maybe_unused]] const std::map<unsigned int, IPAStream> &streamConfig,
> > > > > > const std::map<unsigned int, ControlInfoMap> &entityControls,
> > > > > > const ipa::RPi::IPAConfig &ipaConfig,
> > > > > > - ControlList *controls)
> > > > > > + ControlList *controls, ipa::RPi::IPAConfigResult *result)
> > > > > > {
> > > > > > if (entityControls.size() != 2) {
> > > > > > LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > > > > > @@ -389,6 +389,9 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> > > > > > /* Pass the camera mode to the CamHelper to setup algorithms. */
> > > > > > helper_->SetCameraMode(mode_);
> > > > > >
> > > > > > + /* The pipeline handler passes out the mode's sensitivity. */
> > > > > > + result->modeSensitivity = mode_.sensitivity;
> > > > > > +
> > > > > > if (firstStart_) {
> > > > > > /* Supply initial values for frame durations. */
> > > > > > applyFrameDurations(defaultMinFrameDuration, defaultMaxFrameDuration);
> > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > index 0bdfa727..caf0030e 100644
> > > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > > > > @@ -147,7 +147,7 @@ public:
> > > > > > void frameStarted(uint32_t sequence);
> > > > > >
> > > > > > int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> > > > > > - int configureIPA(const CameraConfiguration *config);
> > > > > > + int configureIPA(CameraConfiguration *config);
> > > > > >
> > > > > > void statsMetadataComplete(uint32_t bufferId, const ControlList &controls);
> > > > > > void runIsp(uint32_t bufferId);
> > > > > > @@ -1250,7 +1250,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> > > > > > return ipa_->init(settings, sensorConfig);
> > > > > > }
> > > > > >
> > > > > > -int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > > > > +int RPiCameraData::configureIPA(CameraConfiguration *config)
> > > > > > {
> > > > > > /* We know config must be an RPiCameraConfiguration. */
> > > > > > const RPiCameraConfiguration *rpiConfig =
> > > > > > @@ -1299,13 +1299,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
> > > > > >
> > > > > > /* Ready the IPA - it must know about the sensor resolution. */
> > > > > > ControlList controls;
> > > > > > + ipa::RPi::IPAConfigResult result;
> > > > > > ret = ipa_->configure(sensorInfo_, streamConfig, entityControls, ipaConfig,
> > > > > > - &controls);
> > > > > > + &controls, &result);
> > > > >
> > > > > This should probably be being passed back as a control/property here.
> > > > > But I think right now the &controls is a set of V4L2 Controls, and not
> > > > > libcamera controls.
> > > > >
> > > > > We've been discussing about changing that so that the IPAs pass
> > > > > libcamera::controls instead. Currently Isolated IPAs will break when we
> > > > > try to pass lens controls because that's 'yet another infomap' that will
> > > > > have to be serialised.
> > > > >
> > > > > If we instead use only libcamera controls in the interface between the
> > > > > pipeline handler and the IPA this would be resolved, and it would mean
> > > > > that list could also pass the sensitivity property.
> > > > >
> > > > > Is there any reason you're aware of as to why we could or couldn't use
> > > > > libcamera controls here? I know Laurent has been concerned in the past
> > > > > about perhaps if there was any precision issues or rounding that the IPA
> > > > > might need to know about that would otherwise occur in the pipeline
> > > > > handler. Do you see this as an issue? or would we be able to
> > > > > successfully convert the interfaces to use libcamera control lists only
> > > > > (and keep the V4L2'ness' hidden in the V4L2 layer / pipeline handler
> > > > > only).
> > > >
> > > > I can't think of any precision issues that particularly bother me
> > > > (which of course doesn't mean there aren't any). As I said in my other
> > > > reply, I do need this value to be available from the call to
> > > > configure(), before you get to call start(), so it's not clear to me
> > > > at the minute where I would read this value back - but I'm guessing
> > > > someone has some ideas?
> > >
> > > That discussion above was more about the type of the Control rather than
> > > where it gets returned.
> > >
> > > I'm confused here if the start() and configure() you are referencing is
> > > the public API, or the IPA interface API?
> >
> > I'm referring to the public Camera API. It's applications that will
> > want to know this number.
>
> Ok, so it can certainly be updated by the configure operation, but I
> wonder if it's something that the model would expect to be determined
> during validate()?
>
> Configure() is supposed to be a 'non-changing' operation ... but this is
> more like a return value. Would an application expect to know the mode
> sensitivity after they've validated a configuration? (to check it is
> what they expect?) before even calling configure?
Aha - this is the question I'd like answered before merging ;-)
--
Kieran
>
> --
> Kieran
>
> >
> > David
> >
> > >
> > > --
> > > Kieran
> > >
> > >
> > > >
> > > > David
> > > >
> > > > >
> > > > > All that said, changing the controls interface there is still possible
> > > > > on top, even with this patch, so I don't object to passing it directly
> > > > > back for now.
> > > > >
> > > > >
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > >
> > > > > --
> > > > > Kieran
> > > > >
> > > > >
> > > > > > if (ret < 0) {
> > > > > > LOG(RPI, Error) << "IPA configuration failed!";
> > > > > > return -EPIPE;
> > > > > > }
> > > > > >
> > > > > > + /* Store the mode sensitivity for the application. */
> > > > > > + config->modeSensitivity = result.modeSensitivity;
> > > > > > +
> > > > > > /*
> > > > > > * Configure the H/V flip controls based on the combination of
> > > > > > * the sensor and user transform.
> > > > > > --
> > > > > > 2.20.1
> > > > > >
More information about the libcamera-devel
mailing list