| Message ID | 20210622023654.969162-2-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote: > Provides a function to set the camera sensor a test pattern mode. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > include/libcamera/internal/camera_sensor.h | 1 + > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++ > 2 files changed, 40 insertions(+) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index e133ebf4..8b9f84c9 100644 > --- a/include/libcamera/internal/camera_sensor.h > +++ b/include/libcamera/internal/camera_sensor.h > @@ -43,6 +43,7 @@ public: > { > return testPatternModes_; > } > + int setTestPatternMode(uint8_t testPatternMode); > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > const Size &size) const; > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 70bbd97a..ce8ff274 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const > * \return The list of test pattern modes > */ > > +/** > + * \brief Set the camera sensor a specified controls::TestPatternMode Doxygen is puzzling me recently... the testPatternMode paramter is not documented but I don't get a warning :/ Also, I'm a bit debated about where to actually place this function (or better, it's initTestPatternModes which is misplaced) if we have to respect the declaration order or the way you sorted them here (which is more logical) is better. > + * > + * \return 0 on success or a negative error code otherwise > + */ > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode) > +{ > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(), > + testPatternMode) == testPatternModes_.end()) { > + LOG(CameraSensor, Error) << "Unsupported test pattern mode: " > + << testPatternMode; > + return -EINVAL; > + } Do we need this ? The testPatternModes_ map is constructed using the camera sensor properties, the below find() on the props->testPatternModes map should gurantee that is supported. Also, if a test pattern mode is not reported as part of testPatternModes_ applications shouldn't set it... > + > + const CameraSensorProperties *props = CameraSensorProperties::get(model_); > + if (!props) > + return -EINVAL; > + > + auto it = props->testPatternModes.find(testPatternMode); > + ASSERT(it != props->testPatternModes.end()); Yeah, I think the assertion here is more than enough.. > + const uint8_t index = it->second; > + > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN }); > + if (ctrls.empty()) { > + LOG(CameraSensor, Error) > + << "Failed to retrieve test pattern control"; > + return -EINVAL; > + } We won't register the TestPatternMode control in first place the V4L2 control is not supported.. > + > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN); > + if (value.get<int32_t>() == index) > + return 0; The V4L2 control framework should take care of this by itself, I think we can set the control regardless > + > + value.set<int32_t>(index); > + ctrls.set(V4L2_CID_TEST_PATTERN, value); > + > + return setControls(&ctrls); > +} > + > /** > * \brief Retrieve the best sensor format for a desired output > * \param[in] mbusCodes The list of acceptable media bus codes > -- > 2.32.0.288.g62a8d224e6-goog >
Hi Jacopo, thank you for reviewing. On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro, > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote: > > Provides a function to set the camera sensor a test pattern mode. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > include/libcamera/internal/camera_sensor.h | 1 + > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++ > > 2 files changed, 40 insertions(+) > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > index e133ebf4..8b9f84c9 100644 > > --- a/include/libcamera/internal/camera_sensor.h > > +++ b/include/libcamera/internal/camera_sensor.h > > @@ -43,6 +43,7 @@ public: > > { > > return testPatternModes_; > > } > > + int setTestPatternMode(uint8_t testPatternMode); > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > const Size &size) const; > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > index 70bbd97a..ce8ff274 100644 > > --- a/src/libcamera/camera_sensor.cpp > > +++ b/src/libcamera/camera_sensor.cpp > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const > > * \return The list of test pattern modes > > */ > > > > +/** > > + * \brief Set the camera sensor a specified controls::TestPatternMode > > Doxygen is puzzling me recently... the testPatternMode paramter is not > documented but I don't get a warning :/ > > Also, I'm a bit debated about where to actually place this function > (or better, it's initTestPatternModes which is misplaced) > if we have to respect the declaration order or the way you sorted them > here (which is more logical) is better. > > > + * > > + * \return 0 on success or a negative error code otherwise > > + */ > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode) > > +{ > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(), > > + testPatternMode) == testPatternModes_.end()) { > > + LOG(CameraSensor, Error) << "Unsupported test pattern mode: " > > + << testPatternMode; > > + return -EINVAL; > > + } > > Do we need this ? The testPatternModes_ map is constructed using the > camera sensor properties, the below find() on the props->testPatternModes > map should gurantee that is supported. Also, if a test pattern mode is > not reported as part of testPatternModes_ applications shouldn't set > it... > > > + > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > + if (!props) > > + return -EINVAL; > > + > > + auto it = props->testPatternModes.find(testPatternMode); > > + ASSERT(it != props->testPatternModes.end()); > > Yeah, I think the assertion here is more than enough.. > > > + const uint8_t index = it->second; > > + > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN }); > > + if (ctrls.empty()) { > > + LOG(CameraSensor, Error) > > + << "Failed to retrieve test pattern control"; > > + return -EINVAL; > > + } > > We won't register the TestPatternMode control in first place the V4L2 > control is not supported.. Replace this with ASSERT. > > > + > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN); > > + if (value.get<int32_t>() == index) > > + return 0; > > The V4L2 control framework should take care of this by itself, I think > we can set the control regardless > Yes, I put this in order to save a redundant Ioctl call. Do you want to remove that? -Hiro > > + > > + value.set<int32_t>(index); > > + ctrls.set(V4L2_CID_TEST_PATTERN, value); > > + > > + return setControls(&ctrls); > > +} > > + > > /** > > * \brief Retrieve the best sensor format for a desired output > > * \param[in] mbusCodes The list of acceptable media bus codes > > -- > > 2.32.0.288.g62a8d224e6-goog > >
Hi Hiro, On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote: > Hi Jacopo, thank you for reviewing. > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Hiro, > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote: > > > Provides a function to set the camera sensor a test pattern mode. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > include/libcamera/internal/camera_sensor.h | 1 + > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++ > > > 2 files changed, 40 insertions(+) > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > index e133ebf4..8b9f84c9 100644 > > > --- a/include/libcamera/internal/camera_sensor.h > > > +++ b/include/libcamera/internal/camera_sensor.h > > > @@ -43,6 +43,7 @@ public: > > > { > > > return testPatternModes_; > > > } > > > + int setTestPatternMode(uint8_t testPatternMode); > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > > const Size &size) const; > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > index 70bbd97a..ce8ff274 100644 > > > --- a/src/libcamera/camera_sensor.cpp > > > +++ b/src/libcamera/camera_sensor.cpp > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const > > > * \return The list of test pattern modes > > > */ > > > > > > +/** > > > + * \brief Set the camera sensor a specified controls::TestPatternMode > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not > > documented but I don't get a warning :/ > > > > Also, I'm a bit debated about where to actually place this function > > (or better, it's initTestPatternModes which is misplaced) > > if we have to respect the declaration order or the way you sorted them > > here (which is more logical) is better. > > > > > + * > > > + * \return 0 on success or a negative error code otherwise > > > + */ > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode) > > > +{ > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(), > > > + testPatternMode) == testPatternModes_.end()) { > > > + LOG(CameraSensor, Error) << "Unsupported test pattern mode: " > > > + << testPatternMode; > > > + return -EINVAL; > > > + } > > > > Do we need this ? The testPatternModes_ map is constructed using the > > camera sensor properties, the below find() on the props->testPatternModes > > map should gurantee that is supported. Also, if a test pattern mode is > > not reported as part of testPatternModes_ applications shouldn't set > > it... > > > > > + > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > + if (!props) > > > + return -EINVAL; > > > + > > > + auto it = props->testPatternModes.find(testPatternMode); > > > + ASSERT(it != props->testPatternModes.end()); > > > > Yeah, I think the assertion here is more than enough.. > > > > > + const uint8_t index = it->second; > > > + > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN }); > > > + if (ctrls.empty()) { > > > + LOG(CameraSensor, Error) > > > + << "Failed to retrieve test pattern control"; > > > + return -EINVAL; > > > + } > > > > We won't register the TestPatternMode control in first place the V4L2 > > control is not supported.. > > Replace this with ASSERT. > > > > > > + > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN); > > > + if (value.get<int32_t>() == index) > > > + return 0; > > > > The V4L2 control framework should take care of this by itself, I think > > we can set the control regardless > > > > Yes, I put this in order to save a redundant Ioctl call. > Do you want to remove that? Well, the ctrls ControlList you create with getControls() goes through an IOCTL :) We're talking details, but if we want to retain the state of the testPatterMode we can make it a class member, so that the code can be reduced to (not tested pseudo-code) { if (testPatternMode == testPatternMode_) return 0; const CameraSensorProperties *props = CameraSensorProperties::get(model_); if (!props) return -EINVAL; auto it = props->testPatternModes.find(testPatternMode); ASSERT(it != props->testPatternModes.end()); ControlList ctrls(controls()); ctrls.set(V4L2_CID_TEST_PATTERN, it->second); int ret = setControls(&ctrls); if (ret) return ret; testPatternMode_ = testPatternMode; return 0; } I also wonder if the ASSERT() is not too harsh and we shouldn't just error out. If the application tries to set an unsupported test pattern, the whole library would crash, maybe we should be a little more indulgent. Thanks j > > -Hiro > > > + > > > + value.set<int32_t>(index); > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value); > > > + > > > + return setControls(&ctrls); > > > +} > > > + > > > /** > > > * \brief Retrieve the best sensor format for a desired output > > > * \param[in] mbusCodes The list of acceptable media bus codes > > > -- > > > 2.32.0.288.g62a8d224e6-goog > > >
Hi Jacopo, On Wed, Jun 23, 2021 at 5:06 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro, > > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote: > > Hi Jacopo, thank you for reviewing. > > > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi Hiro, > > > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote: > > > > Provides a function to set the camera sensor a test pattern mode. > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > --- > > > > include/libcamera/internal/camera_sensor.h | 1 + > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++ > > > > 2 files changed, 40 insertions(+) > > > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > > index e133ebf4..8b9f84c9 100644 > > > > --- a/include/libcamera/internal/camera_sensor.h > > > > +++ b/include/libcamera/internal/camera_sensor.h > > > > @@ -43,6 +43,7 @@ public: > > > > { > > > > return testPatternModes_; > > > > } > > > > + int setTestPatternMode(uint8_t testPatternMode); > > > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > > > const Size &size) const; > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > > index 70bbd97a..ce8ff274 100644 > > > > --- a/src/libcamera/camera_sensor.cpp > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const > > > > * \return The list of test pattern modes > > > > */ > > > > > > > > +/** > > > > + * \brief Set the camera sensor a specified controls::TestPatternMode > > > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not > > > documented but I don't get a warning :/ > > > > > > Also, I'm a bit debated about where to actually place this function > > > (or better, it's initTestPatternModes which is misplaced) > > > if we have to respect the declaration order or the way you sorted them > > > here (which is more logical) is better. > > > > > > > + * > > > > + * \return 0 on success or a negative error code otherwise > > > > + */ > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode) > > > > +{ > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(), > > > > + testPatternMode) == testPatternModes_.end()) { > > > > + LOG(CameraSensor, Error) << "Unsupported test pattern mode: " > > > > + << testPatternMode; > > > > + return -EINVAL; > > > > + } > > > > > > Do we need this ? The testPatternModes_ map is constructed using the > > > camera sensor properties, the below find() on the props->testPatternModes > > > map should gurantee that is supported. Also, if a test pattern mode is > > > not reported as part of testPatternModes_ applications shouldn't set > > > it... > > > > > > > + > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > > + if (!props) > > > > + return -EINVAL; > > > > + > > > > + auto it = props->testPatternModes.find(testPatternMode); > > > > + ASSERT(it != props->testPatternModes.end()); > > > > > > Yeah, I think the assertion here is more than enough.. > > > > > > > + const uint8_t index = it->second; > > > > + > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN }); > > > > + if (ctrls.empty()) { > > > > + LOG(CameraSensor, Error) > > > > + << "Failed to retrieve test pattern control"; > > > > + return -EINVAL; > > > > + } > > > > > > We won't register the TestPatternMode control in first place the V4L2 > > > control is not supported.. > > > > Replace this with ASSERT. > > > > > > > > > + > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN); > > > > + if (value.get<int32_t>() == index) > > > > + return 0; > > > > > > The V4L2 control framework should take care of this by itself, I think > > > we can set the control regardless > > > > > > > Yes, I put this in order to save a redundant Ioctl call. > > Do you want to remove that? > > Well, the ctrls ControlList you create with getControls() goes through > an IOCTL :) We're talking details, but if we want to retain the > state of the testPatterMode we can make it a class member, so that the > code can be reduced to (not tested pseudo-code) > > { > if (testPatternMode == testPatternMode_) > return 0; > > const CameraSensorProperties *props = CameraSensorProperties::get(model_); > if (!props) > return -EINVAL; > > auto it = props->testPatternModes.find(testPatternMode); > ASSERT(it != props->testPatternModes.end()); > > ControlList ctrls(controls()); > ctrls.set(V4L2_CID_TEST_PATTERN, it->second); > > int ret = setControls(&ctrls); > if (ret) > return ret; > > testPatternMode_ = testPatternMode; > > return 0; > } > > I also wonder if the ASSERT() is not too harsh and we shouldn't just > error out. If the application tries to set an unsupported test pattern, > the whole library would crash, maybe we should be a little more > indulgent. > I see what you meant. Thanks! > Thanks > j > > > > > -Hiro > > > > + > > > > + value.set<int32_t>(index); > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value); > > > > + > > > > + return setControls(&ctrls); > > > > +} > > > > + > > > > /** > > > > * \brief Retrieve the best sensor format for a desired output > > > > * \param[in] mbusCodes The list of acceptable media bus codes > > > > -- > > > > 2.32.0.288.g62a8d224e6-goog > > > >
Hi Jacopo, Thank you for the patch. On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote: > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote: > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote: > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote: > > > > Provides a function to set the camera sensor a test pattern mode. > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > --- > > > > include/libcamera/internal/camera_sensor.h | 1 + > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++ > > > > 2 files changed, 40 insertions(+) > > > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > > index e133ebf4..8b9f84c9 100644 > > > > --- a/include/libcamera/internal/camera_sensor.h > > > > +++ b/include/libcamera/internal/camera_sensor.h > > > > @@ -43,6 +43,7 @@ public: > > > > { > > > > return testPatternModes_; > > > > } > > > > + int setTestPatternMode(uint8_t testPatternMode); > > > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > > > const Size &size) const; > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > > index 70bbd97a..ce8ff274 100644 > > > > --- a/src/libcamera/camera_sensor.cpp > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const > > > > * \return The list of test pattern modes > > > > */ > > > > > > > > +/** > > > > + * \brief Set the camera sensor a specified controls::TestPatternMode > > > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not > > > documented but I don't get a warning :/ > > > > > > Also, I'm a bit debated about where to actually place this function > > > (or better, it's initTestPatternModes which is misplaced) > > > if we have to respect the declaration order or the way you sorted them > > > here (which is more logical) is better. > > > > > > > + * > > > > + * \return 0 on success or a negative error code otherwise > > > > + */ > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode) > > > > +{ > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(), > > > > + testPatternMode) == testPatternModes_.end()) { > > > > + LOG(CameraSensor, Error) << "Unsupported test pattern mode: " > > > > + << testPatternMode; > > > > + return -EINVAL; > > > > + } > > > > > > Do we need this ? The testPatternModes_ map is constructed using the > > > camera sensor properties, the below find() on the props->testPatternModes > > > map should gurantee that is supported. Also, if a test pattern mode is > > > not reported as part of testPatternModes_ applications shouldn't set > > > it... > > > > > > > + > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > > + if (!props) > > > > + return -EINVAL; We already retrieve this in CameraSensor::initStaticProperties(), let's cache the pointer in the CameraSensor class. > > > > + > > > > + auto it = props->testPatternModes.find(testPatternMode); > > > > + ASSERT(it != props->testPatternModes.end()); > > > > > > Yeah, I think the assertion here is more than enough.. > > > > > > > + const uint8_t index = it->second; > > > > + > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN }); > > > > + if (ctrls.empty()) { > > > > + LOG(CameraSensor, Error) > > > > + << "Failed to retrieve test pattern control"; > > > > + return -EINVAL; > > > > + } > > > > > > We won't register the TestPatternMode control in first place the V4L2 > > > control is not supported.. > > > > Replace this with ASSERT. > > > > > > + > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN); > > > > + if (value.get<int32_t>() == index) > > > > + return 0; > > > > > > The V4L2 control framework should take care of this by itself, I think > > > we can set the control regardless > > > > Yes, I put this in order to save a redundant Ioctl call. > > Do you want to remove that? > > Well, the ctrls ControlList you create with getControls() goes through > an IOCTL :) We're talking details, but if we want to retain the > state of the testPatterMode we can make it a class member, so that the > code can be reduced to (not tested pseudo-code) > > { > if (testPatternMode == testPatternMode_) > return 0; > > const CameraSensorProperties *props = CameraSensorProperties::get(model_); > if (!props) > return -EINVAL; > > auto it = props->testPatternModes.find(testPatternMode); > ASSERT(it != props->testPatternModes.end()); > > ControlList ctrls(controls()); > ctrls.set(V4L2_CID_TEST_PATTERN, it->second); > > int ret = setControls(&ctrls); > if (ret) > return ret; > > testPatternMode_ = testPatternMode; > > return 0; > } > > I also wonder if the ASSERT() is not too harsh and we shouldn't just > error out. If the application tries to set an unsupported test pattern, > the whole library would crash, maybe we should be a little more > indulgent. Yes, not crashing is a good idea :-) > > > > + > > > > + value.set<int32_t>(index); > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value); > > > > + > > > > + return setControls(&ctrls); > > > > +} > > > > + > > > > /** > > > > * \brief Retrieve the best sensor format for a desired output > > > > * \param[in] mbusCodes The list of acceptable media bus codes
On Mon, Jun 28, 2021 at 06:12:32AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. The patch is from Hiro of course :-) Another comment, do we need such a ad-hoc function, or could we handle the test pattern mode using a more generic interface, with a ControlList ? The CameraSensor::setControls() takes V4L2 controls today, which makes it unsuitable for this purpose. I would ideally like to see all this fixed, but I suppose it's a too big task for this series. It adds up to the technical debt though, so there's a high chance that I'll push back more strongly on the next patch that adds a ad-hoc function to CameraSensor, until we reach a point where the push back will be a strong nack and all this will need to be refactored by an unlucky person. All this means that nobody should think that delaying the handling of technical debt is a smart move to move it to someone else's plate, it may come back around :-) > On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote: > > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote: > > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote: > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote: > > > > > Provides a function to set the camera sensor a test pattern mode. > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > --- > > > > > include/libcamera/internal/camera_sensor.h | 1 + > > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++ > > > > > 2 files changed, 40 insertions(+) > > > > > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > > > index e133ebf4..8b9f84c9 100644 > > > > > --- a/include/libcamera/internal/camera_sensor.h > > > > > +++ b/include/libcamera/internal/camera_sensor.h > > > > > @@ -43,6 +43,7 @@ public: > > > > > { > > > > > return testPatternModes_; > > > > > } > > > > > + int setTestPatternMode(uint8_t testPatternMode); > > > > > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > > > > const Size &size) const; > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > > > index 70bbd97a..ce8ff274 100644 > > > > > --- a/src/libcamera/camera_sensor.cpp > > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const > > > > > * \return The list of test pattern modes > > > > > */ > > > > > > > > > > +/** > > > > > + * \brief Set the camera sensor a specified controls::TestPatternMode > > > > > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not > > > > documented but I don't get a warning :/ > > > > > > > > Also, I'm a bit debated about where to actually place this function > > > > (or better, it's initTestPatternModes which is misplaced) > > > > if we have to respect the declaration order or the way you sorted them > > > > here (which is more logical) is better. > > > > > > > > > + * > > > > > + * \return 0 on success or a negative error code otherwise > > > > > + */ > > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode) > > > > > +{ > > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(), > > > > > + testPatternMode) == testPatternModes_.end()) { > > > > > + LOG(CameraSensor, Error) << "Unsupported test pattern mode: " > > > > > + << testPatternMode; > > > > > + return -EINVAL; > > > > > + } > > > > > > > > Do we need this ? The testPatternModes_ map is constructed using the > > > > camera sensor properties, the below find() on the props->testPatternModes > > > > map should gurantee that is supported. Also, if a test pattern mode is > > > > not reported as part of testPatternModes_ applications shouldn't set > > > > it... > > > > > > > > > + > > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > > > + if (!props) > > > > > + return -EINVAL; > > We already retrieve this in CameraSensor::initStaticProperties(), let's > cache the pointer in the CameraSensor class. > > > > > > + > > > > > + auto it = props->testPatternModes.find(testPatternMode); > > > > > + ASSERT(it != props->testPatternModes.end()); > > > > > > > > Yeah, I think the assertion here is more than enough.. > > > > > > > > > + const uint8_t index = it->second; > > > > > + > > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN }); > > > > > + if (ctrls.empty()) { > > > > > + LOG(CameraSensor, Error) > > > > > + << "Failed to retrieve test pattern control"; > > > > > + return -EINVAL; > > > > > + } > > > > > > > > We won't register the TestPatternMode control in first place the V4L2 > > > > control is not supported.. > > > > > > Replace this with ASSERT. > > > > > > > > + > > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN); > > > > > + if (value.get<int32_t>() == index) > > > > > + return 0; > > > > > > > > The V4L2 control framework should take care of this by itself, I think > > > > we can set the control regardless > > > > > > Yes, I put this in order to save a redundant Ioctl call. > > > Do you want to remove that? > > > > Well, the ctrls ControlList you create with getControls() goes through > > an IOCTL :) We're talking details, but if we want to retain the > > state of the testPatterMode we can make it a class member, so that the > > code can be reduced to (not tested pseudo-code) > > > > { > > if (testPatternMode == testPatternMode_) > > return 0; > > > > const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > if (!props) > > return -EINVAL; > > > > auto it = props->testPatternModes.find(testPatternMode); > > ASSERT(it != props->testPatternModes.end()); > > > > ControlList ctrls(controls()); > > ctrls.set(V4L2_CID_TEST_PATTERN, it->second); > > > > int ret = setControls(&ctrls); > > if (ret) > > return ret; > > > > testPatternMode_ = testPatternMode; > > > > return 0; > > } > > > > I also wonder if the ASSERT() is not too harsh and we shouldn't just > > error out. If the application tries to set an unsupported test pattern, > > the whole library would crash, maybe we should be a little more > > indulgent. > > Yes, not crashing is a good idea :-) > > > > > > + > > > > > + value.set<int32_t>(index); > > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value); > > > > > + > > > > > + return setControls(&ctrls); > > > > > +} > > > > > + > > > > > /** > > > > > * \brief Retrieve the best sensor format for a desired output > > > > > * \param[in] mbusCodes The list of acceptable media bus codes
Hi Laurent, On Mon, Jun 28, 2021 at 12:28 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Mon, Jun 28, 2021 at 06:12:32AM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > The patch is from Hiro of course :-) > > Another comment, do we need such a ad-hoc function, or could we handle > the test pattern mode using a more generic interface, with a ControlList > ? The CameraSensor::setControls() takes V4L2 controls today, which makes > it unsuitable for this purpose. I would ideally like to see all this > fixed, but I suppose it's a too big task for this series. It adds up to > the technical debt though, so there's a high chance that I'll push back > more strongly on the next patch that adds a ad-hoc function to > CameraSensor, until we reach a point where the push back will be a > strong nack and all this will need to be refactored by an unlucky > person. All this means that nobody should think that delaying the > handling of technical debt is a smart move to move it to someone else's > plate, it may come back around :-) > If we want to use CameraSensor::setControls(), it is necessary for a pipeline handler to know the v4l2 index associated with a specific test pattern mode. How can we do that? Is it okay for a pipeline handler to read test pattern mode in camera properties? > > On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote: > > > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote: > > > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote: > > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote: > > > > > > Provides a function to set the camera sensor a test pattern mode. > > > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > --- > > > > > > include/libcamera/internal/camera_sensor.h | 1 + > > > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++ > > > > > > 2 files changed, 40 insertions(+) > > > > > > > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > > > > index e133ebf4..8b9f84c9 100644 > > > > > > --- a/include/libcamera/internal/camera_sensor.h > > > > > > +++ b/include/libcamera/internal/camera_sensor.h > > > > > > @@ -43,6 +43,7 @@ public: > > > > > > { > > > > > > return testPatternModes_; > > > > > > } > > > > > > + int setTestPatternMode(uint8_t testPatternMode); > > > > > > > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > > > > > const Size &size) const; > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > > > > index 70bbd97a..ce8ff274 100644 > > > > > > --- a/src/libcamera/camera_sensor.cpp > > > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const > > > > > > * \return The list of test pattern modes > > > > > > */ > > > > > > > > > > > > +/** > > > > > > + * \brief Set the camera sensor a specified controls::TestPatternMode > > > > > > > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not > > > > > documented but I don't get a warning :/ > > > > > > > > > > Also, I'm a bit debated about where to actually place this function > > > > > (or better, it's initTestPatternModes which is misplaced) > > > > > if we have to respect the declaration order or the way you sorted them > > > > > here (which is more logical) is better. > > > > > > > > > > > + * > > > > > > + * \return 0 on success or a negative error code otherwise > > > > > > + */ > > > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode) > > > > > > +{ > > > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(), > > > > > > + testPatternMode) == testPatternModes_.end()) { > > > > > > + LOG(CameraSensor, Error) << "Unsupported test pattern mode: " > > > > > > + << testPatternMode; > > > > > > + return -EINVAL; > > > > > > + } > > > > > > > > > > Do we need this ? The testPatternModes_ map is constructed using the > > > > > camera sensor properties, the below find() on the props->testPatternModes > > > > > map should gurantee that is supported. Also, if a test pattern mode is > > > > > not reported as part of testPatternModes_ applications shouldn't set > > > > > it... > > > > > > > > > > > + > > > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > > > > + if (!props) > > > > > > + return -EINVAL; > > > > We already retrieve this in CameraSensor::initStaticProperties(), let's > > cache the pointer in the CameraSensor class. > > > > > > > > + > > > > > > + auto it = props->testPatternModes.find(testPatternMode); > > > > > > + ASSERT(it != props->testPatternModes.end()); > > > > > > > > > > Yeah, I think the assertion here is more than enough.. > > > > > > > > > > > + const uint8_t index = it->second; > > > > > > + > > > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN }); > > > > > > + if (ctrls.empty()) { > > > > > > + LOG(CameraSensor, Error) > > > > > > + << "Failed to retrieve test pattern control"; > > > > > > + return -EINVAL; > > > > > > + } > > > > > > > > > > We won't register the TestPatternMode control in first place the V4L2 > > > > > control is not supported.. > > > > > > > > Replace this with ASSERT. > > > > > > > > > > + > > > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN); > > > > > > + if (value.get<int32_t>() == index) > > > > > > + return 0; > > > > > > > > > > The V4L2 control framework should take care of this by itself, I think > > > > > we can set the control regardless > > > > > > > > Yes, I put this in order to save a redundant Ioctl call. > > > > Do you want to remove that? > > > > > > Well, the ctrls ControlList you create with getControls() goes through > > > an IOCTL :) We're talking details, but if we want to retain the > > > state of the testPatterMode we can make it a class member, so that the > > > code can be reduced to (not tested pseudo-code) > > > > > > { > > > if (testPatternMode == testPatternMode_) > > > return 0; > > > > > > const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > if (!props) > > > return -EINVAL; > > > > > > auto it = props->testPatternModes.find(testPatternMode); > > > ASSERT(it != props->testPatternModes.end()); > > > > > > ControlList ctrls(controls()); > > > ctrls.set(V4L2_CID_TEST_PATTERN, it->second); > > > > > > int ret = setControls(&ctrls); > > > if (ret) > > > return ret; > > > > > > testPatternMode_ = testPatternMode; > > > > > > return 0; > > > } > > > > > > I also wonder if the ASSERT() is not too harsh and we shouldn't just > > > error out. If the application tries to set an unsupported test pattern, > > > the whole library would crash, maybe we should be a little more > > > indulgent. > > > > Yes, not crashing is a good idea :-) > > > > > > > > + > > > > > > + value.set<int32_t>(index); > > > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value); > > > > > > + > > > > > > + return setControls(&ctrls); > > > > > > +} > > > > > > + > > > > > > /** > > > > > > * \brief Retrieve the best sensor format for a desired output > > > > > > * \param[in] mbusCodes The list of acceptable media bus codes > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Mon, Jun 28, 2021 at 02:16:00PM +0900, Hirokazu Honda wrote: > On Mon, Jun 28, 2021 at 12:28 PM Laurent Pinchart wrote: > > On Mon, Jun 28, 2021 at 06:12:32AM +0300, Laurent Pinchart wrote: > > > Hi Jacopo, > > > > > > Thank you for the patch. > > > > The patch is from Hiro of course :-) > > > > Another comment, do we need such a ad-hoc function, or could we handle > > the test pattern mode using a more generic interface, with a ControlList > > ? The CameraSensor::setControls() takes V4L2 controls today, which makes > > it unsuitable for this purpose. I would ideally like to see all this > > fixed, but I suppose it's a too big task for this series. It adds up to > > the technical debt though, so there's a high chance that I'll push back > > more strongly on the next patch that adds a ad-hoc function to > > CameraSensor, until we reach a point where the push back will be a > > strong nack and all this will need to be refactored by an unlucky > > person. All this means that nobody should think that delaying the > > handling of technical debt is a smart move to move it to someone else's > > plate, it may come back around :-) > > If we want to use CameraSensor::setControls(), it is necessary for a > pipeline handler to know the v4l2 index associated with a specific > test pattern mode. > How can we do that? Is it okay for a pipeline handler to read test > pattern mode in camera properties? I'd prefer going the other way around and avoid exposing V4L2 controls from CameraSensor. I haven't really thought about how to do so yet though. > > > On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote: > > > > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote: > > > > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote: > > > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote: > > > > > > > Provides a function to set the camera sensor a test pattern mode. > > > > > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > --- > > > > > > > include/libcamera/internal/camera_sensor.h | 1 + > > > > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++ > > > > > > > 2 files changed, 40 insertions(+) > > > > > > > > > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > > > > > index e133ebf4..8b9f84c9 100644 > > > > > > > --- a/include/libcamera/internal/camera_sensor.h > > > > > > > +++ b/include/libcamera/internal/camera_sensor.h > > > > > > > @@ -43,6 +43,7 @@ public: > > > > > > > { > > > > > > > return testPatternModes_; > > > > > > > } > > > > > > > + int setTestPatternMode(uint8_t testPatternMode); > > > > > > > > > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > > > > > > const Size &size) const; > > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > > > > > index 70bbd97a..ce8ff274 100644 > > > > > > > --- a/src/libcamera/camera_sensor.cpp > > > > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const > > > > > > > * \return The list of test pattern modes > > > > > > > */ > > > > > > > > > > > > > > +/** > > > > > > > + * \brief Set the camera sensor a specified controls::TestPatternMode > > > > > > > > > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not > > > > > > documented but I don't get a warning :/ > > > > > > > > > > > > Also, I'm a bit debated about where to actually place this function > > > > > > (or better, it's initTestPatternModes which is misplaced) > > > > > > if we have to respect the declaration order or the way you sorted them > > > > > > here (which is more logical) is better. > > > > > > > > > > > > > + * > > > > > > > + * \return 0 on success or a negative error code otherwise > > > > > > > + */ > > > > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode) > > > > > > > +{ > > > > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(), > > > > > > > + testPatternMode) == testPatternModes_.end()) { > > > > > > > + LOG(CameraSensor, Error) << "Unsupported test pattern mode: " > > > > > > > + << testPatternMode; > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > > > > > > > Do we need this ? The testPatternModes_ map is constructed using the > > > > > > camera sensor properties, the below find() on the props->testPatternModes > > > > > > map should gurantee that is supported. Also, if a test pattern mode is > > > > > > not reported as part of testPatternModes_ applications shouldn't set > > > > > > it... > > > > > > > > > > > > > + > > > > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > > > > > + if (!props) > > > > > > > + return -EINVAL; > > > > > > We already retrieve this in CameraSensor::initStaticProperties(), let's > > > cache the pointer in the CameraSensor class. > > > > > > > > > > + > > > > > > > + auto it = props->testPatternModes.find(testPatternMode); > > > > > > > + ASSERT(it != props->testPatternModes.end()); > > > > > > > > > > > > Yeah, I think the assertion here is more than enough.. > > > > > > > > > > > > > + const uint8_t index = it->second; > > > > > > > + > > > > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN }); > > > > > > > + if (ctrls.empty()) { > > > > > > > + LOG(CameraSensor, Error) > > > > > > > + << "Failed to retrieve test pattern control"; > > > > > > > + return -EINVAL; > > > > > > > + } > > > > > > > > > > > > We won't register the TestPatternMode control in first place the V4L2 > > > > > > control is not supported.. > > > > > > > > > > Replace this with ASSERT. > > > > > > > > > > > > + > > > > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN); > > > > > > > + if (value.get<int32_t>() == index) > > > > > > > + return 0; > > > > > > > > > > > > The V4L2 control framework should take care of this by itself, I think > > > > > > we can set the control regardless > > > > > > > > > > Yes, I put this in order to save a redundant Ioctl call. > > > > > Do you want to remove that? > > > > > > > > Well, the ctrls ControlList you create with getControls() goes through > > > > an IOCTL :) We're talking details, but if we want to retain the > > > > state of the testPatterMode we can make it a class member, so that the > > > > code can be reduced to (not tested pseudo-code) > > > > > > > > { > > > > if (testPatternMode == testPatternMode_) > > > > return 0; > > > > > > > > const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > > if (!props) > > > > return -EINVAL; > > > > > > > > auto it = props->testPatternModes.find(testPatternMode); > > > > ASSERT(it != props->testPatternModes.end()); > > > > > > > > ControlList ctrls(controls()); > > > > ctrls.set(V4L2_CID_TEST_PATTERN, it->second); > > > > > > > > int ret = setControls(&ctrls); > > > > if (ret) > > > > return ret; > > > > > > > > testPatternMode_ = testPatternMode; > > > > > > > > return 0; > > > > } > > > > > > > > I also wonder if the ASSERT() is not too harsh and we shouldn't just > > > > error out. If the application tries to set an unsupported test pattern, > > > > the whole library would crash, maybe we should be a little more > > > > indulgent. > > > > > > Yes, not crashing is a good idea :-) > > > > > > > > > > + > > > > > > > + value.set<int32_t>(index); > > > > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value); > > > > > > > + > > > > > > > + return setControls(&ctrls); > > > > > > > +} > > > > > > > + > > > > > > > /** > > > > > > > * \brief Retrieve the best sensor format for a desired output > > > > > > > * \param[in] mbusCodes The list of acceptable media bus codes
Hi Laurent, On Mon, Jun 28, 2021 at 2:26 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Mon, Jun 28, 2021 at 02:16:00PM +0900, Hirokazu Honda wrote: > > On Mon, Jun 28, 2021 at 12:28 PM Laurent Pinchart wrote: > > > On Mon, Jun 28, 2021 at 06:12:32AM +0300, Laurent Pinchart wrote: > > > > Hi Jacopo, > > > > > > > > Thank you for the patch. > > > > > > The patch is from Hiro of course :-) > > > > > > Another comment, do we need such a ad-hoc function, or could we handle > > > the test pattern mode using a more generic interface, with a ControlList > > > ? The CameraSensor::setControls() takes V4L2 controls today, which makes > > > it unsuitable for this purpose. I would ideally like to see all this > > > fixed, but I suppose it's a too big task for this series. It adds up to > > > the technical debt though, so there's a high chance that I'll push back > > > more strongly on the next patch that adds a ad-hoc function to > > > CameraSensor, until we reach a point where the push back will be a > > > strong nack and all this will need to be refactored by an unlucky > > > person. All this means that nobody should think that delaying the > > > handling of technical debt is a smart move to move it to someone else's > > > plate, it may come back around :-) > > > > If we want to use CameraSensor::setControls(), it is necessary for a > > pipeline handler to know the v4l2 index associated with a specific > > test pattern mode. > > How can we do that? Is it okay for a pipeline handler to read test > > pattern mode in camera properties? > > I'd prefer going the other way around and avoid exposing V4L2 controls > from CameraSensor. I haven't really thought about how to do so yet > though. > Hmm, so how shall I do in this patch series? > > > > On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote: > > > > > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote: > > > > > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote: > > > > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote: > > > > > > > > Provides a function to set the camera sensor a test pattern mode. > > > > > > > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > > --- > > > > > > > > include/libcamera/internal/camera_sensor.h | 1 + > > > > > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++ > > > > > > > > 2 files changed, 40 insertions(+) > > > > > > > > > > > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > > > > > > index e133ebf4..8b9f84c9 100644 > > > > > > > > --- a/include/libcamera/internal/camera_sensor.h > > > > > > > > +++ b/include/libcamera/internal/camera_sensor.h > > > > > > > > @@ -43,6 +43,7 @@ public: > > > > > > > > { > > > > > > > > return testPatternModes_; > > > > > > > > } > > > > > > > > + int setTestPatternMode(uint8_t testPatternMode); > > > > > > > > > > > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > > > > > > > const Size &size) const; > > > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > > > > > > index 70bbd97a..ce8ff274 100644 > > > > > > > > --- a/src/libcamera/camera_sensor.cpp > > > > > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const > > > > > > > > * \return The list of test pattern modes > > > > > > > > */ > > > > > > > > > > > > > > > > +/** > > > > > > > > + * \brief Set the camera sensor a specified controls::TestPatternMode > > > > > > > > > > > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not > > > > > > > documented but I don't get a warning :/ > > > > > > > > > > > > > > Also, I'm a bit debated about where to actually place this function > > > > > > > (or better, it's initTestPatternModes which is misplaced) > > > > > > > if we have to respect the declaration order or the way you sorted them > > > > > > > here (which is more logical) is better. > > > > > > > > > > > > > > > + * > > > > > > > > + * \return 0 on success or a negative error code otherwise > > > > > > > > + */ > > > > > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode) > > > > > > > > +{ > > > > > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(), > > > > > > > > + testPatternMode) == testPatternModes_.end()) { > > > > > > > > + LOG(CameraSensor, Error) << "Unsupported test pattern mode: " > > > > > > > > + << testPatternMode; > > > > > > > > + return -EINVAL; > > > > > > > > + } > > > > > > > > > > > > > > Do we need this ? The testPatternModes_ map is constructed using the > > > > > > > camera sensor properties, the below find() on the props->testPatternModes > > > > > > > map should gurantee that is supported. Also, if a test pattern mode is > > > > > > > not reported as part of testPatternModes_ applications shouldn't set > > > > > > > it... > > > > > > > > > > > > > > > + > > > > > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > > > > > > + if (!props) > > > > > > > > + return -EINVAL; > > > > > > > > We already retrieve this in CameraSensor::initStaticProperties(), let's > > > > cache the pointer in the CameraSensor class. > > > > > > > > > > > > + > > > > > > > > + auto it = props->testPatternModes.find(testPatternMode); > > > > > > > > + ASSERT(it != props->testPatternModes.end()); > > > > > > > > > > > > > > Yeah, I think the assertion here is more than enough.. > > > > > > > > > > > > > > > + const uint8_t index = it->second; > > > > > > > > + > > > > > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN }); > > > > > > > > + if (ctrls.empty()) { > > > > > > > > + LOG(CameraSensor, Error) > > > > > > > > + << "Failed to retrieve test pattern control"; > > > > > > > > + return -EINVAL; > > > > > > > > + } > > > > > > > > > > > > > > We won't register the TestPatternMode control in first place the V4L2 > > > > > > > control is not supported.. > > > > > > > > > > > > Replace this with ASSERT. > > > > > > > > > > > > > > + > > > > > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN); > > > > > > > > + if (value.get<int32_t>() == index) > > > > > > > > + return 0; > > > > > > > > > > > > > > The V4L2 control framework should take care of this by itself, I think > > > > > > > we can set the control regardless > > > > > > > > > > > > Yes, I put this in order to save a redundant Ioctl call. > > > > > > Do you want to remove that? > > > > > > > > > > Well, the ctrls ControlList you create with getControls() goes through > > > > > an IOCTL :) We're talking details, but if we want to retain the > > > > > state of the testPatterMode we can make it a class member, so that the > > > > > code can be reduced to (not tested pseudo-code) > > > > > > > > > > { > > > > > if (testPatternMode == testPatternMode_) > > > > > return 0; > > > > > > > > > > const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > > > if (!props) > > > > > return -EINVAL; > > > > > > > > > > auto it = props->testPatternModes.find(testPatternMode); > > > > > ASSERT(it != props->testPatternModes.end()); > > > > > > > > > > ControlList ctrls(controls()); > > > > > ctrls.set(V4L2_CID_TEST_PATTERN, it->second); > > > > > > > > > > int ret = setControls(&ctrls); > > > > > if (ret) > > > > > return ret; > > > > > > > > > > testPatternMode_ = testPatternMode; > > > > > > > > > > return 0; > > > > > } > > > > > > > > > > I also wonder if the ASSERT() is not too harsh and we shouldn't just > > > > > error out. If the application tries to set an unsupported test pattern, > > > > > the whole library would crash, maybe we should be a little more > > > > > indulgent. > > > > > > > > Yes, not crashing is a good idea :-) > > > > > > > > > > > > + > > > > > > > > + value.set<int32_t>(index); > > > > > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value); > > > > > > > > + > > > > > > > > + return setControls(&ctrls); > > > > > > > > +} > > > > > > > > + > > > > > > > > /** > > > > > > > > * \brief Retrieve the best sensor format for a desired output > > > > > > > > * \param[in] mbusCodes The list of acceptable media bus codes > > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Mon, Jun 28, 2021 at 02:33:56PM +0900, Hirokazu Honda wrote: > On Mon, Jun 28, 2021 at 2:26 PM Laurent Pinchart wrote: > > On Mon, Jun 28, 2021 at 02:16:00PM +0900, Hirokazu Honda wrote: > > > On Mon, Jun 28, 2021 at 12:28 PM Laurent Pinchart wrote: > > > > On Mon, Jun 28, 2021 at 06:12:32AM +0300, Laurent Pinchart wrote: > > > > > Hi Jacopo, > > > > > > > > > > Thank you for the patch. > > > > > > > > The patch is from Hiro of course :-) > > > > > > > > Another comment, do we need such a ad-hoc function, or could we handle > > > > the test pattern mode using a more generic interface, with a ControlList > > > > ? The CameraSensor::setControls() takes V4L2 controls today, which makes > > > > it unsuitable for this purpose. I would ideally like to see all this > > > > fixed, but I suppose it's a too big task for this series. It adds up to > > > > the technical debt though, so there's a high chance that I'll push back > > > > more strongly on the next patch that adds a ad-hoc function to > > > > CameraSensor, until we reach a point where the push back will be a > > > > strong nack and all this will need to be refactored by an unlucky > > > > person. All this means that nobody should think that delaying the > > > > handling of technical debt is a smart move to move it to someone else's > > > > plate, it may come back around :-) > > > > > > If we want to use CameraSensor::setControls(), it is necessary for a > > > pipeline handler to know the v4l2 index associated with a specific > > > test pattern mode. > > > How can we do that? Is it okay for a pipeline handler to read test > > > pattern mode in camera properties? > > > > I'd prefer going the other way around and avoid exposing V4L2 controls > > from CameraSensor. I haven't really thought about how to do so yet > > though. > > Hmm, so how shall I do in this patch series? I think it may be too big for this series, so I'm ok adding the setTestPatternMode() for now. We'll have to address this issue though, so if you think of a way this could be done, we can start brainstorming it. > > > > > On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote: > > > > > > On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote: > > > > > > > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote: > > > > > > > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote: > > > > > > > > > Provides a function to set the camera sensor a test pattern mode. > > > > > > > > > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > > > > > --- > > > > > > > > > include/libcamera/internal/camera_sensor.h | 1 + > > > > > > > > > src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++ > > > > > > > > > 2 files changed, 40 insertions(+) > > > > > > > > > > > > > > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > > > > > > > > > index e133ebf4..8b9f84c9 100644 > > > > > > > > > --- a/include/libcamera/internal/camera_sensor.h > > > > > > > > > +++ b/include/libcamera/internal/camera_sensor.h > > > > > > > > > @@ -43,6 +43,7 @@ public: > > > > > > > > > { > > > > > > > > > return testPatternModes_; > > > > > > > > > } > > > > > > > > > + int setTestPatternMode(uint8_t testPatternMode); > > > > > > > > > > > > > > > > > > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, > > > > > > > > > const Size &size) const; > > > > > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > > > > > > > > > index 70bbd97a..ce8ff274 100644 > > > > > > > > > --- a/src/libcamera/camera_sensor.cpp > > > > > > > > > +++ b/src/libcamera/camera_sensor.cpp > > > > > > > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const > > > > > > > > > * \return The list of test pattern modes > > > > > > > > > */ > > > > > > > > > > > > > > > > > > +/** > > > > > > > > > + * \brief Set the camera sensor a specified controls::TestPatternMode > > > > > > > > > > > > > > > > Doxygen is puzzling me recently... the testPatternMode paramter is not > > > > > > > > documented but I don't get a warning :/ > > > > > > > > > > > > > > > > Also, I'm a bit debated about where to actually place this function > > > > > > > > (or better, it's initTestPatternModes which is misplaced) > > > > > > > > if we have to respect the declaration order or the way you sorted them > > > > > > > > here (which is more logical) is better. > > > > > > > > > > > > > > > > > + * > > > > > > > > > + * \return 0 on success or a negative error code otherwise > > > > > > > > > + */ > > > > > > > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode) > > > > > > > > > +{ > > > > > > > > > + if (std::find(testPatternModes_.begin(), testPatternModes_.end(), > > > > > > > > > + testPatternMode) == testPatternModes_.end()) { > > > > > > > > > + LOG(CameraSensor, Error) << "Unsupported test pattern mode: " > > > > > > > > > + << testPatternMode; > > > > > > > > > + return -EINVAL; > > > > > > > > > + } > > > > > > > > > > > > > > > > Do we need this ? The testPatternModes_ map is constructed using the > > > > > > > > camera sensor properties, the below find() on the props->testPatternModes > > > > > > > > map should gurantee that is supported. Also, if a test pattern mode is > > > > > > > > not reported as part of testPatternModes_ applications shouldn't set > > > > > > > > it... > > > > > > > > > > > > > > > > > + > > > > > > > > > + const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > > > > > > > + if (!props) > > > > > > > > > + return -EINVAL; > > > > > > > > > > We already retrieve this in CameraSensor::initStaticProperties(), let's > > > > > cache the pointer in the CameraSensor class. > > > > > > > > > > > > > > + > > > > > > > > > + auto it = props->testPatternModes.find(testPatternMode); > > > > > > > > > + ASSERT(it != props->testPatternModes.end()); > > > > > > > > > > > > > > > > Yeah, I think the assertion here is more than enough.. > > > > > > > > > > > > > > > > > + const uint8_t index = it->second; > > > > > > > > > + > > > > > > > > > + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN }); > > > > > > > > > + if (ctrls.empty()) { > > > > > > > > > + LOG(CameraSensor, Error) > > > > > > > > > + << "Failed to retrieve test pattern control"; > > > > > > > > > + return -EINVAL; > > > > > > > > > + } > > > > > > > > > > > > > > > > We won't register the TestPatternMode control in first place the V4L2 > > > > > > > > control is not supported.. > > > > > > > > > > > > > > Replace this with ASSERT. > > > > > > > > > > > > > > > > + > > > > > > > > > + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN); > > > > > > > > > + if (value.get<int32_t>() == index) > > > > > > > > > + return 0; > > > > > > > > > > > > > > > > The V4L2 control framework should take care of this by itself, I think > > > > > > > > we can set the control regardless > > > > > > > > > > > > > > Yes, I put this in order to save a redundant Ioctl call. > > > > > > > Do you want to remove that? > > > > > > > > > > > > Well, the ctrls ControlList you create with getControls() goes through > > > > > > an IOCTL :) We're talking details, but if we want to retain the > > > > > > state of the testPatterMode we can make it a class member, so that the > > > > > > code can be reduced to (not tested pseudo-code) > > > > > > > > > > > > { > > > > > > if (testPatternMode == testPatternMode_) > > > > > > return 0; > > > > > > > > > > > > const CameraSensorProperties *props = CameraSensorProperties::get(model_); > > > > > > if (!props) > > > > > > return -EINVAL; > > > > > > > > > > > > auto it = props->testPatternModes.find(testPatternMode); > > > > > > ASSERT(it != props->testPatternModes.end()); > > > > > > > > > > > > ControlList ctrls(controls()); > > > > > > ctrls.set(V4L2_CID_TEST_PATTERN, it->second); > > > > > > > > > > > > int ret = setControls(&ctrls); > > > > > > if (ret) > > > > > > return ret; > > > > > > > > > > > > testPatternMode_ = testPatternMode; > > > > > > > > > > > > return 0; > > > > > > } > > > > > > > > > > > > I also wonder if the ASSERT() is not too harsh and we shouldn't just > > > > > > error out. If the application tries to set an unsupported test pattern, > > > > > > the whole library would crash, maybe we should be a little more > > > > > > indulgent. > > > > > > > > > > Yes, not crashing is a good idea :-) > > > > > > > > > > > > > > + > > > > > > > > > + value.set<int32_t>(index); > > > > > > > > > + ctrls.set(V4L2_CID_TEST_PATTERN, value); > > > > > > > > > + > > > > > > > > > + return setControls(&ctrls); > > > > > > > > > +} > > > > > > > > > + > > > > > > > > > /** > > > > > > > > > * \brief Retrieve the best sensor format for a desired output > > > > > > > > > * \param[in] mbusCodes The list of acceptable media bus codes
diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h index e133ebf4..8b9f84c9 100644 --- a/include/libcamera/internal/camera_sensor.h +++ b/include/libcamera/internal/camera_sensor.h @@ -43,6 +43,7 @@ public: { return testPatternModes_; } + int setTestPatternMode(uint8_t testPatternMode); V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes, const Size &size) const; diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 70bbd97a..ce8ff274 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const * \return The list of test pattern modes */ +/** + * \brief Set the camera sensor a specified controls::TestPatternMode + * + * \return 0 on success or a negative error code otherwise + */ +int CameraSensor::setTestPatternMode(uint8_t testPatternMode) +{ + if (std::find(testPatternModes_.begin(), testPatternModes_.end(), + testPatternMode) == testPatternModes_.end()) { + LOG(CameraSensor, Error) << "Unsupported test pattern mode: " + << testPatternMode; + return -EINVAL; + } + + const CameraSensorProperties *props = CameraSensorProperties::get(model_); + if (!props) + return -EINVAL; + + auto it = props->testPatternModes.find(testPatternMode); + ASSERT(it != props->testPatternModes.end()); + const uint8_t index = it->second; + + ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN }); + if (ctrls.empty()) { + LOG(CameraSensor, Error) + << "Failed to retrieve test pattern control"; + return -EINVAL; + } + + ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN); + if (value.get<int32_t>() == index) + return 0; + + value.set<int32_t>(index); + ctrls.set(V4L2_CID_TEST_PATTERN, value); + + return setControls(&ctrls); +} + /** * \brief Retrieve the best sensor format for a desired output * \param[in] mbusCodes The list of acceptable media bus codes
Provides a function to set the camera sensor a test pattern mode. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- include/libcamera/internal/camera_sensor.h | 1 + src/libcamera/camera_sensor.cpp | 39 ++++++++++++++++++++++ 2 files changed, 40 insertions(+)