[libcamera-devel,v4,2/3] libcamera: camera_sensor: Enable to set a test pattern mode

14453 diff mbox series

This adds a function to set a camera sensor driver a test pattern
mode.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 include/libcamera/internal/camera_sensor.h |  8 ++
 src/libcamera/camera_sensor.cpp            | 95 ++++++++++++++++++++--
 src/libcamera/control_ids.yaml             |  5 ++
 3 files changed, 100 insertions(+), 8 deletions(-)

Hi Hiro

On Thu, Nov 04, 2021 at 03:42:51PM +0900, Hirokazu Honda wrote:
> This adds a function to set a camera sensor driver a test pattern
> mode.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/camera_sensor.h |  8 ++
>  src/libcamera/camera_sensor.cpp            | 95 ++++++++++++++++++++--
>  src/libcamera/control_ids.yaml             |  5 ++
>  3 files changed, 100 insertions(+), 8 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index edef2220..40db792a 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -27,6 +27,9 @@ namespace libcamera {
>
>  class BayerFormat;
>  class MediaEntity;
> +class Request;
> +
> +struct CameraSensorProperties;
>
>  class CameraSensor : protected Loggable
>  {
> @@ -47,6 +50,7 @@ public:
>  	{
>  		return testPatternModes_;
>  	}
> +	int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);

Should testPatternMode be a const & ? It's a int32_t so it might not make any
difference
>
>  	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
>  				      const Size &size) const;
> @@ -55,6 +59,7 @@ public:
>  	const ControlInfoMap &controls() const;
>  	ControlList getControls(const std::vector<uint32_t> &ids);
>  	int setControls(ControlList *ctrls);
> +	int applyRequestControls(Request *request);

Please introduce this when it is used or in a separate patch before
3/3.

Also, we now have setControls(ControlList) and
applyRequestControls(Request). The first takes V4L2 controls and apply
them to the sensor subdevice. The second takes libcamera::controls
from the Request and applies one of them according to a notion of
priority that should not be defined in this class imho.

We indeed have to finalize the CameraSensor class controls interface
once all the use cases are handled, but for the moment I won't add
more stuff here. I know Kieran was instead happy with this, so let's
see what his take is.

>
>  	V4L2Subdevice *device() { return subdev_.get(); }
>
> @@ -82,6 +87,8 @@ private:
>  	std::unique_ptr<V4L2Subdevice> subdev_;
>  	unsigned int pad_;
>
> +	const CameraSensorProperties *staticProps_;
> +
>  	std::string model_;
>  	std::string id_;
>
> @@ -89,6 +96,7 @@ private:
>  	std::vector<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
>  	std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> +	controls::draft::TestPatternModeEnum testPatternMode_;
>
>  	Size pixelArraySize_;
>  	Rectangle activeArea_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index f0aa9f24..bb429b3e 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -17,6 +17,7 @@
>  #include <string.h>
>
>  #include <libcamera/property_ids.h>
> +#include <libcamera/request.h>
>
>  #include <libcamera/base/utils.h>
>
> @@ -54,8 +55,9 @@ LOG_DEFINE_CATEGORY(CameraSensor)
>   * Once constructed the instance must be initialized with init().
>   */
>  CameraSensor::CameraSensor(const MediaEntity *entity)
> -	: entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),
> -	  properties_(properties::properties)
> +	: entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),
> +	  testPatternMode_(controls::draft::TestPatternModeUnset),
> +	  bayerFormat_(nullptr), properties_(properties::properties)
>  {
>  }
>
> @@ -300,25 +302,30 @@ void CameraSensor::initVimcDefaultProperties()
>
>  void CameraSensor::initStaticProperties()
>  {
> -	const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> -	if (!props)
> +	staticProps_ = CameraSensorProperties::get(model_);
> +	if (!staticProps_)
>  		return;
>
>  	/* Register the properties retrieved from the sensor database. */
> -	properties_.set(properties::UnitCellSize, props->unitCellSize);
> +	properties_.set(properties::UnitCellSize, staticProps_->unitCellSize);
>
> -	initTestPatternModes(props->testPatternModes);
> +	initTestPatternModes(staticProps_->testPatternModes);
>  }
>
>  void CameraSensor::initTestPatternModes(
>  	const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
>  {
> -	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> -	if (v4l2TestPattern == controls().end()) {
> +	if (testPatternModes.empty()) {
>  		LOG(CameraSensor, Debug) << "No static test pattern map for \'"
>  					 << model() << "\'";
>  		return;
>  	}
> +	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> +	if (v4l2TestPattern == controls().end()) {
> +		LOG(CameraSensor, Debug)
> +			<< "V4L2_CID_TEST_PATTERN is not supported";
> +		return;
> +	}
>
>  	/*
>  	 * Create a map that associates the V4L2 control index to the test
> @@ -531,6 +538,44 @@ Size CameraSensor::resolution() const
>   * \return The list of test pattern modes
>   */
>
> +/**
> + * \brief Set the test pattern mode for the camera sensor
> + * \param[in] testPatternMode Test pattern mode control value to set the camera
> + * sensor
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CameraSensor::setTestPatternMode(
> +	controls::draft::TestPatternModeEnum testPatternMode)

Fits on one line maybe ?

> +{
> +	if (testPatternMode_ == testPatternMode)
> +		return 0;
> +
> +	if (!staticProps_) {
> +		return 0;
> +	}

No braces.

And this anyway won't catch the !V4L2_CID_TEST_PATTERN.

As suggested in the previous version you can check for

        if (testPatternModes_.empty())
                return 0;

as it will evaluate to true if:
- No staticProps_
- staticProps_->testPatternModes.empty()
- V4L2_CID_TEST_PATTERN not available

> +
> +	auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> +			    testPatternMode);
> +	if (it != testPatternModes_.end()) {

You can also simply rely on this, but you will have an error message
in case any of the above three conditions is true, something the class
already complains about at initialization time and that I would rather
handle slently here.

Let's wait for comments about applyRequestControls()

Thanks
   j

> +		LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> +					 << testPatternMode;
> +		return -EINVAL;
> +	}
> +
> +	int32_t index = staticProps_->testPatternModes.at(testPatternMode);
> +	ControlList ctrls{ controls() };
> +	ctrls.set(V4L2_CID_TEST_PATTERN, index);
> +
> +	int ret = setControls(&ctrls);
> +	if (ret)
> +		return ret;
> +
> +	testPatternMode_ = testPatternMode;
> +
> +	return 0;
> +}
> +
>  /**
>   * \brief Retrieve the best sensor format for a desired output
>   * \param[in] mbusCodes The list of acceptable media bus codes
> @@ -705,6 +750,40 @@ int CameraSensor::setControls(ControlList *ctrls)
>  	return subdev_->setControls(ctrls);
>  }
>
> +/**
> + * \brief Apply controls associated with Request
> + * \param[in] request Request that may contain contorls to be applied
> + *
> + * Some controls have to be applied for a capture associated with Request.
> + * This picks up such controls and set the driver them.
> + *
> + * \return 0 on success or an error code otherwise
> + */
> +int32_t CameraSensor::applyRequestControls(Request *request)
> +{
> +	/* Assumes applying the test pattern mode affects immediately. */
> +	if (request->controls().contains(controls::draft::TestPatternMode)) {
> +		const int32_t testPatternMode = request->controls().get(
> +			controls::draft::TestPatternMode);
> +
> +		LOG(CameraSensor, Debug) << "Apply test pattern mode: "
> +					 << testPatternMode;
> +
> +		int ret = setTestPatternMode(
> +			static_cast<controls::draft::TestPatternModeEnum>(testPatternMode));
> +		if (ret) {
> +			LOG(CameraSensor, Error)
> +				<< "Failed to set test pattern mode: " << ret;
> +			return ret;
> +		}
> +
> +		request->metadata().set(controls::draft::TestPatternMode,
> +					testPatternMode);
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * \fn CameraSensor::device()
>   * \brief Retrieve the camera sensor device
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 9d4638ae..083bac7b 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -639,6 +639,11 @@ controls:
>          Control to select the test pattern mode. Currently identical to
>          ANDROID_SENSOR_TEST_PATTERN_MODE.
>        enum:
> +        - name: TestPatternModeUnset
> +          value: -1
> +          description: |
> +            No test pattern is set. Returned frames by the camera device are
> +            undefined.
>          - name: TestPatternModeOff
>            value: 0
>            description: |
> --
> 2.33.1.1089.g2158813163f-goog
>

Kieran, could you review this patch?

Thanks,
-Hiro

On Thu, Nov 4, 2021 at 6:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Hiro
>
> On Thu, Nov 04, 2021 at 03:42:51PM +0900, Hirokazu Honda wrote:
> > This adds a function to set a camera sensor driver a test pattern
> > mode.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  8 ++
> >  src/libcamera/camera_sensor.cpp            | 95 ++++++++++++++++++++--
> >  src/libcamera/control_ids.yaml             |  5 ++
> >  3 files changed, 100 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index edef2220..40db792a 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -27,6 +27,9 @@ namespace libcamera {
> >
> >  class BayerFormat;
> >  class MediaEntity;
> > +class Request;
> > +
> > +struct CameraSensorProperties;
> >
> >  class CameraSensor : protected Loggable
> >  {
> > @@ -47,6 +50,7 @@ public:
> >       {
> >               return testPatternModes_;
> >       }
> > +     int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
>
> Should testPatternMode be a const & ? It's a int32_t so it might not make any
> difference
> >
> >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> >                                     const Size &size) const;
> > @@ -55,6 +59,7 @@ public:
> >       const ControlInfoMap &controls() const;
> >       ControlList getControls(const std::vector<uint32_t> &ids);
> >       int setControls(ControlList *ctrls);
> > +     int applyRequestControls(Request *request);
>
> Please introduce this when it is used or in a separate patch before
> 3/3.
>
> Also, we now have setControls(ControlList) and
> applyRequestControls(Request). The first takes V4L2 controls and apply
> them to the sensor subdevice. The second takes libcamera::controls
> from the Request and applies one of them according to a notion of
> priority that should not be defined in this class imho.
>
> We indeed have to finalize the CameraSensor class controls interface
> once all the use cases are handled, but for the moment I won't add
> more stuff here. I know Kieran was instead happy with this, so let's
> see what his take is.
>
> >
> >       V4L2Subdevice *device() { return subdev_.get(); }
> >
> > @@ -82,6 +87,8 @@ private:
> >       std::unique_ptr<V4L2Subdevice> subdev_;
> >       unsigned int pad_;
> >
> > +     const CameraSensorProperties *staticProps_;
> > +
> >       std::string model_;
> >       std::string id_;
> >
> > @@ -89,6 +96,7 @@ private:
> >       std::vector<unsigned int> mbusCodes_;
> >       std::vector<Size> sizes_;
> >       std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> > +     controls::draft::TestPatternModeEnum testPatternMode_;
> >
> >       Size pixelArraySize_;
> >       Rectangle activeArea_;
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index f0aa9f24..bb429b3e 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -17,6 +17,7 @@
> >  #include <string.h>
> >
> >  #include <libcamera/property_ids.h>
> > +#include <libcamera/request.h>
> >
> >  #include <libcamera/base/utils.h>
> >
> > @@ -54,8 +55,9 @@ LOG_DEFINE_CATEGORY(CameraSensor)
> >   * Once constructed the instance must be initialized with init().
> >   */
> >  CameraSensor::CameraSensor(const MediaEntity *entity)
> > -     : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),
> > -       properties_(properties::properties)
> > +     : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),
> > +       testPatternMode_(controls::draft::TestPatternModeUnset),
> > +       bayerFormat_(nullptr), properties_(properties::properties)
> >  {
> >  }
> >
> > @@ -300,25 +302,30 @@ void CameraSensor::initVimcDefaultProperties()
> >
> >  void CameraSensor::initStaticProperties()
> >  {
> > -     const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> > -     if (!props)
> > +     staticProps_ = CameraSensorProperties::get(model_);
> > +     if (!staticProps_)
> >               return;
> >
> >       /* Register the properties retrieved from the sensor database. */
> > -     properties_.set(properties::UnitCellSize, props->unitCellSize);
> > +     properties_.set(properties::UnitCellSize, staticProps_->unitCellSize);
> >
> > -     initTestPatternModes(props->testPatternModes);
> > +     initTestPatternModes(staticProps_->testPatternModes);
> >  }
> >
> >  void CameraSensor::initTestPatternModes(
> >       const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
> >  {
> > -     const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > -     if (v4l2TestPattern == controls().end()) {
> > +     if (testPatternModes.empty()) {
> >               LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> >                                        << model() << "\'";
> >               return;
> >       }
> > +     const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > +     if (v4l2TestPattern == controls().end()) {
> > +             LOG(CameraSensor, Debug)
> > +                     << "V4L2_CID_TEST_PATTERN is not supported";
> > +             return;
> > +     }
> >
> >       /*
> >        * Create a map that associates the V4L2 control index to the test
> > @@ -531,6 +538,44 @@ Size CameraSensor::resolution() const
> >   * \return The list of test pattern modes
> >   */
> >
> > +/**
> > + * \brief Set the test pattern mode for the camera sensor
> > + * \param[in] testPatternMode Test pattern mode control value to set the camera
> > + * sensor
> > + *
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int CameraSensor::setTestPatternMode(
> > +     controls::draft::TestPatternModeEnum testPatternMode)
>
> Fits on one line maybe ?
>
> > +{
> > +     if (testPatternMode_ == testPatternMode)
> > +             return 0;
> > +
> > +     if (!staticProps_) {
> > +             return 0;
> > +     }
>
> No braces.
>
> And this anyway won't catch the !V4L2_CID_TEST_PATTERN.
>
> As suggested in the previous version you can check for
>
>         if (testPatternModes_.empty())
>                 return 0;
>
> as it will evaluate to true if:
> - No staticProps_
> - staticProps_->testPatternModes.empty()
> - V4L2_CID_TEST_PATTERN not available
>
> > +
> > +     auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > +                         testPatternMode);
> > +     if (it != testPatternModes_.end()) {
>
> You can also simply rely on this, but you will have an error message
> in case any of the above three conditions is true, something the class
> already complains about at initialization time and that I would rather
> handle slently here.
>
> Let's wait for comments about applyRequestControls()
>
> Thanks
>    j
>
> > +             LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> > +                                      << testPatternMode;
> > +             return -EINVAL;
> > +     }
> > +
> > +     int32_t index = staticProps_->testPatternModes.at(testPatternMode);
> > +     ControlList ctrls{ controls() };
> > +     ctrls.set(V4L2_CID_TEST_PATTERN, index);
> > +
> > +     int ret = setControls(&ctrls);
> > +     if (ret)
> > +             return ret;
> > +
> > +     testPatternMode_ = testPatternMode;
> > +
> > +     return 0;
> > +}
> > +
> >  /**
> >   * \brief Retrieve the best sensor format for a desired output
> >   * \param[in] mbusCodes The list of acceptable media bus codes
> > @@ -705,6 +750,40 @@ int CameraSensor::setControls(ControlList *ctrls)
> >       return subdev_->setControls(ctrls);
> >  }
> >
> > +/**
> > + * \brief Apply controls associated with Request
> > + * \param[in] request Request that may contain contorls to be applied
> > + *
> > + * Some controls have to be applied for a capture associated with Request.
> > + * This picks up such controls and set the driver them.
> > + *
> > + * \return 0 on success or an error code otherwise
> > + */
> > +int32_t CameraSensor::applyRequestControls(Request *request)
> > +{
> > +     /* Assumes applying the test pattern mode affects immediately. */
> > +     if (request->controls().contains(controls::draft::TestPatternMode)) {
> > +             const int32_t testPatternMode = request->controls().get(
> > +                     controls::draft::TestPatternMode);
> > +
> > +             LOG(CameraSensor, Debug) << "Apply test pattern mode: "
> > +                                      << testPatternMode;
> > +
> > +             int ret = setTestPatternMode(
> > +                     static_cast<controls::draft::TestPatternModeEnum>(testPatternMode));
> > +             if (ret) {
> > +                     LOG(CameraSensor, Error)
> > +                             << "Failed to set test pattern mode: " << ret;
> > +                     return ret;
> > +             }
> > +
> > +             request->metadata().set(controls::draft::TestPatternMode,
> > +                                     testPatternMode);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  /**
> >   * \fn CameraSensor::device()
> >   * \brief Retrieve the camera sensor device
> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > index 9d4638ae..083bac7b 100644
> > --- a/src/libcamera/control_ids.yaml
> > +++ b/src/libcamera/control_ids.yaml
> > @@ -639,6 +639,11 @@ controls:
> >          Control to select the test pattern mode. Currently identical to
> >          ANDROID_SENSOR_TEST_PATTERN_MODE.
> >        enum:
> > +        - name: TestPatternModeUnset
> > +          value: -1
> > +          description: |
> > +            No test pattern is set. Returned frames by the camera device are
> > +            undefined.
> >          - name: TestPatternModeOff
> >            value: 0
> >            description: |
> > --
> > 2.33.1.1089.g2158813163f-goog
> >

Quoting Hirokazu Honda (2021-11-08 04:34:01)
> Kieran, could you review this patch?
> 
> Thanks,
> -Hiro
> 
> On Thu, Nov 4, 2021 at 6:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Hiro
> >
> > On Thu, Nov 04, 2021 at 03:42:51PM +0900, Hirokazu Honda wrote:
> > > This adds a function to set a camera sensor driver a test pattern
> > > mode.
> > >
> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  include/libcamera/internal/camera_sensor.h |  8 ++
> > >  src/libcamera/camera_sensor.cpp            | 95 ++++++++++++++++++++--
> > >  src/libcamera/control_ids.yaml             |  5 ++
> > >  3 files changed, 100 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > index edef2220..40db792a 100644
> > > --- a/include/libcamera/internal/camera_sensor.h
> > > +++ b/include/libcamera/internal/camera_sensor.h
> > > @@ -27,6 +27,9 @@ namespace libcamera {
> > >
> > >  class BayerFormat;
> > >  class MediaEntity;
> > > +class Request;
> > > +
> > > +struct CameraSensorProperties;
> > >
> > >  class CameraSensor : protected Loggable
> > >  {
> > > @@ -47,6 +50,7 @@ public:
> > >       {
> > >               return testPatternModes_;
> > >       }
> > > +     int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
> >
> > Should testPatternMode be a const & ? It's a int32_t so it might not make any
> > difference

It probably won't make much difference here.

But I suspect setTestPatternMode could be internal and private to
CameraSensor class if the initialisation of the TestPatternMode is moved
to init() from 3/3.


> > >
> > >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > >                                     const Size &size) const;
> > > @@ -55,6 +59,7 @@ public:
> > >       const ControlInfoMap &controls() const;
> > >       ControlList getControls(const std::vector<uint32_t> &ids);
> > >       int setControls(ControlList *ctrls);
> > > +     int applyRequestControls(Request *request);
> >
> > Please introduce this when it is used or in a separate patch before
> > 3/3.
> >
> > Also, we now have setControls(ControlList) and
> > applyRequestControls(Request). The first takes V4L2 controls and apply
> > them to the sensor subdevice. The second takes libcamera::controls
> > from the Request and applies one of them according to a notion of
> > priority that should not be defined in this class imho.
> >
> > We indeed have to finalize the CameraSensor class controls interface
> > once all the use cases are handled, but for the moment I won't add
> > more stuff here. I know Kieran was instead happy with this, so let's
> > see what his take is.
> >
> > >
> > >       V4L2Subdevice *device() { return subdev_.get(); }
> > >
> > > @@ -82,6 +87,8 @@ private:
> > >       std::unique_ptr<V4L2Subdevice> subdev_;
> > >       unsigned int pad_;
> > >
> > > +     const CameraSensorProperties *staticProps_;
> > > +
> > >       std::string model_;
> > >       std::string id_;
> > >
> > > @@ -89,6 +96,7 @@ private:
> > >       std::vector<unsigned int> mbusCodes_;
> > >       std::vector<Size> sizes_;
> > >       std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> > > +     controls::draft::TestPatternModeEnum testPatternMode_;

It's a shame the control id generator makes this a bit more verbose than
desired, but that's ok for now. At least the type is explicit.


> > >
> > >       Size pixelArraySize_;
> > >       Rectangle activeArea_;
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index f0aa9f24..bb429b3e 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -17,6 +17,7 @@
> > >  #include <string.h>
> > >
> > >  #include <libcamera/property_ids.h>
> > > +#include <libcamera/request.h>
> > >
> > >  #include <libcamera/base/utils.h>
> > >
> > > @@ -54,8 +55,9 @@ LOG_DEFINE_CATEGORY(CameraSensor)
> > >   * Once constructed the instance must be initialized with init().
> > >   */
> > >  CameraSensor::CameraSensor(const MediaEntity *entity)
> > > -     : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),
> > > -       properties_(properties::properties)
> > > +     : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),
> > > +       testPatternMode_(controls::draft::TestPatternModeUnset),
> > > +       bayerFormat_(nullptr), properties_(properties::properties)
> > >  {
> > >  }
> > >
> > > @@ -300,25 +302,30 @@ void CameraSensor::initVimcDefaultProperties()
> > >
> > >  void CameraSensor::initStaticProperties()
> > >  {
> > > -     const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> > > -     if (!props)
> > > +     staticProps_ = CameraSensorProperties::get(model_);
> > > +     if (!staticProps_)
> > >               return;
> > >
> > >       /* Register the properties retrieved from the sensor database. */
> > > -     properties_.set(properties::UnitCellSize, props->unitCellSize);
> > > +     properties_.set(properties::UnitCellSize, staticProps_->unitCellSize);
> > >
> > > -     initTestPatternModes(props->testPatternModes);
> > > +     initTestPatternModes(staticProps_->testPatternModes);
> > >  }
> > >
> > >  void CameraSensor::initTestPatternModes(
> > >       const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
> > >  {
> > > -     const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > -     if (v4l2TestPattern == controls().end()) {
> > > +     if (testPatternModes.empty()) {
> > >               LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> > >                                        << model() << "\'";
> > >               return;
> > >       }
> > > +     const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > +     if (v4l2TestPattern == controls().end()) {
> > > +             LOG(CameraSensor, Debug)

I think I'd make this an Error not a Debug.  If we get here, then
presumably our CameraSensor Database has declared a mapping of Test
Pattern modes to V4L2 patterns. - so if the V4L2 control is not
available, then it sounds like an error worth noting on the logs.

Or of course we could do the checks the other way, and be louder if
there is a V4L2_CID_TEST_PATTERN control, but no mapping, which might be
a more realistic thing to occur - and suggest that someone should add
the mapping to the CameraSensorDatabase to enable the feature...

So I think we should do:

	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
	if v4l2TestPattern == controls().end() {
		LOG(Debug) << "No test pattern support on the sensor";
		return;
	}
	
	if (testPatternModes.empty()) {
		LOG(Error) << "No test pattern modes defined"
		return;

		// This is the one where we want to be loud, as the
		// camera sensor supports test patterns but we don't
		// know how to map them so soomeone shoudl fix that.
	}


> > > +                     << "V4L2_CID_TEST_PATTERN is not supported";
> > > +             return;
> > > +     }
> > >
> > >       /*
> > >        * Create a map that associates the V4L2 control index to the test
> > > @@ -531,6 +538,44 @@ Size CameraSensor::resolution() const
> > >   * \return The list of test pattern modes
> > >   */
> > >
> > > +/**
> > > + * \brief Set the test pattern mode for the camera sensor
> > > + * \param[in] testPatternMode Test pattern mode control value to set the camera
> > > + * sensor
> > > + *
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int CameraSensor::setTestPatternMode(
> > > +     controls::draft::TestPatternModeEnum testPatternMode)
> >
> > Fits on one line maybe ?
> >
> > > +{
> > > +     if (testPatternMode_ == testPatternMode)
> > > +             return 0;
> > > +
> > > +     if (!staticProps_) {
> > > +             return 0;
> > > +     }
> >
> > No braces.
> >
> > And this anyway won't catch the !V4L2_CID_TEST_PATTERN.
> >
> > As suggested in the previous version you can check for
> >
> >         if (testPatternModes_.empty())
> >                 return 0;
> >
> > as it will evaluate to true if:
> > - No staticProps_
> > - staticProps_->testPatternModes.empty()
> > - V4L2_CID_TEST_PATTERN not available
> >

I haven't verified if all those three statements are valid, but it
certainly sounds like a better check if they are all covered by
testPatternModes_.empty().



> > > +
> > > +     auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > > +                         testPatternMode);
> > > +     if (it != testPatternModes_.end()) {
> >
> > You can also simply rely on this, but you will have an error message
> > in case any of the above three conditions is true, something the class
> > already complains about at initialization time and that I would rather
> > handle slently here.
> >
> > Let's wait for comments about applyRequestControls()
> >
> > Thanks
> >    j
> >
> > > +             LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> > > +                                      << testPatternMode;
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     int32_t index = staticProps_->testPatternModes.at(testPatternMode);
> > > +     ControlList ctrls{ controls() };
> > > +     ctrls.set(V4L2_CID_TEST_PATTERN, index);
> > > +
> > > +     int ret = setControls(&ctrls);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     testPatternMode_ = testPatternMode;
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  /**
> > >   * \brief Retrieve the best sensor format for a desired output
> > >   * \param[in] mbusCodes The list of acceptable media bus codes
> > > @@ -705,6 +750,40 @@ int CameraSensor::setControls(ControlList *ctrls)
> > >       return subdev_->setControls(ctrls);
> > >  }
> > >
> > > +/**
> > > + * \brief Apply controls associated with Request
> > > + * \param[in] request Request that may contain contorls to be applied
> > > + *
> > > + * Some controls have to be applied for a capture associated with Request.
> > > + * This picks up such controls and set the driver them.
> > > + *
> > > + * \return 0 on success or an error code otherwise
> > > + */
> > > +int32_t CameraSensor::applyRequestControls(Request *request)
> > > +{
> > > +     /* Assumes applying the test pattern mode affects immediately. */
> > > +     if (request->controls().contains(controls::draft::TestPatternMode)) {
> > > +             const int32_t testPatternMode = request->controls().get(
> > > +                     controls::draft::TestPatternMode);
> > > +
> > > +             LOG(CameraSensor, Debug) << "Apply test pattern mode: "
> > > +                                      << testPatternMode;
> > > +
> > > +             int ret = setTestPatternMode(
> > > +                     static_cast<controls::draft::TestPatternModeEnum>(testPatternMode));
> > > +             if (ret) {
> > > +                     LOG(CameraSensor, Error)
> > > +                             << "Failed to set test pattern mode: " << ret;
> > > +                     return ret;
> > > +             }
> > > +
> > > +             request->metadata().set(controls::draft::TestPatternMode,
> > > +                                     testPatternMode);
> > > +     }

Later, I'd expect this function to be more efficient, and process all
controls (or as many as possible) in one ControlList to be able to set
them as a group. But as we only have one for now, I think this is fine.

That would proabbly mean later we'll split setTestPatternMode to a
function which just does the mapping of a TestPatternMode and puts it in
a control list ready to be set ... or such ... but that can all be done
on top when we have other controls to set from a request.



> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  /**
> > >   * \fn CameraSensor::device()
> > >   * \brief Retrieve the camera sensor device
> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > index 9d4638ae..083bac7b 100644
> > > --- a/src/libcamera/control_ids.yaml
> > > +++ b/src/libcamera/control_ids.yaml
> > > @@ -639,6 +639,11 @@ controls:
> > >          Control to select the test pattern mode. Currently identical to
> > >          ANDROID_SENSOR_TEST_PATTERN_MODE.
> > >        enum:
> > > +        - name: TestPatternModeUnset
> > > +          value: -1
> > > +          description: |
> > > +            No test pattern is set. Returned frames by the camera device are
> > > +            undefined.

I'm still not sure this is needed, but I don't object to it being added
if you're sure this state is required.

I think the CameraSensor class should set TestPatternModeOff at init()
time if there is a test pattern control available on the sensor.

Then any test patterns will only be applicable if explictly requested by
the application (/request).



> > >          - name: TestPatternModeOff
> > >            value: 0
> > >            description: |
> > > --
> > > 2.33.1.1089.g2158813163f-goog
> > >

Hi Kieran,

On Mon, Nov 08, 2021 at 10:43:36PM +0000, Kieran Bingham wrote:
> Quoting Hirokazu Honda (2021-11-08 04:34:01)
> > Kieran, could you review this patch?
> >
> > Thanks,
> > -Hiro
> >
> > On Thu, Nov 4, 2021 at 6:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > >
> > > Hi Hiro
> > >
> > > On Thu, Nov 04, 2021 at 03:42:51PM +0900, Hirokazu Honda wrote:
> > > > This adds a function to set a camera sensor driver a test pattern
> > > > mode.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  include/libcamera/internal/camera_sensor.h |  8 ++
> > > >  src/libcamera/camera_sensor.cpp            | 95 ++++++++++++++++++++--
> > > >  src/libcamera/control_ids.yaml             |  5 ++
> > > >  3 files changed, 100 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > index edef2220..40db792a 100644
> > > > --- a/include/libcamera/internal/camera_sensor.h
> > > > +++ b/include/libcamera/internal/camera_sensor.h
> > > > @@ -27,6 +27,9 @@ namespace libcamera {
> > > >
> > > >  class BayerFormat;
> > > >  class MediaEntity;
> > > > +class Request;
> > > > +
> > > > +struct CameraSensorProperties;
> > > >
> > > >  class CameraSensor : protected Loggable
> > > >  {
> > > > @@ -47,6 +50,7 @@ public:
> > > >       {
> > > >               return testPatternModes_;
> > > >       }
> > > > +     int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
> > >
> > > Should testPatternMode be a const & ? It's a int32_t so it might not make any
> > > difference
>
> It probably won't make much difference here.
>
> But I suspect setTestPatternMode could be internal and private to
> CameraSensor class if the initialisation of the TestPatternMode is moved
> to init() from 3/3.
>
>
> > > >
> > > >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > > >                                     const Size &size) const;
> > > > @@ -55,6 +59,7 @@ public:
> > > >       const ControlInfoMap &controls() const;
> > > >       ControlList getControls(const std::vector<uint32_t> &ids);
> > > >       int setControls(ControlList *ctrls);
> > > > +     int applyRequestControls(Request *request);
> > >
> > > Please introduce this when it is used or in a separate patch before
> > > 3/3.
> > >
> > > Also, we now have setControls(ControlList) and
> > > applyRequestControls(Request). The first takes V4L2 controls and apply
> > > them to the sensor subdevice. The second takes libcamera::controls
> > > from the Request and applies one of them according to a notion of
> > > priority that should not be defined in this class imho.
> > >
> > > We indeed have to finalize the CameraSensor class controls interface
> > > once all the use cases are handled, but for the moment I won't add
> > > more stuff here. I know Kieran was instead happy with this, so let's
> > > see what his take is.
> > >
> > > >
> > > >       V4L2Subdevice *device() { return subdev_.get(); }
> > > >
> > > > @@ -82,6 +87,8 @@ private:
> > > >       std::unique_ptr<V4L2Subdevice> subdev_;
> > > >       unsigned int pad_;
> > > >
> > > > +     const CameraSensorProperties *staticProps_;
> > > > +
> > > >       std::string model_;
> > > >       std::string id_;
> > > >
> > > > @@ -89,6 +96,7 @@ private:
> > > >       std::vector<unsigned int> mbusCodes_;
> > > >       std::vector<Size> sizes_;
> > > >       std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> > > > +     controls::draft::TestPatternModeEnum testPatternMode_;
>
> It's a shame the control id generator makes this a bit more verbose than
> desired, but that's ok for now. At least the type is explicit.
>
>
> > > >
> > > >       Size pixelArraySize_;
> > > >       Rectangle activeArea_;
> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > index f0aa9f24..bb429b3e 100644
> > > > --- a/src/libcamera/camera_sensor.cpp
> > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > @@ -17,6 +17,7 @@
> > > >  #include <string.h>
> > > >
> > > >  #include <libcamera/property_ids.h>
> > > > +#include <libcamera/request.h>
> > > >
> > > >  #include <libcamera/base/utils.h>
> > > >
> > > > @@ -54,8 +55,9 @@ LOG_DEFINE_CATEGORY(CameraSensor)
> > > >   * Once constructed the instance must be initialized with init().
> > > >   */
> > > >  CameraSensor::CameraSensor(const MediaEntity *entity)
> > > > -     : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),
> > > > -       properties_(properties::properties)
> > > > +     : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),
> > > > +       testPatternMode_(controls::draft::TestPatternModeUnset),
> > > > +       bayerFormat_(nullptr), properties_(properties::properties)
> > > >  {
> > > >  }
> > > >
> > > > @@ -300,25 +302,30 @@ void CameraSensor::initVimcDefaultProperties()
> > > >
> > > >  void CameraSensor::initStaticProperties()
> > > >  {
> > > > -     const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> > > > -     if (!props)
> > > > +     staticProps_ = CameraSensorProperties::get(model_);
> > > > +     if (!staticProps_)
> > > >               return;
> > > >
> > > >       /* Register the properties retrieved from the sensor database. */
> > > > -     properties_.set(properties::UnitCellSize, props->unitCellSize);
> > > > +     properties_.set(properties::UnitCellSize, staticProps_->unitCellSize);
> > > >
> > > > -     initTestPatternModes(props->testPatternModes);
> > > > +     initTestPatternModes(staticProps_->testPatternModes);
> > > >  }
> > > >
> > > >  void CameraSensor::initTestPatternModes(
> > > >       const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
> > > >  {
> > > > -     const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > > -     if (v4l2TestPattern == controls().end()) {
> > > > +     if (testPatternModes.empty()) {
> > > >               LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> > > >                                        << model() << "\'";
> > > >               return;
> > > >       }
> > > > +     const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > > +     if (v4l2TestPattern == controls().end()) {
> > > > +             LOG(CameraSensor, Debug)
>
> I think I'd make this an Error not a Debug.  If we get here, then
> presumably our CameraSensor Database has declared a mapping of Test
> Pattern modes to V4L2 patterns. - so if the V4L2 control is not
> available, then it sounds like an error worth noting on the logs.
>
> Or of course we could do the checks the other way, and be louder if
> there is a V4L2_CID_TEST_PATTERN control, but no mapping, which might be
> a more realistic thing to occur - and suggest that someone should add
> the mapping to the CameraSensorDatabase to enable the feature...
>
> So I think we should do:
>
> 	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> 	if v4l2TestPattern == controls().end() {
> 		LOG(Debug) << "No test pattern support on the sensor";
> 		return;
> 	}
>
> 	if (testPatternModes.empty()) {
> 		LOG(Error) << "No test pattern modes defined"
> 		return;
>
> 		// This is the one where we want to be loud, as the
> 		// camera sensor supports test patterns but we don't
> 		// know how to map them so soomeone shoudl fix that.
> 	}
>
>
> > > > +                     << "V4L2_CID_TEST_PATTERN is not supported";
> > > > +             return;
> > > > +     }
> > > >
> > > >       /*
> > > >        * Create a map that associates the V4L2 control index to the test
> > > > @@ -531,6 +538,44 @@ Size CameraSensor::resolution() const
> > > >   * \return The list of test pattern modes
> > > >   */
> > > >
> > > > +/**
> > > > + * \brief Set the test pattern mode for the camera sensor
> > > > + * \param[in] testPatternMode Test pattern mode control value to set the camera
> > > > + * sensor
> > > > + *
> > > > + * \return 0 on success or a negative error code otherwise
> > > > + */
> > > > +int CameraSensor::setTestPatternMode(
> > > > +     controls::draft::TestPatternModeEnum testPatternMode)
> > >
> > > Fits on one line maybe ?
> > >
> > > > +{
> > > > +     if (testPatternMode_ == testPatternMode)
> > > > +             return 0;
> > > > +
> > > > +     if (!staticProps_) {
> > > > +             return 0;
> > > > +     }
> > >
> > > No braces.
> > >
> > > And this anyway won't catch the !V4L2_CID_TEST_PATTERN.
> > >
> > > As suggested in the previous version you can check for
> > >
> > >         if (testPatternModes_.empty())
> > >                 return 0;
> > >
> > > as it will evaluate to true if:
> > > - No staticProps_
> > > - staticProps_->testPatternModes.empty()
> > > - V4L2_CID_TEST_PATTERN not available
> > >
>
> I haven't verified if all those three statements are valid, but it
> certainly sounds like a better check if they are all covered by
> testPatternModes_.empty().
>
>
>
> > > > +
> > > > +     auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > > > +                         testPatternMode);
> > > > +     if (it != testPatternModes_.end()) {
> > >
> > > You can also simply rely on this, but you will have an error message
> > > in case any of the above three conditions is true, something the class
> > > already complains about at initialization time and that I would rather
> > > handle slently here.
> > >
> > > Let's wait for comments about applyRequestControls()

Actually the pending point here is your suggestion to add
applyRequestControls()

Thanks
   j

> > >
> > > Thanks
> > >    j
> > >
> > > > +             LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> > > > +                                      << testPatternMode;
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     int32_t index = staticProps_->testPatternModes.at(testPatternMode);
> > > > +     ControlList ctrls{ controls() };
> > > > +     ctrls.set(V4L2_CID_TEST_PATTERN, index);
> > > > +
> > > > +     int ret = setControls(&ctrls);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     testPatternMode_ = testPatternMode;
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  /**
> > > >   * \brief Retrieve the best sensor format for a desired output
> > > >   * \param[in] mbusCodes The list of acceptable media bus codes
> > > > @@ -705,6 +750,40 @@ int CameraSensor::setControls(ControlList *ctrls)
> > > >       return subdev_->setControls(ctrls);
> > > >  }
> > > >
> > > > +/**
> > > > + * \brief Apply controls associated with Request
> > > > + * \param[in] request Request that may contain contorls to be applied
> > > > + *
> > > > + * Some controls have to be applied for a capture associated with Request.
> > > > + * This picks up such controls and set the driver them.
> > > > + *
> > > > + * \return 0 on success or an error code otherwise
> > > > + */
> > > > +int32_t CameraSensor::applyRequestControls(Request *request)
> > > > +{
> > > > +     /* Assumes applying the test pattern mode affects immediately. */
> > > > +     if (request->controls().contains(controls::draft::TestPatternMode)) {
> > > > +             const int32_t testPatternMode = request->controls().get(
> > > > +                     controls::draft::TestPatternMode);
> > > > +
> > > > +             LOG(CameraSensor, Debug) << "Apply test pattern mode: "
> > > > +                                      << testPatternMode;
> > > > +
> > > > +             int ret = setTestPatternMode(
> > > > +                     static_cast<controls::draft::TestPatternModeEnum>(testPatternMode));
> > > > +             if (ret) {
> > > > +                     LOG(CameraSensor, Error)
> > > > +                             << "Failed to set test pattern mode: " << ret;
> > > > +                     return ret;
> > > > +             }
> > > > +
> > > > +             request->metadata().set(controls::draft::TestPatternMode,
> > > > +                                     testPatternMode);
> > > > +     }
>
> Later, I'd expect this function to be more efficient, and process all
> controls (or as many as possible) in one ControlList to be able to set
> them as a group. But as we only have one for now, I think this is fine.
>
> That would proabbly mean later we'll split setTestPatternMode to a
> function which just does the mapping of a TestPatternMode and puts it in
> a control list ready to be set ... or such ... but that can all be done
> on top when we have other controls to set from a request.
>
>
>
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  /**
> > > >   * \fn CameraSensor::device()
> > > >   * \brief Retrieve the camera sensor device
> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > index 9d4638ae..083bac7b 100644
> > > > --- a/src/libcamera/control_ids.yaml
> > > > +++ b/src/libcamera/control_ids.yaml
> > > > @@ -639,6 +639,11 @@ controls:
> > > >          Control to select the test pattern mode. Currently identical to
> > > >          ANDROID_SENSOR_TEST_PATTERN_MODE.
> > > >        enum:
> > > > +        - name: TestPatternModeUnset
> > > > +          value: -1
> > > > +          description: |
> > > > +            No test pattern is set. Returned frames by the camera device are
> > > > +            undefined.
>
> I'm still not sure this is needed, but I don't object to it being added
> if you're sure this state is required.
>
> I think the CameraSensor class should set TestPatternModeOff at init()
> time if there is a test pattern control available on the sensor.
>
> Then any test patterns will only be applicable if explictly requested by
> the application (/request).
>
>
>
> > > >          - name: TestPatternModeOff
> > > >            value: 0
> > > >            description: |
> > > > --
> > > > 2.33.1.1089.g2158813163f-goog
> > > >

Quoting Jacopo Mondi (2021-11-09 08:55:58)
> Hi Kieran,
> 
> On Mon, Nov 08, 2021 at 10:43:36PM +0000, Kieran Bingham wrote:
> > Quoting Hirokazu Honda (2021-11-08 04:34:01)
> > > Kieran, could you review this patch?
> > >
> > > Thanks,
> > > -Hiro
> > >
> > > On Thu, Nov 4, 2021 at 6:07 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
> > > >
> > > > Hi Hiro
> > > >
> > > > On Thu, Nov 04, 2021 at 03:42:51PM +0900, Hirokazu Honda wrote:
> > > > > This adds a function to set a camera sensor driver a test pattern
> > > > > mode.
> > > > >
> > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > >  include/libcamera/internal/camera_sensor.h |  8 ++
> > > > >  src/libcamera/camera_sensor.cpp            | 95 ++++++++++++++++++++--
> > > > >  src/libcamera/control_ids.yaml             |  5 ++
> > > > >  3 files changed, 100 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > > index edef2220..40db792a 100644
> > > > > --- a/include/libcamera/internal/camera_sensor.h
> > > > > +++ b/include/libcamera/internal/camera_sensor.h
> > > > > @@ -27,6 +27,9 @@ namespace libcamera {
> > > > >
> > > > >  class BayerFormat;
> > > > >  class MediaEntity;
> > > > > +class Request;
> > > > > +
> > > > > +struct CameraSensorProperties;
> > > > >
> > > > >  class CameraSensor : protected Loggable
> > > > >  {
> > > > > @@ -47,6 +50,7 @@ public:
> > > > >       {
> > > > >               return testPatternModes_;
> > > > >       }
> > > > > +     int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
> > > >
> > > > Should testPatternMode be a const & ? It's a int32_t so it might not make any
> > > > difference
> >
> > It probably won't make much difference here.
> >
> > But I suspect setTestPatternMode could be internal and private to
> > CameraSensor class if the initialisation of the TestPatternMode is moved
> > to init() from 3/3.
> >
> >
> > > > >
> > > > >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > > > >                                     const Size &size) const;
> > > > > @@ -55,6 +59,7 @@ public:
> > > > >       const ControlInfoMap &controls() const;
> > > > >       ControlList getControls(const std::vector<uint32_t> &ids);
> > > > >       int setControls(ControlList *ctrls);
> > > > > +     int applyRequestControls(Request *request);
> > > >
> > > > Please introduce this when it is used or in a separate patch before
> > > > 3/3.
> > > >
> > > > Also, we now have setControls(ControlList) and
> > > > applyRequestControls(Request). The first takes V4L2 controls and apply
> > > > them to the sensor subdevice. The second takes libcamera::controls
> > > > from the Request and applies one of them according to a notion of
> > > > priority that should not be defined in this class imho.

My intention was not that the priority is specified by the CameraSensor
class, but that the CameraSensor class handles controls that are
appropriate to it.

Part of (my) aim was to prevent every pipeline handler having duplicated
boilerplate filtering out the testpattern control to set on the camera sensor.

Perhaps the key point is the name 'applyRequestControls' is disliked.

But I also disliked that it lead towards having a 

	sensor()->setTestPatternMode().

which would imply we might have to add lots of setters on a sensor:

	sensor()->setVblank();
	sensor()->setExposure();
	sensor()->setGain();
	sensor()->setOtherPerFrameControls();?

But perhaps I was wrong ... if there's not expected to be anything else
added ...?


> > > >
> > > > We indeed have to finalize the CameraSensor class controls interface
> > > > once all the use cases are handled, but for the moment I won't add
> > > > more stuff here. I know Kieran was instead happy with this, so let's
> > > > see what his take is.
> > > >
> > > > >
> > > > >       V4L2Subdevice *device() { return subdev_.get(); }
> > > > >
> > > > > @@ -82,6 +87,8 @@ private:
> > > > >       std::unique_ptr<V4L2Subdevice> subdev_;
> > > > >       unsigned int pad_;
> > > > >
> > > > > +     const CameraSensorProperties *staticProps_;
> > > > > +
> > > > >       std::string model_;
> > > > >       std::string id_;
> > > > >
> > > > > @@ -89,6 +96,7 @@ private:
> > > > >       std::vector<unsigned int> mbusCodes_;
> > > > >       std::vector<Size> sizes_;
> > > > >       std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> > > > > +     controls::draft::TestPatternModeEnum testPatternMode_;
> >
> > It's a shame the control id generator makes this a bit more verbose than
> > desired, but that's ok for now. At least the type is explicit.
> >
> >
> > > > >
> > > > >       Size pixelArraySize_;
> > > > >       Rectangle activeArea_;
> > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > > index f0aa9f24..bb429b3e 100644
> > > > > --- a/src/libcamera/camera_sensor.cpp
> > > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > > @@ -17,6 +17,7 @@
> > > > >  #include <string.h>
> > > > >
> > > > >  #include <libcamera/property_ids.h>
> > > > > +#include <libcamera/request.h>
> > > > >
> > > > >  #include <libcamera/base/utils.h>
> > > > >
> > > > > @@ -54,8 +55,9 @@ LOG_DEFINE_CATEGORY(CameraSensor)
> > > > >   * Once constructed the instance must be initialized with init().
> > > > >   */
> > > > >  CameraSensor::CameraSensor(const MediaEntity *entity)
> > > > > -     : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),
> > > > > -       properties_(properties::properties)
> > > > > +     : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),
> > > > > +       testPatternMode_(controls::draft::TestPatternModeUnset),
> > > > > +       bayerFormat_(nullptr), properties_(properties::properties)
> > > > >  {
> > > > >  }
> > > > >
> > > > > @@ -300,25 +302,30 @@ void CameraSensor::initVimcDefaultProperties()
> > > > >
> > > > >  void CameraSensor::initStaticProperties()
> > > > >  {
> > > > > -     const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> > > > > -     if (!props)
> > > > > +     staticProps_ = CameraSensorProperties::get(model_);
> > > > > +     if (!staticProps_)
> > > > >               return;
> > > > >
> > > > >       /* Register the properties retrieved from the sensor database. */
> > > > > -     properties_.set(properties::UnitCellSize, props->unitCellSize);
> > > > > +     properties_.set(properties::UnitCellSize, staticProps_->unitCellSize);
> > > > >
> > > > > -     initTestPatternModes(props->testPatternModes);
> > > > > +     initTestPatternModes(staticProps_->testPatternModes);
> > > > >  }
> > > > >
> > > > >  void CameraSensor::initTestPatternModes(
> > > > >       const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
> > > > >  {
> > > > > -     const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > > > -     if (v4l2TestPattern == controls().end()) {
> > > > > +     if (testPatternModes.empty()) {
> > > > >               LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> > > > >                                        << model() << "\'";
> > > > >               return;
> > > > >       }
> > > > > +     const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > > > +     if (v4l2TestPattern == controls().end()) {
> > > > > +             LOG(CameraSensor, Debug)
> >
> > I think I'd make this an Error not a Debug.  If we get here, then
> > presumably our CameraSensor Database has declared a mapping of Test
> > Pattern modes to V4L2 patterns. - so if the V4L2 control is not
> > available, then it sounds like an error worth noting on the logs.
> >
> > Or of course we could do the checks the other way, and be louder if
> > there is a V4L2_CID_TEST_PATTERN control, but no mapping, which might be
> > a more realistic thing to occur - and suggest that someone should add
> > the mapping to the CameraSensorDatabase to enable the feature...
> >
> > So I think we should do:
> >
> >       const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> >       if v4l2TestPattern == controls().end() {
> >               LOG(Debug) << "No test pattern support on the sensor";
> >               return;
> >       }
> >
> >       if (testPatternModes.empty()) {
> >               LOG(Error) << "No test pattern modes defined"
> >               return;
> >
> >               // This is the one where we want to be loud, as the
> >               // camera sensor supports test patterns but we don't
> >               // know how to map them so soomeone shoudl fix that.
> >       }
> >
> >
> > > > > +                     << "V4L2_CID_TEST_PATTERN is not supported";
> > > > > +             return;
> > > > > +     }
> > > > >
> > > > >       /*
> > > > >        * Create a map that associates the V4L2 control index to the test
> > > > > @@ -531,6 +538,44 @@ Size CameraSensor::resolution() const
> > > > >   * \return The list of test pattern modes
> > > > >   */
> > > > >
> > > > > +/**
> > > > > + * \brief Set the test pattern mode for the camera sensor
> > > > > + * \param[in] testPatternMode Test pattern mode control value to set the camera
> > > > > + * sensor
> > > > > + *
> > > > > + * \return 0 on success or a negative error code otherwise
> > > > > + */
> > > > > +int CameraSensor::setTestPatternMode(
> > > > > +     controls::draft::TestPatternModeEnum testPatternMode)
> > > >
> > > > Fits on one line maybe ?
> > > >
> > > > > +{
> > > > > +     if (testPatternMode_ == testPatternMode)
> > > > > +             return 0;
> > > > > +
> > > > > +     if (!staticProps_) {
> > > > > +             return 0;
> > > > > +     }
> > > >
> > > > No braces.
> > > >
> > > > And this anyway won't catch the !V4L2_CID_TEST_PATTERN.
> > > >
> > > > As suggested in the previous version you can check for
> > > >
> > > >         if (testPatternModes_.empty())
> > > >                 return 0;
> > > >
> > > > as it will evaluate to true if:
> > > > - No staticProps_
> > > > - staticProps_->testPatternModes.empty()
> > > > - V4L2_CID_TEST_PATTERN not available
> > > >
> >
> > I haven't verified if all those three statements are valid, but it
> > certainly sounds like a better check if they are all covered by
> > testPatternModes_.empty().
> >
> >
> >
> > > > > +
> > > > > +     auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > > > > +                         testPatternMode);
> > > > > +     if (it != testPatternModes_.end()) {
> > > >
> > > > You can also simply rely on this, but you will have an error message
> > > > in case any of the above three conditions is true, something the class
> > > > already complains about at initialization time and that I would rather
> > > > handle slently here.
> > > >
> > > > Let's wait for comments about applyRequestControls()
> 
> Actually the pending point here is your suggestion to add
> applyRequestControls()
> 
> Thanks
>    j
> 
> > > >
> > > > Thanks
> > > >    j
> > > >
> > > > > +             LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> > > > > +                                      << testPatternMode;
> > > > > +             return -EINVAL;
> > > > > +     }
> > > > > +
> > > > > +     int32_t index = staticProps_->testPatternModes.at(testPatternMode);
> > > > > +     ControlList ctrls{ controls() };
> > > > > +     ctrls.set(V4L2_CID_TEST_PATTERN, index);
> > > > > +
> > > > > +     int ret = setControls(&ctrls);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     testPatternMode_ = testPatternMode;
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * \brief Retrieve the best sensor format for a desired output
> > > > >   * \param[in] mbusCodes The list of acceptable media bus codes
> > > > > @@ -705,6 +750,40 @@ int CameraSensor::setControls(ControlList *ctrls)
> > > > >       return subdev_->setControls(ctrls);
> > > > >  }
> > > > >
> > > > > +/**
> > > > > + * \brief Apply controls associated with Request
> > > > > + * \param[in] request Request that may contain contorls to be applied
> > > > > + *
> > > > > + * Some controls have to be applied for a capture associated with Request.
> > > > > + * This picks up such controls and set the driver them.
> > > > > + *
> > > > > + * \return 0 on success or an error code otherwise
> > > > > + */
> > > > > +int32_t CameraSensor::applyRequestControls(Request *request)
> > > > > +{
> > > > > +     /* Assumes applying the test pattern mode affects immediately. */
> > > > > +     if (request->controls().contains(controls::draft::TestPatternMode)) {
> > > > > +             const int32_t testPatternMode = request->controls().get(
> > > > > +                     controls::draft::TestPatternMode);
> > > > > +
> > > > > +             LOG(CameraSensor, Debug) << "Apply test pattern mode: "
> > > > > +                                      << testPatternMode;
> > > > > +
> > > > > +             int ret = setTestPatternMode(
> > > > > +                     static_cast<controls::draft::TestPatternModeEnum>(testPatternMode));
> > > > > +             if (ret) {
> > > > > +                     LOG(CameraSensor, Error)
> > > > > +                             << "Failed to set test pattern mode: " << ret;
> > > > > +                     return ret;
> > > > > +             }
> > > > > +
> > > > > +             request->metadata().set(controls::draft::TestPatternMode,
> > > > > +                                     testPatternMode);
> > > > > +     }
> >
> > Later, I'd expect this function to be more efficient, and process all
> > controls (or as many as possible) in one ControlList to be able to set
> > them as a group. But as we only have one for now, I think this is fine.
> >
> > That would proabbly mean later we'll split setTestPatternMode to a
> > function which just does the mapping of a TestPatternMode and puts it in
> > a control list ready to be set ... or such ... but that can all be done
> > on top when we have other controls to set from a request.
> >
> >
> >
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * \fn CameraSensor::device()
> > > > >   * \brief Retrieve the camera sensor device
> > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > index 9d4638ae..083bac7b 100644
> > > > > --- a/src/libcamera/control_ids.yaml
> > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > @@ -639,6 +639,11 @@ controls:
> > > > >          Control to select the test pattern mode. Currently identical to
> > > > >          ANDROID_SENSOR_TEST_PATTERN_MODE.
> > > > >        enum:
> > > > > +        - name: TestPatternModeUnset
> > > > > +          value: -1
> > > > > +          description: |
> > > > > +            No test pattern is set. Returned frames by the camera device are
> > > > > +            undefined.
> >
> > I'm still not sure this is needed, but I don't object to it being added
> > if you're sure this state is required.
> >
> > I think the CameraSensor class should set TestPatternModeOff at init()
> > time if there is a test pattern control available on the sensor.
> >
> > Then any test patterns will only be applicable if explictly requested by
> > the application (/request).
> >
> >
> >
> > > > >          - name: TestPatternModeOff
> > > > >            value: 0
> > > > >            description: |
> > > > > --
> > > > > 2.33.1.1089.g2158813163f-goog
> > > > >

Hello,

On Tue, Nov 09, 2021 at 04:35:31PM +0000, Kieran Bingham wrote:
> Quoting Jacopo Mondi (2021-11-09 08:55:58)
> > On Mon, Nov 08, 2021 at 10:43:36PM +0000, Kieran Bingham wrote:
> > > Quoting Hirokazu Honda (2021-11-08 04:34:01)
> > > > Kieran, could you review this patch?
> > > >
> > > > Thanks,
> > > > -Hiro
> > > >
> > > > On Thu, Nov 4, 2021 at 6:07 PM Jacopo Mondi wrote:
> > > > > On Thu, Nov 04, 2021 at 03:42:51PM +0900, Hirokazu Honda wrote:
> > > > > > This adds a function to set a camera sensor driver a test pattern
> > > > > > mode.
> > > > > >
> > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > ---
> > > > > >  include/libcamera/internal/camera_sensor.h |  8 ++
> > > > > >  src/libcamera/camera_sensor.cpp            | 95 ++++++++++++++++++++--
> > > > > >  src/libcamera/control_ids.yaml             |  5 ++
> > > > > >  3 files changed, 100 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > > > index edef2220..40db792a 100644
> > > > > > --- a/include/libcamera/internal/camera_sensor.h
> > > > > > +++ b/include/libcamera/internal/camera_sensor.h
> > > > > > @@ -27,6 +27,9 @@ namespace libcamera {
> > > > > >
> > > > > >  class BayerFormat;
> > > > > >  class MediaEntity;
> > > > > > +class Request;
> > > > > > +
> > > > > > +struct CameraSensorProperties;
> > > > > >
> > > > > >  class CameraSensor : protected Loggable
> > > > > >  {
> > > > > > @@ -47,6 +50,7 @@ public:
> > > > > >       {
> > > > > >               return testPatternModes_;
> > > > > >       }
> > > > > > +     int setTestPatternMode(controls::draft::TestPatternModeEnum testPatternMode);
> > > > >
> > > > > Should testPatternMode be a const & ? It's a int32_t so it might not make any
> > > > > difference
> > >
> > > It probably won't make much difference here.
> > >
> > > But I suspect setTestPatternMode could be internal and private to
> > > CameraSensor class if the initialisation of the TestPatternMode is moved
> > > to init() from 3/3.
> > >
> > > > > >
> > > > > >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > > > > >                                     const Size &size) const;
> > > > > > @@ -55,6 +59,7 @@ public:
> > > > > >       const ControlInfoMap &controls() const;
> > > > > >       ControlList getControls(const std::vector<uint32_t> &ids);
> > > > > >       int setControls(ControlList *ctrls);
> > > > > > +     int applyRequestControls(Request *request);
> > > > >
> > > > > Please introduce this when it is used or in a separate patch before
> > > > > 3/3.
> > > > >
> > > > > Also, we now have setControls(ControlList) and
> > > > > applyRequestControls(Request). The first takes V4L2 controls and apply
> > > > > them to the sensor subdevice. The second takes libcamera::controls
> > > > > from the Request and applies one of them according to a notion of
> > > > > priority that should not be defined in this class imho.
> 
> My intention was not that the priority is specified by the CameraSensor
> class, but that the CameraSensor class handles controls that are
> appropriate to it.
> 
> Part of (my) aim was to prevent every pipeline handler having duplicated
> boilerplate filtering out the testpattern control to set on the camera sensor.
> 
> Perhaps the key point is the name 'applyRequestControls' is disliked.
> 
> But I also disliked that it lead towards having a 
> 
> 	sensor()->setTestPatternMode().
> 
> which would imply we might have to add lots of setters on a sensor:
> 
> 	sensor()->setVblank();
> 	sensor()->setExposure();
> 	sensor()->setGain();
> 	sensor()->setOtherPerFrameControls();?
> 
> But perhaps I was wrong ... if there's not expected to be anything else
> added ...?

I agree with Jacopo that the CameraSensor class shouldn't deal with
requests. It should instead take a ControList of non-V4L2 controls
(possibly libcamera controls if we have all we need there, or a new set
of CameraSensor controls). It's the pipeline handler that should be
responsible for dispatching the contents of a request to the appropriate
components.

> > > > > We indeed have to finalize the CameraSensor class controls interface
> > > > > once all the use cases are handled, but for the moment I won't add
> > > > > more stuff here. I know Kieran was instead happy with this, so let's
> > > > > see what his take is.
> > > > >
> > > > > >       V4L2Subdevice *device() { return subdev_.get(); }
> > > > > >
> > > > > > @@ -82,6 +87,8 @@ private:
> > > > > >       std::unique_ptr<V4L2Subdevice> subdev_;
> > > > > >       unsigned int pad_;
> > > > > >
> > > > > > +     const CameraSensorProperties *staticProps_;
> > > > > > +
> > > > > >       std::string model_;
> > > > > >       std::string id_;
> > > > > >
> > > > > > @@ -89,6 +96,7 @@ private:
> > > > > >       std::vector<unsigned int> mbusCodes_;
> > > > > >       std::vector<Size> sizes_;
> > > > > >       std::vector<controls::draft::TestPatternModeEnum> testPatternModes_;
> > > > > > +     controls::draft::TestPatternModeEnum testPatternMode_;
> > >
> > > It's a shame the control id generator makes this a bit more verbose than
> > > desired, but that's ok for now. At least the type is explicit.
> > >
> > > > > >
> > > > > >       Size pixelArraySize_;
> > > > > >       Rectangle activeArea_;
> > > > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > > > index f0aa9f24..bb429b3e 100644
> > > > > > --- a/src/libcamera/camera_sensor.cpp
> > > > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > > > @@ -17,6 +17,7 @@
> > > > > >  #include <string.h>
> > > > > >
> > > > > >  #include <libcamera/property_ids.h>
> > > > > > +#include <libcamera/request.h>
> > > > > >
> > > > > >  #include <libcamera/base/utils.h>
> > > > > >
> > > > > > @@ -54,8 +55,9 @@ LOG_DEFINE_CATEGORY(CameraSensor)
> > > > > >   * Once constructed the instance must be initialized with init().
> > > > > >   */
> > > > > >  CameraSensor::CameraSensor(const MediaEntity *entity)
> > > > > > -     : entity_(entity), pad_(UINT_MAX), bayerFormat_(nullptr),
> > > > > > -       properties_(properties::properties)
> > > > > > +     : entity_(entity), pad_(UINT_MAX), staticProps_(nullptr),
> > > > > > +       testPatternMode_(controls::draft::TestPatternModeUnset),
> > > > > > +       bayerFormat_(nullptr), properties_(properties::properties)
> > > > > >  {
> > > > > >  }
> > > > > >
> > > > > > @@ -300,25 +302,30 @@ void CameraSensor::initVimcDefaultProperties()
> > > > > >
> > > > > >  void CameraSensor::initStaticProperties()
> > > > > >  {
> > > > > > -     const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> > > > > > -     if (!props)
> > > > > > +     staticProps_ = CameraSensorProperties::get(model_);
> > > > > > +     if (!staticProps_)
> > > > > >               return;
> > > > > >
> > > > > >       /* Register the properties retrieved from the sensor database. */
> > > > > > -     properties_.set(properties::UnitCellSize, props->unitCellSize);
> > > > > > +     properties_.set(properties::UnitCellSize, staticProps_->unitCellSize);
> > > > > >
> > > > > > -     initTestPatternModes(props->testPatternModes);
> > > > > > +     initTestPatternModes(staticProps_->testPatternModes);
> > > > > >  }
> > > > > >
> > > > > >  void CameraSensor::initTestPatternModes(
> > > > > >       const std::map<controls::draft::TestPatternModeEnum, int32_t> &testPatternModes)
> > > > > >  {
> > > > > > -     const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > > > > -     if (v4l2TestPattern == controls().end()) {
> > > > > > +     if (testPatternModes.empty()) {
> > > > > >               LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> > > > > >                                        << model() << "\'";
> > > > > >               return;
> > > > > >       }
> > > > > > +     const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > > > > > +     if (v4l2TestPattern == controls().end()) {
> > > > > > +             LOG(CameraSensor, Debug)
> > >
> > > I think I'd make this an Error not a Debug.  If we get here, then
> > > presumably our CameraSensor Database has declared a mapping of Test
> > > Pattern modes to V4L2 patterns. - so if the V4L2 control is not
> > > available, then it sounds like an error worth noting on the logs.
> > >
> > > Or of course we could do the checks the other way, and be louder if
> > > there is a V4L2_CID_TEST_PATTERN control, but no mapping, which might be
> > > a more realistic thing to occur - and suggest that someone should add
> > > the mapping to the CameraSensorDatabase to enable the feature...
> > >
> > > So I think we should do:
> > >
> > >       const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> > >       if v4l2TestPattern == controls().end() {
> > >               LOG(Debug) << "No test pattern support on the sensor";
> > >               return;
> > >       }
> > >
> > >       if (testPatternModes.empty()) {
> > >               LOG(Error) << "No test pattern modes defined"
> > >               return;
> > >
> > >               // This is the one where we want to be loud, as the
> > >               // camera sensor supports test patterns but we don't
> > >               // know how to map them so soomeone shoudl fix that.
> > >       }
> > >
> > >
> > > > > > +                     << "V4L2_CID_TEST_PATTERN is not supported";
> > > > > > +             return;
> > > > > > +     }
> > > > > >
> > > > > >       /*
> > > > > >        * Create a map that associates the V4L2 control index to the test
> > > > > > @@ -531,6 +538,44 @@ Size CameraSensor::resolution() const
> > > > > >   * \return The list of test pattern modes
> > > > > >   */
> > > > > >
> > > > > > +/**
> > > > > > + * \brief Set the test pattern mode for the camera sensor
> > > > > > + * \param[in] testPatternMode Test pattern mode control value to set the camera
> > > > > > + * sensor
> > > > > > + *
> > > > > > + * \return 0 on success or a negative error code otherwise
> > > > > > + */
> > > > > > +int CameraSensor::setTestPatternMode(
> > > > > > +     controls::draft::TestPatternModeEnum testPatternMode)
> > > > >
> > > > > Fits on one line maybe ?
> > > > >
> > > > > > +{
> > > > > > +     if (testPatternMode_ == testPatternMode)
> > > > > > +             return 0;
> > > > > > +
> > > > > > +     if (!staticProps_) {
> > > > > > +             return 0;
> > > > > > +     }
> > > > >
> > > > > No braces.
> > > > >
> > > > > And this anyway won't catch the !V4L2_CID_TEST_PATTERN.
> > > > >
> > > > > As suggested in the previous version you can check for
> > > > >
> > > > >         if (testPatternModes_.empty())
> > > > >                 return 0;
> > > > >
> > > > > as it will evaluate to true if:
> > > > > - No staticProps_
> > > > > - staticProps_->testPatternModes.empty()
> > > > > - V4L2_CID_TEST_PATTERN not available
> > > > >
> > >
> > > I haven't verified if all those three statements are valid, but it
> > > certainly sounds like a better check if they are all covered by
> > > testPatternModes_.empty().
> > >
> > > > > > +
> > > > > > +     auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > > > > > +                         testPatternMode);
> > > > > > +     if (it != testPatternModes_.end()) {
> > > > >
> > > > > You can also simply rely on this, but you will have an error message
> > > > > in case any of the above three conditions is true, something the class
> > > > > already complains about at initialization time and that I would rather
> > > > > handle slently here.
> > > > >
> > > > > Let's wait for comments about applyRequestControls()
> > 
> > Actually the pending point here is your suggestion to add
> > applyRequestControls()
> > 
> > > > > > +             LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> > > > > > +                                      << testPatternMode;
> > > > > > +             return -EINVAL;
> > > > > > +     }
> > > > > > +
> > > > > > +     int32_t index = staticProps_->testPatternModes.at(testPatternMode);
> > > > > > +     ControlList ctrls{ controls() };
> > > > > > +     ctrls.set(V4L2_CID_TEST_PATTERN, index);
> > > > > > +
> > > > > > +     int ret = setControls(&ctrls);
> > > > > > +     if (ret)
> > > > > > +             return ret;
> > > > > > +
> > > > > > +     testPatternMode_ = testPatternMode;
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > >  /**
> > > > > >   * \brief Retrieve the best sensor format for a desired output
> > > > > >   * \param[in] mbusCodes The list of acceptable media bus codes
> > > > > > @@ -705,6 +750,40 @@ int CameraSensor::setControls(ControlList *ctrls)
> > > > > >       return subdev_->setControls(ctrls);
> > > > > >  }
> > > > > >
> > > > > > +/**
> > > > > > + * \brief Apply controls associated with Request
> > > > > > + * \param[in] request Request that may contain contorls to be applied
> > > > > > + *
> > > > > > + * Some controls have to be applied for a capture associated with Request.
> > > > > > + * This picks up such controls and set the driver them.
> > > > > > + *
> > > > > > + * \return 0 on success or an error code otherwise
> > > > > > + */
> > > > > > +int32_t CameraSensor::applyRequestControls(Request *request)
> > > > > > +{
> > > > > > +     /* Assumes applying the test pattern mode affects immediately. */
> > > > > > +     if (request->controls().contains(controls::draft::TestPatternMode)) {
> > > > > > +             const int32_t testPatternMode = request->controls().get(
> > > > > > +                     controls::draft::TestPatternMode);
> > > > > > +
> > > > > > +             LOG(CameraSensor, Debug) << "Apply test pattern mode: "
> > > > > > +                                      << testPatternMode;
> > > > > > +
> > > > > > +             int ret = setTestPatternMode(
> > > > > > +                     static_cast<controls::draft::TestPatternModeEnum>(testPatternMode));
> > > > > > +             if (ret) {
> > > > > > +                     LOG(CameraSensor, Error)
> > > > > > +                             << "Failed to set test pattern mode: " << ret;
> > > > > > +                     return ret;
> > > > > > +             }
> > > > > > +
> > > > > > +             request->metadata().set(controls::draft::TestPatternMode,
> > > > > > +                                     testPatternMode);
> > > > > > +     }
> > >
> > > Later, I'd expect this function to be more efficient, and process all
> > > controls (or as many as possible) in one ControlList to be able to set
> > > them as a group. But as we only have one for now, I think this is fine.
> > >
> > > That would proabbly mean later we'll split setTestPatternMode to a
> > > function which just does the mapping of a TestPatternMode and puts it in
> > > a control list ready to be set ... or such ... but that can all be done
> > > on top when we have other controls to set from a request.
> > >
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > >  /**
> > > > > >   * \fn CameraSensor::device()
> > > > > >   * \brief Retrieve the camera sensor device
> > > > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> > > > > > index 9d4638ae..083bac7b 100644
> > > > > > --- a/src/libcamera/control_ids.yaml
> > > > > > +++ b/src/libcamera/control_ids.yaml
> > > > > > @@ -639,6 +639,11 @@ controls:
> > > > > >          Control to select the test pattern mode. Currently identical to
> > > > > >          ANDROID_SENSOR_TEST_PATTERN_MODE.
> > > > > >        enum:
> > > > > > +        - name: TestPatternModeUnset
> > > > > > +          value: -1
> > > > > > +          description: |
> > > > > > +            No test pattern is set. Returned frames by the camera device are
> > > > > > +            undefined.
> > >
> > > I'm still not sure this is needed, but I don't object to it being added
> > > if you're sure this state is required.
> > >
> > > I think the CameraSensor class should set TestPatternModeOff at init()
> > > time if there is a test pattern control available on the sensor.
> > >
> > > Then any test patterns will only be applicable if explictly requested by
> > > the application (/request).
> > >
> > > > > >          - name: TestPatternModeOff
> > > > > >            value: 0
> > > > > >            description: |
> > > > > > --
> > > > > > 2.33.1.1089.g2158813163f-goog
> > > > > >