<div dir="ltr"><div dir="ltr">Hi Jacopo, thank you for reviewing.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, May 27, 2021 at 6:19 AM Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Hiro<br>
<br>
On Wed, May 19, 2021 at 04:59:39PM +0900, Hirokazu Honda wrote:<br>
> This enables retrieving supported test pattern modes through<br>
> CameraSensorInfo.<br>
><br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
>  include/libcamera/internal/camera_sensor.h |  7 +++++<br>
>  src/libcamera/camera_sensor.cpp            | 34 ++++++++++++++++++++++<br>
>  2 files changed, 41 insertions(+)<br>
><br>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h<br>
> index 2a5c51a1..59d1188f 100644<br>
> --- a/include/libcamera/internal/camera_sensor.h<br>
> +++ b/include/libcamera/internal/camera_sensor.h<br>
> @@ -58,6 +58,10 @@ public:<br>
>       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,<br>
>                                     const Size &size) const;<br>
>       int setFormat(V4L2SubdeviceFormat *format);<br>
> +     const std::vector<uint8_t> &testPatternModes() const<br>
> +     {<br>
> +             return testPatternModes_;<br>
> +     }<br>
<br>
I would move this just below resolution().<br>
It is not related to get/setFormat()<br>
<br>
><br>
>       const ControlInfoMap &controls() const;<br>
>       ControlList getControls(const std::vector<uint32_t> &ids);<br>
> @@ -81,6 +85,8 @@ private:<br>
>       void initVimcDefaultProperties();<br>
>       void initStaticProperties();<br>
>       int initProperties();<br>
> +     void initTestPatternModes(<br>
> +             const std::map<uint8_t, uint8_t> &testPatternModeMap);<br>
><br>
>       const MediaEntity *entity_;<br>
>       std::unique_ptr<V4L2Subdevice> subdev_;<br>
> @@ -92,6 +98,7 @@ private:<br>
>       V4L2Subdevice::Formats formats_;<br>
>       std::vector<unsigned int> mbusCodes_;<br>
>       std::vector<Size> sizes_;<br>
> +     std::vector<uint8_t> testPatternModes_;<br>
><br>
>       Size pixelArraySize_;<br>
>       Rectangle activeArea_;<br>
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp<br>
> index eb84d9eb..a6aed417 100644<br>
> --- a/src/libcamera/camera_sensor.cpp<br>
> +++ b/src/libcamera/camera_sensor.cpp<br>
> @@ -408,6 +408,32 @@ void CameraSensor::initVimcDefaultProperties()<br>
>       activeArea_ = Rectangle(pixelArraySize_);<br>
>  }<br>
><br>
> +void CameraSensor::initTestPatternModes(<br>
> +     const std::map<uint8_t, uint8_t> &testPatternModeMap)<br>
> +{<br>
> +     const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);<br>
> +     if (v4l2TestPattern == controls().end()) {<br>
> +             LOG(CameraSensor, Debug) << "No static test pattern map for \'"<br>
> +                                      << model() << "\'";<br>
> +             return;<br>
> +     }<br>
> +<br>
> +     for (const ControlValue &value : v4l2TestPattern->second.values()) {<br>
<br>
We have the index-control association in the sensor db, and the list of<br>
indexes in the sensor's controls.<br>
<br>
I would be fine with just using the map in the sensor db, without<br>
going through indexes to be honest.<br>
<br>
> +             const int32_t index = value.get<int32_t>();<br>
> +<br>
> +             const auto it = testPatternModeMap.find(index);<br>
> +             if (it != testPatternModeMap.end()) {<br>
> +                     LOG(CameraSensor, Debug) << "Test pattern mode (index="<br>
> +                                              << index << ") ignored";<br>
<br>
But if you feel it's safer to cross-check between the two, maybe it is<br>
worth to report this issue ( I would s/\(index=\)// )<br>
as it might indicate the sensor driver and the sensor db diverged<br>
<br></blockquote><div><br></div><div>I see. I promote the log level to Warning.</div><div>-Hiro</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +                     continue;<br>
> +             }<br>
> +<br>
> +             LOG(CameraSensor, Debug) << "Test pattern mode (index="<br>
> +                                      << index << ") added";<br>
<br>
But this one has no real value at run-time<br>
<br>
> +             testPatternModes_.push_back(it->second);<br>
> +     }<br>
> +}<br>
> +<br>
>  void CameraSensor::initStaticProperties()<br>
>  {<br>
>       const CameraSensorProperties *props = CameraSensorProperties::get(model_);<br>
> @@ -416,6 +442,8 @@ void CameraSensor::initStaticProperties()<br>
><br>
>       /* Register the properties retrieved from the sensor database. */<br>
>       properties_.set(properties::UnitCellSize, props->unitCellSize);<br>
> +<br>
> +     initTestPatternModes(props->testPatternModeMap);<br>
>  }<br>
><br>
>  int CameraSensor::initProperties()<br>
> @@ -767,6 +795,12 @@ int CameraSensor::setControls(ControlList *ctrls)<br>
>   * \return The list of camera sensor properties<br>
>   */<br>
><br>
> +/**<br>
> + * \fn CameraSensor::testPatternModes()<br>
> + * \brief Retrieve all the supported test pattern modes of the camera sensor<br>
> + * \return The list of test pattern modes.<br>
<br>
No full stop please.<br>
<br>
Mostly minors<br>
Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>><br>
<br>
Thanks<br>
  j<br>
<br>
<br>
> + */<br>
> +<br>
>  /**<br>
>   * \brief Assemble and return the camera sensor info<br>
>   * \param[out] info The camera sensor info<br>
> --<br>
> 2.31.1.751.gd2f1c929bd-goog<br>
><br>
</blockquote></div></div>