[libcamera-devel] [PATCH 2/3] libcamera: camera_sensor: Restrict sensor info to raw sensors

[Jacopo Mondi]

Hi Laurent,

On Mon, Feb 01, 2021 at 10:41:30PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Feb 01, 2021 at 10:52:12AM +0100, Jacopo Mondi wrote:
> > On Sun, Jan 31, 2021 at 08:17:21PM +0200, Laurent Pinchart wrote:
> > > YUV sensors don't provide the necessary information to fill
> > > CameraSensorInfo, as they include an ISP and provide a higher-level API
> > > that doesn't always expose low-level information. The CameraSensor class
> > > makes low-level V4L2 controls mandatory for all sensors, which prevents
> > > usage of YUV sensors with the simple pipeline handler.
> > >
> > > Make CameraSensor::sensorInfo() available for raw sensors only. This
> > > won't introduce any regression in pipeline handlers that currently use
> > > the sensorInfo() function as they all operate with raw sensors, and
> > > won't be a limitation for the simple pipeline handler as well as it
> > > doesn't use sensor info. If part of the sensor info (such as the active
> > > pixel array size for instance) becomes useful to expose for YUV sensors
> > > in the future, the sensorInfo() function can be extended to report that
> > > information only and skip data that is only available for raw sensors.
> >
> > It makes sense, and I see the reasons why you won't this.
> > But I'm a little concerned that with the sensorInfo() function
> > returning -EINVAL we're limiting pipeline handlers that use
> > sensorInfo to work with raw sensor only, while I assume it's totally
> > plausible that someone wants to connect a YUV sensor to the pi, in
> > example.
>
> That's absolutely correct, but in that case, the YUV sensor will include
> an ISP and will produce processed images. The RPi ISP and the IPA should
> be bypassed completely. I think we should support this at some point,
> and it will require changes in the RPi pipeline handler, which will
> include not use sensorInfo() in that case (as the sensor info is meant
> for the IPA).
>
> > Alternatively, sensorInfo can return only information that are
> > available in YUV sensor, but then the pipeline-IPA would break anyway.
>
> I think it would make sense if some of the information is still useful
> for pipeline that don't use an ISP and IPA. As I can't tell yet what
> information that would be, I've decided not to implement this yet until
> we get a use case.
>
> > This is less trivial than it seems and I'm afraid it will require a
> > differnet handling of RAW/YUV sensor in the pipeline handlers. Or do
> > you think a YUV sensor connected to a platform with a ISP it's not
> > that likely as a combination and we should not care too much ?
>
> You're right in my opinion that we should support this, but I believe
> only when bypassing the ISP and IPA.

It all makes sense. I'm a bit afraid than with this change someone
trying an YUV sensor with a pipeline that requires SensorInfo will
have an hard failure.

Anyway, I have no better options, so please add
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>

Thanks
  j

>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > > ---
> > >  Documentation/sensor_driver_requirements.rst |  2 +-
> > >  src/libcamera/camera_sensor.cpp              | 59 ++++++++++++--------
> > >  2 files changed, 36 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst
> > > index 6dcd4e68d64d..64e00fd5fc7c 100644
> > > --- a/Documentation/sensor_driver_requirements.rst
> > > +++ b/Documentation/sensor_driver_requirements.rst
> > > @@ -22,7 +22,7 @@ Mandatory Requirements
> > >
> > >  The sensor driver is assumed to be fully compliant with the V4L2 specification.
> > >
> > > -The sensor driver shall support the following V4L2 controls:
> > > +For RAW sensors, the sensor driver shall support the following V4L2 controls:
> > >
> > >  * `V4L2_CID_EXPOSURE`_
> > >  * `V4L2_CID_HBLANK`_
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index 35312857ff90..10713d3a0c29 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -246,27 +246,6 @@ int CameraSensor::init()
> > >
> > >  int CameraSensor::validateSensorDriver()
> > >  {
> > > -	/*
> > > -	 * Make sure the sensor driver supports the mandatory controls
> > > -	 * required by the CameraSensor class.
> > > -	 */
> > > -	const std::vector<uint32_t> mandatoryControls{
> > > -		V4L2_CID_EXPOSURE,
> > > -		V4L2_CID_HBLANK,
> > > -		V4L2_CID_PIXEL_RATE,
> > > -	};
> > > -
> > > -	ControlList ctrls = subdev_->getControls(mandatoryControls);
> > > -	if (ctrls.empty()) {
> > > -		LOG(CameraSensor, Error)
> > > -			<< "Mandatory V4L2 controls not available";
> > > -		LOG(CameraSensor, Error)
> > > -			<< "The sensor kernel driver needs to be fixed";
> > > -		LOG(CameraSensor, Error)
> > > -			<< "See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information";
> > > -		return -EINVAL;
> > > -	}
> > > -
> > >  	/*
> > >  	 * Optional controls are used to register optional sensor properties. If
> > >  	 * not present, some values will be defaulted.
> > > @@ -276,7 +255,7 @@ int CameraSensor::validateSensorDriver()
> > >  		V4L2_CID_CAMERA_SENSOR_ROTATION,
> > >  	};
> > >
> > > -	ctrls = subdev_->getControls(optionalControls);
> > > +	ControlList ctrls = subdev_->getControls(optionalControls);
> > >  	if (ctrls.empty())
> > >  		LOG(CameraSensor, Debug) << "Optional V4L2 controls not supported";
> > >
> > > @@ -326,6 +305,30 @@ int CameraSensor::validateSensorDriver()
> > >  			<< "See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information";
> > >  	}
> > >
> > > +	if (!bayerFormat_)
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * For raw sensors, make sure the sensor driver supports the controls
> > > +	 * required by the CameraSensor class.
> > > +	 */
> > > +	const std::vector<uint32_t> mandatoryControls{
> > > +		V4L2_CID_EXPOSURE,
> > > +		V4L2_CID_HBLANK,
> > > +		V4L2_CID_PIXEL_RATE,
> > > +	};
> > > +
> > > +	ctrls = subdev_->getControls(mandatoryControls);
> > > +	if (ctrls.empty()) {
> > > +		LOG(CameraSensor, Error)
> > > +			<< "Mandatory V4L2 controls not available";
> > > +		LOG(CameraSensor, Error)
> > > +			<< "The sensor kernel driver needs to be fixed";
> > > +		LOG(CameraSensor, Error)
> > > +			<< "See Documentation/sensor_driver_requirements.rst in the libcamera sources for more information";
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >
> > > @@ -662,13 +665,21 @@ int CameraSensor::setControls(ControlList *ctrls)
> > >   * \brief Assemble and return the camera sensor info
> > >   * \param[out] info The camera sensor info
> > >   *
> > > - * The CameraSensorInfo content is assembled by inspecting the currently
> > > - * applied sensor configuration and the sensor static properties.
> > > + * This function fills \a info with information that describes the camera sensor
> > > + * and its current configuration. The information combines static data (such as
> > > + * the the sensor model or active pixel array size) and data specific to the
> > > + * current sensor configuration (such as the line length and pixel rate).
> > > + *
> > > + * Sensor information is only available for raw sensors. When called for a YUV
> > > + * sensor, this function returns -EINVAL.
> > >   *
> > >   * \return 0 on success, a negative error code otherwise
> > >   */
> > >  int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> > >  {
> > > +	if (!bayerFormat_)
> > > +		return -EINVAL;
> > > +
> > >  	info->model = model();
> > >
> > >  	/*
>
> --
> Regards,
>
> Laurent Pinchart