[libcamera-devel] [PATCH v3 2/4] libcamera: camera_sensor: Register static properties

[Jacopo Mondi]

Hi Niklas,

On Tue, Dec 29, 2020 at 06:25:00PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your patch.
>
> On 2020-12-28 17:52:01 +0100, Jacopo Mondi wrote:
> > Register static properties, retrieved inspecting the sensor
> > database, in the CameraSensor class.
> >
> > Static properties are overridden by properties retrieved from
> > the kernel interface at run-time if any overlap between the two
> > sets occurs.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  1 +
> >  src/libcamera/camera_sensor.cpp            | 21 ++++++++++++++++++++-
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 10877b9d1d9d..2790c55b6178 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -71,6 +71,7 @@ protected:
> >  private:
> >  	int generateId();
> >  	int validateSensorDriver();
> > +	void initStaticProperties();
> >  	int initProperties();
> >  	int initControls();
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index ea57943647b1..d86fc211c4f6 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -20,6 +20,7 @@
> >
> >  #include "libcamera/internal/bayer_format.h"
> >  #include "libcamera/internal/formats.h"
> > +#include "libcamera/internal/sensor_database.h"
> >  #include "libcamera/internal/sysfs.h"
> >  #include "libcamera/internal/utils.h"
> >
> > @@ -295,6 +296,21 @@ int CameraSensor::validateSensorDriver()
> >  	return 0;
> >  }
> >
> > +void CameraSensor::initStaticProperties()
> > +{
> > +	const SensorInfo *info = SensorDatabase::get(model_);
> > +	if (!info) {
> > +		LOG(CameraSensor, Warning)
> > +			<< "No static properties available for '" << model_ << "'";
> > +		LOG(CameraSensor, Warning)
> > +			<< "Please consider updating the sensor database";
>
> I think this could be a single LOG().
>

I wanted to avoid too long lines.

> > +		return;
> > +	}
> > +
> > +	/* Register the properties retrieved from the sensor database. */
> > +	properties_.set(properties::UnitCellSize, info->unitCellSize);
>
> This implies all future data members of SensorInfo are mandatory. Would
> it make sens to add a 'valid' flag for each data member and only set the
> property if it's actually recorded in the sensor database?
>

Correct, I currently assume all SensorInfo fields are there.
Currently I don't think it's a big deal, there's only one field :)

Going forward we might need to ensure that either all fields are
there, or implement a smarter lookup method, but right now I'm not
sure what direction to take. In the back of my mind the idea to
generate the sensor database from a .yaml input file is still an
option worth exploring.

Thanks
   j

> > +}
> > +
> >  int CameraSensor::initProperties()
> >  {
> >  	/*
> > @@ -332,7 +348,10 @@ int CameraSensor::initProperties()
> >  	if (ret)
> >  		return ret;
> >
> > -	/* Retrieve and store the camera sensor properties. */
> > +	/* Initialize the static properties from the sensor database. */
> > +	initStaticProperties();
> > +
> > +	/* Retrieve and register properties from the kernel interface. */
> >  	const ControlInfoMap &controls = subdev_->controls();
> >  	int32_t propertyValue;
> >
> > --
> > 2.29.2
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund