[libcamera-devel,v5,09/10] libcamera: camera_sensor: Initialize VIMC properties

10812 diff mbox series

The VIMC driver does not yet support all the features required
for all sensor drivers. As it is the main testing platforms and the
driver changes might take a long time to land in the developments
and testing platforms, temporary close the gap by skipping driver
validation and initializing properties with static information such
as the sensor resolution.

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

Hi Jacopo,

Thanks for your work.

On 2021-01-05 13:31:27 +0100, Jacopo Mondi wrote:
> The VIMC driver does not yet support all the features required
> for all sensor drivers. As it is the main testing platforms and the
> driver changes might take a long time to land in the developments
> and testing platforms, temporary close the gap by skipping driver
> validation and initializing properties with static information such
> as the sensor resolution.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

It would have been nice if the workaround could be done inside the VIMC 
pipeline handler but that may be to messy :-) As a temporary workaround 
I think this is good enough.

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  include/libcamera/internal/camera_sensor.h |  1 +
>  src/libcamera/camera_sensor.cpp            | 27 ++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index de6a0202d19a..759a12d16f72 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -70,6 +70,7 @@ protected:
>  private:
>  	int generateId();
>  	int validateSensorDriver();
> +	void initVIMCDefaultProperties();
>  	int initProperties();
>  
>  	const MediaEntity *entity_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 0e8aff27b712..046474c03f4a 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include "libcamera/internal/camera_sensor.h"
> +#include "libcamera/internal/media_device.h"
>  
>  #include <algorithm>
>  #include <float.h>
> @@ -207,6 +208,21 @@ int CameraSensor::init()
>  	 */
>  	resolution_ = sizes_.back();
>  
> +	/*
> +	 * VIMC is a bit special, as it does not yet support all the
> +	 * mandatory requirements regular sensors have to respect.
> +	 *
> +	 * Do not validate the driver if it's VIMC and initialize the
> +	 * the sensor properties with static information.
> +	 *
> +	 * \todo Remove the special case once the VIMC driver has been
> +	 * updated in all test platforms.
> +	 */
> +	if (entity_->device()->driver() == "vimc") {
> +		initVIMCDefaultProperties();
> +		return initProperties();
> +	}
> +
>  	ret = validateSensorDriver();
>  	if (ret)
>  		return ret;
> @@ -306,6 +322,17 @@ int CameraSensor::validateSensorDriver()
>  	return 0;
>  }
>  
> +/*
> + * \brief Initialize properties that cannot be intialized by the
> + * regular initProperties() function for VIMC
> + */
> +void CameraSensor::initVIMCDefaultProperties()
> +{
> +	pixelArraySize_ = resolution();
> +	activeArea_ = Rectangle(pixelArraySize_);
> +	analogueCrop_ = activeArea_;
> +}
> +
>  int CameraSensor::initProperties()
>  {
>  	/*
> -- 
> 2.29.2
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Jacopo,

Thank you for the patch.

On Tue, Jan 05, 2021 at 01:31:27PM +0100, Jacopo Mondi wrote:
> The VIMC driver does not yet support all the features required
> for all sensor drivers. As it is the main testing platforms and the
> driver changes might take a long time to land in the developments
> and testing platforms, temporary close the gap by skipping driver
> validation and initializing properties with static information such
> as the sensor resolution.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  1 +
>  src/libcamera/camera_sensor.cpp            | 27 ++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index de6a0202d19a..759a12d16f72 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -70,6 +70,7 @@ protected:
>  private:
>  	int generateId();
>  	int validateSensorDriver();
> +	void initVIMCDefaultProperties();

s/VIMC/Vimc/

>  	int initProperties();
>  
>  	const MediaEntity *entity_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 0e8aff27b712..046474c03f4a 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include "libcamera/internal/camera_sensor.h"
> +#include "libcamera/internal/media_device.h"
>  
>  #include <algorithm>
>  #include <float.h>
> @@ -207,6 +208,21 @@ int CameraSensor::init()
>  	 */
>  	resolution_ = sizes_.back();
>  
> +	/*
> +	 * VIMC is a bit special, as it does not yet support all the
> +	 * mandatory requirements regular sensors have to respect.
> +	 *
> +	 * Do not validate the driver if it's VIMC and initialize the
> +	 * the sensor properties with static information.

s/the the/the/

(You could also reflow the text up to 80 columns)

> +	 *
> +	 * \todo Remove the special case once the VIMC driver has been
> +	 * updated in all test platforms.
> +	 */
> +	if (entity_->device()->driver() == "vimc") {
> +		initVIMCDefaultProperties();

You could have inlined the function here as it's really small, but it's
no big deal.

Did you btw implement the corresponding changes in vimc yet ? If not I
can help.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +		return initProperties();
> +	}
> +
>  	ret = validateSensorDriver();
>  	if (ret)
>  		return ret;
> @@ -306,6 +322,17 @@ int CameraSensor::validateSensorDriver()
>  	return 0;
>  }
>  
> +/*
> + * \brief Initialize properties that cannot be intialized by the
> + * regular initProperties() function for VIMC
> + */
> +void CameraSensor::initVIMCDefaultProperties()
> +{
> +	pixelArraySize_ = resolution();
> +	activeArea_ = Rectangle(pixelArraySize_);
> +	analogueCrop_ = activeArea_;
> +}
> +
>  int CameraSensor::initProperties()
>  {
>  	/*

Hi Laurent,

On Thu, Jan 07, 2021 at 04:59:56AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Jan 05, 2021 at 01:31:27PM +0100, Jacopo Mondi wrote:
> > The VIMC driver does not yet support all the features required
> > for all sensor drivers. As it is the main testing platforms and the
> > driver changes might take a long time to land in the developments
> > and testing platforms, temporary close the gap by skipping driver
> > validation and initializing properties with static information such
> > as the sensor resolution.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  1 +
> >  src/libcamera/camera_sensor.cpp            | 27 ++++++++++++++++++++++
> >  2 files changed, 28 insertions(+)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index de6a0202d19a..759a12d16f72 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -70,6 +70,7 @@ protected:
> >  private:
> >  	int generateId();
> >  	int validateSensorDriver();
> > +	void initVIMCDefaultProperties();
>
> s/VIMC/Vimc/
>
> >  	int initProperties();
> >
> >  	const MediaEntity *entity_;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 0e8aff27b712..046474c03f4a 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #include "libcamera/internal/camera_sensor.h"
> > +#include "libcamera/internal/media_device.h"
> >
> >  #include <algorithm>
> >  #include <float.h>
> > @@ -207,6 +208,21 @@ int CameraSensor::init()
> >  	 */
> >  	resolution_ = sizes_.back();
> >
> > +	/*
> > +	 * VIMC is a bit special, as it does not yet support all the
> > +	 * mandatory requirements regular sensors have to respect.
> > +	 *
> > +	 * Do not validate the driver if it's VIMC and initialize the
> > +	 * the sensor properties with static information.
>
> s/the the/the/
>
> (You could also reflow the text up to 80 columns)
>
> > +	 *
> > +	 * \todo Remove the special case once the VIMC driver has been
> > +	 * updated in all test platforms.
> > +	 */
> > +	if (entity_->device()->driver() == "vimc") {
> > +		initVIMCDefaultProperties();
>
> You could have inlined the function here as it's really small, but it's
> no big deal.
>
> Did you btw implement the corresponding changes in vimc yet ? If not I
> can help.

Not yet. The 'hardest' part should be figure out meaningful values to
return for the PIXEL_RATE and VBLANK controls, the implementation
itself should be trivial.

Thanks
  j

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > +		return initProperties();
> > +	}
> > +
> >  	ret = validateSensorDriver();
> >  	if (ret)
> >  		return ret;
> > @@ -306,6 +322,17 @@ int CameraSensor::validateSensorDriver()
> >  	return 0;
> >  }
> >
> > +/*
> > + * \brief Initialize properties that cannot be intialized by the
> > + * regular initProperties() function for VIMC
> > + */
> > +void CameraSensor::initVIMCDefaultProperties()
> > +{
> > +	pixelArraySize_ = resolution();
> > +	activeArea_ = Rectangle(pixelArraySize_);
> > +	analogueCrop_ = activeArea_;
> > +}
> > +
> >  int CameraSensor::initProperties()
> >  {
> >  	/*
>
> --
> Regards,
>
> Laurent Pinchart