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

[Kieran Bingham]

Quoting Kieran Bingham (2025-02-18 11:54:19)
> Quoting Stanislaw Gruszka (2025-02-18 09:19:51)
> > 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>
> > 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>
> 
> This still fixes the flicker on X13s
> 
> Tested-by: Kieran Bingham <kieran.bingham at ideasonboard.com> # Lenovo X13s
> 
> But I'm getting segfaults on stopping the camera. I think that's
> unrelated to this patch - and exists on mainline so :

Confirmed,

git revert 9bc8b6a573a63b8c9f5588cdea6da5ae71abd138

and these patches provides stable image and no crashes on
start/stop/qcam-exit.

I know Milan is working on a fix for the above so no /need/ to revert,
we can wait for the fix - but I think it means these two patches are
good to go if I can get another RB tag on patch 1/2.

--
Kieran


> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.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_;
> > 
> >         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()) {
> > +               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_)
> > --
> > 2.43.0
> >