[PATCH v3 2/2] pipeline: simple: Use proper device for frame start events

[Laurent Pinchart]

Hi Stanislaw,

Thank you for the patch.

On Tue, Feb 18, 2025 at 10:19:51AM +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

s/DelayedConntrols:/DelayedControls::/

> 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.

It seems that this patch fixes two issues. One of them is using the
proper device to receive frame start events, the other one is related to
this assertion failure. I'm not sure to see how this patch fixes the
second issue. Could you split it in two, and maybe provide a bit more
information about the fix for the assertion failure ?

> 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>
> Reviewed-by: Hans de Goede <hdegoede at redhat.com> # v2
> Tested-by: Hans de Goede <hdegoede at redhat.com> # Lenovo X1 Yoga IPU6 + ov2740 # v2
> Reviewed-by: Milan Zamazal <mzamazal at redhat.com> # v2
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 40 +++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 6e039bf3..ad2167dc 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_;

Maybe frameStartEmitter_ to make it move explicit ?

> 
>  	std::vector<Configuration> configs_;
>  	std::map<PixelFormat, std::vector<const Configuration *>> formats_;
> @@ -579,6 +580,19 @@ int SimpleCameraData::init()
> 
>  	properties_ = sensor_->properties();
> 
> +	eventEmitter_ = nullptr;
> +	for (auto it = entities_.rbegin(); it != entities_.rend(); ++it) {
> +		V4L2Subdevice *sd = pipe->subdev(it->entity);
> +		if (!sd || !sd->supportsFrameStartEvent())
> +			continue;
> +
> +		LOG(SimplePipeline, Debug)
> +			<< "Using " << it->entity->name() << " frameStart signal";
> +
> +		eventEmitter_ = sd;
> +		break;
> +	}
> +
>  	return 0;
>  }
> 
> @@ -897,8 +911,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_ || !eventEmitter_->frameStartEnabled()) {

What's the use case for the second check, when do we have a non-null
eventEmitter_ that doesn't emit frame start events ?

> +		ControlList ctrls(sensorControls);
> +		sensor_->setControls(&ctrls);
> +	}
>  }
> 
>  /* Retrieve all source pads connected to a sink pad through active routes. */
> @@ -1285,8 +1302,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;
> @@ -1325,6 +1340,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>  {
>  	SimpleCameraData *data = cameraData(camera);
>  	V4L2VideoDevice *video = data->video_;
> +	V4L2Subdevice *eventEmitter = data->eventEmitter_;
>  	int ret;
> 
>  	const MediaPad *pad = acquirePipeline(data);
> @@ -1354,6 +1370,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
> 
>  	video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);
> 
> +	if (eventEmitter) {
> +		ret = eventEmitter->setFrameStartEnabled(true);
> +		if (ret == 0)
> +			eventEmitter->frameStart.connect(data->delayedCtrls_.get(),
> +							 &DelayedControls::applyControls);
> +	}
> +
> +	data->delayedCtrls_->reset();
> +
>  	ret = video->streamOn();
>  	if (ret < 0) {
>  		stop(camera);
> @@ -1385,6 +1410,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>  {
>  	SimpleCameraData *data = cameraData(camera);
>  	V4L2VideoDevice *video = data->video_;
> +	V4L2Subdevice *eventEmitter = data->eventEmitter_;
> +
> +	if (eventEmitter) {
> +		eventEmitter->setFrameStartEnabled(false);
> +		eventEmitter->frameStart.disconnect(data->delayedCtrls_.get(),
> +						    &DelayedControls::applyControls);
> +	}
> 
>  	if (data->useConversion_) {
>  		if (data->converter_)

-- 
Regards,

Laurent Pinchart