| Message ID | 20210622023654.969162-3-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 ? > return 0; > } > > -- > 2.32.0.288.g62a8d224e6-goog >
Hi Jacopo, On Tue, Jun 22, 2021 at 7:33 PM Jacopo Mondi <jacopo@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@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. > 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(). 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 > >
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@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@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 > > >
Hi Jacopo, On Wed, Jun 23, 2021 at 4:33 PM Jacopo Mondi <jacopo@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@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@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 > > > >
+Laurent Pinchart On Wed, Jun 23, 2021 at 5:00 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > Hi Jacopo, > > On Wed, Jun 23, 2021 at 4:33 PM Jacopo Mondi <jacopo@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@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@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 > > > > >
Hi Hiro On Wed, Jun 23, 2021 at 05:00:30PM +0900, Hirokazu Honda wrote: > Hi Jacopo, > > On Wed, Jun 23, 2021 at 4:33 PM Jacopo Mondi <jacopo@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@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@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? I wasn't certainly asking you do so as part of this series :) I was suprised it was not there, that's all :)
Hi Jacopo On Wed, Jun 23, 2021 at 5:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro > > On Wed, Jun 23, 2021 at 05:00:30PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, > > > > On Wed, Jun 23, 2021 at 4:33 PM Jacopo Mondi <jacopo@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@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@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? > > I wasn't certainly asking you do so as part of this series :) > I was suprised it was not there, that's all :) Okay, thanks! >
Hello, 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@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. > > But the libcamera implementation keeps opening the node and in > > libcamera we should reset in configure(). > > > > > > return 0; > > > > } > > > >
Hi Laurent, On Mon, Jun 28, 2021 at 12:28 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > 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@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? > > > But the libcamera implementation keeps opening the node and in > > > libcamera we should reset in configure(). > > > > > > > > return 0; > > > > > } > > > > > > > -- > Regards, > > 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@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; > > > > > > } > > > > > >
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; + } + return 0; }
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@chromium.org> --- src/libcamera/pipeline/ipu3/cio2.cpp | 8 ++++++++ 1 file changed, 8 insertions(+)