[libcamera-devel,v5,4/6] libcamera: CameraSensor: Enable retrieving supported test pattern modes
diff mbox series

Message ID 20210519075941.1337388-4-hiroh@chromium.org
State Superseded
Headers show
Series
  • [libcamera-devel,v5,1/6] libcamera: controls: Add sensor test pattern mode
Related show

Commit Message

Hirokazu Honda May 19, 2021, 7:59 a.m. UTC 
This enables retrieving supported test pattern modes through
CameraSensorInfo.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/internal/camera_sensor.h |  7 +++++
 src/libcamera/camera_sensor.cpp            | 34 ++++++++++++++++++++++
 2 files changed, 41 insertions(+)

Comments

Jacopo Mondi May 26, 2021, 9:20 p.m. UTC | #1 
Hi Hiro

On Wed, May 19, 2021 at 04:59:39PM +0900, Hirokazu Honda wrote:
> This enables retrieving supported test pattern modes through
> CameraSensorInfo.
>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  7 +++++
>  src/libcamera/camera_sensor.cpp            | 34 ++++++++++++++++++++++
>  2 files changed, 41 insertions(+)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 2a5c51a1..59d1188f 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -58,6 +58,10 @@ public:
>  	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
>  				      const Size &size) const;
>  	int setFormat(V4L2SubdeviceFormat *format);
> +	const std::vector<uint8_t> &testPatternModes() const
> +	{
> +		return testPatternModes_;
> +	}

I would move this just below resolution().
It is not related to get/setFormat()

>
>  	const ControlInfoMap &controls() const;
>  	ControlList getControls(const std::vector<uint32_t> &ids);
> @@ -81,6 +85,8 @@ private:
>  	void initVimcDefaultProperties();
>  	void initStaticProperties();
>  	int initProperties();
> +	void initTestPatternModes(
> +		const std::map<uint8_t, uint8_t> &testPatternModeMap);
>
>  	const MediaEntity *entity_;
>  	std::unique_ptr<V4L2Subdevice> subdev_;
> @@ -92,6 +98,7 @@ private:
>  	V4L2Subdevice::Formats formats_;
>  	std::vector<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
> +	std::vector<uint8_t> testPatternModes_;
>
>  	Size pixelArraySize_;
>  	Rectangle activeArea_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index eb84d9eb..a6aed417 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -408,6 +408,32 @@ void CameraSensor::initVimcDefaultProperties()
>  	activeArea_ = Rectangle(pixelArraySize_);
>  }
>
> +void CameraSensor::initTestPatternModes(
> +	const std::map<uint8_t, uint8_t> &testPatternModeMap)
> +{
> +	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> +	if (v4l2TestPattern == controls().end()) {
> +		LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> +					 << model() << "\'";
> +		return;
> +	}
> +
> +	for (const ControlValue &value : v4l2TestPattern->second.values()) {

We have the index-control association in the sensor db, and the list of
indexes in the sensor's controls.

I would be fine with just using the map in the sensor db, without
going through indexes to be honest.

> +		const int32_t index = value.get<int32_t>();
> +
> +		const auto it = testPatternModeMap.find(index);
> +		if (it != testPatternModeMap.end()) {
> +			LOG(CameraSensor, Debug) << "Test pattern mode (index="
> +						 << index << ") ignored";

But if you feel it's safer to cross-check between the two, maybe it is
worth to report this issue ( I would s/\(index=\)// )
as it might indicate the sensor driver and the sensor db diverged

> +			continue;
> +		}
> +
> +		LOG(CameraSensor, Debug) << "Test pattern mode (index="
> +					 << index << ") added";

But this one has no real value at run-time

> +		testPatternModes_.push_back(it->second);
> +	}
> +}
> +
>  void CameraSensor::initStaticProperties()
>  {
>  	const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> @@ -416,6 +442,8 @@ void CameraSensor::initStaticProperties()
>
>  	/* Register the properties retrieved from the sensor database. */
>  	properties_.set(properties::UnitCellSize, props->unitCellSize);
> +
> +	initTestPatternModes(props->testPatternModeMap);
>  }
>
>  int CameraSensor::initProperties()
> @@ -767,6 +795,12 @@ int CameraSensor::setControls(ControlList *ctrls)
>   * \return The list of camera sensor properties
>   */
>
> +/**
> + * \fn CameraSensor::testPatternModes()
> + * \brief Retrieve all the supported test pattern modes of the camera sensor
> + * \return The list of test pattern modes.

No full stop please.

Mostly minors
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j


> + */
> +
>  /**
>   * \brief Assemble and return the camera sensor info
>   * \param[out] info The camera sensor info
> --
> 2.31.1.751.gd2f1c929bd-goog
>
Hirokazu Honda May 27, 2021, 6:34 a.m. UTC | #2 
Hi Jacopo, thank you for reviewing.

On Thu, May 27, 2021 at 6:19 AM Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Hiro
>
> On Wed, May 19, 2021 at 04:59:39PM +0900, Hirokazu Honda wrote:
> > This enables retrieving supported test pattern modes through
> > CameraSensorInfo.
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  7 +++++
> >  src/libcamera/camera_sensor.cpp            | 34 ++++++++++++++++++++++
> >  2 files changed, 41 insertions(+)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h
> b/include/libcamera/internal/camera_sensor.h
> > index 2a5c51a1..59d1188f 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -58,6 +58,10 @@ public:
> >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int>
> &mbusCodes,
> >                                     const Size &size) const;
> >       int setFormat(V4L2SubdeviceFormat *format);
> > +     const std::vector<uint8_t> &testPatternModes() const
> > +     {
> > +             return testPatternModes_;
> > +     }
>
> I would move this just below resolution().
> It is not related to get/setFormat()
>
> >
> >       const ControlInfoMap &controls() const;
> >       ControlList getControls(const std::vector<uint32_t> &ids);
> > @@ -81,6 +85,8 @@ private:
> >       void initVimcDefaultProperties();
> >       void initStaticProperties();
> >       int initProperties();
> > +     void initTestPatternModes(
> > +             const std::map<uint8_t, uint8_t> &testPatternModeMap);
> >
> >       const MediaEntity *entity_;
> >       std::unique_ptr<V4L2Subdevice> subdev_;
> > @@ -92,6 +98,7 @@ private:
> >       V4L2Subdevice::Formats formats_;
> >       std::vector<unsigned int> mbusCodes_;
> >       std::vector<Size> sizes_;
> > +     std::vector<uint8_t> testPatternModes_;
> >
> >       Size pixelArraySize_;
> >       Rectangle activeArea_;
> > diff --git a/src/libcamera/camera_sensor.cpp
> b/src/libcamera/camera_sensor.cpp
> > index eb84d9eb..a6aed417 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -408,6 +408,32 @@ void CameraSensor::initVimcDefaultProperties()
> >       activeArea_ = Rectangle(pixelArraySize_);
> >  }
> >
> > +void CameraSensor::initTestPatternModes(
> > +     const std::map<uint8_t, uint8_t> &testPatternModeMap)
> > +{
> > +     const auto &v4l2TestPattern =
> controls().find(V4L2_CID_TEST_PATTERN);
> > +     if (v4l2TestPattern == controls().end()) {
> > +             LOG(CameraSensor, Debug) << "No static test pattern map
> for \'"
> > +                                      << model() << "\'";
> > +             return;
> > +     }
> > +
> > +     for (const ControlValue &value : v4l2TestPattern->second.values())
> {
>
> We have the index-control association in the sensor db, and the list of
> indexes in the sensor's controls.
>
> I would be fine with just using the map in the sensor db, without
> going through indexes to be honest.
>
> > +             const int32_t index = value.get<int32_t>();
> > +
> > +             const auto it = testPatternModeMap.find(index);
> > +             if (it != testPatternModeMap.end()) {
> > +                     LOG(CameraSensor, Debug) << "Test pattern mode
> (index="
> > +                                              << index << ") ignored";
>
> But if you feel it's safer to cross-check between the two, maybe it is
> worth to report this issue ( I would s/\(index=\)// )
> as it might indicate the sensor driver and the sensor db diverged
>
>
I see. I promote the log level to Warning.
-Hiro


> > +                     continue;
> > +             }
> > +
> > +             LOG(CameraSensor, Debug) << "Test pattern mode (index="
> > +                                      << index << ") added";
>
> But this one has no real value at run-time
>
> > +             testPatternModes_.push_back(it->second);
> > +     }
> > +}
> > +
> >  void CameraSensor::initStaticProperties()
> >  {
> >       const CameraSensorProperties *props =
> CameraSensorProperties::get(model_);
> > @@ -416,6 +442,8 @@ void CameraSensor::initStaticProperties()
> >
> >       /* Register the properties retrieved from the sensor database. */
> >       properties_.set(properties::UnitCellSize, props->unitCellSize);
> > +
> > +     initTestPatternModes(props->testPatternModeMap);
> >  }
> >
> >  int CameraSensor::initProperties()
> > @@ -767,6 +795,12 @@ int CameraSensor::setControls(ControlList *ctrls)
> >   * \return The list of camera sensor properties
> >   */
> >
> > +/**
> > + * \fn CameraSensor::testPatternModes()
> > + * \brief Retrieve all the supported test pattern modes of the camera
> sensor
> > + * \return The list of test pattern modes.
>
> No full stop please.
>
> Mostly minors
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>   j
>
>
> > + */
> > +
> >  /**
> >   * \brief Assemble and return the camera sensor info
> >   * \param[out] info The camera sensor info
> > --
> > 2.31.1.751.gd2f1c929bd-goog
> >
>
Hirokazu Honda May 28, 2021, 2:45 a.m. UTC | #3 
On Thu, May 27, 2021 at 3:34 PM Hirokazu Honda <hiroh@chromium.org> wrote:

> Hi Jacopo, thank you for reviewing.
>
> On Thu, May 27, 2021 at 6:19 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
>> Hi Hiro
>>
>> On Wed, May 19, 2021 at 04:59:39PM +0900, Hirokazu Honda wrote:
>> > This enables retrieving supported test pattern modes through
>> > CameraSensorInfo.
>> >
>> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
>> > ---
>> >  include/libcamera/internal/camera_sensor.h |  7 +++++
>> >  src/libcamera/camera_sensor.cpp            | 34 ++++++++++++++++++++++
>> >  2 files changed, 41 insertions(+)
>> >
>> > diff --git a/include/libcamera/internal/camera_sensor.h
>> b/include/libcamera/internal/camera_sensor.h
>> > index 2a5c51a1..59d1188f 100644
>> > --- a/include/libcamera/internal/camera_sensor.h
>> > +++ b/include/libcamera/internal/camera_sensor.h
>> > @@ -58,6 +58,10 @@ public:
>> >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int>
>> &mbusCodes,
>> >                                     const Size &size) const;
>> >       int setFormat(V4L2SubdeviceFormat *format);
>> > +     const std::vector<uint8_t> &testPatternModes() const
>> > +     {
>> > +             return testPatternModes_;
>> > +     }
>>
>> I would move this just below resolution().
>> It is not related to get/setFormat()
>>
>> >
>> >       const ControlInfoMap &controls() const;
>> >       ControlList getControls(const std::vector<uint32_t> &ids);
>> > @@ -81,6 +85,8 @@ private:
>> >       void initVimcDefaultProperties();
>> >       void initStaticProperties();
>> >       int initProperties();
>> > +     void initTestPatternModes(
>> > +             const std::map<uint8_t, uint8_t> &testPatternModeMap);
>> >
>> >       const MediaEntity *entity_;
>> >       std::unique_ptr<V4L2Subdevice> subdev_;
>> > @@ -92,6 +98,7 @@ private:
>> >       V4L2Subdevice::Formats formats_;
>> >       std::vector<unsigned int> mbusCodes_;
>> >       std::vector<Size> sizes_;
>> > +     std::vector<uint8_t> testPatternModes_;
>> >
>> >       Size pixelArraySize_;
>> >       Rectangle activeArea_;
>> > diff --git a/src/libcamera/camera_sensor.cpp
>> b/src/libcamera/camera_sensor.cpp
>> > index eb84d9eb..a6aed417 100644
>> > --- a/src/libcamera/camera_sensor.cpp
>> > +++ b/src/libcamera/camera_sensor.cpp
>> > @@ -408,6 +408,32 @@ void CameraSensor::initVimcDefaultProperties()
>> >       activeArea_ = Rectangle(pixelArraySize_);
>> >  }
>> >
>> > +void CameraSensor::initTestPatternModes(
>> > +     const std::map<uint8_t, uint8_t> &testPatternModeMap)
>> > +{
>> > +     const auto &v4l2TestPattern =
>> controls().find(V4L2_CID_TEST_PATTERN);
>> > +     if (v4l2TestPattern == controls().end()) {
>> > +             LOG(CameraSensor, Debug) << "No static test pattern map
>> for \'"
>> > +                                      << model() << "\'";
>> > +             return;
>> > +     }
>> > +
>> > +     for (const ControlValue &value :
>> v4l2TestPattern->second.values()) {
>>
>> We have the index-control association in the sensor db, and the list of
>> indexes in the sensor's controls.
>>
>> I would be fine with just using the map in the sensor db, without
>> going through indexes to be honest.
>>
>> > +             const int32_t index = value.get<int32_t>();
>> > +
>> > +             const auto it = testPatternModeMap.find(index);
>> > +             if (it != testPatternModeMap.end()) {
>> > +                     LOG(CameraSensor, Debug) << "Test pattern mode
>> (index="
>> > +                                              << index << ") ignored";
>>
>> But if you feel it's safer to cross-check between the two, maybe it is
>> worth to report this issue ( I would s/\(index=\)// )
>> as it might indicate the sensor driver and the sensor db diverged
>>
>>
> I see. I promote the log level to Warning.
>

I found ov5693 has test pattern values that no test pattern mode
corresponds to and they will hit this Warning.
Then logging them in warning here is misleading. So I set this log level to
Debug again.


> -Hiro
>
>
>> > +                     continue;
>> > +             }
>> > +
>> > +             LOG(CameraSensor, Debug) << "Test pattern mode (index="
>> > +                                      << index << ") added";
>>
>> But this one has no real value at run-time
>>
>> > +             testPatternModes_.push_back(it->second);
>> > +     }
>> > +}
>> > +
>> >  void CameraSensor::initStaticProperties()
>> >  {
>> >       const CameraSensorProperties *props =
>> CameraSensorProperties::get(model_);
>> > @@ -416,6 +442,8 @@ void CameraSensor::initStaticProperties()
>> >
>> >       /* Register the properties retrieved from the sensor database. */
>> >       properties_.set(properties::UnitCellSize, props->unitCellSize);
>> > +
>> > +     initTestPatternModes(props->testPatternModeMap);
>> >  }
>> >
>> >  int CameraSensor::initProperties()
>> > @@ -767,6 +795,12 @@ int CameraSensor::setControls(ControlList *ctrls)
>> >   * \return The list of camera sensor properties
>> >   */
>> >
>> > +/**
>> > + * \fn CameraSensor::testPatternModes()
>> > + * \brief Retrieve all the supported test pattern modes of the camera
>> sensor
>> > + * \return The list of test pattern modes.
>>
>> No full stop please.
>>
>> Mostly minors
>> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>>
>> Thanks
>>   j
>>
>>
>> > + */
>> > +
>> >  /**
>> >   * \brief Assemble and return the camera sensor info
>> >   * \param[out] info The camera sensor info
>> > --
>> > 2.31.1.751.gd2f1c929bd-goog
>> >
>>
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index 2a5c51a1..59d1188f 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -58,6 +58,10 @@  public:
 	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
 				      const Size &size) const;
 	int setFormat(V4L2SubdeviceFormat *format);
+	const std::vector<uint8_t> &testPatternModes() const
+	{
+		return testPatternModes_;
+	}
 
 	const ControlInfoMap &controls() const;
 	ControlList getControls(const std::vector<uint32_t> &ids);
@@ -81,6 +85,8 @@  private:
 	void initVimcDefaultProperties();
 	void initStaticProperties();
 	int initProperties();
+	void initTestPatternModes(
+		const std::map<uint8_t, uint8_t> &testPatternModeMap);
 
 	const MediaEntity *entity_;
 	std::unique_ptr<V4L2Subdevice> subdev_;
@@ -92,6 +98,7 @@  private:
 	V4L2Subdevice::Formats formats_;
 	std::vector<unsigned int> mbusCodes_;
 	std::vector<Size> sizes_;
+	std::vector<uint8_t> testPatternModes_;
 
 	Size pixelArraySize_;
 	Rectangle activeArea_;
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index eb84d9eb..a6aed417 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -408,6 +408,32 @@  void CameraSensor::initVimcDefaultProperties()
 	activeArea_ = Rectangle(pixelArraySize_);
 }
 
+void CameraSensor::initTestPatternModes(
+	const std::map<uint8_t, uint8_t> &testPatternModeMap)
+{
+	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
+	if (v4l2TestPattern == controls().end()) {
+		LOG(CameraSensor, Debug) << "No static test pattern map for \'"
+					 << model() << "\'";
+		return;
+	}
+
+	for (const ControlValue &value : v4l2TestPattern->second.values()) {
+		const int32_t index = value.get<int32_t>();
+
+		const auto it = testPatternModeMap.find(index);
+		if (it != testPatternModeMap.end()) {
+			LOG(CameraSensor, Debug) << "Test pattern mode (index="
+						 << index << ") ignored";
+			continue;
+		}
+
+		LOG(CameraSensor, Debug) << "Test pattern mode (index="
+					 << index << ") added";
+		testPatternModes_.push_back(it->second);
+	}
+}
+
 void CameraSensor::initStaticProperties()
 {
 	const CameraSensorProperties *props = CameraSensorProperties::get(model_);
@@ -416,6 +442,8 @@  void CameraSensor::initStaticProperties()
 
 	/* Register the properties retrieved from the sensor database. */
 	properties_.set(properties::UnitCellSize, props->unitCellSize);
+
+	initTestPatternModes(props->testPatternModeMap);
 }
 
 int CameraSensor::initProperties()
@@ -767,6 +795,12 @@  int CameraSensor::setControls(ControlList *ctrls)
  * \return The list of camera sensor properties
  */
 
+/**
+ * \fn CameraSensor::testPatternModes()
+ * \brief Retrieve all the supported test pattern modes of the camera sensor
+ * \return The list of test pattern modes.
+ */
+
 /**
  * \brief Assemble and return the camera sensor info
  * \param[out] info The camera sensor info