[libcamera-devel,v4,3/3] ipa: ipu3: Ensure controls exists in before they are queried

15559 diff mbox series

Add a validation check for sensor controls validateSensorControls()
before they are queried in IPAIPU3::updateSessionConfiguration().
Fail the IPAIPU3::configure() if the required sensor controls are
not found.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/ipa/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Quoting Umang Jain via libcamera-devel (2022-03-25 10:34:10)
> Add a validation check for sensor controls validateSensorControls()
> before they are queried in IPAIPU3::updateSessionConfiguration().
> Fail the IPAIPU3::configure() if the required sensor controls are
> not found.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/ipa/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> index 3717d893..50b52d8b 100644
> --- a/src/ipa/ipu3/ipu3.cpp
> +++ b/src/ipa/ipu3/ipu3.cpp
> @@ -155,6 +155,7 @@ private:
>         void parseStatistics(unsigned int frame,
>                              int64_t frameTimestamp,
>                              const ipu3_uapi_stats_3a *stats);
> +       bool validateSensorControls();
>  
>         void setControls(unsigned int frame);
>         void calculateBdsGrid(const Size &bdsOutputSize);
> @@ -268,6 +269,28 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
>         *ipaControls = ControlInfoMap(std::move(controls), controls::controls);
>  }
>  
> +/**
> + * \brief Validate that the sensor controls mandatory for the IPA exists
> + */
> +bool IPAIPU3::validateSensorControls()
> +{
> +       static const uint32_t ctrls[] = {
> +               V4L2_CID_ANALOGUE_GAIN,
> +               V4L2_CID_EXPOSURE,
> +               V4L2_CID_VBLANK,
> +       };
> +
> +       for (auto c : ctrls) {
> +               if (sensorCtrls_.find(c) == sensorCtrls_.end()) {
> +                       LOG(IPAIPU3, Error) << "Unable to find sensor control "
> +                                           << utils::hex(c);
> +                       return false;
> +               }
> +       }
> +
> +       return true;
> +}
> +
>  /**
>   * \brief Initialize the IPA module and its controls
>   *
> @@ -433,6 +456,11 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
>         /* Clean frameContext at each reconfiguration. */
>         context_.frameContext = {};
>  
> +       if (!validateSensorControls()) {
> +               LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> +               return -EINVAL;
> +       }
> +

I think this is fine, but I would have probably put it directly after
the assignment of

	sensorCtrls_ = configInfo.sensorControls;

in IPAIPU3::configure().

Or better yet, if IPAIPU3::validateSensorControls() takes a reference to
a control list, we can validate the configInfo.sensorControls; before we
assign it? That way we're logically validating the input before we
accept it.


Either way though - it's not a big deal, so:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


>         /* Update the camera controls using the new sensor settings. */
>         updateControls(sensorInfo_, sensorCtrls_, ipaControls);
>  
> -- 
> 2.31.1
>

On Fri, Mar 25, 2022 at 11:48:57AM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Umang Jain via libcamera-devel (2022-03-25 10:34:10)
> > Add a validation check for sensor controls validateSensorControls()
> > before they are queried in IPAIPU3::updateSessionConfiguration().
> > Fail the IPAIPU3::configure() if the required sensor controls are
> > not found.
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  src/ipa/ipu3/ipu3.cpp | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp
> > index 3717d893..50b52d8b 100644
> > --- a/src/ipa/ipu3/ipu3.cpp
> > +++ b/src/ipa/ipu3/ipu3.cpp
> > @@ -155,6 +155,7 @@ private:
> >         void parseStatistics(unsigned int frame,
> >                              int64_t frameTimestamp,
> >                              const ipu3_uapi_stats_3a *stats);
> > +       bool validateSensorControls();
> >  
> >         void setControls(unsigned int frame);
> >         void calculateBdsGrid(const Size &bdsOutputSize);
> > @@ -268,6 +269,28 @@ void IPAIPU3::updateControls(const IPACameraSensorInfo &sensorInfo,
> >         *ipaControls = ControlInfoMap(std::move(controls), controls::controls);
> >  }
> >  
> > +/**
> > + * \brief Validate that the sensor controls mandatory for the IPA exists
> > + */
> > +bool IPAIPU3::validateSensorControls()
> > +{
> > +       static const uint32_t ctrls[] = {
> > +               V4L2_CID_ANALOGUE_GAIN,
> > +               V4L2_CID_EXPOSURE,
> > +               V4L2_CID_VBLANK,
> > +       };
> > +
> > +       for (auto c : ctrls) {
> > +               if (sensorCtrls_.find(c) == sensorCtrls_.end()) {
> > +                       LOG(IPAIPU3, Error) << "Unable to find sensor control "
> > +                                           << utils::hex(c);
> > +                       return false;
> > +               }
> > +       }
> > +
> > +       return true;
> > +}
> > +
> >  /**
> >   * \brief Initialize the IPA module and its controls
> >   *
> > @@ -433,6 +456,11 @@ int IPAIPU3::configure(const IPAConfigInfo &configInfo,
> >         /* Clean frameContext at each reconfiguration. */
> >         context_.frameContext = {};
> >  
> > +       if (!validateSensorControls()) {
> > +               LOG(IPAIPU3, Error) << "Sensor control validation failed.";
> > +               return -EINVAL;
> > +       }
> > +
> 
> I think this is fine, but I would have probably put it directly after
> the assignment of
> 
> 	sensorCtrls_ = configInfo.sensorControls;
> 
> in IPAIPU3::configure().
> 
> Or better yet, if IPAIPU3::validateSensorControls() takes a reference to
> a control list, we can validate the configInfo.sensorControls; before we
> assign it? That way we're logically validating the input before we
> accept it.

I like that.

> Either way though - it's not a big deal, so:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Likewise,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >         /* Update the camera controls using the new sensor settings. */
> >         updateControls(sensorInfo_, sensorCtrls_, ipaControls);
> >