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

[Stanislaw Gruszka]

Hi Laurent,

On Wed, Feb 19, 2025 at 02:42:31PM +0200, Laurent Pinchart wrote:
> 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 ?

Not really sure how to split the patch.

The assertion failure can happen when delayedCtrls_->get(frame)
give us empty control in processStats:

void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
{
	swIsp_->processStats(frame, bufferId,
			     delayedCtrls_->get(frame));
}

Below 2 conditions have to be meet to trigger the issue.

1) we process first 16 frames, otherwise DelayedControls::values_
ControlRingBuffer is filled - no empty controls there

2) there must be error on CSI bus and some frames dropped there,
such index calculated by:

 ControlList DelayedControls::get(uint32_t sequence)
 {
	unsigned int index = std::max<int>(0, sequence - maxDelay_);

points to entry in ControlRingBuffer not filled previously by:

void SimpleCameraData::setSensorControls(const ControlList &sensorControls)
{
	delayedCtrls_->push(sensorControls);

For reference, it was discussed here a little:
https://patchwork.libcamera.org/patch/22210/ 

Assertion is consequence of not handling frame start event correctly.
With the fix, we fill proper entry in ControlRingBuffer, before it's
read later in processStats(), even if the frame number is not in
sequence.

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

Ok.

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

This is for case (below in this patch) when in SimplePipelineHandler::start()

		ret = eventEmitter->setFrameStartEnabled(true);

fails for some reason, even if previous ioctl() call to detect
eventEmitter was successful.

Regards
Stanislaw

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