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

[Hirokazu Honda]

+Laurent Pinchart

On Wed, Jun 23, 2021 at 5:00 PM Hirokazu Honda <hiroh at chromium.org> wrote:
>
> Hi Jacopo,
>
> On Wed, Jun 23, 2021 at 4:33 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> >
> > 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...
> >
>
> I assume the task is orthogonal to this patch series, although it is
> better to do before this series.
> Additionally I am so puzzled by the control classes that I couldn't
> properly make it.
> May I ask you to do that?
>
> > > > 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
> > > > >