Message ID | 20250403074551.263496-4-stanislaw.gruszka@linux.intel.com |
---|---|
State | Accepted |
Commit | 183bab1643466c5d62d2cb93feea286571c73fbb |
Headers | show |
Series |
|
Related | show |
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@ideasonboard.com> # v6 > Co-developed-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Co-developed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@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@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 >
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. + * + * \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_)