[libcamera-devel,v2,11/12] ipa: raspberrypi: Pass sensor config back from configure()
diff mbox series

Message ID 20200704005227.21782-12-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • Support passing custom data to IPA configure()
Related show

Commit Message

Laurent Pinchart July 4, 2020, 12:52 a.m. UTC 
The Raspberry Pi IPA uses the custom RPI_IPA_ACTION_SET_SENSOR_CONFIG
frame action to send the sensor staggered write configuration to the
pipeline handler when the IPA is configured. Replace this ad-hoc
mechanism by passing the corresponding data back from the IPA to the
pipeline handler through the configure() response. This allows
synchronous handling of the response on the pipeline handler side.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:

- Restore RPI_IPA_ACTION_V4L2_SET_ISP handling in stop state
- Drop sensor orientation handling based on IPA result
---
 include/libcamera/ipa/raspberrypi.h           |  3 +-
 src/ipa/raspberrypi/raspberrypi.cpp           | 42 ++++++++-------
 .../pipeline/raspberrypi/raspberrypi.cpp      | 53 +++++++++++--------
 3 files changed, 56 insertions(+), 42 deletions(-)

Comments

Jacopo Mondi July 6, 2020, 9:25 a.m. UTC | #1 
Hi Laurent,

On Sat, Jul 04, 2020 at 03:52:26AM +0300, Laurent Pinchart wrote:
> The Raspberry Pi IPA uses the custom RPI_IPA_ACTION_SET_SENSOR_CONFIG
> frame action to send the sensor staggered write configuration to the
> pipeline handler when the IPA is configured. Replace this ad-hoc
> mechanism by passing the corresponding data back from the IPA to the
> pipeline handler through the configure() response. This allows
> synchronous handling of the response on the pipeline handler side.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Looks good to me, but of course let's wait for RPi people to validate
this :)

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
> Changes since v1:
>
> - Restore RPI_IPA_ACTION_V4L2_SET_ISP handling in stop state
> - Drop sensor orientation handling based on IPA result
> ---
>  include/libcamera/ipa/raspberrypi.h           |  3 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 42 ++++++++-------
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 53 +++++++++++--------
>  3 files changed, 56 insertions(+), 42 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 46ce7c286b20..a49377695f22 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -12,6 +12,8 @@
>
>  enum RPiConfigParameters {
>  	RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
> +	RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> +	RPI_IPA_CONFIG_SENSOR = (1 << 2),
>  };
>
>  enum RPiOperations {
> @@ -20,7 +22,6 @@ enum RPiOperations {
>  	RPI_IPA_ACTION_STATS_METADATA_COMPLETE,
>  	RPI_IPA_ACTION_RUN_ISP,
>  	RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME,
> -	RPI_IPA_ACTION_SET_SENSOR_CONFIG,
>  	RPI_IPA_ACTION_EMBEDDED_COMPLETE,
>  	RPI_IPA_EVENT_SIGNAL_STAT_READY,
>  	RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 0e39a1137cd0..378ea309fa81 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -93,7 +93,7 @@ private:
>  	void reportMetadata();
>  	bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);
>  	void processStats(unsigned int bufferId);
> -	void applyAGC(const struct AgcStatus *agcStatus);
> +	void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
>  	void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
>  	void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);
>  	void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);
> @@ -195,6 +195,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  	if (entityControls.empty())
>  		return;
>
> +	result->operation = 0;
> +
>  	unicam_ctrls_ = entityControls.at(0);
>  	isp_ctrls_ = entityControls.at(1);
>  	/* Setup a metadata ControlList to output metadata. */
> @@ -216,13 +218,11 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>  		helper_->GetDelays(exposureDelay, gainDelay);
>  		sensorMetadata = helper_->SensorEmbeddedDataPresent();
>
> -		IPAOperationData op;
> -		op.operation = RPI_IPA_ACTION_SET_SENSOR_CONFIG;
> -		op.data.push_back(gainDelay);
> -		op.data.push_back(exposureDelay);
> -		op.data.push_back(sensorMetadata);
> +		result->data.push_back(gainDelay);
> +		result->data.push_back(exposureDelay);
> +		result->data.push_back(sensorMetadata);
>
> -		queueFrameAction.emit(0, op);
> +		result->operation |= RPI_IPA_CONFIG_STAGGERED_WRITE;
>  	}
>
>  	/* Re-assemble camera mode using the sensor info. */
> @@ -267,8 +267,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>
>  	/* SwitchMode may supply updated exposure/gain values to use. */
>  	metadata.Get("agc.status", agcStatus);
> -	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)
> -		applyAGC(&agcStatus);
> +	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> +		ControlList ctrls(unicam_ctrls_);
> +		applyAGC(&agcStatus, ctrls);
> +		result->controls.push_back(ctrls);
> +
> +		result->operation |= RPI_IPA_CONFIG_SENSOR;
> +	}
>
>  	lastMode_ = mode_;
>
> @@ -775,8 +780,15 @@ void IPARPi::processStats(unsigned int bufferId)
>  	controller_.Process(statistics, &rpiMetadata_);
>
>  	struct AgcStatus agcStatus;
> -	if (rpiMetadata_.Get("agc.status", agcStatus) == 0)
> -		applyAGC(&agcStatus);
> +	if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
> +		ControlList ctrls(unicam_ctrls_);
> +		applyAGC(&agcStatus, ctrls);
> +
> +		IPAOperationData op;
> +		op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
> +		op.controls.push_back(ctrls);
> +		queueFrameAction.emit(0, op);
> +	}
>  }
>
>  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> @@ -802,11 +814,8 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>  		  static_cast<int32_t>(awbStatus->gain_b * 1000));
>  }
>
> -void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
> +void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  {
> -	IPAOperationData op;
> -	op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
> -
>  	int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
>  	int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
>
> @@ -825,11 +834,8 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
>  			   << agcStatus->analogue_gain << " (Gain Code: "
>  			   << gain_code << ")";
>
> -	ControlList ctrls(unicam_ctrls_);
>  	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
>  	ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> -	op.controls.push_back(ctrls);
> -	queueFrameAction.emit(0, op);
>  }
>
>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 57602349cab2..9babf9f4a19c 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -819,7 +819,7 @@ int PipelineHandlerRPi::start(Camera *camera)
>  	/*
>  	 * Write the last set of gain and exposure values to the camera before
>  	 * starting. First check that the staggered ctrl has been initialised
> -	 * by the IPA action.
> +	 * by configure().
>  	 */
>  	ASSERT(data->staggeredCtrl_);
>  	data->staggeredCtrl_.reset();
> @@ -1170,15 +1170,36 @@ int RPiCameraData::configureIPA()
>  	}
>
>  	/* Ready the IPA - it must know about the sensor resolution. */
> +	IPAOperationData result;
> +
>  	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> -			nullptr);
> +			&result);
>
> -	/* Configure the H/V flip controls based on the sensor rotation. */
> -	ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> -	int32_t rotation = sensor_->properties().get(properties::Rotation);
> -	ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> -	ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> -	unicam_[Unicam::Image].dev()->setControls(&ctrls);
> +	if (result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE) {
> +		/*
> +		 * Setup our staggered control writer with the sensor default
> +		 * gain and exposure delays.
> +		 */
> +		if (!staggeredCtrl_) {
> +			staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> +					    { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
> +					      { V4L2_CID_EXPOSURE, result.data[1] } });
> +			sensorMetadata_ = result.data[2];
> +		}
> +
> +		/* Configure the H/V flip controls based on the sensor rotation. */
> +		ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> +		int32_t rotation = sensor_->properties().get(properties::Rotation);
> +		ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> +		ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> +		unicam_[Unicam::Image].dev()->setControls(&ctrls);
> +	}
> +
> +	if (result.operation & RPI_IPA_CONFIG_SENSOR) {
> +		const ControlList &ctrls = result.controls[0];
> +		if (!staggeredCtrl_.set(ctrls))
> +			LOG(RPI, Error) << "V4L2 staggered set failed";
> +	}
>
>  	return 0;
>  }
> @@ -1191,26 +1212,12 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>  	 */
>  	switch (action.operation) {
>  	case RPI_IPA_ACTION_V4L2_SET_STAGGERED: {
> -		ControlList controls = action.controls[0];
> +		const ControlList &controls = action.controls[0];
>  		if (!staggeredCtrl_.set(controls))
>  			LOG(RPI, Error) << "V4L2 staggered set failed";
>  		return;
>  	}
>
> -	case RPI_IPA_ACTION_SET_SENSOR_CONFIG: {
> -		/*
> -		 * Setup our staggered control writer with the sensor default
> -		 * gain and exposure delays.
> -		 */
> -		if (!staggeredCtrl_) {
> -			staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> -					    { { V4L2_CID_ANALOGUE_GAIN, action.data[0] },
> -					      { V4L2_CID_EXPOSURE, action.data[1] } });
> -			sensorMetadata_ = action.data[2];
> -		}
> -		return;
> -	}
> -
>  	case RPI_IPA_ACTION_V4L2_SET_ISP: {
>  		ControlList controls = action.controls[0];
>  		isp_[Isp::Input].dev()->setControls(&controls);
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Naushir Patuck July 8, 2020, 11:08 a.m. UTC | #2 
Hi Laurent,

Thank you for the patch.  Apart from the one note below,

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> The Raspberry Pi IPA uses the custom RPI_IPA_ACTION_SET_SENSOR_CONFIG
> frame action to send the sensor staggered write configuration to the
> pipeline handler when the IPA is configured. Replace this ad-hoc
> mechanism by passing the corresponding data back from the IPA to the
> pipeline handler through the configure() response. This allows
> synchronous handling of the response on the pipeline handler side.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Restore RPI_IPA_ACTION_V4L2_SET_ISP handling in stop state
> - Drop sensor orientation handling based on IPA result
> ---
>  include/libcamera/ipa/raspberrypi.h           |  3 +-
>  src/ipa/raspberrypi/raspberrypi.cpp           | 42 ++++++++-------
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 53 +++++++++++--------
>  3 files changed, 56 insertions(+), 42 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 46ce7c286b20..a49377695f22 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -12,6 +12,8 @@
>
>  enum RPiConfigParameters {
>         RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
> +       RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> +       RPI_IPA_CONFIG_SENSOR = (1 << 2),
>  };
>
>  enum RPiOperations {
> @@ -20,7 +22,6 @@ enum RPiOperations {
>         RPI_IPA_ACTION_STATS_METADATA_COMPLETE,
>         RPI_IPA_ACTION_RUN_ISP,
>         RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME,
> -       RPI_IPA_ACTION_SET_SENSOR_CONFIG,
>         RPI_IPA_ACTION_EMBEDDED_COMPLETE,
>         RPI_IPA_EVENT_SIGNAL_STAT_READY,
>         RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 0e39a1137cd0..378ea309fa81 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -93,7 +93,7 @@ private:
>         void reportMetadata();
>         bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);
>         void processStats(unsigned int bufferId);
> -       void applyAGC(const struct AgcStatus *agcStatus);
> +       void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
>         void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
>         void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);
>         void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);
> @@ -195,6 +195,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>         if (entityControls.empty())
>                 return;
>
> +       result->operation = 0;
> +
>         unicam_ctrls_ = entityControls.at(0);
>         isp_ctrls_ = entityControls.at(1);
>         /* Setup a metadata ControlList to output metadata. */
> @@ -216,13 +218,11 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>                 helper_->GetDelays(exposureDelay, gainDelay);
>                 sensorMetadata = helper_->SensorEmbeddedDataPresent();
>
> -               IPAOperationData op;
> -               op.operation = RPI_IPA_ACTION_SET_SENSOR_CONFIG;
> -               op.data.push_back(gainDelay);
> -               op.data.push_back(exposureDelay);
> -               op.data.push_back(sensorMetadata);
> +               result->data.push_back(gainDelay);
> +               result->data.push_back(exposureDelay);
> +               result->data.push_back(sensorMetadata);
>
> -               queueFrameAction.emit(0, op);
> +               result->operation |= RPI_IPA_CONFIG_STAGGERED_WRITE;
>         }
>
>         /* Re-assemble camera mode using the sensor info. */
> @@ -267,8 +267,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>
>         /* SwitchMode may supply updated exposure/gain values to use. */
>         metadata.Get("agc.status", agcStatus);
> -       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)
> -               applyAGC(&agcStatus);
> +       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> +               ControlList ctrls(unicam_ctrls_);
> +               applyAGC(&agcStatus, ctrls);
> +               result->controls.push_back(ctrls);
> +
> +               result->operation |= RPI_IPA_CONFIG_SENSOR;
> +       }
>
>         lastMode_ = mode_;
>
> @@ -775,8 +780,15 @@ void IPARPi::processStats(unsigned int bufferId)
>         controller_.Process(statistics, &rpiMetadata_);
>
>         struct AgcStatus agcStatus;
> -       if (rpiMetadata_.Get("agc.status", agcStatus) == 0)
> -               applyAGC(&agcStatus);
> +       if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
> +               ControlList ctrls(unicam_ctrls_);
> +               applyAGC(&agcStatus, ctrls);
> +
> +               IPAOperationData op;
> +               op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
> +               op.controls.push_back(ctrls);
> +               queueFrameAction.emit(0, op);
> +       }
>  }
>
>  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> @@ -802,11 +814,8 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
>                   static_cast<int32_t>(awbStatus->gain_b * 1000));
>  }
>
> -void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
> +void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  {
> -       IPAOperationData op;
> -       op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
> -
>         int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
>         int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
>
> @@ -825,11 +834,8 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
>                            << agcStatus->analogue_gain << " (Gain Code: "
>                            << gain_code << ")";
>
> -       ControlList ctrls(unicam_ctrls_);
>         ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
>         ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> -       op.controls.push_back(ctrls);
> -       queueFrameAction.emit(0, op);
>  }
>
>  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 57602349cab2..9babf9f4a19c 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -819,7 +819,7 @@ int PipelineHandlerRPi::start(Camera *camera)
>         /*
>          * Write the last set of gain and exposure values to the camera before
>          * starting. First check that the staggered ctrl has been initialised
> -        * by the IPA action.
> +        * by configure().
>          */
>         ASSERT(data->staggeredCtrl_);
>         data->staggeredCtrl_.reset();
> @@ -1170,15 +1170,36 @@ int RPiCameraData::configureIPA()
>         }
>
>         /* Ready the IPA - it must know about the sensor resolution. */
> +       IPAOperationData result;
> +
>         ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> -                       nullptr);
> +                       &result);
>
> -       /* Configure the H/V flip controls based on the sensor rotation. */
> -       ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> -       int32_t rotation = sensor_->properties().get(properties::Rotation);
> -       ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> -       ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> -       unicam_[Unicam::Image].dev()->setControls(&ctrls);
> +       if (result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE) {
> +               /*
> +                * Setup our staggered control writer with the sensor default
> +                * gain and exposure delays.
> +                */
> +               if (!staggeredCtrl_) {
> +                       staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> +                                           { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
> +                                             { V4L2_CID_EXPOSURE, result.data[1] } });
> +                       sensorMetadata_ = result.data[2];
> +               }
> +
> +               /* Configure the H/V flip controls based on the sensor rotation. */
> +               ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> +               int32_t rotation = sensor_->properties().get(properties::Rotation);
> +               ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> +               ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> +               unicam_[Unicam::Image].dev()->setControls(&ctrls);

This will work fine for now, given we only have sensors either at 0
degrees or 180 degrees.  I wonder if we should make this more generic
and allow further transformations for other orientations?  Something
for us to consider separately.

> +       }
> +
> +       if (result.operation & RPI_IPA_CONFIG_SENSOR) {
> +               const ControlList &ctrls = result.controls[0];
> +               if (!staggeredCtrl_.set(ctrls))
> +                       LOG(RPI, Error) << "V4L2 staggered set failed";
> +       }
>
>         return 0;
>  }
> @@ -1191,26 +1212,12 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
>          */
>         switch (action.operation) {
>         case RPI_IPA_ACTION_V4L2_SET_STAGGERED: {
> -               ControlList controls = action.controls[0];
> +               const ControlList &controls = action.controls[0];
>                 if (!staggeredCtrl_.set(controls))
>                         LOG(RPI, Error) << "V4L2 staggered set failed";
>                 return;
>         }
>
> -       case RPI_IPA_ACTION_SET_SENSOR_CONFIG: {
> -               /*
> -                * Setup our staggered control writer with the sensor default
> -                * gain and exposure delays.
> -                */
> -               if (!staggeredCtrl_) {
> -                       staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> -                                           { { V4L2_CID_ANALOGUE_GAIN, action.data[0] },
> -                                             { V4L2_CID_EXPOSURE, action.data[1] } });
> -                       sensorMetadata_ = action.data[2];
> -               }
> -               return;
> -       }
> -
>         case RPI_IPA_ACTION_V4L2_SET_ISP: {
>                 ControlList controls = action.controls[0];
>                 isp_[Isp::Input].dev()->setControls(&controls);
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart July 8, 2020, 8:42 p.m. UTC | #3 
Hi Naush,

On Wed, Jul 08, 2020 at 12:08:27PM +0100, Naushir Patuck wrote:
> Hi Laurent,
> 
> Thank you for the patch.  Apart from the one note below,
> 
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> 
> On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > The Raspberry Pi IPA uses the custom RPI_IPA_ACTION_SET_SENSOR_CONFIG
> > frame action to send the sensor staggered write configuration to the
> > pipeline handler when the IPA is configured. Replace this ad-hoc
> > mechanism by passing the corresponding data back from the IPA to the
> > pipeline handler through the configure() response. This allows
> > synchronous handling of the response on the pipeline handler side.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Restore RPI_IPA_ACTION_V4L2_SET_ISP handling in stop state
> > - Drop sensor orientation handling based on IPA result
> > ---
> >  include/libcamera/ipa/raspberrypi.h           |  3 +-
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 42 ++++++++-------
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 53 +++++++++++--------
> >  3 files changed, 56 insertions(+), 42 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> > index 46ce7c286b20..a49377695f22 100644
> > --- a/include/libcamera/ipa/raspberrypi.h
> > +++ b/include/libcamera/ipa/raspberrypi.h
> > @@ -12,6 +12,8 @@
> >
> >  enum RPiConfigParameters {
> >         RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
> > +       RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> > +       RPI_IPA_CONFIG_SENSOR = (1 << 2),
> >  };
> >
> >  enum RPiOperations {
> > @@ -20,7 +22,6 @@ enum RPiOperations {
> >         RPI_IPA_ACTION_STATS_METADATA_COMPLETE,
> >         RPI_IPA_ACTION_RUN_ISP,
> >         RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME,
> > -       RPI_IPA_ACTION_SET_SENSOR_CONFIG,
> >         RPI_IPA_ACTION_EMBEDDED_COMPLETE,
> >         RPI_IPA_EVENT_SIGNAL_STAT_READY,
> >         RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 0e39a1137cd0..378ea309fa81 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -93,7 +93,7 @@ private:
> >         void reportMetadata();
> >         bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);
> >         void processStats(unsigned int bufferId);
> > -       void applyAGC(const struct AgcStatus *agcStatus);
> > +       void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> >         void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
> >         void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);
> >         void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);
> > @@ -195,6 +195,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >         if (entityControls.empty())
> >                 return;
> >
> > +       result->operation = 0;
> > +
> >         unicam_ctrls_ = entityControls.at(0);
> >         isp_ctrls_ = entityControls.at(1);
> >         /* Setup a metadata ControlList to output metadata. */
> > @@ -216,13 +218,11 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >                 helper_->GetDelays(exposureDelay, gainDelay);
> >                 sensorMetadata = helper_->SensorEmbeddedDataPresent();
> >
> > -               IPAOperationData op;
> > -               op.operation = RPI_IPA_ACTION_SET_SENSOR_CONFIG;
> > -               op.data.push_back(gainDelay);
> > -               op.data.push_back(exposureDelay);
> > -               op.data.push_back(sensorMetadata);
> > +               result->data.push_back(gainDelay);
> > +               result->data.push_back(exposureDelay);
> > +               result->data.push_back(sensorMetadata);
> >
> > -               queueFrameAction.emit(0, op);
> > +               result->operation |= RPI_IPA_CONFIG_STAGGERED_WRITE;
> >         }
> >
> >         /* Re-assemble camera mode using the sensor info. */
> > @@ -267,8 +267,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> >
> >         /* SwitchMode may supply updated exposure/gain values to use. */
> >         metadata.Get("agc.status", agcStatus);
> > -       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)
> > -               applyAGC(&agcStatus);
> > +       if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> > +               ControlList ctrls(unicam_ctrls_);
> > +               applyAGC(&agcStatus, ctrls);
> > +               result->controls.push_back(ctrls);
> > +
> > +               result->operation |= RPI_IPA_CONFIG_SENSOR;
> > +       }
> >
> >         lastMode_ = mode_;
> >
> > @@ -775,8 +780,15 @@ void IPARPi::processStats(unsigned int bufferId)
> >         controller_.Process(statistics, &rpiMetadata_);
> >
> >         struct AgcStatus agcStatus;
> > -       if (rpiMetadata_.Get("agc.status", agcStatus) == 0)
> > -               applyAGC(&agcStatus);
> > +       if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
> > +               ControlList ctrls(unicam_ctrls_);
> > +               applyAGC(&agcStatus, ctrls);
> > +
> > +               IPAOperationData op;
> > +               op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
> > +               op.controls.push_back(ctrls);
> > +               queueFrameAction.emit(0, op);
> > +       }
> >  }
> >
> >  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> > @@ -802,11 +814,8 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> >                   static_cast<int32_t>(awbStatus->gain_b * 1000));
> >  }
> >
> > -void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
> > +void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> >  {
> > -       IPAOperationData op;
> > -       op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
> > -
> >         int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
> >         int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
> >
> > @@ -825,11 +834,8 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
> >                            << agcStatus->analogue_gain << " (Gain Code: "
> >                            << gain_code << ")";
> >
> > -       ControlList ctrls(unicam_ctrls_);
> >         ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
> >         ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> > -       op.controls.push_back(ctrls);
> > -       queueFrameAction.emit(0, op);
> >  }
> >
> >  void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 57602349cab2..9babf9f4a19c 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -819,7 +819,7 @@ int PipelineHandlerRPi::start(Camera *camera)
> >         /*
> >          * Write the last set of gain and exposure values to the camera before
> >          * starting. First check that the staggered ctrl has been initialised
> > -        * by the IPA action.
> > +        * by configure().
> >          */
> >         ASSERT(data->staggeredCtrl_);
> >         data->staggeredCtrl_.reset();
> > @@ -1170,15 +1170,36 @@ int RPiCameraData::configureIPA()
> >         }
> >
> >         /* Ready the IPA - it must know about the sensor resolution. */
> > +       IPAOperationData result;
> > +
> >         ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> > -                       nullptr);
> > +                       &result);
> >
> > -       /* Configure the H/V flip controls based on the sensor rotation. */
> > -       ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> > -       int32_t rotation = sensor_->properties().get(properties::Rotation);
> > -       ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> > -       ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> > -       unicam_[Unicam::Image].dev()->setControls(&ctrls);
> > +       if (result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE) {
> > +               /*
> > +                * Setup our staggered control writer with the sensor default
> > +                * gain and exposure delays.
> > +                */
> > +               if (!staggeredCtrl_) {
> > +                       staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> > +                                           { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
> > +                                             { V4L2_CID_EXPOSURE, result.data[1] } });
> > +                       sensorMetadata_ = result.data[2];
> > +               }
> > +
> > +               /* Configure the H/V flip controls based on the sensor rotation. */
> > +               ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> > +               int32_t rotation = sensor_->properties().get(properties::Rotation);
> > +               ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> > +               ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> > +               unicam_[Unicam::Image].dev()->setControls(&ctrls);
> 
> This will work fine for now, given we only have sensors either at 0
> degrees or 180 degrees.  I wonder if we should make this more generic
> and allow further transformations for other orientations?  Something
> for us to consider separately.

I think we should. David has started a separate mail thread on that
topic. I think it should build on top of this series.

> > +       }
> > +
> > +       if (result.operation & RPI_IPA_CONFIG_SENSOR) {
> > +               const ControlList &ctrls = result.controls[0];
> > +               if (!staggeredCtrl_.set(ctrls))
> > +                       LOG(RPI, Error) << "V4L2 staggered set failed";
> > +       }
> >
> >         return 0;
> >  }
> > @@ -1191,26 +1212,12 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> >          */
> >         switch (action.operation) {
> >         case RPI_IPA_ACTION_V4L2_SET_STAGGERED: {
> > -               ControlList controls = action.controls[0];
> > +               const ControlList &controls = action.controls[0];
> >                 if (!staggeredCtrl_.set(controls))
> >                         LOG(RPI, Error) << "V4L2 staggered set failed";
> >                 return;
> >         }
> >
> > -       case RPI_IPA_ACTION_SET_SENSOR_CONFIG: {
> > -               /*
> > -                * Setup our staggered control writer with the sensor default
> > -                * gain and exposure delays.
> > -                */
> > -               if (!staggeredCtrl_) {
> > -                       staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> > -                                           { { V4L2_CID_ANALOGUE_GAIN, action.data[0] },
> > -                                             { V4L2_CID_EXPOSURE, action.data[1] } });
> > -                       sensorMetadata_ = action.data[2];
> > -               }
> > -               return;
> > -       }
> > -
> >         case RPI_IPA_ACTION_V4L2_SET_ISP: {
> >                 ControlList controls = action.controls[0];
> >                 isp_[Isp::Input].dev()->setControls(&controls);

Patch
diff mbox series 

diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
index 46ce7c286b20..a49377695f22 100644
--- a/include/libcamera/ipa/raspberrypi.h
+++ b/include/libcamera/ipa/raspberrypi.h
@@ -12,6 +12,8 @@ 
 
 enum RPiConfigParameters {
 	RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
+	RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
+	RPI_IPA_CONFIG_SENSOR = (1 << 2),
 };
 
 enum RPiOperations {
@@ -20,7 +22,6 @@  enum RPiOperations {
 	RPI_IPA_ACTION_STATS_METADATA_COMPLETE,
 	RPI_IPA_ACTION_RUN_ISP,
 	RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME,
-	RPI_IPA_ACTION_SET_SENSOR_CONFIG,
 	RPI_IPA_ACTION_EMBEDDED_COMPLETE,
 	RPI_IPA_EVENT_SIGNAL_STAT_READY,
 	RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 0e39a1137cd0..378ea309fa81 100644
--- a/src/ipa/raspberrypi/raspberrypi.cpp
+++ b/src/ipa/raspberrypi/raspberrypi.cpp
@@ -93,7 +93,7 @@  private:
 	void reportMetadata();
 	bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);
 	void processStats(unsigned int bufferId);
-	void applyAGC(const struct AgcStatus *agcStatus);
+	void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
 	void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
 	void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);
 	void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);
@@ -195,6 +195,8 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 	if (entityControls.empty())
 		return;
 
+	result->operation = 0;
+
 	unicam_ctrls_ = entityControls.at(0);
 	isp_ctrls_ = entityControls.at(1);
 	/* Setup a metadata ControlList to output metadata. */
@@ -216,13 +218,11 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 		helper_->GetDelays(exposureDelay, gainDelay);
 		sensorMetadata = helper_->SensorEmbeddedDataPresent();
 
-		IPAOperationData op;
-		op.operation = RPI_IPA_ACTION_SET_SENSOR_CONFIG;
-		op.data.push_back(gainDelay);
-		op.data.push_back(exposureDelay);
-		op.data.push_back(sensorMetadata);
+		result->data.push_back(gainDelay);
+		result->data.push_back(exposureDelay);
+		result->data.push_back(sensorMetadata);
 
-		queueFrameAction.emit(0, op);
+		result->operation |= RPI_IPA_CONFIG_STAGGERED_WRITE;
 	}
 
 	/* Re-assemble camera mode using the sensor info. */
@@ -267,8 +267,13 @@  void IPARPi::configure(const CameraSensorInfo &sensorInfo,
 
 	/* SwitchMode may supply updated exposure/gain values to use. */
 	metadata.Get("agc.status", agcStatus);
-	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0)
-		applyAGC(&agcStatus);
+	if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
+		ControlList ctrls(unicam_ctrls_);
+		applyAGC(&agcStatus, ctrls);
+		result->controls.push_back(ctrls);
+
+		result->operation |= RPI_IPA_CONFIG_SENSOR;
+	}
 
 	lastMode_ = mode_;
 
@@ -775,8 +780,15 @@  void IPARPi::processStats(unsigned int bufferId)
 	controller_.Process(statistics, &rpiMetadata_);
 
 	struct AgcStatus agcStatus;
-	if (rpiMetadata_.Get("agc.status", agcStatus) == 0)
-		applyAGC(&agcStatus);
+	if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
+		ControlList ctrls(unicam_ctrls_);
+		applyAGC(&agcStatus, ctrls);
+
+		IPAOperationData op;
+		op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
+		op.controls.push_back(ctrls);
+		queueFrameAction.emit(0, op);
+	}
 }
 
 void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
@@ -802,11 +814,8 @@  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
 		  static_cast<int32_t>(awbStatus->gain_b * 1000));
 }
 
-void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
+void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
 {
-	IPAOperationData op;
-	op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
-
 	int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
 	int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
 
@@ -825,11 +834,8 @@  void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
 			   << agcStatus->analogue_gain << " (Gain Code: "
 			   << gain_code << ")";
 
-	ControlList ctrls(unicam_ctrls_);
 	ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
 	ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
-	op.controls.push_back(ctrls);
-	queueFrameAction.emit(0, op);
 }
 
 void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index 57602349cab2..9babf9f4a19c 100644
--- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
+++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
@@ -819,7 +819,7 @@  int PipelineHandlerRPi::start(Camera *camera)
 	/*
 	 * Write the last set of gain and exposure values to the camera before
 	 * starting. First check that the staggered ctrl has been initialised
-	 * by the IPA action.
+	 * by configure().
 	 */
 	ASSERT(data->staggeredCtrl_);
 	data->staggeredCtrl_.reset();
@@ -1170,15 +1170,36 @@  int RPiCameraData::configureIPA()
 	}
 
 	/* Ready the IPA - it must know about the sensor resolution. */
+	IPAOperationData result;
+
 	ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
-			nullptr);
+			&result);
 
-	/* Configure the H/V flip controls based on the sensor rotation. */
-	ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
-	int32_t rotation = sensor_->properties().get(properties::Rotation);
-	ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
-	ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
-	unicam_[Unicam::Image].dev()->setControls(&ctrls);
+	if (result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE) {
+		/*
+		 * Setup our staggered control writer with the sensor default
+		 * gain and exposure delays.
+		 */
+		if (!staggeredCtrl_) {
+			staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
+					    { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
+					      { V4L2_CID_EXPOSURE, result.data[1] } });
+			sensorMetadata_ = result.data[2];
+		}
+
+		/* Configure the H/V flip controls based on the sensor rotation. */
+		ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
+		int32_t rotation = sensor_->properties().get(properties::Rotation);
+		ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
+		ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
+		unicam_[Unicam::Image].dev()->setControls(&ctrls);
+	}
+
+	if (result.operation & RPI_IPA_CONFIG_SENSOR) {
+		const ControlList &ctrls = result.controls[0];
+		if (!staggeredCtrl_.set(ctrls))
+			LOG(RPI, Error) << "V4L2 staggered set failed";
+	}
 
 	return 0;
 }
@@ -1191,26 +1212,12 @@  void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
 	 */
 	switch (action.operation) {
 	case RPI_IPA_ACTION_V4L2_SET_STAGGERED: {
-		ControlList controls = action.controls[0];
+		const ControlList &controls = action.controls[0];
 		if (!staggeredCtrl_.set(controls))
 			LOG(RPI, Error) << "V4L2 staggered set failed";
 		return;
 	}
 
-	case RPI_IPA_ACTION_SET_SENSOR_CONFIG: {
-		/*
-		 * Setup our staggered control writer with the sensor default
-		 * gain and exposure delays.
-		 */
-		if (!staggeredCtrl_) {
-			staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
-					    { { V4L2_CID_ANALOGUE_GAIN, action.data[0] },
-					      { V4L2_CID_EXPOSURE, action.data[1] } });
-			sensorMetadata_ = action.data[2];
-		}
-		return;
-	}
-
 	case RPI_IPA_ACTION_V4L2_SET_ISP: {
 		ControlList controls = action.controls[0];
 		isp_[Isp::Input].dev()->setControls(&controls);