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

[Hirokazu Honda]

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