[libcamera-devel] [RFC 5/7] libcamera: sensor: ov5670: Register pixel array properties

[Sakari Ailus]

Hi Jacopo,

On Tue, Jan 21, 2020 at 09:40:12AM +0100, Jacopo Mondi wrote:
> Hello Sakari,
>     happy to have you here!
> 
> On Mon, Jan 20, 2020 at 11:18:56PM +0200, Sakari Ailus wrote:
> > Hi Jacopo,
> >
> > On Wed, Dec 18, 2019 at 03:49:59PM +0100, Jacopo Mondi wrote:
> > > Implement sensor specific pixel array properties initialization for the
> > > OV5670 sensor driver by overriding CameraSensor::initProperties()
> > > method.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > >  src/libcamera/sensor/ov5670.cpp | 19 +++++++++++++++++++
> > >  src/libcamera/sensor/ov5670.h   |  3 +++
> > >  2 files changed, 22 insertions(+)
> > >
> > > diff --git a/src/libcamera/sensor/ov5670.cpp b/src/libcamera/sensor/ov5670.cpp
> > > index ca9f3c1d544f..c2d996785717 100644
> > > --- a/src/libcamera/sensor/ov5670.cpp
> > > +++ b/src/libcamera/sensor/ov5670.cpp
> > > @@ -6,6 +6,10 @@
> > >   */
> > >
> > >  #include "ov5670.h"
> > > +
> > > +#include <libcamera/controls.h>
> > > +#include <libcamera/property_ids.h>
> > > +
> > >  #include "camera_sensor.h"
> > >
> > >  namespace libcamera {
> > > @@ -15,4 +19,19 @@ OV5670CameraSensor::OV5670CameraSensor(const MediaEntity *entity)
> > >  {
> > >  }
> > >
> > > +int OV5670CameraSensor::initProperties(const ControlInfoMap &controlMap)
> > > +{
> > > +	/* Pixel Array Properties. */
> > > +	properties_.set(properties::PixelArraySize, { 2.9f, 1.18f });
> > > +	properties_.set(properties::PixelArrayBounds, { 2592, 1944 });
> > > +	properties_.set(properties::PixelArrays, { 2592, 1944 });
> > > +	properties_.set(properties::ActiveAreaSize, { 16, 6, 2560, 1920 });
> >
> > Could these two be obtained from the driver?
> >
> > At least in principle there's the NATIVE_SIZE target. Although not all
> > drivers support modes without cropping...
> >
> 
> Do you mean something like this ? Maybe adjusted to report the full
> pixel array size through NATIVE_SIZE and the active area through
> CROP_BOUNDS...
> 
> https://lore.kernel.org/linux-media/20190827092339.8858-1-jacopo@jmondi.org/T/#med4e833fddd3f1b21e2a4ea82fc2b4fceaeb4bc3
> 
> I recall I decided to drop those patches from the next iterations of
> that series as we there where controversy there, which to be honest, I
> don't fully remember. Would you be fine seeing patches for the sensor
> driver adding those two selection drivers ?

Hmm. The V4L2 sub-device interface is ill suited for register list based sensors
anyway, you can't convey all the details of the restrictions the mode
definitions have.

Either way, I think the native size definitely makes sense. Why not
cropping either, but given that those modes are set by a single size
(SUBDEV_S_FMT), there's not much use for it.

> 
> > > +	int32_t bayerFilter = properties::BayerFilterGRBG;
> > > +	properties_.set(properties::BayerFilterArrangement, bayerFilter);
> >
> > This one as well, perhaps?
> >
> 
> do we have a control for that ? I haven't seen any in the Camera and
> Image source control classes. Are you suggesting to try and add a new
> one ;) ?

Hmm. I thought this information could be deducted from the format
enumeration, couldn't it?

-- 
Kind regards,

Sakari Ailus