[libcamera-devel] [RFC PATCH v2 3/5] libcamera: ipu3: Disable a sensor test pattern mode at initialization

[Jacopo Mondi]

Hi Hiro,

On Wed, Jun 23, 2021 at 12:43:22PM +0900, Hirokazu Honda wrote:
> Hi Jacopo,
>
> On Tue, Jun 22, 2021 at 7:33 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > Hi Hiro,
> >
> > On Tue, Jun 22, 2021 at 11:36:52AM +0900, Hirokazu Honda wrote:
> > > Turns off a sensor test pattern mode at the initialization of the
> > > sensor. Without this, the camera sensor is configured with the last
> > > test pattern mode that has been set.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > ---
> > >  src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > index 1be2cbcd..8548f749 100644
> > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > > @@ -9,6 +9,7 @@
> > >
> > >  #include <linux/media-bus-format.h>
> > >
> > > +#include <libcamera/control_ids.h>
> > >  #include <libcamera/formats.h>
> > >  #include <libcamera/geometry.h>
> > >  #include <libcamera/stream.h>
> > > @@ -192,6 +193,13 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat)
> > >
> > >       LOG(IPU3, Debug) << "CIO2 output format " << outputFormat->toString();
> > >
> > > +     ret = sensor_->setTestPatternMode(controls::draft::TestPatternModeOff);
> > > +     if (ret) {
> > > +             LOG(IPU3, Error)
> > > +                     << "Failed to reset test pattern mode: " << ret;
> > > +             return ret;
> > > +     }
> > > +
> >
> > Now I see why you need all the checks in the previous patch. But, do
> > we need to do so ? Isn't the application which is in control of the
> > test pattern ? Shouldn't the app explicitly disable it if they want to
> > ?
> >
>
> An application implementation is not controlled by us.
> It can specify an invalid test pattern mode. I would like to have the guard.
>

What I meant was summarized in the below here question
 "Shouldn't the app explicitly disable it if they want to ?"

Rearding the value of the test pattern to apply, I was almost sure
that the control validator verified the provided value is in the range
of supported ones. But it does only verify that the control id is
valid for the camera (see camera_controls.cpp). It would be rather
straightforward to add a .validate() to ControlInfoMap though and make
sure all the values we receive are valid in the current ranges...

> > Shouldn't the app explicitly disable it if they want to
>
> I am not sure honestly. I *think* most apps don't reset the test
> pattern (i.e. disable) in either start up and termination, because a
> v4l2 sensor driver resets the test pattern in open() or close().

I think that's part of the API semantic we have to better define,
that's why I wanted to rope Laurent in.

What v4l2 does should not strictly drive our design though.

Thanks
   j

> But the libcamera implementation keeps opening the node and in
> libcamera we should reset in configure().
>
> -Hiro
> > >       return 0;
> > >  }
> > >
> > > --
> > > 2.32.0.288.g62a8d224e6-goog
> > >