[libcamera-devel,v3,11/23] libcamera: camera_sensor: Expose DelayedControls interface

16470 diff mbox series

Now that the CameraSensor class has an instance of delayed controls
expose its interface for pipeline handlers to use it.

The interface towards DelayedControls still operates on lists of
V4L2 controls.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/internal/camera_sensor.h    |  4 +++
 src/libcamera/camera_sensor/camera_sensor.cpp | 31 +++++++++++++++++++
 2 files changed, 35 insertions(+)

Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:38:50)
> Now that the CameraSensor class has an instance of delayed controls
> expose its interface for pipeline handlers to use it.
> 
> The interface towards DelayedControls still operates on lists of
> V4L2 controls.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h    |  4 +++
>  src/libcamera/camera_sensor/camera_sensor.cpp | 31 +++++++++++++++++++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index bd5aa0dbc27d..a606bc5d1cb5 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -71,6 +71,10 @@ public:
>  
>         CameraLens *focusLens() { return focusLens_.get(); }
>  
> +       void frameStart(uint32_t sequence);
> +       void pushControls(const ControlList &ctrls);
> +       ControlList getControls(uint32_t sequence);
> +
>  protected:
>         std::string logPrefix() const override;
>  
> diff --git a/src/libcamera/camera_sensor/camera_sensor.cpp b/src/libcamera/camera_sensor/camera_sensor.cpp
> index 211c7461f5c6..e1770e8fa130 100644
> --- a/src/libcamera/camera_sensor/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor/camera_sensor.cpp
> @@ -1043,6 +1043,37 @@ void CameraSensor::updateControlInfo()
>         updateControls();
>  }
>  
> +/**
> + * \brief Signal to the camera sensor class that a new frame has start exposing
> + * \param[in] sequence The frame sequence number
> + */
> +void CameraSensor::frameStart(uint32_t sequence)
> +{
> +       /* A new capture session requires to start in a clean state. */
> +       if (sequence == 0)
> +               delayedCtrls_->reset();

We want to have queued up some controls *before* we start the device,
and so here this races with anything that might have been queued up
before the first frame SoF is called.

I don't know 'how much' of a race it is ... but I don't think it's good
to have this reset here. I think it should be an explict reset tied in
elsewhere.


> +
> +       delayedCtrls_->applyControls(sequence);
> +}
> +
> +/**
> + * \brief Push a new set of controls to the CameraSensor
> + * \param[in] ctrls The list of controls to push to the sensor
> + */
> +void CameraSensor::pushControls(const ControlList &ctrls)

I think we're going to want to pass in the sequence number here, and
call it setControls probably. But while this is currently push in the
DelayedControls, keeping the existing implementation during the move is
good I think.



> +{
> +       delayedCtrls_->push(ctrls);
> +}
> +
> +/**
> + * \brief Get the list of controls applied at frame \a sequence
> + * \param[in] sequence The frame sequence number
> + */
> +ControlList CameraSensor::getControls(uint32_t sequence)
> +{
> +       return delayedCtrls_->get(sequence);
> +}
> +
>  /**
>   * \fn CameraSensor::focusLens()
>   * \brief Retrieve the focus lens controller
> -- 
> 2.36.1
>

Hi Kieran,

On Fri, Jul 01, 2022 at 12:13:22AM +0100, Kieran Bingham wrote:
> Quoting Jacopo Mondi via libcamera-devel (2022-06-30 14:38:50)
> > Now that the CameraSensor class has an instance of delayed controls
> > expose its interface for pipeline handlers to use it.
> >
> > The interface towards DelayedControls still operates on lists of
> > V4L2 controls.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/libcamera/internal/camera_sensor.h    |  4 +++
> >  src/libcamera/camera_sensor/camera_sensor.cpp | 31 +++++++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index bd5aa0dbc27d..a606bc5d1cb5 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -71,6 +71,10 @@ public:
> >
> >         CameraLens *focusLens() { return focusLens_.get(); }
> >
> > +       void frameStart(uint32_t sequence);
> > +       void pushControls(const ControlList &ctrls);
> > +       ControlList getControls(uint32_t sequence);
> > +
> >  protected:
> >         std::string logPrefix() const override;
> >
> > diff --git a/src/libcamera/camera_sensor/camera_sensor.cpp b/src/libcamera/camera_sensor/camera_sensor.cpp
> > index 211c7461f5c6..e1770e8fa130 100644
> > --- a/src/libcamera/camera_sensor/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor/camera_sensor.cpp
> > @@ -1043,6 +1043,37 @@ void CameraSensor::updateControlInfo()
> >         updateControls();
> >  }
> >
> > +/**
> > + * \brief Signal to the camera sensor class that a new frame has start exposing
> > + * \param[in] sequence The frame sequence number
> > + */
> > +void CameraSensor::frameStart(uint32_t sequence)
> > +{
> > +       /* A new capture session requires to start in a clean state. */
> > +       if (sequence == 0)
> > +               delayedCtrls_->reset();
>
> We want to have queued up some controls *before* we start the device,
> and so here this races with anything that might have been queued up
> before the first frame SoF is called.

AH! You're right, there could be controls queued up at Camera::start()
that would be overridden by this.

I tried to be smart and avoid having to add a 'start()' function to
CameraSensor, but that won't work well..

Good catch, we need an explicit reset to be issued at
Camera::configure() time.

>
> I don't know 'how much' of a race it is ... but I don't think it's good
> to have this reset here. I think it should be an explict reset tied in
> elsewhere.
>
>
> > +
> > +       delayedCtrls_->applyControls(sequence);
> > +}
> > +
> > +/**
> > + * \brief Push a new set of controls to the CameraSensor
> > + * \param[in] ctrls The list of controls to push to the sensor
> > + */
> > +void CameraSensor::pushControls(const ControlList &ctrls)
>
> I think we're going to want to pass in the sequence number here, and
> call it setControls probably. But while this is currently push in the
> DelayedControls, keeping the existing implementation during the move is
> good I think.
>
>
>
> > +{
> > +       delayedCtrls_->push(ctrls);
> > +}
> > +
> > +/**
> > + * \brief Get the list of controls applied at frame \a sequence
> > + * \param[in] sequence The frame sequence number
> > + */
> > +ControlList CameraSensor::getControls(uint32_t sequence)
> > +{
> > +       return delayedCtrls_->get(sequence);
> > +}
> > +
> >  /**
> >   * \fn CameraSensor::focusLens()
> >   * \brief Retrieve the focus lens controller
> > --
> > 2.36.1
> >