[libcamera-devel] [PATCH v2 4/4] pipeline: raspberrypi: Add notion of immediate write to StaggeredCtrl

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Sun, Jan 24, 2021 at 02:05:06PM +0000, Naushir Patuck wrote:
> If an exposure time change adjusts the vblanking limits, and we write
> both VBLANK and EXPOSURE controls as a set, the latter may fail if the
> value is outside of the limits calculated by the old VBLANK value. This
> is a limitation in V4L2 and cannot be fixed by writing VBLANK before
> EXPOSURE.
> 
> The workaround here is to have the StaggeredCtrl mark the
> VBLANK control as "immediate write", which then write VBLANK separately
> from (and ahead of) any other controls. This way, the sensor driver will
> update the EXPOSURE control with new limits before the new values is
> presented, and will thus be seen as valid.
> 
> StaggeredCtrl is due to be deprecated and replaced by DelayedCtrl, so
> this change serves more a working proof-of-concept on the workaround,
> and not much care has been taken to provide a nice new API for applying
> this immediate write flag to the control. A similar workaround must be
> available to DelayedCtrl eventually.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/ipa/raspberrypi/raspberrypi.cpp           |  5 ++-
>  .../pipeline/raspberrypi/raspberrypi.cpp      | 11 ++++--
>  .../pipeline/raspberrypi/staggered_ctrl.cpp   | 39 ++++++++++++++-----
>  .../pipeline/raspberrypi/staggered_ctrl.h     |  3 +-
>  4 files changed, 43 insertions(+), 15 deletions(-)
> 
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 75c9e404dcc1..fefca32ce187 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -1040,8 +1040,9 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
>  
>  	/*
>  	 * Due to the behavior of V4L2, the current value of VBLANK could clip the
> -	 * exposure time without us knowing. The next time though this function should
> -	 * clip exposure correctly.
> +	 * exposure time without us knowing. We get around this by ensuring the
> +	 * staggered write component submits VBLANK separately from, and before the
> +	 * EXPOSURE control.
>  	 */
>  	ctrls.set(V4L2_CID_VBLANK, vblanking);
>  	ctrls.set(V4L2_CID_EXPOSURE, exposureLines);
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 524cc960dd37..1485999ad2a0 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1229,12 +1229,17 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config)
>  		/*
>  		 * Setup our staggered control writer with the sensor default
>  		 * gain and exposure delays.
> +		 *
> +		 * VBLANK must be flagged as "immediate write" to allow it to
> +		 * be set immediately instead of being batched with all other
> +		 * controls. This is needed so that any update to the EXPOSURE
> +		 * control will be validated based on the new VBLANK control value.
>  		 */
>  		if (!staggeredCtrl_) {
>  			staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> -					    { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] },
> -					      { V4L2_CID_EXPOSURE, result.data[resultIdx++] },
> -					      { V4L2_CID_VBLANK, result.data[resultIdx++] } });
> +					    { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++], false },
> +					      { V4L2_CID_EXPOSURE, result.data[resultIdx++], false },
> +					      { V4L2_CID_VBLANK, result.data[resultIdx++], true } });
>  			sensorMetadata_ = result.data[resultIdx++];
>  		}
>  	}
> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> index 62605c0fceee..07f8c95d4f2c 100644
> --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp
> @@ -22,21 +22,29 @@ LOG_DEFINE_CATEGORY(RPI_S_W)
>  namespace RPi {
>  
>  void StaggeredCtrl::init(V4L2VideoDevice *dev,
> -	  std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList)
> +			 std::initializer_list<std::tuple<uint32_t, uint8_t, bool>> delayList)

It could make sense to define a struct type to group the three
parameters.

>  {
>  	std::lock_guard<std::mutex> lock(lock_);
>  
>  	dev_ = dev;
> -	delay_ = delayList;
> +	delay_.clear();
>  	ctrl_.clear();
>  
> -	/* Find the largest delay across all controls. */
>  	maxDelay_ = 0;
> -	for (auto const &p : delay_) {
> +	for (auto const &c : delayList) {
> +		uint32_t id = std::get<0>(c);
> +		uint8_t delay = std::get<1>(c);
> +		bool immediateWrite = std::get<2>(c);
> +
> +		delay_[id] = delay;
> +		immediateWrite_[id] = immediateWrite;

"immediate write" isn't a great name, as the control isn't written
immediately, it's still delayed.

> +
>  		LOG(RPI_S_W, Info) << "Init ctrl "
> -				   << utils::hex(p.first) << " with delay "
> -				   << static_cast<int>(p.second);
> -		maxDelay_ = std::max(maxDelay_, p.second);
> +				   << utils::hex(id) << " with delay "
> +				   << static_cast<int>(delay);
> +
> +		/* Find the largest delay across all controls. */
> +		maxDelay_ = std::max(maxDelay_, delay);
>  	}
>  
>  	init_ = true;
> @@ -121,8 +129,21 @@ int StaggeredCtrl::write()
>  		int index = std::max<int>(0, setCount_ - delayDiff);
>  
>  		if (p.second[index].updated) {
> -			/* We need to write this value out. */
> -			controls.set(p.first, p.second[index].value);
> +			if (immediateWrite_[p.first]) {
> +				/*
> +				 * This control must be written now, it could
> +				 * affect validity of the other controls.
> +				 */
> +				ControlList immediate(dev_->controls());
> +				immediate.set(p.first, p.second[index].value);
> +				dev_->setControls(&immediate);
> +			} else {
> +				/*
> +				 * Batch up the list of controls and write them
> +				 * at the end of the function.
> +				 */
> +				controls.set(p.first, p.second[index].value);
> +			}
>  			p.second[index].updated = false;
>  			LOG(RPI_S_W, Debug) << "Writing ctrl "
>  					    << utils::hex(p.first) << " to "
> diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h
> index 382fa31a6853..7c920c3a13c7 100644
> --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h
> +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h
> @@ -34,7 +34,7 @@ public:
>  	}
>  
>  	void init(V4L2VideoDevice *dev,
> -		  std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList);
> +		  std::initializer_list<std::tuple<uint32_t, uint8_t, bool>> delayList);
>  	void reset();
>  
>  	void get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t offset = 0);
> @@ -85,6 +85,7 @@ private:
>  	uint8_t maxDelay_;
>  	V4L2VideoDevice *dev_;
>  	std::unordered_map<uint32_t, uint8_t> delay_;
> +	std::unordered_map<uint32_t, bool> immediateWrite_;

Should we store both the delay and the immediate write flag in a single
map ?

Otherwise this looks fine. We could possibly solve these issues as part
of the DelayedControls work. I'll let Niklas discuss his preference.

>  	std::unordered_map<uint32_t, CircularArray> ctrl_;
>  	std::mutex lock_;
>  };

-- 
Regards,

Laurent Pinchart