[libcamera-devel] [PATCH v3 12/23] libcamera: ipu3: Drop DelayedControls

[Jacopo Mondi]

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