| Message ID | 20210105190522.682324-3-jacopo@jmondi.org |
|---|---|
| State | Accepted |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
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@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@ideasonboard.com> > > -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);
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@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@ideasonboard.com> I like Laurent's suggestion, but with or without it used, Reviewed-by: Niklas Söderlund <niklas.soderlund@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@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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 + +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. -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);
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@jmondi.org> --- Documentation/sensor_driver_requirements.rst | 11 ++++++++++- src/libcamera/camera_sensor.cpp | 1 + 2 files changed, 11 insertions(+), 1 deletion(-)