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