<div dir="ltr"><div dir="ltr">Hi Jacopo, thanks for reviewing.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, May 18, 2021 at 9:21 PM 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 Thu, May 06, 2021 at 04:54:46PM +0900, Hirokazu Honda wrote:<br>
> In V4L2 API, a driver returns an index (also with a name) to<br>
> represent a test pattern, but it is a driver specific what test<br>
> pattern is represented by the index. Therefore, this adds a<br>
> mapping table from the index and a test pattern into a static<br>
> configuration of a sensor.<br>
<br>
A bit convoluted... What about something along the lines of:<br>
<br>
The V4L2 specification defines the sensor test pattern modes<br>
through a menu control, where a numerical index is associated to<br>
a string that describes the test pattern. The index-to-pattern<br>
mapping is driver specific and requires a corresponding representation<br>
in the library.<br>
<br>
Add to the static list of CameraSensorProperties a map of indexes to<br>
libcamera::controls::TestPatternModes values to be able to map the<br>
indexes returned by the driver to the corresponding test pattern mode.<br>
<br>
<br>
><br>
> Signed-off-by: Hirokazu Honda <<a href="mailto:hiroh@chromium.org" target="_blank">hiroh@chromium.org</a>><br>
> ---<br>
> .../internal/camera_sensor_properties.h | 2 ++<br>
> src/libcamera/camera_sensor_properties.cpp | 20 +++++++++++++++++++<br>
> 2 files changed, 22 insertions(+)<br>
><br>
> diff --git a/include/libcamera/internal/camera_sensor_properties.h b/include/libcamera/internal/camera_sensor_properties.h<br>
> index f5e242cb..f52890b4 100644<br>
> --- a/include/libcamera/internal/camera_sensor_properties.h<br>
> +++ b/include/libcamera/internal/camera_sensor_properties.h<br>
> @@ -7,6 +7,7 @@<br>
> #ifndef __LIBCAMERA_SENSOR_CAMERA_SENSOR_PROPERTIES_H__<br>
> #define __LIBCAMERA_SENSOR_CAMERA_SENSOR_PROPERTIES_H__<br>
><br>
> +#include <map><br>
> #include <string><br>
><br>
> #include <libcamera/geometry.h><br>
> @@ -17,6 +18,7 @@ struct CameraSensorProperties {<br>
> static const CameraSensorProperties *get(const std::string &sensor);<br>
><br>
> Size unitCellSize;<br>
> + std::map<int32_t, int32_t> testPatternModeMap;<br>
<br>
I would expect that an uint8_t to uint8_t should do<br>
<br></blockquote><div><br></div><div>It should be enough although I do so because control value and index value are int32_t.</div><div>I don't have a strong opinion. Changed to uint8_t.</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">
> };<br>
><br>
> } /* namespace libcamera */<br>
> diff --git a/src/libcamera/camera_sensor_properties.cpp b/src/libcamera/camera_sensor_properties.cpp<br>
> index 6ded31dc..89eaf9f8 100644<br>
> --- a/src/libcamera/camera_sensor_properties.cpp<br>
> +++ b/src/libcamera/camera_sensor_properties.cpp<br>
> @@ -9,6 +9,8 @@<br>
><br>
> #include <map><br>
><br>
> +#include <libcamera/control_ids.h><br>
> +<br>
> #include "libcamera/internal/log.h"<br>
><br>
> /**<br>
> @@ -44,15 +46,33 @@ LOG_DEFINE_CATEGORY(CameraSensorProperties)<br>
> */<br>
> const CameraSensorProperties *CameraSensorProperties::get(const std::string &sensor)<br>
> {<br>
> +<br>
<br>
Unrelated blank line<br>
<br>
> static const std::map<std::string, const CameraSensorProperties> sensorProps = {<br>
> { "imx219", {<br>
> .unitCellSize = { 1120, 1120 },<br>
> + .testPatternModeMap = {<br>
> + { 0, controls::draft::TestPatternModeOff },<br>
> + { 1, controls::draft::TestPatternModeColorBars },<br>
> + { 2, controls::draft::TestPatternModeSolidColor },<br>
> + { 3, controls::draft::TestPatternModeColorBarsFadeToGray },<br>
> + { 4, controls::draft::TestPatternModePn9 },<br>
> + },<br>
<br>
This is from the imx219 driver<br>
<br>
#define IMX219_TEST_PATTERN_DISABLE 0<br>
#define IMX219_TEST_PATTERN_SOLID_COLOR 1<br>
#define IMX219_TEST_PATTERN_COLOR_BARS 2<br>
#define IMX219_TEST_PATTERN_GREY_COLOR 3<br>
#define IMX219_TEST_PATTERN_PN9 4<br>
<br>
Should you map 1 to controls::draft::TestPatternModeSolidColor and 2<br>
to controls::draft::TestPatternModeColorBars ?<br>
<br></blockquote><div><br></div><div>Those values are for the imx219 register.</div><div>Interestingly, imx_219_test_pattern_menu[] and imx_219_test_pattern_val[] are in different order from the values.</div><div>I think my map is correct. This is very confusing. IMO, the different order among them doesn't make sense, which should be fixed.</div><div><a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/next/drivers/media/i2c/imx219.c;l=395;drc=c90b4d70b1746f5a46fb7bad988731e604a44d4e">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/next/drivers/media/i2c/imx219.c;l=395;drc=c90b4d70b1746f5a46fb7bad988731e604a44d4e</a><br></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">
> } },<br>
> { "ov5670", {<br>
> .unitCellSize = { 1120, 1120 },<br>
> + .testPatternModeMap = {<br>
> + { 0, controls::draft::TestPatternModeOff },<br>
> + { 1, controls::draft::TestPatternModeColorBars },<br>
> + },<br>
<br>
The driver only defines:<br>
#define OV5670_TEST_PATTERN_ENABLE BIT(3)<br>
<br>
does this map to ColorBars ?<br>
<br></blockquote><div><br></div><div>The value is a value to notify the ov5670 sensor tha a test pattern mode is enabled.</div><div>The array for th test pattern menu is <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/next/drivers/media/i2c/ov5670.c;l=1718;drc=eba08021e15076afc21b506e71e2f4e523f27f8c">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/next/drivers/media/i2c/ov5670.c;l=1718;drc=eba08021e15076afc21b506e71e2f4e523f27f8c</a>.<br></div><div>I think my map is correct.</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">
> +<br>
<br>
Unrelated blank line<br>
<br>
> } },<br>
> { "ov13858", {<br>
> .unitCellSize = { 1120, 1120 },<br>
> + .testPatternModeMap = {<br>
> + { 0, controls::draft::TestPatternModeOff },<br>
> + { 1, controls::draft::TestPatternModeColorBars },<br>
> + { 2, controls::draft::TestPatternModeColorBarsFadeToGray },<br>
<br>
The driver only reports 1 mode:<br>
#define OV13858_TEST_PATTERN_ENABLE BIT(7)<br>
<br>
Where does the second come from ?<br>
<br></blockquote><div><br></div><div>Same. The value is a value to notify the ov13858 sensor tha a test pattern mode is enabled.</div><div>The array for the test pattern menu is <a href="https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/next/drivers/media/i2c/ov13858.c;l=929;drc=74c3ddd9887f60824891d2574a1689e8c13bf191">https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/next/drivers/media/i2c/ov13858.c;l=929;drc=74c3ddd9887f60824891d2574a1689e8c13bf191</a><br></div><div>I think my map is correct.</div><div><br></div><div>Thanks,</div><div>-Hiro</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Thanks<br>
j<br>
<br>
> + },<br>
> } },<br>
> };<br>
><br>
> --<br>
> 2.31.1.607.g51e8a6a459-goog<br>
><br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
</blockquote></div></div>