| Message ID | 20221122120343.4825-2-david.plowman@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi David, On Tue, Nov 22, 2022 at 12:03:41PM +0000, David Plowman via libcamera-devel wrote: > The existing code to reset the HBLANK interval to the minimum value > (in the init method) is moved into a separate function that pipeline > handlers will be able to call. > > We no longer reset the HBLANK in init because we may not own the > camera and the operation may fail, so pipeline handlers will have to > call the new method for themselves. These calls will be added in > subsequent commits. > This was again part of a discussion with Dave on a recent series for a camera sensor https://patchwork.linuxtv.org/project/linux-media/patch/20221005190613.394277-8-jacopo@jmondi.org/#140629 To sum it up: I would have liked to standardize on the idea that when a new mode is applied on a sensor, the blankings are reset to their minimum by the driver. As nowadays every driver behaves as it likes the most, and changing mode might eventually change the blankings value anyway, I considered a known behaviour to be more predictable for applications. To be honest, I overlooked the fact we reset hblank unconditionally (but only at init() time). Curious to know what you think on this and what would your PH expect (I presume it doesn't make much difference, as the IPA wants to control HBLANK anyway). > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > include/libcamera/internal/camera_sensor.h | 2 ++ > src/libcamera/camera_sensor.cpp | 10 +++++++--- > 2 files changed, 9 insertions(+), 3 deletions(-) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index b9f4d786..c97b5698 100644 > --- a/include/libcamera/internal/camera_sensor.h > +++ b/include/libcamera/internal/camera_sensor.h > @@ -68,6 +68,8 @@ public: > > CameraLens *focusLens() { return focusLens_.get(); } > > + int resetHblank(); > + > protected: > std::string logPrefix() const override; > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 572a313a..589e9736 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -176,6 +176,11 @@ int CameraSensor::init() > if (ret) > return ret; > > + return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > +} > + > +int CameraSensor::resetHblank() I'm not super excited by this, it's a very ad-hoc call for PH, if they don't know they have to call it explicitly it will be easily missed. If my idea of "starting in a known state in drivers" doesn't fly, this could be moved to CameraSensor::setFormat() maybe ? IPAs that control HBLANK will override it anyway. > +{ > /* > * Set HBLANK to the minimum to start with a well-defined line length, > * allowing IPA modules that do not modify HBLANK to use the sensor > @@ -192,17 +197,16 @@ int CameraSensor::init() > const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > const int32_t hblankMin = hblank.min().get<int32_t>(); > const int32_t hblankMax = hblank.max().get<int32_t>(); > + int ret = 0; > > if (hblankMin != hblankMax) { > ControlList ctrl(subdev_->controls()); > > ctrl.set(V4L2_CID_HBLANK, hblankMin); > ret = subdev_->setControls(&ctrl); > - if (ret) > - return ret; > } > > - return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > + return ret; > } > > int CameraSensor::validateSensorDriver() > -- > 2.30.2 >
Hi Jacopo Thanks for the comments. Indeed I defer to folks who spend much more time on camera drivers than I do! On Tue, 22 Nov 2022 at 13:07, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David, > > On Tue, Nov 22, 2022 at 12:03:41PM +0000, David Plowman via libcamera-devel wrote: > > The existing code to reset the HBLANK interval to the minimum value > > (in the init method) is moved into a separate function that pipeline > > handlers will be able to call. > > > > We no longer reset the HBLANK in init because we may not own the > > camera and the operation may fail, so pipeline handlers will have to > > call the new method for themselves. These calls will be added in > > subsequent commits. > > > > This was again part of a discussion with Dave on a recent series for a > camera sensor > https://patchwork.linuxtv.org/project/linux-media/patch/20221005190613.394277-8-jacopo@jmondi.org/#140629 > > To sum it up: I would have liked to standardize on the idea that when > a new mode is applied on a sensor, the blankings are reset to their > minimum by the driver. > > As nowadays every driver behaves as it likes the most, and changing > mode might eventually change the blankings value anyway, I considered > a known behaviour to be more predictable for applications. > > To be honest, I overlooked the fact we reset hblank unconditionally > (but only at init() time). > > Curious to know what you think on this and what would your PH expect > (I presume it doesn't make much difference, as the IPA wants to > control HBLANK anyway). > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > include/libcamera/internal/camera_sensor.h | 2 ++ > > src/libcamera/camera_sensor.cpp | 10 +++++++--- > > 2 files changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > index b9f4d786..c97b5698 100644 > > --- a/include/libcamera/internal/camera_sensor.h > > +++ b/include/libcamera/internal/camera_sensor.h > > @@ -68,6 +68,8 @@ public: > > > > CameraLens *focusLens() { return focusLens_.get(); } > > > > + int resetHblank(); > > + > > protected: > > std::string logPrefix() const override; > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 572a313a..589e9736 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -176,6 +176,11 @@ int CameraSensor::init() > > if (ret) > > return ret; > > > > + return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > +} > > + > > +int CameraSensor::resetHblank() > > I'm not super excited by this, it's a very ad-hoc call for PH, if they > don't know they have to call it explicitly it will be easily missed. > > If my idea of "starting in a known state in drivers" doesn't fly, this > could be moved to CameraSensor::setFormat() maybe ? IPAs that control > HBLANK will override it anyway. That sounds pretty convincing to me. What do other folks think? Thanks David > > > +{ > > /* > > * Set HBLANK to the minimum to start with a well-defined line length, > > * allowing IPA modules that do not modify HBLANK to use the sensor > > @@ -192,17 +197,16 @@ int CameraSensor::init() > > const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > > const int32_t hblankMin = hblank.min().get<int32_t>(); > > const int32_t hblankMax = hblank.max().get<int32_t>(); > > + int ret = 0; > > > > if (hblankMin != hblankMax) { > > ControlList ctrl(subdev_->controls()); > > > > ctrl.set(V4L2_CID_HBLANK, hblankMin); > > ret = subdev_->setControls(&ctrl); > > - if (ret) > > - return ret; > > } > > > > - return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > + return ret; > > } > > > > int CameraSensor::validateSensorDriver() > > -- > > 2.30.2 > >
Hi David & Jacopo On Tue, 22 Nov 2022 at 13:42, David Plowman via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Hi Jacopo > > Thanks for the comments. Indeed I defer to folks who spend much more > time on camera drivers than I do! > > On Tue, 22 Nov 2022 at 13:07, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi David, > > > > On Tue, Nov 22, 2022 at 12:03:41PM +0000, David Plowman via libcamera-devel wrote: > > > The existing code to reset the HBLANK interval to the minimum value > > > (in the init method) is moved into a separate function that pipeline > > > handlers will be able to call. > > > > > > We no longer reset the HBLANK in init because we may not own the > > > camera and the operation may fail, so pipeline handlers will have to > > > call the new method for themselves. These calls will be added in > > > subsequent commits. > > > > > > > This was again part of a discussion with Dave on a recent series for a > > camera sensor > > https://patchwork.linuxtv.org/project/linux-media/patch/20221005190613.394277-8-jacopo@jmondi.org/#140629 > > > > To sum it up: I would have liked to standardize on the idea that when > > a new mode is applied on a sensor, the blankings are reset to their > > minimum by the driver. We really need Sakari to respond on this one as the maintainer of image sensor drivers. Even if the documentation is updated to state that the expected behaviour is to reset the blankings, you then have a fair task to validate and potentially correct that behaviour on all mainline sensor drivers. I guess you could restrict it to just those that have tunings or other config within libcamera, but that's still a moderate number. > > As nowadays every driver behaves as it likes the most, and changing > > mode might eventually change the blankings value anyway, I considered > > a known behaviour to be more predictable for applications. > > > > To be honest, I overlooked the fact we reset hblank unconditionally > > (but only at init() time). > > > > Curious to know what you think on this and what would your PH expect > > (I presume it doesn't make much difference, as the IPA wants to > > control HBLANK anyway). This is the flip side - if the PH already knows how to handle setting all the blankings, what does it matter what the sensor driver does? > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > include/libcamera/internal/camera_sensor.h | 2 ++ > > > src/libcamera/camera_sensor.cpp | 10 +++++++--- > > > 2 files changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > index b9f4d786..c97b5698 100644 > > > --- a/include/libcamera/internal/camera_sensor.h > > > +++ b/include/libcamera/internal/camera_sensor.h > > > @@ -68,6 +68,8 @@ public: > > > > > > CameraLens *focusLens() { return focusLens_.get(); } > > > > > > + int resetHblank(); > > > + > > > protected: > > > std::string logPrefix() const override; > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > index 572a313a..589e9736 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -176,6 +176,11 @@ int CameraSensor::init() > > > if (ret) > > > return ret; > > > > > > + return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > > +} > > > + > > > +int CameraSensor::resetHblank() > > > > I'm not super excited by this, it's a very ad-hoc call for PH, if they > > don't know they have to call it explicitly it will be easily missed. > > > > If my idea of "starting in a known state in drivers" doesn't fly, this > > could be moved to CameraSensor::setFormat() maybe ? IPAs that control > > HBLANK will override it anyway. > > That sounds pretty convincing to me. What do other folks think? I'm not developing libcamera so can't really comment. I will say that you've added another userspace/kernel context switch unless controls are all batched together. Then again setFormat is having to call VIDIOC_QUERT_EXT_CTRL on all controls anyway, so that swamps the one extra VIDIOC_S_CTRL. Dave > Thanks > David > > > > > > +{ > > > /* > > > * Set HBLANK to the minimum to start with a well-defined line length, > > > * allowing IPA modules that do not modify HBLANK to use the sensor > > > @@ -192,17 +197,16 @@ int CameraSensor::init() > > > const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); > > > const int32_t hblankMin = hblank.min().get<int32_t>(); > > > const int32_t hblankMax = hblank.max().get<int32_t>(); > > > + int ret = 0; > > > > > > if (hblankMin != hblankMax) { > > > ControlList ctrl(subdev_->controls()); > > > > > > ctrl.set(V4L2_CID_HBLANK, hblankMin); > > > ret = subdev_->setControls(&ctrl); > > > - if (ret) > > > - return ret; > > > } > > > > > > - return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); > > > + return ret; > > > } > > > > > > int CameraSensor::validateSensorDriver() > > > -- > > > 2.30.2 > > >
diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h index b9f4d786..c97b5698 100644 --- a/include/libcamera/internal/camera_sensor.h +++ b/include/libcamera/internal/camera_sensor.h @@ -68,6 +68,8 @@ public: CameraLens *focusLens() { return focusLens_.get(); } + int resetHblank(); + protected: std::string logPrefix() const override; diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 572a313a..589e9736 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -176,6 +176,11 @@ int CameraSensor::init() if (ret) return ret; + return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); +} + +int CameraSensor::resetHblank() +{ /* * Set HBLANK to the minimum to start with a well-defined line length, * allowing IPA modules that do not modify HBLANK to use the sensor @@ -192,17 +197,16 @@ int CameraSensor::init() const ControlInfo hblank = ctrls.infoMap()->at(V4L2_CID_HBLANK); const int32_t hblankMin = hblank.min().get<int32_t>(); const int32_t hblankMax = hblank.max().get<int32_t>(); + int ret = 0; if (hblankMin != hblankMax) { ControlList ctrl(subdev_->controls()); ctrl.set(V4L2_CID_HBLANK, hblankMin); ret = subdev_->setControls(&ctrl); - if (ret) - return ret; } - return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff); + return ret; } int CameraSensor::validateSensorDriver()
The existing code to reset the HBLANK interval to the minimum value (in the init method) is moved into a separate function that pipeline handlers will be able to call. We no longer reset the HBLANK in init because we may not own the camera and the operation may fail, so pipeline handlers will have to call the new method for themselves. These calls will be added in subsequent commits. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- include/libcamera/internal/camera_sensor.h | 2 ++ src/libcamera/camera_sensor.cpp | 10 +++++++--- 2 files changed, 9 insertions(+), 3 deletions(-)