| Message ID | 20220630133902.321099-13-jacopo@jmondi.org |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series |
|
| Related | show |
Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:38:51) > Now that the CameraSensor class exposes the DelayedControl interface > use the sensor to operate controls. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 85e5f8a70408..a5a35b6585c6 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -26,7 +26,6 @@ > #include "libcamera/internal/camera.h" > #include "libcamera/internal/camera_lens.h" > #include "libcamera/internal/camera_sensor.h" > -#include "libcamera/internal/delayed_controls.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/framebuffer.h" > #include "libcamera/internal/ipa_manager.h" > @@ -74,7 +73,6 @@ public: > bool supportsFlips_; > Transform rotationTransform_; > > - std::unique_ptr<DelayedControls> delayedCtrls_; > IPU3Frames frameInfos_; > > std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_; > @@ -785,8 +783,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis > if (ret) > goto error; > > - data->delayedCtrls_->reset(); > - > /* > * Start the ImgU video devices, buffers will be queued to the > * ImgU output and viewfinder when requests will be queued. > @@ -1136,9 +1132,6 @@ int PipelineHandlerIPU3::registerCameras() > { V4L2_CID_EXPOSURE, { 2, false } }, > }; > params can be removed above too. > - data->delayedCtrls_ = > - std::make_unique<DelayedControls>(cio2->sensor()->device(), > - params); > data->cio2_.frameStart().connect(data.get(), > &IPU3CameraData::frameStart); Does the FrameStart event only come from the cio2_ video device? It seems like something that should/could already be handled directly by the CameraSensor class? (But maybe the sensor subdevs don't expose it as it's an event determined by the CSI2 recievers?) > @@ -1258,9 +1251,11 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id, > const ControlList &sensorControls, > const ControlList &lensControls) > { > - delayedCtrls_->push(sensorControls); > + CameraSensor *sensor = cio2_.sensor(); > + > + sensor->pushControls(sensorControls); > > - CameraLens *focusLens = cio2_.sensor()->focusLens(); > + CameraLens *focusLens = sensor->focusLens(); > if (!focusLens) > return; > > @@ -1375,7 +1370,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > request->metadata().set(controls::SensorTimestamp, > buffer->metadata().timestamp); > > - info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence); > + info->effectiveSensorControls = cio2_.sensor()->getControls(buffer->metadata().sequence); > > if (request->findBuffer(&rawStream_)) > pipe()->completeBuffer(request, buffer); > @@ -1441,7 +1436,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer) > */ > void IPU3CameraData::frameStart(uint32_t sequence) > { > - delayedCtrls_->applyControls(sequence); > + cio2_.sensor()->frameStart(sequence); I wonder if this could be tied into the CameraSensor class directly somehow ... but this is fine I think for now. > > if (processingRequests_.empty()) > return; > -- > 2.36.1 >
Hi Kieran On Thu, Jun 30, 2022 at 10:14:47PM +0100, Kieran Bingham wrote: > Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:38:51) > > Now that the CameraSensor class exposes the DelayedControl interface > > use the sensor to operate controls. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 17 ++++++----------- > > 1 file changed, 6 insertions(+), 11 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 85e5f8a70408..a5a35b6585c6 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -26,7 +26,6 @@ > > #include "libcamera/internal/camera.h" > > #include "libcamera/internal/camera_lens.h" > > #include "libcamera/internal/camera_sensor.h" > > -#include "libcamera/internal/delayed_controls.h" > > #include "libcamera/internal/device_enumerator.h" > > #include "libcamera/internal/framebuffer.h" > > #include "libcamera/internal/ipa_manager.h" > > @@ -74,7 +73,6 @@ public: > > bool supportsFlips_; > > Transform rotationTransform_; > > > > - std::unique_ptr<DelayedControls> delayedCtrls_; > > IPU3Frames frameInfos_; > > > > std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_; > > @@ -785,8 +783,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis > > if (ret) > > goto error; > > > > - data->delayedCtrls_->reset(); > > - > > /* > > * Start the ImgU video devices, buffers will be queued to the > > * ImgU output and viewfinder when requests will be queued. > > @@ -1136,9 +1132,6 @@ int PipelineHandlerIPU3::registerCameras() > > { V4L2_CID_EXPOSURE, { 2, false } }, > > }; > > > > params can be removed above too. > Right! Sorry, missed it. > > > - data->delayedCtrls_ = > > - std::make_unique<DelayedControls>(cio2->sensor()->device(), > > - params); > > data->cio2_.frameStart().connect(data.get(), > > &IPU3CameraData::frameStart); > > Does the FrameStart event only come from the cio2_ video device? As far as I can see the event is generated by the CSI-2 receiver on the SOF interrupt handler. > It seems like something that should/could already be handled directly > by the CameraSensor class? (But maybe the sensor subdevs don't expose it > as it's an event determined by the CSI2 recievers?) > Exactly. > > @@ -1258,9 +1251,11 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id, > > const ControlList &sensorControls, > > const ControlList &lensControls) > > { > > - delayedCtrls_->push(sensorControls); > > + CameraSensor *sensor = cio2_.sensor(); > > + > > + sensor->pushControls(sensorControls); > > > > - CameraLens *focusLens = cio2_.sensor()->focusLens(); > > + CameraLens *focusLens = sensor->focusLens(); > > if (!focusLens) > > return; > > > > @@ -1375,7 +1370,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > > request->metadata().set(controls::SensorTimestamp, > > buffer->metadata().timestamp); > > > > - info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence); > > + info->effectiveSensorControls = cio2_.sensor()->getControls(buffer->metadata().sequence); > > > > if (request->findBuffer(&rawStream_)) > > pipe()->completeBuffer(request, buffer); > > @@ -1441,7 +1436,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer) > > */ > > void IPU3CameraData::frameStart(uint32_t sequence) > > { > > - delayedCtrls_->applyControls(sequence); > > + cio2_.sensor()->frameStart(sequence); > > I wonder if this could be tied into the CameraSensor class directly > somehow ... but this is fine I think for now. As the PH has to do other things upon this event, I think it makes sense to keep it here. The "other things" in this case it's the badly stiched up handling of test patterns (or any other immediate controls for the matter) something we don't have an interface for atm as this series basically just expose delayed controls through CameraSensor. I think in future we shall be able to move handling of this to the CameraSensor class, but for now I think we should keep it the way it is. > > > > > > if (processingRequests_.empty()) > > return; > > -- > > 2.36.1 > >
Quoting Jacopo Mondi (2022-07-01 09:35:53) > Hi Kieran > > On Thu, Jun 30, 2022 at 10:14:47PM +0100, Kieran Bingham wrote: > > Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:38:51) > > > Now that the CameraSensor class exposes the DelayedControl interface > > > use the sensor to operate controls. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 17 ++++++----------- > > > 1 file changed, 6 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index 85e5f8a70408..a5a35b6585c6 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -26,7 +26,6 @@ > > > #include "libcamera/internal/camera.h" > > > #include "libcamera/internal/camera_lens.h" > > > #include "libcamera/internal/camera_sensor.h" > > > -#include "libcamera/internal/delayed_controls.h" > > > #include "libcamera/internal/device_enumerator.h" > > > #include "libcamera/internal/framebuffer.h" > > > #include "libcamera/internal/ipa_manager.h" > > > @@ -74,7 +73,6 @@ public: > > > bool supportsFlips_; > > > Transform rotationTransform_; > > > > > > - std::unique_ptr<DelayedControls> delayedCtrls_; > > > IPU3Frames frameInfos_; > > > > > > std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_; > > > @@ -785,8 +783,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis > > > if (ret) > > > goto error; > > > > > > - data->delayedCtrls_->reset(); > > > - > > > /* > > > * Start the ImgU video devices, buffers will be queued to the > > > * ImgU output and viewfinder when requests will be queued. > > > @@ -1136,9 +1132,6 @@ int PipelineHandlerIPU3::registerCameras() > > > { V4L2_CID_EXPOSURE, { 2, false } }, > > > }; > > > > > > > params can be removed above too. > > > > Right! Sorry, missed it. > > > > > > - data->delayedCtrls_ = > > > - std::make_unique<DelayedControls>(cio2->sensor()->device(), > > > - params); > > > data->cio2_.frameStart().connect(data.get(), > > > &IPU3CameraData::frameStart); > > > > Does the FrameStart event only come from the cio2_ video device? > > As far as I can see the event is generated by the CSI-2 receiver on > the SOF interrupt handler. > > > It seems like something that should/could already be handled directly > > by the CameraSensor class? (But maybe the sensor subdevs don't expose it > > as it's an event determined by the CSI2 recievers?) > > > > Exactly. > > > > @@ -1258,9 +1251,11 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id, > > > const ControlList &sensorControls, > > > const ControlList &lensControls) > > > { > > > - delayedCtrls_->push(sensorControls); > > > + CameraSensor *sensor = cio2_.sensor(); > > > + > > > + sensor->pushControls(sensorControls); > > > > > > - CameraLens *focusLens = cio2_.sensor()->focusLens(); > > > + CameraLens *focusLens = sensor->focusLens(); > > > if (!focusLens) > > > return; > > > > > > @@ -1375,7 +1370,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > > > request->metadata().set(controls::SensorTimestamp, > > > buffer->metadata().timestamp); > > > > > > - info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence); > > > + info->effectiveSensorControls = cio2_.sensor()->getControls(buffer->metadata().sequence); > > > > > > if (request->findBuffer(&rawStream_)) > > > pipe()->completeBuffer(request, buffer); > > > @@ -1441,7 +1436,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer) > > > */ > > > void IPU3CameraData::frameStart(uint32_t sequence) > > > { > > > - delayedCtrls_->applyControls(sequence); > > > + cio2_.sensor()->frameStart(sequence); > > > > I wonder if this could be tied into the CameraSensor class directly > > somehow ... but this is fine I think for now. > > As the PH has to do other things upon this event, I think it makes > sense to keep it here. > > The "other things" in this case it's the badly stiched up handling of > test patterns (or any other immediate controls for the matter) > something we don't have an interface for atm as this series basically > just expose delayed controls through CameraSensor. > > I think in future we shall be able to move handling of this to the > CameraSensor class, but for now I think we should keep it the way it > is. Yes, I think that was one of my pain points when we pulled in the test pattern controls in the first place, but the great thing about this series is that I can see a route to cleaning that up! \o/ So - perhaps when the test patterns are cleaned up would be an opportunity to see if the frameStart events can be tied directly to the CameraSensor, and maybe it doesn't need to have the PH involvement. But I'm not worried for now. -- Kieran > > > > > > > > > > > if (processingRequests_.empty()) > > > return; > > > -- > > > 2.36.1 > > >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 85e5f8a70408..a5a35b6585c6 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -26,7 +26,6 @@ #include "libcamera/internal/camera.h" #include "libcamera/internal/camera_lens.h" #include "libcamera/internal/camera_sensor.h" -#include "libcamera/internal/delayed_controls.h" #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/framebuffer.h" #include "libcamera/internal/ipa_manager.h" @@ -74,7 +73,6 @@ public: bool supportsFlips_; Transform rotationTransform_; - std::unique_ptr<DelayedControls> delayedCtrls_; IPU3Frames frameInfos_; std::unique_ptr<ipa::ipu3::IPAProxyIPU3> ipa_; @@ -785,8 +783,6 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] const ControlLis if (ret) goto error; - data->delayedCtrls_->reset(); - /* * Start the ImgU video devices, buffers will be queued to the * ImgU output and viewfinder when requests will be queued. @@ -1136,9 +1132,6 @@ int PipelineHandlerIPU3::registerCameras() { V4L2_CID_EXPOSURE, { 2, false } }, }; - data->delayedCtrls_ = - std::make_unique<DelayedControls>(cio2->sensor()->device(), - params); data->cio2_.frameStart().connect(data.get(), &IPU3CameraData::frameStart); @@ -1258,9 +1251,11 @@ void IPU3CameraData::setSensorControls([[maybe_unused]] unsigned int id, const ControlList &sensorControls, const ControlList &lensControls) { - delayedCtrls_->push(sensorControls); + CameraSensor *sensor = cio2_.sensor(); + + sensor->pushControls(sensorControls); - CameraLens *focusLens = cio2_.sensor()->focusLens(); + CameraLens *focusLens = sensor->focusLens(); if (!focusLens) return; @@ -1375,7 +1370,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) request->metadata().set(controls::SensorTimestamp, buffer->metadata().timestamp); - info->effectiveSensorControls = delayedCtrls_->get(buffer->metadata().sequence); + info->effectiveSensorControls = cio2_.sensor()->getControls(buffer->metadata().sequence); if (request->findBuffer(&rawStream_)) pipe()->completeBuffer(request, buffer); @@ -1441,7 +1436,7 @@ void IPU3CameraData::statBufferReady(FrameBuffer *buffer) */ void IPU3CameraData::frameStart(uint32_t sequence) { - delayedCtrls_->applyControls(sequence); + cio2_.sensor()->frameStart(sequence); if (processingRequests_.empty()) return;
Now that the CameraSensor class exposes the DelayedControl interface use the sensor to operate controls. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)