[PATCH v2] pipeline: simple: Use proper device for frame start events

[Laurent Pinchart]

Hi Stanislaw,

Thank you for the patch, and sorry this got out of my radar.

On Wed, Dec 18, 2024 at 03:22:17PM +0100, Stanislaw Gruszka wrote:
> Currently we use frame start event from video capture device to
> apply controls. But the capture device might not generate the events.
> Usually CSI-2 receiver is proper device to subscribe for start
> frame events.
> 
> Without DelayedConntrols:applyControls() is possible that we can get
> call to DelayedControls::get() with frame number that exceed number
> of saved entries and get below assertion failure:
> 
> ../src/libcamera/delayed_controls.cpp:227:
> libcamera::ControlList libcamera::DelayedControls::get(uint32_t):
> Assertion `info.type() != ControlTypeNone' failed
> 
> Assertion failure can happen at the beginning of streaming when
> ControlRingBuffer is not yet filled and there are errors on CSI-2.
> 
> To fix, loop over devices in the pipeline (starting from the last one),
> find one that emits start frame events and connect applyControls()
> to it. Additionally remove direct call to sensor_->setControls() if
> the emitter device was found.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=241
> Co-developed-by: Hans de Goede <hdegoede at redhat.com>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>
> ---
> v1 -> v2:
>  - make eventEmitter_ subdevice part of SimpleCameraData
>  - add debug log when found event emitter device
>  - nullify eventEmitter_ on stop
>  - remove direct sensor_->setControls()
>  - add delayedCtrls_->reset() on start
> 
>  src/libcamera/pipeline/simple/simple.cpp | 39 +++++++++++++++++++++---
>  1 file changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 8ac24e6e..a7594c2c 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -277,6 +277,7 @@ public:
>  	std::list<Entity> entities_;
>  	std::unique_ptr<CameraSensor> sensor_;
>  	V4L2VideoDevice *video_;
> +	V4L2Subdevice *eventEmitter_;
> 
>  	std::vector<Configuration> configs_;
>  	std::map<PixelFormat, std::vector<const Configuration *>> formats_;
> @@ -911,8 +912,11 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>  void SimpleCameraData::setSensorControls(const ControlList &sensorControls)
>  {
>  	delayedCtrls_->push(sensorControls);
> -	ControlList ctrls(sensorControls);
> -	sensor_->setControls(&ctrls);
> +	/* Directly apply controls now if there is no frameStart signal */
> +	if (!eventEmitter_) {
> +		ControlList ctrls(sensorControls);
> +		sensor_->setControls(&ctrls);
> +	}
>  }
> 
>  /* Retrieve all source pads connected to a sink pad through active routes. */
> @@ -1299,8 +1303,6 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  	data->delayedCtrls_ =
>  		std::make_unique<DelayedControls>(data->sensor_->device(),
>  						  params);
> -	data->video_->frameStart.connect(data->delayedCtrls_.get(),
> -					 &DelayedControls::applyControls);
> 
>  	StreamConfiguration inputCfg;
>  	inputCfg.pixelFormat = pipeConfig->captureFormat;
> @@ -1368,6 +1370,28 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> 
>  	video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);
> 
> +	/*
> +	 * Enable frame start event on last device in the pipeline
> +	 * that provides the events.
> +	 */
> +	for (auto it = data->entities_.rbegin(); it != data->entities_.rend(); ++it) {
> +		V4L2Subdevice *sd = subdev(it->entity);
> +		if (!sd)
> +			continue;
> +		if (sd->setFrameStartEnabled(true) < 0)
> +			continue;
> +
> +		LOG(SimplePipeline, Debug)
> +			<< "Using " << it->entity->name() << " frameStart signal";
> +
> +		sd->frameStart.connect(data->delayedCtrls_.get(),
> +				       &DelayedControls::applyControls);
> +		data->eventEmitter_ = sd;
> +		break;
> +	}

Could we do this at init time instead ? V4L2 doesn't offer an API to
list what events are supported by a subdev (that should probably be
added to the kernel, but that's a separate problem), so we need to
subscribe to an event in order to know if it's supported. This could be
implemented as a V4L2Subdevice::supportsEvent() helper function that
would subscribe (and unsubscribe is the subscription is successful).

> +
> +	data->delayedCtrls_->reset();
> +
>  	ret = video->streamOn();
>  	if (ret < 0) {
>  		stop(camera);
> @@ -1400,6 +1424,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>  	SimpleCameraData *data = cameraData(camera);
>  	V4L2VideoDevice *video = data->video_;
> 
> +	if (data->eventEmitter_) {
> +		data->eventEmitter_->setFrameStartEnabled(false);
> +		data->eventEmitter_->frameStart.disconnect(data->delayedCtrls_.get(),
> +							   &DelayedControls::applyControls);
> +		data->eventEmitter_ = NULL;
> +	}
> +
>  	if (data->useConversion_) {
>  		if (data->converter_)
>  			data->converter_->stop();

-- 
Regards,

Laurent Pinchart