[PATCH v7 3/5] pipeline: simple: Enable frame start events

[Kieran Bingham]

Quoting Stanislaw Gruszka (2025-04-03 08:45:49)
> The simple pipeline handler uses frame start events to apply sensor
> controls through the DelayedControls class. The setSensorControls()
> function applies the controls directly, which would result in controls
> being applied twice, if it wasn't for the fact that the pipeline handler
> forgot to enable the frame start events in the first place. Those two
> issues cancel each other, but cause controls to always be applied
> directly.
> 
> Fix the issue by only applying controls directly in setSensorControls()
> if no frame start event emitter is available, and by enabling the frame
> start events in startDevice() otherwise. Disable them in stopDevice()
> for symmetry.
> 
> Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com> # v6
> Co-developed-by: Hans de Goede <hdegoede at redhat.com>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> Co-developed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka at linux.intel.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 49 +++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 6 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 06e805d89caa..c97904076b63 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -327,6 +327,7 @@ public:
>         std::list<Entity> entities_;
>         std::unique_ptr<CameraSensor> sensor_;
>         V4L2VideoDevice *video_;
> +       V4L2Subdevice *frameStartEmitter_;
>  
>         std::vector<Configuration> configs_;
>         std::map<PixelFormat, std::vector<const Configuration *>> formats_;
> @@ -633,6 +634,20 @@ int SimpleCameraData::init()
>  
>         properties_ = sensor_->properties();
>  
> +       /* Find the first subdev that can generate a frame start signal, if any. */
> +       frameStartEmitter_ = nullptr;
> +       for (const Entity &entity : entities_) {
> +               V4L2Subdevice *sd = pipe->subdev(entity.entity);
> +               if (!sd || !sd->supportsFrameStartEvent())
> +                       continue;
> +
> +               LOG(SimplePipeline, Debug)
> +                       << "Using frameStart signal from '"
> +                       << entity.entity->name() << "'";
> +               frameStartEmitter_ = sd;
> +               break;
> +       }
> +
>         return 0;
>  }
>  
> @@ -983,8 +998,18 @@ void SimpleCameraData::metadataReady(uint32_t frame, const ControlList &metadata
>  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.

Smallest nit on whitespace here that I'll fix up when applying.

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>


> +        *
> +        * \todo Applying controls directly not only increases the risk of
> +        * applying them to the wrong frame (or across a frame boundary),
> +        * but it also bypasses delayedCtrls_, creating AGC regulation issues.
> +        * Both problems should be fixed.
> +        */
> +       if (!frameStartEmitter_) {
> +               ControlList ctrls(sensorControls);
> +               sensor_->setControls(&ctrls);
> +       }
>  }
>  
>  /* Retrieve all source pads connected to a sink pad through active routes. */
> @@ -1409,6 +1434,7 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>  {
>         SimpleCameraData *data = cameraData(camera);
>         V4L2VideoDevice *video = data->video_;
> +       V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;
>         int ret;
>  
>         const MediaPad *pad = acquirePipeline(data);
> @@ -1438,8 +1464,15 @@ int SimplePipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlL
>  
>         video->bufferReady.connect(data, &SimpleCameraData::imageBufferReady);
>  
> -       data->video_->frameStart.connect(data->delayedCtrls_.get(),
> -                                        &DelayedControls::applyControls);
> +       if (frameStartEmitter) {
> +               ret = frameStartEmitter->setFrameStartEnabled(true);
> +               if (ret) {
> +                       stop(camera);
> +                       return ret;
> +               }
> +               frameStartEmitter->frameStart.connect(data->delayedCtrls_.get(),
> +                                                     &DelayedControls::applyControls);
> +       }
>  
>         ret = video->streamOn();
>         if (ret < 0) {
> @@ -1472,9 +1505,13 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>  {
>         SimpleCameraData *data = cameraData(camera);
>         V4L2VideoDevice *video = data->video_;
> +       V4L2Subdevice *frameStartEmitter = data->frameStartEmitter_;
>  
> -       data->video_->frameStart.disconnect(data->delayedCtrls_.get(),
> -                                           &DelayedControls::applyControls);
> +       if (frameStartEmitter) {
> +               frameStartEmitter->setFrameStartEnabled(false);
> +               frameStartEmitter->frameStart.disconnect(data->delayedCtrls_.get(),
> +                                                        &DelayedControls::applyControls);
> +       }
>  
>         if (data->useConversion_) {
>                 if (data->converter_)
> -- 
> 2.43.0
>