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

Message ID 20191218145001.22283-6-jacopo@jmondi.org
State Superseded
Delegated to: Jacopo Mondi
Headers show
Series
  • Define and register 'sensor' and 'lens' properties
Related show

Commit Message

Jacopo Mondi Dec. 18, 2019, 2:49 p.m. UTC 
Implement sensor specific pixel array properties initialization for the
OV5670 sensor driver by overriding CameraSensor::initProperties()
method.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/sensor/ov5670.cpp | 19 +++++++++++++++++++
 src/libcamera/sensor/ov5670.h   |  3 +++
 2 files changed, 22 insertions(+)

Comments

Niklas Söderlund Jan. 15, 2020, 8:17 p.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2019-12-18 15:49:59 +0100, Jacopo Mondi wrote:
> Implement sensor specific pixel array properties initialization for the
> OV5670 sensor driver by overriding CameraSensor::initProperties()
> method.

Would it be possible to reference what document source was used to find 
the values used in the code?

> 
> Signed-off-by: Jacopo Mondi <jacopo@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 });
> +	int32_t bayerFilter = properties::BayerFilterGRBG;
> +	properties_.set(properties::BayerFilterArrangement, bayerFilter);
> +	properties_.set(properties::ISOSensitivityRange, { 50, 800 });
> +
> +	return CameraSensor::initProperties(controlMap);
> +}
> +
>  }; /* namespace libcamera */
> +
> diff --git a/src/libcamera/sensor/ov5670.h b/src/libcamera/sensor/ov5670.h
> index f84239c8411d..564a4546d69b 100644
> --- a/src/libcamera/sensor/ov5670.h
> +++ b/src/libcamera/sensor/ov5670.h
> @@ -11,8 +11,11 @@
>  
>  namespace libcamera {
>  
> +class ControlInfoMap;

Missing newline.

>  class OV5670CameraSensor final : public CameraSensor
>  {
> +	int initProperties(const ControlInfoMap &controlMap);
> +
>  private:
>  	OV5670CameraSensor(const MediaEntity *entity);
>  	friend CameraSensorFactory;
> -- 
> 2.24.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Sakari Ailus Jan. 20, 2020, 9:18 p.m. UTC | #2 
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@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...

> +	int32_t bayerFilter = properties::BayerFilterGRBG;
> +	properties_.set(properties::BayerFilterArrangement, bayerFilter);

This one as well, perhaps?

> +	properties_.set(properties::ISOSensitivityRange, { 50, 800 });
> +
> +	return CameraSensor::initProperties(controlMap);
> +}
> +
>  }; /* namespace libcamera */
> +
> diff --git a/src/libcamera/sensor/ov5670.h b/src/libcamera/sensor/ov5670.h
> index f84239c8411d..564a4546d69b 100644
> --- a/src/libcamera/sensor/ov5670.h
> +++ b/src/libcamera/sensor/ov5670.h
> @@ -11,8 +11,11 @@
>  
>  namespace libcamera {
>  
> +class ControlInfoMap;
>  class OV5670CameraSensor final : public CameraSensor
>  {
> +	int initProperties(const ControlInfoMap &controlMap);
> +
>  private:
>  	OV5670CameraSensor(const MediaEntity *entity);
>  	friend CameraSensorFactory;
Jacopo Mondi Jan. 21, 2020, 8:40 a.m. UTC | #3 
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@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 ?

> > +	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 ;) ?

> > +	properties_.set(properties::ISOSensitivityRange, { 50, 800 });
> > +
> > +	return CameraSensor::initProperties(controlMap);
> > +}
> > +
> >  }; /* namespace libcamera */
> > +
> > diff --git a/src/libcamera/sensor/ov5670.h b/src/libcamera/sensor/ov5670.h
> > index f84239c8411d..564a4546d69b 100644
> > --- a/src/libcamera/sensor/ov5670.h
> > +++ b/src/libcamera/sensor/ov5670.h
> > @@ -11,8 +11,11 @@
> >
> >  namespace libcamera {
> >
> > +class ControlInfoMap;
> >  class OV5670CameraSensor final : public CameraSensor
> >  {
> > +	int initProperties(const ControlInfoMap &controlMap);
> > +
> >  private:
> >  	OV5670CameraSensor(const MediaEntity *entity);
> >  	friend CameraSensorFactory;
>
> --
> Regards,
>
> Sakari Ailus
Sakari Ailus Jan. 23, 2020, 8:52 p.m. UTC | #4 
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@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?

Patch
diff mbox series 

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 });
+	int32_t bayerFilter = properties::BayerFilterGRBG;
+	properties_.set(properties::BayerFilterArrangement, bayerFilter);
+	properties_.set(properties::ISOSensitivityRange, { 50, 800 });
+
+	return CameraSensor::initProperties(controlMap);
+}
+
 }; /* namespace libcamera */
+
diff --git a/src/libcamera/sensor/ov5670.h b/src/libcamera/sensor/ov5670.h
index f84239c8411d..564a4546d69b 100644
--- a/src/libcamera/sensor/ov5670.h
+++ b/src/libcamera/sensor/ov5670.h
@@ -11,8 +11,11 @@ 
 
 namespace libcamera {
 
+class ControlInfoMap;
 class OV5670CameraSensor final : public CameraSensor
 {
+	int initProperties(const ControlInfoMap &controlMap);
+
 private:
 	OV5670CameraSensor(const MediaEntity *entity);
 	friend CameraSensorFactory;