[libcamera-devel] [PATCH v2 2/2] libcamera: raspberrypi: Fetch correct value for sensor's modeSensitivity
Naushir Patuck
naush at raspberrypi.com
Wed Apr 13 12:44:14 CEST 2022
Hi Kieran,
On Wed, 13 Apr 2022 at 11:20, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:
> 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.
>
I think this is the part that may be troublesome. Assume an application
did not call CameraConfiguration::validate() directly, but it does get
called
from PipelineHandler::configure(). The validate() will then adjust the
sensitivity value and return a Status::Adjusted. This will then fail the
PipelineHandler::configure() call. Perhaps CameraConfiguration::validate()
should be a mandatory call from the application? Or if we adjust the
sensitivity in CameraConfiguration::validate(), we don't return
Status::Adjusted?
Naush
>
> 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
> > > >
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220413/af919e27/attachment-0001.htm>
More information about the libcamera-devel
mailing list