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

[Laurent Pinchart]

Hi Hiro,

On Mon, Jun 28, 2021 at 02:27:59PM +0900, Hirokazu Honda wrote:
> On Mon, Jun 28, 2021 at 12:28 PM Laurent Pinchart wrote:
> > On Wed, Jun 23, 2021 at 09:34:32AM +0200, Jacopo Mondi wrote:
> > > On Wed, Jun 23, 2021 at 12:43:22PM +0900, Hirokazu Honda wrote:
> > > > On Tue, Jun 22, 2021 at 7:33 PM Jacopo Mondi wrote:
> > > > > 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.
> >
> > We need to ensure a consistent startup state, regardless of how the
> > camera has been used before. There are (at least) two ways to do so:
> >
> > - Force all applications to set all controls in the first request
> > - Set default values internally (at the latest) at start time for any
> >   control that isn't specified by the application
> >
> > The first option would require implementing something similar to the
> > Android's request templates, as we really can't let applications to this
> > manually, they won't get it right.
> >
> > In both options, it should be the pipeline handler and IPA that control
> > what default values to set (either directly in the second option, or
> > indirectly through the provided request template in the first option).
> > However, it would be nice to avoid having to duplicate the above code in
> > all pipeline handlers, so some involvement of the CameraSensor class
> > would help. It also needs need to be done in a way that let the pipeline
> > handler override any decision made by the CameraSensor class.
> 
> Could you elaborate on the improvement of the CameraSensor class?

I'd like to avoid duplicating the sensor_->setTestPatternMode() in the
configure() function of every pipeline handler. If it was just for the
test pattern, the call could simply move to the CameraSensor class.
However, there can be other controls too, and for some of them, the
pipeline handler and/or IPA will need to select or at least influence
the default value. I'd like the CameraSensor class to help as much as
possible to avoid code duplication in pipeline handlers. How this could
be done, I have no idea, it needs to be researched and designed.

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

-- 
Regards,

Laurent Pinchart