[PATCH v4 1/2] libcamera: camera_sensor_properties: Add sensor control delays

[Kieran Bingham]

Quoting Jacopo Mondi (2024-11-19 16:50:43)
> Hi Dan
> 
> On Tue, Nov 19, 2024 at 04:20:16PM +0000, Dan Scally wrote:
> > Hi Jacopo - thank you for the review
> >
> > On 19/11/2024 15:57, Jacopo Mondi wrote:
> > > Hi Dan
> > >
> > > On Tue, Nov 19, 2024 at 01:03:00PM +0000, Daniel Scally wrote:
> > > > Add properties covering the sensor control application delays to both
> > > > the static CameraSensorProperties definitions. The values used are the
> > > > defaults that're in use across the library, with deviations from that
> > > This probably needs to be updated
> >
> > Indeed
> >
> > >
> > > > taken from Raspberry Pi's CamHelper class definitions.
> > > >
> > > > Signed-off-by: Daniel Scally <dan.scally at ideasonboard.com>
> > > > ---
> > > > Changes in v4:
> > > >
> > > >   - getSensorDelays() renamed to sensorDelays()
> > > >   - sensorDelays() returns a reference to the SensorDelays struct rather
> > > >     than each of the values.
> > > >   - Reworded the comments to make the default values seem less acceptable
> > > >   - Added delay values for AR0144
> > > >
> > > > Changes in v3:
> > > >
> > > >   - Rebased on top of the CameraSensorFactory introduction
> > > >   - Some rephrasing
> > > >   - Defined the sensorDelays member as empty where Raspberry Pi didn't
> > > >     have any specific values. Check for the empty struct in
> > > >     getSensorDelays() and return the defaults from there with a warning.
> > > >
> > > > Changes in v2:
> > > >
> > > >   - Rather than adding the delays to the properties ControlList, added a
> > > >     new function in CameraSensor that allows PipelineHandlers to retreive
> > > >     the delay values.
> > > >
> > > >   include/libcamera/internal/camera_sensor.h    |   2 +
> > > >   .../internal/camera_sensor_properties.h       |  10 ++
> > > >   src/libcamera/sensor/camera_sensor.cpp        |  12 ++
> > > >   src/libcamera/sensor/camera_sensor_legacy.cpp |  18 +++
> > > >   .../sensor/camera_sensor_properties.cpp       | 121 ++++++++++++++++++
> > > >   5 files changed, 163 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > index 8aafd82e..bbdb83a1 100644
> > > > --- a/include/libcamera/internal/camera_sensor.h
> > > > +++ b/include/libcamera/internal/camera_sensor.h
> > > > @@ -20,6 +20,7 @@
> > > >   #include <libcamera/orientation.h>
> > > >   #include <libcamera/transform.h>
> > > >
> > > > +#include "libcamera/internal/camera_sensor_properties.h"
> > > >   #include "libcamera/internal/bayer_format.h"
> > > >   #include "libcamera/internal/v4l2_subdevice.h"
> > > >
> > > > @@ -73,6 +74,7 @@ public:
> > > >           virtual const std::vector<controls::draft::TestPatternModeEnum> &
> > > >           testPatternModes() const = 0;
> > > >           virtual int setTestPatternMode(controls::draft::TestPatternModeEnum mode) = 0;
> > > > + virtual const CameraSensorProperties::SensorDelays &sensorDelays() = 0;
> > > >   };
> > > >
> > > >   class CameraSensorFactoryBase
> > > > diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h
> > > > index 480ac121..fe40bd7f 100644
> > > > --- a/include/libcamera/internal/camera_sensor_properties.h
> > > > +++ b/include/libcamera/internal/camera_sensor_properties.h
> > > > @@ -8,6 +8,7 @@
> > > >   #pragma once
> > > >
> > > >   #include <map>
> > > > +#include <stdint.h>
> > > >   #include <string>
> > > >
> > > >   #include <libcamera/control_ids.h>
> > > > @@ -20,6 +21,15 @@ struct CameraSensorProperties {
> > > >
> > > >           Size unitCellSize;
> > > >           std::map<controls::draft::TestPatternModeEnum, int32_t> testPatternModes;
> > > > +
> > > > + struct SensorDelays {
> > > > +         uint8_t exposureDelay;
> > > > +         uint8_t gainDelay;
> > > > +         uint8_t vblankDelay;
> > > > +         uint8_t hblankDelay;
> > > > + } sensorDelays;
> > > >   };
> > > >
> > > > +extern const CameraSensorProperties::SensorDelays defaultSensorDelays;
> > > > +
> > > Fine. I thought we could have defined this inside
> > > CameraSensor::sensorDelays() as
> > >
> > > const CameraSensorProperties::SensorDelays &CameraSensorLegacy::sensorDelays()
> > > {
> > >          static constexpr CameraSensorProperties::SensorDelays defaultSensorDelays = {
> > >                  .exposureDelay = 2,
> > >                  .gainDelay = 1,
> > >                  .vblankDelay = 2,
> > >                  .hblankDelay = 2,
> > >          };
> > Are references to that safe from outside the function scope? If so then sure we could do that instead...

Yes, because it's a static constexpr - I believe it's safe to define it
in scope here - and it will be part of the static data - and valid when
we return to the callers with the 'address/reference' to this const
data.

> 
> I don't see any code reference outside of this, but indeed
> documentation links would be broken.

I don't think we need to document the defaultSensorDelays specifically.

--
Kieran