[libcamera-devel] [PATCH 02/12] libcamera: camera_sensor: Make V4L2_CID_EXPOSURE mandatory
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Jan 18 16:07:25 CET 2021
Hi Jacopo,
Thanks for your work.
On 2021-01-11 02:23:16 +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Jan 05, 2021 at 08:05:12PM +0100, Jacopo Mondi wrote:
> > Add the V4L2_CID_EXPOSURE control to the list of mandatory controls the
> > sensor driver has to report and document this new requirement.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> > Documentation/sensor_driver_requirements.rst | 11 ++++++++++-
> > src/libcamera/camera_sensor.cpp | 1 +
> > 2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/sensor_driver_requirements.rst b/Documentation/sensor_driver_requirements.rst
> > index 0e658aaa03b5..97e98b9e12ef 100644
> > --- a/Documentation/sensor_driver_requirements.rst
> > +++ b/Documentation/sensor_driver_requirements.rst
> > @@ -26,11 +26,20 @@ The sensor driver shall support the following V4L2 controls:
> >
> > * `V4L2_CID_HBLANK`_
> > * `V4L2_CID_PIXEL_RATE`_
> > +* `V4L2_CID_EXPOSURE`_
> >
> > .. _V4L2_CID_HBLANK: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-source.html
> > .. _V4L2_CID_PIXEL_RATE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/ext-ctrls-image-process.html
> > +.. _V4L2_CID_EXPOSURE: https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/control.html
>
> Could you keep these alphabetically sorted, here, below and in the code
> ?
>
> > +
> > +The `HBLANK` and `PIXEL_RATE` controls are used to compute the sensor
> > +output timings.
> > +
> > +The `EXPOSURE` control shall report the image integration time in number of
> > +lines. The V4L2 documentation does not specify a unit for this control, drivers
> > +compliant with the V4L2 specification might need to be changed to be used by
> > +libcamera.
>
> I'm not sure why I would have gone for the opposite order (V4L2 first,
> then libcamera) so you can ignore the following if you wish.
>
> While V4L2 doesn't specify a unit for the `EXPOSURE` control, libcamera requires
> it to be expressed as a number of image lines. Camera sensor drivers that do not
> comply with this requirement will need to be adapted or will produce incorrect
> results.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
I like Laurent's suggestion, but with or without it used,
Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
>
> >
> > -Both controls are used to compute the sensor output timings.
> >
> > Optional Requirements
> > ---------------------
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 05a1d7c22e97..f7939b94d2f8 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -245,6 +245,7 @@ int CameraSensor::validateSensorDriver()
> > const std::vector<uint32_t> mandatoryControls{
> > V4L2_CID_PIXEL_RATE,
> > V4L2_CID_HBLANK,
> > + V4L2_CID_EXPOSURE,
> > };
> >
> > ControlList ctrls = subdev_->getControls(mandatoryControls);
>
> --
> Regards,
>
> Laurent Pinchart
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list