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

[Jacopo Mondi]

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 at 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 at 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