[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 12:20:01 CEST 2022
Quoting Naushir Patuck (2022-04-13 11:14:01)
> Hi Kieran,
>
> On Wed, 13 Apr 2022 at 10:38, Kieran Bingham <
> kieran.bingham at ideasonboard.com> wrote:
>
> > Hi David, Naush,
> >
> > 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);
> >
> > I've just rebased this series to master to facilitate merging, and with
> > fresh eyes I can't help but wonder if this value shouldn't be returned
> > in the validate() phase. (Not sure if this has been asked before / yet).
> >
> > Is there anything that prevents us adding a validate() to the IPA
> > interface to allow validating the configuration and at that point,
> > setting the mode sensitivity? Or can this value /only/ be determined
> > when configuring?
> >
>
> I can't think of any reason that this couldn't be done in validate(), but
> David
> might have some reasons. However, is an application required to call
> CameraConfiguration::validate() directly? I can see that it does get called
> in Camera::configure(), just before the call to
> PipelineHandler::configure().
> If there is no requirement to call CameraConfiguration::validate(), would it
> matter where the result gets set?
There isn't a 'requirement' to do so as such, but it's recommended. If
the call to configure can't be satisfied 'precisely' then the configure
call should fail. It should not make any changes or adjustments to the
configuration.
Validate will tell you if the pipeline handler made any changes, and
more than that - the validate before configure /must/ report that no
changes were made.
When an application calls validate() - the pipeline handler can make
changes to the Configuration (such as filling in the ModeSensitivity
here) and if a change was made - it can return 'ConfigurationAdjusted' -
which then means the application should check to see what was different
from what it requested - to what the pipeline handler will deliver.
I would like there to be a way to highlight 'what' changes were made if
the validate returns 'Adjusted' - but I'm not sure how we could easily
implement that yet.
--
Kieran
> Regards,
> Naush
>
>
> >
> > Then the validate phase should be able to return the mode sensitivity of
> > the configuration that is being validated.
> >
> > Doing that we could keep the configure calls using a const parameter for
> > their config structures.
> >
> > > 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