[libcamera-devel] [PATCH 1/3] libcamera: camera_sensor: Add a function to reset the HBLANK interval

[David Plowman]

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