* [PATCH 1/3] media: imx: enable V4L2_PIX_FMT_XBGR32, _BGRX32, and _RGBX32
@ 2019-09-12 16:01 Philipp Zabel
2019-09-12 16:01 ` [PATCH 2/3] media: imx: fix and simplify pixel format enumeration Philipp Zabel
2019-09-12 16:01 ` [PATCH 3/3] media: imx: fix media bus " Philipp Zabel
0 siblings, 2 replies; 10+ messages in thread
From: Philipp Zabel @ 2019-09-12 16:01 UTC (permalink / raw)
To: linux-media; +Cc: Steve Longerbeam, kernel
Now that proper V4L2 pixel formats exist for all 32-bit RGB
permutations, drop support for the poorly defined legacy format
V4L2_PIX_FMT_BGR32.
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/staging/media/imx/imx-media-utils.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 4cc6a7462ae2..0788a1874557 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -184,7 +184,15 @@ static const struct imx_media_pixfmt rgb_formats[] = {
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 24,
}, {
- .fourcc = V4L2_PIX_FMT_BGR32,
+ .fourcc = V4L2_PIX_FMT_XBGR32,
+ .cs = IPUV3_COLORSPACE_RGB,
+ .bpp = 32,
+ }, {
+ .fourcc = V4L2_PIX_FMT_BGRX32,
+ .cs = IPUV3_COLORSPACE_RGB,
+ .bpp = 32,
+ }, {
+ .fourcc = V4L2_PIX_FMT_RGBX32,
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 32,
},
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] media: imx: fix and simplify pixel format enumeration
2019-09-12 16:01 [PATCH 1/3] media: imx: enable V4L2_PIX_FMT_XBGR32, _BGRX32, and _RGBX32 Philipp Zabel
@ 2019-09-12 16:01 ` Philipp Zabel
2019-09-14 3:36 ` kbuild test robot
` (2 more replies)
2019-09-12 16:01 ` [PATCH 3/3] media: imx: fix media bus " Philipp Zabel
1 sibling, 3 replies; 10+ messages in thread
From: Philipp Zabel @ 2019-09-12 16:01 UTC (permalink / raw)
To: linux-media; +Cc: Steve Longerbeam, kernel
Merge yuv_formats and rgb_formats into a single array. Always loop over
all entries, skipping those that are incompatible with the requested
enumeration. This simplifies the code, lets us get rid of the manual
counting of array entries, and stops accidentally ignoring some non-mbus
RGB formats.
Before:
$ v4l2-ctl -d /dev/video14 --list-formats-out
ioctl: VIDIOC_ENUM_FMT
Type: Video Output
[0]: 'UYVY' (UYVY 4:2:2)
[1]: 'YUYV' (YUYV 4:2:2)
[2]: 'YU12' (Planar YUV 4:2:0)
[3]: 'YV12' (Planar YVU 4:2:0)
[4]: '422P' (Planar YUV 4:2:2)
[5]: 'NV12' (Y/CbCr 4:2:0)
[6]: 'NV16' (Y/CbCr 4:2:2)
[7]: 'RGBP' (16-bit RGB 5-6-5)
[8]: 'RGB3' (24-bit RGB 8-8-8)
[9]: 'BX24' (32-bit XRGB 8-8-8-8)
After:
$ v4l2-ctl -d /dev/video14 --list-formats-out
ioctl: VIDIOC_ENUM_FMT
Type: Video Output
[0]: 'UYVY' (UYVY 4:2:2)
[1]: 'YUYV' (YUYV 4:2:2)
[2]: 'YU12' (Planar YUV 4:2:0)
[3]: 'YV12' (Planar YVU 4:2:0)
[4]: '422P' (Planar YUV 4:2:2)
[5]: 'NV12' (Y/CbCr 4:2:0)
[6]: 'NV16' (Y/CbCr 4:2:2)
[7]: 'RGBP' (16-bit RGB 5-6-5)
[8]: 'RGB3' (24-bit RGB 8-8-8)
[9]: 'BGR3' (24-bit BGR 8-8-8)
[10]: 'BX24' (32-bit XRGB 8-8-8-8)
[11]: 'XR24' (32-bit BGRX 8-8-8-8)
[12]: 'RX24' (32-bit XBGR 8-8-8-8)
[13]: 'XB24' (32-bit RGBX 8-8-8-8)
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/staging/media/imx/imx-media-utils.c | 170 ++++++--------------
1 file changed, 45 insertions(+), 125 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 0788a1874557..d61a8f4533dc 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -9,12 +9,9 @@
/*
* List of supported pixel formats for the subdevs.
- *
- * In all of these tables, the non-mbus formats (with no
- * mbus codes) must all fall at the end of the table.
*/
-
-static const struct imx_media_pixfmt yuv_formats[] = {
+static const struct imx_media_pixfmt pixel_formats[] = {
+ /*** YUV formats start here ***/
{
.fourcc = V4L2_PIX_FMT_UYVY,
.codes = {
@@ -31,12 +28,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
},
.cs = IPUV3_COLORSPACE_YUV,
.bpp = 16,
- },
- /***
- * non-mbus YUV formats start here. NOTE! when adding non-mbus
- * formats, NUM_NON_MBUS_YUV_FORMATS must be updated below.
- ***/
- {
+ }, {
.fourcc = V4L2_PIX_FMT_YUV420,
.cs = IPUV3_COLORSPACE_YUV,
.bpp = 12,
@@ -62,13 +54,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
.bpp = 16,
.planar = true,
},
-};
-
-#define NUM_NON_MBUS_YUV_FORMATS 5
-#define NUM_YUV_FORMATS ARRAY_SIZE(yuv_formats)
-#define NUM_MBUS_YUV_FORMATS (NUM_YUV_FORMATS - NUM_NON_MBUS_YUV_FORMATS)
-
-static const struct imx_media_pixfmt rgb_formats[] = {
+ /*** RGB formats start here ***/
{
.fourcc = V4L2_PIX_FMT_RGB565,
.codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
@@ -83,12 +69,28 @@ static const struct imx_media_pixfmt rgb_formats[] = {
},
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 24,
+ }, {
+ .fourcc = V4L2_PIX_FMT_BGR24,
+ .cs = IPUV3_COLORSPACE_RGB,
+ .bpp = 24,
}, {
.fourcc = V4L2_PIX_FMT_XRGB32,
.codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 32,
.ipufmt = true,
+ }, {
+ .fourcc = V4L2_PIX_FMT_XBGR32,
+ .cs = IPUV3_COLORSPACE_RGB,
+ .bpp = 32,
+ }, {
+ .fourcc = V4L2_PIX_FMT_BGRX32,
+ .cs = IPUV3_COLORSPACE_RGB,
+ .bpp = 32,
+ }, {
+ .fourcc = V4L2_PIX_FMT_RGBX32,
+ .cs = IPUV3_COLORSPACE_RGB,
+ .bpp = 32,
},
/*** raw bayer and grayscale formats start here ***/
{
@@ -175,33 +177,8 @@ static const struct imx_media_pixfmt rgb_formats[] = {
.bpp = 16,
.bayer = true,
},
- /***
- * non-mbus RGB formats start here. NOTE! when adding non-mbus
- * formats, NUM_NON_MBUS_RGB_FORMATS must be updated below.
- ***/
- {
- .fourcc = V4L2_PIX_FMT_BGR24,
- .cs = IPUV3_COLORSPACE_RGB,
- .bpp = 24,
- }, {
- .fourcc = V4L2_PIX_FMT_XBGR32,
- .cs = IPUV3_COLORSPACE_RGB,
- .bpp = 32,
- }, {
- .fourcc = V4L2_PIX_FMT_BGRX32,
- .cs = IPUV3_COLORSPACE_RGB,
- .bpp = 32,
- }, {
- .fourcc = V4L2_PIX_FMT_RGBX32,
- .cs = IPUV3_COLORSPACE_RGB,
- .bpp = 32,
- },
};
-#define NUM_NON_MBUS_RGB_FORMATS 2
-#define NUM_RGB_FORMATS ARRAY_SIZE(rgb_formats)
-#define NUM_MBUS_RGB_FORMATS (NUM_RGB_FORMATS - NUM_NON_MBUS_RGB_FORMATS)
-
static const struct imx_media_pixfmt ipu_yuv_formats[] = {
{
.fourcc = V4L2_PIX_FMT_YUV32,
@@ -240,20 +217,20 @@ static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
}
static const
-struct imx_media_pixfmt *__find_format(u32 fourcc,
- u32 code,
- bool allow_non_mbus,
- bool allow_bayer,
- const struct imx_media_pixfmt *array,
- u32 array_size)
+struct imx_media_pixfmt *find_format(u32 fourcc,
+ u32 code,
+ enum codespace_sel cs_sel,
+ bool allow_non_mbus,
+ bool allow_bayer)
{
const struct imx_media_pixfmt *fmt;
int i, j;
- for (i = 0; i < array_size; i++) {
- fmt = &array[i];
+ for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
+ fmt = &pixel_formats[i];
- if ((!allow_non_mbus && !fmt->codes[0]) ||
+ if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
+ (!allow_non_mbus && !fmt->codes[0]) ||
(!allow_bayer && fmt->bayer))
continue;
@@ -263,7 +240,7 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
if (!code)
continue;
- for (j = 0; fmt->codes[j]; j++) {
+ for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
if (code == fmt->codes[j])
return fmt;
}
@@ -271,86 +248,29 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
return NULL;
}
-static const struct imx_media_pixfmt *find_format(u32 fourcc,
- u32 code,
- enum codespace_sel cs_sel,
- bool allow_non_mbus,
- bool allow_bayer)
-{
- const struct imx_media_pixfmt *ret;
-
- switch (cs_sel) {
- case CS_SEL_YUV:
- return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
- yuv_formats, NUM_YUV_FORMATS);
- case CS_SEL_RGB:
- return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
- rgb_formats, NUM_RGB_FORMATS);
- case CS_SEL_ANY:
- ret = __find_format(fourcc, code, allow_non_mbus, allow_bayer,
- yuv_formats, NUM_YUV_FORMATS);
- if (ret)
- return ret;
- return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
- rgb_formats, NUM_RGB_FORMATS);
- default:
- return NULL;
- }
-}
-
static int enum_format(u32 *fourcc, u32 *code, u32 index,
enum codespace_sel cs_sel,
bool allow_non_mbus,
bool allow_bayer)
{
const struct imx_media_pixfmt *fmt;
- u32 mbus_yuv_sz = NUM_MBUS_YUV_FORMATS;
- u32 mbus_rgb_sz = NUM_MBUS_RGB_FORMATS;
- u32 yuv_sz = NUM_YUV_FORMATS;
- u32 rgb_sz = NUM_RGB_FORMATS;
+ unsigned int i, j = 0;
- switch (cs_sel) {
- case CS_SEL_YUV:
- if (index >= yuv_sz ||
- (!allow_non_mbus && index >= mbus_yuv_sz))
- return -EINVAL;
- fmt = &yuv_formats[index];
- break;
- case CS_SEL_RGB:
- if (index >= rgb_sz ||
- (!allow_non_mbus && index >= mbus_rgb_sz))
- return -EINVAL;
- fmt = &rgb_formats[index];
- if (!allow_bayer && fmt->bayer)
- return -EINVAL;
- break;
- case CS_SEL_ANY:
- if (!allow_non_mbus) {
- if (index >= mbus_yuv_sz) {
- index -= mbus_yuv_sz;
- if (index >= mbus_rgb_sz)
- return -EINVAL;
- fmt = &rgb_formats[index];
- if (!allow_bayer && fmt->bayer)
- return -EINVAL;
- } else {
- fmt = &yuv_formats[index];
- }
- } else {
- if (index >= yuv_sz + rgb_sz)
- return -EINVAL;
- if (index >= yuv_sz) {
- fmt = &rgb_formats[index - yuv_sz];
- if (!allow_bayer && fmt->bayer)
- return -EINVAL;
- } else {
- fmt = &yuv_formats[index];
- }
- }
- break;
- default:
- return -EINVAL;
+ for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
+ fmt = &pixel_formats[i];
+
+ if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
+ (!allow_non_mbus && !fmt->codes[0]) ||
+ (!allow_bayer && fmt->bayer))
+ continue;
+
+ if (index == j)
+ break;
+
+ j++;
}
+ if (i == ARRAY_SIZE(pixel_formats))
+ return -EINVAL;
if (fourcc)
*fourcc = fmt->fourcc;
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] media: imx: fix media bus format enumeration
2019-09-12 16:01 [PATCH 1/3] media: imx: enable V4L2_PIX_FMT_XBGR32, _BGRX32, and _RGBX32 Philipp Zabel
2019-09-12 16:01 ` [PATCH 2/3] media: imx: fix and simplify pixel format enumeration Philipp Zabel
@ 2019-09-12 16:01 ` Philipp Zabel
2019-09-14 3:49 ` kbuild test robot
2019-09-27 7:33 ` Hans Verkuil
1 sibling, 2 replies; 10+ messages in thread
From: Philipp Zabel @ 2019-09-12 16:01 UTC (permalink / raw)
To: linux-media; +Cc: Steve Longerbeam, kernel
Iterate over all media bus formats, not just over the first format in
each imx_media_pixfmt entry.
Before:
$ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
0x2006: MEDIA_BUS_FMT_UYVY8_2X8
0x2008: MEDIA_BUS_FMT_YUYV8_2X8
0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
0x100a: MEDIA_BUS_FMT_RGB888_1X24
0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
0x2001: MEDIA_BUS_FMT_Y8_1X8
0x200a: MEDIA_BUS_FMT_Y10_1X10
After:
$ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
0x2006: MEDIA_BUS_FMT_UYVY8_2X8
0x200f: MEDIA_BUS_FMT_UYVY8_1X16
0x2008: MEDIA_BUS_FMT_YUYV8_2X8
0x2011: MEDIA_BUS_FMT_YUYV8_1X16
0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
0x100a: MEDIA_BUS_FMT_RGB888_1X24
0x100c: MEDIA_BUS_FMT_RGB888_2X12_LE
0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
0x3008: MEDIA_BUS_FMT_SBGGR12_1X12
0x3019: MEDIA_BUS_FMT_SBGGR14_1X14
0x301d: MEDIA_BUS_FMT_SBGGR16_1X16
0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
0x3010: MEDIA_BUS_FMT_SGBRG12_1X12
0x301a: MEDIA_BUS_FMT_SGBRG14_1X14
0x301e: MEDIA_BUS_FMT_SGBRG16_1X16
0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
0x3011: MEDIA_BUS_FMT_SGRBG12_1X12
0x301b: MEDIA_BUS_FMT_SGRBG14_1X14
0x301f: MEDIA_BUS_FMT_SGRBG16_1X16
0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
0x3012: MEDIA_BUS_FMT_SRGGB12_1X12
0x301c: MEDIA_BUS_FMT_SRGGB14_1X14
0x3020: MEDIA_BUS_FMT_SRGGB16_1X16
0x2001: MEDIA_BUS_FMT_Y8_1X8
0x200a: MEDIA_BUS_FMT_Y10_1X10
0x2013: MEDIA_BUS_FMT_Y12_1X12
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
drivers/staging/media/imx/imx-media-utils.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index d61a8f4533dc..5f8604db4dd4 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -254,7 +254,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
bool allow_bayer)
{
const struct imx_media_pixfmt *fmt;
- unsigned int i, j = 0;
+ unsigned int i, j, k = 0;
for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
fmt = &pixel_formats[i];
@@ -264,18 +264,29 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
(!allow_bayer && fmt->bayer))
continue;
- if (index == j)
+ if (fourcc && index == k)
break;
- j++;
+ if (!code) {
+ k++;
+ continue;
+ }
+
+ for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
+ if (index == k)
+ goto out;
+
+ k++;
+ }
}
if (i == ARRAY_SIZE(pixel_formats))
return -EINVAL;
+out:
if (fourcc)
*fourcc = fmt->fourcc;
if (code)
- *code = fmt->codes[0];
+ *code = fmt->codes[j];
return 0;
}
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] media: imx: fix and simplify pixel format enumeration
2019-09-12 16:01 ` [PATCH 2/3] media: imx: fix and simplify pixel format enumeration Philipp Zabel
@ 2019-09-14 3:36 ` kbuild test robot
2019-09-27 8:03 ` Hans Verkuil
2019-10-25 21:43 ` Steve Longerbeam
2 siblings, 0 replies; 10+ messages in thread
From: kbuild test robot @ 2019-09-14 3:36 UTC (permalink / raw)
To: Philipp Zabel; +Cc: kbuild-all, linux-media, Steve Longerbeam, kernel
[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]
Hi Philipp,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linuxtv-media/master]
[cannot apply to v5.3-rc8 next-20190904]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Philipp-Zabel/media-imx-enable-V4L2_PIX_FMT_XBGR32-_BGRX32-and-_RGBX32/20190914-085415
base: git://linuxtv.org/media_tree.git master
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/staging/media/imx/imx-media-utils.c: In function 'find_format':
>> drivers/staging/media/imx/imx-media-utils.c:232:40: warning: comparison between 'const enum ipu_color_space' and 'enum codespace_sel' [-Wenum-compare]
if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
^~
drivers/staging/media/imx/imx-media-utils.c: In function 'enum_format':
drivers/staging/media/imx/imx-media-utils.c:262:40: warning: comparison between 'const enum ipu_color_space' and 'enum codespace_sel' [-Wenum-compare]
if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
^~
vim +232 drivers/staging/media/imx/imx-media-utils.c
218
219 static const
220 struct imx_media_pixfmt *find_format(u32 fourcc,
221 u32 code,
222 enum codespace_sel cs_sel,
223 bool allow_non_mbus,
224 bool allow_bayer)
225 {
226 const struct imx_media_pixfmt *fmt;
227 int i, j;
228
229 for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
230 fmt = &pixel_formats[i];
231
> 232 if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
233 (!allow_non_mbus && !fmt->codes[0]) ||
234 (!allow_bayer && fmt->bayer))
235 continue;
236
237 if (fourcc && fmt->fourcc == fourcc)
238 return fmt;
239
240 if (!code)
241 continue;
242
243 for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
244 if (code == fmt->codes[j])
245 return fmt;
246 }
247 }
248 return NULL;
249 }
250
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54564 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: imx: fix media bus format enumeration
2019-09-12 16:01 ` [PATCH 3/3] media: imx: fix media bus " Philipp Zabel
@ 2019-09-14 3:49 ` kbuild test robot
2019-09-27 7:33 ` Hans Verkuil
1 sibling, 0 replies; 10+ messages in thread
From: kbuild test robot @ 2019-09-14 3:49 UTC (permalink / raw)
To: Philipp Zabel; +Cc: kbuild-all, linux-media, Steve Longerbeam, kernel
[-- Attachment #1: Type: text/plain, Size: 4113 bytes --]
Hi Philipp,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linuxtv-media/master]
[cannot apply to v5.3-rc8 next-20190904]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Philipp-Zabel/media-imx-enable-V4L2_PIX_FMT_XBGR32-_BGRX32-and-_RGBX32/20190914-085415
base: git://linuxtv.org/media_tree.git master
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=ia64
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
All warnings (new ones prefixed by >>):
drivers/staging/media/imx/imx-media-utils.c: In function 'find_format':
drivers/staging/media/imx/imx-media-utils.c:232:40: warning: comparison between 'const enum ipu_color_space' and 'enum codespace_sel' [-Wenum-compare]
if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
^~
drivers/staging/media/imx/imx-media-utils.c: In function 'enum_format':
drivers/staging/media/imx/imx-media-utils.c:262:40: warning: comparison between 'const enum ipu_color_space' and 'enum codespace_sel' [-Wenum-compare]
if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
^~
>> drivers/staging/media/imx/imx-media-utils.c:257:18: warning: 'j' may be used uninitialized in this function [-Wmaybe-uninitialized]
unsigned int i, j, k = 0;
^
vim +/j +257 drivers/staging/media/imx/imx-media-utils.c
218
219 static const
220 struct imx_media_pixfmt *find_format(u32 fourcc,
221 u32 code,
222 enum codespace_sel cs_sel,
223 bool allow_non_mbus,
224 bool allow_bayer)
225 {
226 const struct imx_media_pixfmt *fmt;
227 int i, j;
228
229 for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
230 fmt = &pixel_formats[i];
231
> 232 if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
233 (!allow_non_mbus && !fmt->codes[0]) ||
234 (!allow_bayer && fmt->bayer))
235 continue;
236
237 if (fourcc && fmt->fourcc == fourcc)
238 return fmt;
239
240 if (!code)
241 continue;
242
243 for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
244 if (code == fmt->codes[j])
245 return fmt;
246 }
247 }
248 return NULL;
249 }
250
251 static int enum_format(u32 *fourcc, u32 *code, u32 index,
252 enum codespace_sel cs_sel,
253 bool allow_non_mbus,
254 bool allow_bayer)
255 {
256 const struct imx_media_pixfmt *fmt;
> 257 unsigned int i, j, k = 0;
258
259 for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
260 fmt = &pixel_formats[i];
261
262 if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
263 (!allow_non_mbus && !fmt->codes[0]) ||
264 (!allow_bayer && fmt->bayer))
265 continue;
266
267 if (fourcc && index == k)
268 break;
269
270 if (!code) {
271 k++;
272 continue;
273 }
274
275 for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
276 if (index == k)
277 goto out;
278
279 k++;
280 }
281 }
282 if (i == ARRAY_SIZE(pixel_formats))
283 return -EINVAL;
284
285 out:
286 if (fourcc)
287 *fourcc = fmt->fourcc;
288 if (code)
289 *code = fmt->codes[j];
290
291 return 0;
292 }
293
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54564 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: imx: fix media bus format enumeration
2019-09-12 16:01 ` [PATCH 3/3] media: imx: fix media bus " Philipp Zabel
2019-09-14 3:49 ` kbuild test robot
@ 2019-09-27 7:33 ` Hans Verkuil
2019-10-25 21:48 ` Steve Longerbeam
1 sibling, 1 reply; 10+ messages in thread
From: Hans Verkuil @ 2019-09-27 7:33 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Steve Longerbeam, kernel
On 9/12/19 6:01 PM, Philipp Zabel wrote:
> Iterate over all media bus formats, not just over the first format in
> each imx_media_pixfmt entry.
>
> Before:
>
> $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
> ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
> 0x2006: MEDIA_BUS_FMT_UYVY8_2X8
> 0x2008: MEDIA_BUS_FMT_YUYV8_2X8
> 0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
> 0x100a: MEDIA_BUS_FMT_RGB888_1X24
> 0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
> 0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
> 0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
> 0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
> 0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
> 0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
> 0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
> 0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
> 0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
> 0x2001: MEDIA_BUS_FMT_Y8_1X8
> 0x200a: MEDIA_BUS_FMT_Y10_1X10
>
> After:
>
> $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
> ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
> 0x2006: MEDIA_BUS_FMT_UYVY8_2X8
> 0x200f: MEDIA_BUS_FMT_UYVY8_1X16
> 0x2008: MEDIA_BUS_FMT_YUYV8_2X8
> 0x2011: MEDIA_BUS_FMT_YUYV8_1X16
> 0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
> 0x100a: MEDIA_BUS_FMT_RGB888_1X24
> 0x100c: MEDIA_BUS_FMT_RGB888_2X12_LE
> 0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
> 0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
> 0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
> 0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
> 0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
> 0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
> 0x3008: MEDIA_BUS_FMT_SBGGR12_1X12
> 0x3019: MEDIA_BUS_FMT_SBGGR14_1X14
> 0x301d: MEDIA_BUS_FMT_SBGGR16_1X16
> 0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
> 0x3010: MEDIA_BUS_FMT_SGBRG12_1X12
> 0x301a: MEDIA_BUS_FMT_SGBRG14_1X14
> 0x301e: MEDIA_BUS_FMT_SGBRG16_1X16
> 0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
> 0x3011: MEDIA_BUS_FMT_SGRBG12_1X12
> 0x301b: MEDIA_BUS_FMT_SGRBG14_1X14
> 0x301f: MEDIA_BUS_FMT_SGRBG16_1X16
> 0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
> 0x3012: MEDIA_BUS_FMT_SRGGB12_1X12
> 0x301c: MEDIA_BUS_FMT_SRGGB14_1X14
> 0x3020: MEDIA_BUS_FMT_SRGGB16_1X16
> 0x2001: MEDIA_BUS_FMT_Y8_1X8
> 0x200a: MEDIA_BUS_FMT_Y10_1X10
> 0x2013: MEDIA_BUS_FMT_Y12_1X12
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> drivers/staging/media/imx/imx-media-utils.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index d61a8f4533dc..5f8604db4dd4 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -254,7 +254,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
> bool allow_bayer)
> {
This function is becoming confusing. I think you should add some comments explaining
what the function does. Specifically the fourcc and code arguments.
Can both be non-NULL? Or only one of the two? I think that if fourcc is non-NULL you
enumerate over the V4L2 pixelformats, if code is non-NULL, then you enumerate over
the mediabus codes.
If so, then I think it would be easier to understand if you just make two functions:
enum_formats and enum_codes, rather than trying to merge them into one.
Patches 1 and 2 of this series look good, so I'll take those.
Regards,
Hans
> const struct imx_media_pixfmt *fmt;
> - unsigned int i, j = 0;
> + unsigned int i, j, k = 0;
>
> for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
> fmt = &pixel_formats[i];
> @@ -264,18 +264,29 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
> (!allow_bayer && fmt->bayer))
> continue;
>
> - if (index == j)
> + if (fourcc && index == k)
> break;
>
> - j++;
> + if (!code) {
> + k++;
> + continue;
> + }
> +
> + for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> + if (index == k)
> + goto out;
> +
> + k++;
> + }
> }
> if (i == ARRAY_SIZE(pixel_formats))
> return -EINVAL;
>
> +out:
> if (fourcc)
> *fourcc = fmt->fourcc;
> if (code)
> - *code = fmt->codes[0];
> + *code = fmt->codes[j];
>
> return 0;
> }
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] media: imx: fix and simplify pixel format enumeration
2019-09-12 16:01 ` [PATCH 2/3] media: imx: fix and simplify pixel format enumeration Philipp Zabel
2019-09-14 3:36 ` kbuild test robot
@ 2019-09-27 8:03 ` Hans Verkuil
2019-10-25 21:43 ` Steve Longerbeam
2 siblings, 0 replies; 10+ messages in thread
From: Hans Verkuil @ 2019-09-27 8:03 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: Steve Longerbeam, kernel
On 9/12/19 6:01 PM, Philipp Zabel wrote:
> Merge yuv_formats and rgb_formats into a single array. Always loop over
> all entries, skipping those that are incompatible with the requested
> enumeration. This simplifies the code, lets us get rid of the manual
> counting of array entries, and stops accidentally ignoring some non-mbus
> RGB formats.
>
> Before:
>
> $ v4l2-ctl -d /dev/video14 --list-formats-out
> ioctl: VIDIOC_ENUM_FMT
> Type: Video Output
>
> [0]: 'UYVY' (UYVY 4:2:2)
> [1]: 'YUYV' (YUYV 4:2:2)
> [2]: 'YU12' (Planar YUV 4:2:0)
> [3]: 'YV12' (Planar YVU 4:2:0)
> [4]: '422P' (Planar YUV 4:2:2)
> [5]: 'NV12' (Y/CbCr 4:2:0)
> [6]: 'NV16' (Y/CbCr 4:2:2)
> [7]: 'RGBP' (16-bit RGB 5-6-5)
> [8]: 'RGB3' (24-bit RGB 8-8-8)
> [9]: 'BX24' (32-bit XRGB 8-8-8-8)
>
> After:
>
> $ v4l2-ctl -d /dev/video14 --list-formats-out
> ioctl: VIDIOC_ENUM_FMT
> Type: Video Output
>
> [0]: 'UYVY' (UYVY 4:2:2)
> [1]: 'YUYV' (YUYV 4:2:2)
> [2]: 'YU12' (Planar YUV 4:2:0)
> [3]: 'YV12' (Planar YVU 4:2:0)
> [4]: '422P' (Planar YUV 4:2:2)
> [5]: 'NV12' (Y/CbCr 4:2:0)
> [6]: 'NV16' (Y/CbCr 4:2:2)
> [7]: 'RGBP' (16-bit RGB 5-6-5)
> [8]: 'RGB3' (24-bit RGB 8-8-8)
> [9]: 'BGR3' (24-bit BGR 8-8-8)
> [10]: 'BX24' (32-bit XRGB 8-8-8-8)
> [11]: 'XR24' (32-bit BGRX 8-8-8-8)
> [12]: 'RX24' (32-bit XBGR 8-8-8-8)
> [13]: 'XB24' (32-bit RGBX 8-8-8-8)
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> drivers/staging/media/imx/imx-media-utils.c | 170 ++++++--------------
> 1 file changed, 45 insertions(+), 125 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 0788a1874557..d61a8f4533dc 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -9,12 +9,9 @@
>
> /*
> * List of supported pixel formats for the subdevs.
> - *
> - * In all of these tables, the non-mbus formats (with no
> - * mbus codes) must all fall at the end of the table.
> */
> -
> -static const struct imx_media_pixfmt yuv_formats[] = {
> +static const struct imx_media_pixfmt pixel_formats[] = {
> + /*** YUV formats start here ***/
> {
> .fourcc = V4L2_PIX_FMT_UYVY,
> .codes = {
> @@ -31,12 +28,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
> },
> .cs = IPUV3_COLORSPACE_YUV,
> .bpp = 16,
> - },
> - /***
> - * non-mbus YUV formats start here. NOTE! when adding non-mbus
> - * formats, NUM_NON_MBUS_YUV_FORMATS must be updated below.
> - ***/
> - {
> + }, {
> .fourcc = V4L2_PIX_FMT_YUV420,
> .cs = IPUV3_COLORSPACE_YUV,
> .bpp = 12,
> @@ -62,13 +54,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
> .bpp = 16,
> .planar = true,
> },
> -};
> -
> -#define NUM_NON_MBUS_YUV_FORMATS 5
> -#define NUM_YUV_FORMATS ARRAY_SIZE(yuv_formats)
> -#define NUM_MBUS_YUV_FORMATS (NUM_YUV_FORMATS - NUM_NON_MBUS_YUV_FORMATS)
> -
> -static const struct imx_media_pixfmt rgb_formats[] = {
> + /*** RGB formats start here ***/
> {
> .fourcc = V4L2_PIX_FMT_RGB565,
> .codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
> @@ -83,12 +69,28 @@ static const struct imx_media_pixfmt rgb_formats[] = {
> },
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 24,
> + }, {
> + .fourcc = V4L2_PIX_FMT_BGR24,
> + .cs = IPUV3_COLORSPACE_RGB,
> + .bpp = 24,
> }, {
> .fourcc = V4L2_PIX_FMT_XRGB32,
> .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 32,
> .ipufmt = true,
> + }, {
> + .fourcc = V4L2_PIX_FMT_XBGR32,
> + .cs = IPUV3_COLORSPACE_RGB,
> + .bpp = 32,
> + }, {
> + .fourcc = V4L2_PIX_FMT_BGRX32,
> + .cs = IPUV3_COLORSPACE_RGB,
> + .bpp = 32,
> + }, {
> + .fourcc = V4L2_PIX_FMT_RGBX32,
> + .cs = IPUV3_COLORSPACE_RGB,
> + .bpp = 32,
> },
> /*** raw bayer and grayscale formats start here ***/
> {
> @@ -175,33 +177,8 @@ static const struct imx_media_pixfmt rgb_formats[] = {
> .bpp = 16,
> .bayer = true,
> },
> - /***
> - * non-mbus RGB formats start here. NOTE! when adding non-mbus
> - * formats, NUM_NON_MBUS_RGB_FORMATS must be updated below.
> - ***/
> - {
> - .fourcc = V4L2_PIX_FMT_BGR24,
> - .cs = IPUV3_COLORSPACE_RGB,
> - .bpp = 24,
> - }, {
> - .fourcc = V4L2_PIX_FMT_XBGR32,
> - .cs = IPUV3_COLORSPACE_RGB,
> - .bpp = 32,
> - }, {
> - .fourcc = V4L2_PIX_FMT_BGRX32,
> - .cs = IPUV3_COLORSPACE_RGB,
> - .bpp = 32,
> - }, {
> - .fourcc = V4L2_PIX_FMT_RGBX32,
> - .cs = IPUV3_COLORSPACE_RGB,
> - .bpp = 32,
> - },
> };
>
> -#define NUM_NON_MBUS_RGB_FORMATS 2
> -#define NUM_RGB_FORMATS ARRAY_SIZE(rgb_formats)
> -#define NUM_MBUS_RGB_FORMATS (NUM_RGB_FORMATS - NUM_NON_MBUS_RGB_FORMATS)
> -
> static const struct imx_media_pixfmt ipu_yuv_formats[] = {
> {
> .fourcc = V4L2_PIX_FMT_YUV32,
> @@ -240,20 +217,20 @@ static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
> }
>
> static const
> -struct imx_media_pixfmt *__find_format(u32 fourcc,
> - u32 code,
> - bool allow_non_mbus,
> - bool allow_bayer,
> - const struct imx_media_pixfmt *array,
> - u32 array_size)
> +struct imx_media_pixfmt *find_format(u32 fourcc,
> + u32 code,
> + enum codespace_sel cs_sel,
> + bool allow_non_mbus,
> + bool allow_bayer)
> {
> const struct imx_media_pixfmt *fmt;
> int i, j;
>
> - for (i = 0; i < array_size; i++) {
> - fmt = &array[i];
> + for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
> + fmt = &pixel_formats[i];
>
> - if ((!allow_non_mbus && !fmt->codes[0]) ||
> + if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
I'm getting this compiler warnings:
drivers/staging/media/imx/imx-media-utils.c: In function ‘find_format’:
drivers/staging/media/imx/imx-media-utils.c:232:40: warning: comparison between ‘const enum ipu_color_space’ and ‘enum codespace_sel’
[-Wenum-compare]
232 | if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
| ^~
> + (!allow_non_mbus && !fmt->codes[0]) ||
> (!allow_bayer && fmt->bayer))
> continue;
>
> @@ -263,7 +240,7 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
> if (!code)
> continue;
>
> - for (j = 0; fmt->codes[j]; j++) {
> + for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> if (code == fmt->codes[j])
> return fmt;
> }
> @@ -271,86 +248,29 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
> return NULL;
> }
>
> -static const struct imx_media_pixfmt *find_format(u32 fourcc,
> - u32 code,
> - enum codespace_sel cs_sel,
> - bool allow_non_mbus,
> - bool allow_bayer)
> -{
> - const struct imx_media_pixfmt *ret;
> -
> - switch (cs_sel) {
> - case CS_SEL_YUV:
> - return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> - yuv_formats, NUM_YUV_FORMATS);
> - case CS_SEL_RGB:
> - return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> - rgb_formats, NUM_RGB_FORMATS);
> - case CS_SEL_ANY:
> - ret = __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> - yuv_formats, NUM_YUV_FORMATS);
> - if (ret)
> - return ret;
> - return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> - rgb_formats, NUM_RGB_FORMATS);
> - default:
> - return NULL;
> - }
> -}
> -
> static int enum_format(u32 *fourcc, u32 *code, u32 index,
> enum codespace_sel cs_sel,
> bool allow_non_mbus,
> bool allow_bayer)
> {
> const struct imx_media_pixfmt *fmt;
> - u32 mbus_yuv_sz = NUM_MBUS_YUV_FORMATS;
> - u32 mbus_rgb_sz = NUM_MBUS_RGB_FORMATS;
> - u32 yuv_sz = NUM_YUV_FORMATS;
> - u32 rgb_sz = NUM_RGB_FORMATS;
> + unsigned int i, j = 0;
>
> - switch (cs_sel) {
> - case CS_SEL_YUV:
> - if (index >= yuv_sz ||
> - (!allow_non_mbus && index >= mbus_yuv_sz))
> - return -EINVAL;
> - fmt = &yuv_formats[index];
> - break;
> - case CS_SEL_RGB:
> - if (index >= rgb_sz ||
> - (!allow_non_mbus && index >= mbus_rgb_sz))
> - return -EINVAL;
> - fmt = &rgb_formats[index];
> - if (!allow_bayer && fmt->bayer)
> - return -EINVAL;
> - break;
> - case CS_SEL_ANY:
> - if (!allow_non_mbus) {
> - if (index >= mbus_yuv_sz) {
> - index -= mbus_yuv_sz;
> - if (index >= mbus_rgb_sz)
> - return -EINVAL;
> - fmt = &rgb_formats[index];
> - if (!allow_bayer && fmt->bayer)
> - return -EINVAL;
> - } else {
> - fmt = &yuv_formats[index];
> - }
> - } else {
> - if (index >= yuv_sz + rgb_sz)
> - return -EINVAL;
> - if (index >= yuv_sz) {
> - fmt = &rgb_formats[index - yuv_sz];
> - if (!allow_bayer && fmt->bayer)
> - return -EINVAL;
> - } else {
> - fmt = &yuv_formats[index];
> - }
> - }
> - break;
> - default:
> - return -EINVAL;
> + for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
> + fmt = &pixel_formats[i];
> +
> + if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
Same warning here.
I'm dropping this patch, I'll only merge the first patch.
Regards,
Hans
> + (!allow_non_mbus && !fmt->codes[0]) ||
> + (!allow_bayer && fmt->bayer))
> + continue;
> +
> + if (index == j)
> + break;
> +
> + j++;
> }
> + if (i == ARRAY_SIZE(pixel_formats))
> + return -EINVAL;
>
> if (fourcc)
> *fourcc = fmt->fourcc;
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/3] media: imx: fix and simplify pixel format enumeration
2019-09-12 16:01 ` [PATCH 2/3] media: imx: fix and simplify pixel format enumeration Philipp Zabel
2019-09-14 3:36 ` kbuild test robot
2019-09-27 8:03 ` Hans Verkuil
@ 2019-10-25 21:43 ` Steve Longerbeam
2 siblings, 0 replies; 10+ messages in thread
From: Steve Longerbeam @ 2019-10-25 21:43 UTC (permalink / raw)
To: Philipp Zabel, linux-media; +Cc: kernel
Hi Philipp,
Thanks for this patch and the next. I agree it simplifies the code.
Besides the compile warning caught by Hans and the kbuild test robot, I
have only one other suggestion. Can you change the name of the index j
(which becomes k in the next patch), to something like "match_index",
which makes it more clear that this is a count of the pixel_formats[]
entries that match the requested search criteria.
For testing I made this change in my github fork of the media-tree:
git@github.com:slongerbeam/mediatree.git, branch imx/philipp-pixfmts-cleanup
In that branch I also fixed the compile warnings.
Finally, I added a patch that also moves the IPU-internal formats (YUV4
and BX24) into pixel_formats[] that results in similar simplifications,
with an additional search parameter to find_format() and enum_format()
to search only for IPU-internal formats or only non-IPU-internal formats.
Let me know if you agree with those changes, and if so please include
them in next version of this series.
I tested on i.MX6Q SabreSD and all is good.
Steve
On 9/12/19 9:01 AM, Philipp Zabel wrote:
> Merge yuv_formats and rgb_formats into a single array. Always loop over
> all entries, skipping those that are incompatible with the requested
> enumeration. This simplifies the code, lets us get rid of the manual
> counting of array entries, and stops accidentally ignoring some non-mbus
> RGB formats.
>
> Before:
>
> $ v4l2-ctl -d /dev/video14 --list-formats-out
> ioctl: VIDIOC_ENUM_FMT
> Type: Video Output
>
> [0]: 'UYVY' (UYVY 4:2:2)
> [1]: 'YUYV' (YUYV 4:2:2)
> [2]: 'YU12' (Planar YUV 4:2:0)
> [3]: 'YV12' (Planar YVU 4:2:0)
> [4]: '422P' (Planar YUV 4:2:2)
> [5]: 'NV12' (Y/CbCr 4:2:0)
> [6]: 'NV16' (Y/CbCr 4:2:2)
> [7]: 'RGBP' (16-bit RGB 5-6-5)
> [8]: 'RGB3' (24-bit RGB 8-8-8)
> [9]: 'BX24' (32-bit XRGB 8-8-8-8)
>
> After:
>
> $ v4l2-ctl -d /dev/video14 --list-formats-out
> ioctl: VIDIOC_ENUM_FMT
> Type: Video Output
>
> [0]: 'UYVY' (UYVY 4:2:2)
> [1]: 'YUYV' (YUYV 4:2:2)
> [2]: 'YU12' (Planar YUV 4:2:0)
> [3]: 'YV12' (Planar YVU 4:2:0)
> [4]: '422P' (Planar YUV 4:2:2)
> [5]: 'NV12' (Y/CbCr 4:2:0)
> [6]: 'NV16' (Y/CbCr 4:2:2)
> [7]: 'RGBP' (16-bit RGB 5-6-5)
> [8]: 'RGB3' (24-bit RGB 8-8-8)
> [9]: 'BGR3' (24-bit BGR 8-8-8)
> [10]: 'BX24' (32-bit XRGB 8-8-8-8)
> [11]: 'XR24' (32-bit BGRX 8-8-8-8)
> [12]: 'RX24' (32-bit XBGR 8-8-8-8)
> [13]: 'XB24' (32-bit RGBX 8-8-8-8)
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> drivers/staging/media/imx/imx-media-utils.c | 170 ++++++--------------
> 1 file changed, 45 insertions(+), 125 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 0788a1874557..d61a8f4533dc 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -9,12 +9,9 @@
>
> /*
> * List of supported pixel formats for the subdevs.
> - *
> - * In all of these tables, the non-mbus formats (with no
> - * mbus codes) must all fall at the end of the table.
> */
> -
> -static const struct imx_media_pixfmt yuv_formats[] = {
> +static const struct imx_media_pixfmt pixel_formats[] = {
> + /*** YUV formats start here ***/
> {
> .fourcc = V4L2_PIX_FMT_UYVY,
> .codes = {
> @@ -31,12 +28,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
> },
> .cs = IPUV3_COLORSPACE_YUV,
> .bpp = 16,
> - },
> - /***
> - * non-mbus YUV formats start here. NOTE! when adding non-mbus
> - * formats, NUM_NON_MBUS_YUV_FORMATS must be updated below.
> - ***/
> - {
> + }, {
> .fourcc = V4L2_PIX_FMT_YUV420,
> .cs = IPUV3_COLORSPACE_YUV,
> .bpp = 12,
> @@ -62,13 +54,7 @@ static const struct imx_media_pixfmt yuv_formats[] = {
> .bpp = 16,
> .planar = true,
> },
> -};
> -
> -#define NUM_NON_MBUS_YUV_FORMATS 5
> -#define NUM_YUV_FORMATS ARRAY_SIZE(yuv_formats)
> -#define NUM_MBUS_YUV_FORMATS (NUM_YUV_FORMATS - NUM_NON_MBUS_YUV_FORMATS)
> -
> -static const struct imx_media_pixfmt rgb_formats[] = {
> + /*** RGB formats start here ***/
> {
> .fourcc = V4L2_PIX_FMT_RGB565,
> .codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
> @@ -83,12 +69,28 @@ static const struct imx_media_pixfmt rgb_formats[] = {
> },
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 24,
> + }, {
> + .fourcc = V4L2_PIX_FMT_BGR24,
> + .cs = IPUV3_COLORSPACE_RGB,
> + .bpp = 24,
> }, {
> .fourcc = V4L2_PIX_FMT_XRGB32,
> .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 32,
> .ipufmt = true,
> + }, {
> + .fourcc = V4L2_PIX_FMT_XBGR32,
> + .cs = IPUV3_COLORSPACE_RGB,
> + .bpp = 32,
> + }, {
> + .fourcc = V4L2_PIX_FMT_BGRX32,
> + .cs = IPUV3_COLORSPACE_RGB,
> + .bpp = 32,
> + }, {
> + .fourcc = V4L2_PIX_FMT_RGBX32,
> + .cs = IPUV3_COLORSPACE_RGB,
> + .bpp = 32,
> },
> /*** raw bayer and grayscale formats start here ***/
> {
> @@ -175,33 +177,8 @@ static const struct imx_media_pixfmt rgb_formats[] = {
> .bpp = 16,
> .bayer = true,
> },
> - /***
> - * non-mbus RGB formats start here. NOTE! when adding non-mbus
> - * formats, NUM_NON_MBUS_RGB_FORMATS must be updated below.
> - ***/
> - {
> - .fourcc = V4L2_PIX_FMT_BGR24,
> - .cs = IPUV3_COLORSPACE_RGB,
> - .bpp = 24,
> - }, {
> - .fourcc = V4L2_PIX_FMT_XBGR32,
> - .cs = IPUV3_COLORSPACE_RGB,
> - .bpp = 32,
> - }, {
> - .fourcc = V4L2_PIX_FMT_BGRX32,
> - .cs = IPUV3_COLORSPACE_RGB,
> - .bpp = 32,
> - }, {
> - .fourcc = V4L2_PIX_FMT_RGBX32,
> - .cs = IPUV3_COLORSPACE_RGB,
> - .bpp = 32,
> - },
> };
>
> -#define NUM_NON_MBUS_RGB_FORMATS 2
> -#define NUM_RGB_FORMATS ARRAY_SIZE(rgb_formats)
> -#define NUM_MBUS_RGB_FORMATS (NUM_RGB_FORMATS - NUM_NON_MBUS_RGB_FORMATS)
> -
> static const struct imx_media_pixfmt ipu_yuv_formats[] = {
> {
> .fourcc = V4L2_PIX_FMT_YUV32,
> @@ -240,20 +217,20 @@ static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
> }
>
> static const
> -struct imx_media_pixfmt *__find_format(u32 fourcc,
> - u32 code,
> - bool allow_non_mbus,
> - bool allow_bayer,
> - const struct imx_media_pixfmt *array,
> - u32 array_size)
> +struct imx_media_pixfmt *find_format(u32 fourcc,
> + u32 code,
> + enum codespace_sel cs_sel,
> + bool allow_non_mbus,
> + bool allow_bayer)
> {
> const struct imx_media_pixfmt *fmt;
> int i, j;
>
> - for (i = 0; i < array_size; i++) {
> - fmt = &array[i];
> + for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
> + fmt = &pixel_formats[i];
>
> - if ((!allow_non_mbus && !fmt->codes[0]) ||
> + if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
> + (!allow_non_mbus && !fmt->codes[0]) ||
> (!allow_bayer && fmt->bayer))
> continue;
>
> @@ -263,7 +240,7 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
> if (!code)
> continue;
>
> - for (j = 0; fmt->codes[j]; j++) {
> + for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> if (code == fmt->codes[j])
> return fmt;
> }
> @@ -271,86 +248,29 @@ struct imx_media_pixfmt *__find_format(u32 fourcc,
> return NULL;
> }
>
> -static const struct imx_media_pixfmt *find_format(u32 fourcc,
> - u32 code,
> - enum codespace_sel cs_sel,
> - bool allow_non_mbus,
> - bool allow_bayer)
> -{
> - const struct imx_media_pixfmt *ret;
> -
> - switch (cs_sel) {
> - case CS_SEL_YUV:
> - return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> - yuv_formats, NUM_YUV_FORMATS);
> - case CS_SEL_RGB:
> - return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> - rgb_formats, NUM_RGB_FORMATS);
> - case CS_SEL_ANY:
> - ret = __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> - yuv_formats, NUM_YUV_FORMATS);
> - if (ret)
> - return ret;
> - return __find_format(fourcc, code, allow_non_mbus, allow_bayer,
> - rgb_formats, NUM_RGB_FORMATS);
> - default:
> - return NULL;
> - }
> -}
> -
> static int enum_format(u32 *fourcc, u32 *code, u32 index,
> enum codespace_sel cs_sel,
> bool allow_non_mbus,
> bool allow_bayer)
> {
> const struct imx_media_pixfmt *fmt;
> - u32 mbus_yuv_sz = NUM_MBUS_YUV_FORMATS;
> - u32 mbus_rgb_sz = NUM_MBUS_RGB_FORMATS;
> - u32 yuv_sz = NUM_YUV_FORMATS;
> - u32 rgb_sz = NUM_RGB_FORMATS;
> + unsigned int i, j = 0;
>
> - switch (cs_sel) {
> - case CS_SEL_YUV:
> - if (index >= yuv_sz ||
> - (!allow_non_mbus && index >= mbus_yuv_sz))
> - return -EINVAL;
> - fmt = &yuv_formats[index];
> - break;
> - case CS_SEL_RGB:
> - if (index >= rgb_sz ||
> - (!allow_non_mbus && index >= mbus_rgb_sz))
> - return -EINVAL;
> - fmt = &rgb_formats[index];
> - if (!allow_bayer && fmt->bayer)
> - return -EINVAL;
> - break;
> - case CS_SEL_ANY:
> - if (!allow_non_mbus) {
> - if (index >= mbus_yuv_sz) {
> - index -= mbus_yuv_sz;
> - if (index >= mbus_rgb_sz)
> - return -EINVAL;
> - fmt = &rgb_formats[index];
> - if (!allow_bayer && fmt->bayer)
> - return -EINVAL;
> - } else {
> - fmt = &yuv_formats[index];
> - }
> - } else {
> - if (index >= yuv_sz + rgb_sz)
> - return -EINVAL;
> - if (index >= yuv_sz) {
> - fmt = &rgb_formats[index - yuv_sz];
> - if (!allow_bayer && fmt->bayer)
> - return -EINVAL;
> - } else {
> - fmt = &yuv_formats[index];
> - }
> - }
> - break;
> - default:
> - return -EINVAL;
> + for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
> + fmt = &pixel_formats[i];
> +
> + if ((cs_sel != CS_SEL_ANY && fmt->cs != cs_sel) ||
> + (!allow_non_mbus && !fmt->codes[0]) ||
> + (!allow_bayer && fmt->bayer))
> + continue;
> +
> + if (index == j)
> + break;
> +
> + j++;
> }
> + if (i == ARRAY_SIZE(pixel_formats))
> + return -EINVAL;
>
> if (fourcc)
> *fourcc = fmt->fourcc;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: imx: fix media bus format enumeration
2019-09-27 7:33 ` Hans Verkuil
@ 2019-10-25 21:48 ` Steve Longerbeam
2019-10-25 22:59 ` Steve Longerbeam
0 siblings, 1 reply; 10+ messages in thread
From: Steve Longerbeam @ 2019-10-25 21:48 UTC (permalink / raw)
To: Hans Verkuil, Philipp Zabel, linux-media; +Cc: kernel
Hi Hans,
On 9/27/19 12:33 AM, Hans Verkuil wrote:
> On 9/12/19 6:01 PM, Philipp Zabel wrote:
>> Iterate over all media bus formats, not just over the first format in
>> each imx_media_pixfmt entry.
>>
>> Before:
>>
>> $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
>> ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
>> 0x2006: MEDIA_BUS_FMT_UYVY8_2X8
>> 0x2008: MEDIA_BUS_FMT_YUYV8_2X8
>> 0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
>> 0x100a: MEDIA_BUS_FMT_RGB888_1X24
>> 0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
>> 0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
>> 0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
>> 0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
>> 0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
>> 0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
>> 0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
>> 0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
>> 0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
>> 0x2001: MEDIA_BUS_FMT_Y8_1X8
>> 0x200a: MEDIA_BUS_FMT_Y10_1X10
>>
>> After:
>>
>> $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
>> ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
>> 0x2006: MEDIA_BUS_FMT_UYVY8_2X8
>> 0x200f: MEDIA_BUS_FMT_UYVY8_1X16
>> 0x2008: MEDIA_BUS_FMT_YUYV8_2X8
>> 0x2011: MEDIA_BUS_FMT_YUYV8_1X16
>> 0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
>> 0x100a: MEDIA_BUS_FMT_RGB888_1X24
>> 0x100c: MEDIA_BUS_FMT_RGB888_2X12_LE
>> 0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
>> 0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
>> 0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
>> 0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
>> 0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
>> 0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
>> 0x3008: MEDIA_BUS_FMT_SBGGR12_1X12
>> 0x3019: MEDIA_BUS_FMT_SBGGR14_1X14
>> 0x301d: MEDIA_BUS_FMT_SBGGR16_1X16
>> 0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
>> 0x3010: MEDIA_BUS_FMT_SGBRG12_1X12
>> 0x301a: MEDIA_BUS_FMT_SGBRG14_1X14
>> 0x301e: MEDIA_BUS_FMT_SGBRG16_1X16
>> 0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
>> 0x3011: MEDIA_BUS_FMT_SGRBG12_1X12
>> 0x301b: MEDIA_BUS_FMT_SGRBG14_1X14
>> 0x301f: MEDIA_BUS_FMT_SGRBG16_1X16
>> 0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
>> 0x3012: MEDIA_BUS_FMT_SRGGB12_1X12
>> 0x301c: MEDIA_BUS_FMT_SRGGB14_1X14
>> 0x3020: MEDIA_BUS_FMT_SRGGB16_1X16
>> 0x2001: MEDIA_BUS_FMT_Y8_1X8
>> 0x200a: MEDIA_BUS_FMT_Y10_1X10
>> 0x2013: MEDIA_BUS_FMT_Y12_1X12
>>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> ---
>> drivers/staging/media/imx/imx-media-utils.c | 19 +++++++++++++++----
>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>> index d61a8f4533dc..5f8604db4dd4 100644
>> --- a/drivers/staging/media/imx/imx-media-utils.c
>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>> @@ -254,7 +254,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>> bool allow_bayer)
>> {
> This function is becoming confusing. I think you should add some comments explaining
> what the function does. Specifically the fourcc and code arguments.
>
> Can both be non-NULL? Or only one of the two? I think that if fourcc is non-NULL you
> enumerate over the V4L2 pixelformats, if code is non-NULL, then you enumerate over
> the mediabus codes.
>
> If so, then I think it would be easier to understand if you just make two functions:
> enum_formats and enum_codes, rather than trying to merge them into one.
I don't think the function is that confusing, but I'm fine with
splitting it into enum_formats() and enum_codes().
I do agree it needs some comments describing how it works. I think my
suggestion to rename the index that counts entries that match the search
criteria to "match_index" will also help to follow the code.
Steve
>
> Patches 1 and 2 of this series look good, so I'll take those.
>
> Regards,
>
> Hans
>
>> const struct imx_media_pixfmt *fmt;
>> - unsigned int i, j = 0;
>> + unsigned int i, j, k = 0;
>>
>> for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
>> fmt = &pixel_formats[i];
>> @@ -264,18 +264,29 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>> (!allow_bayer && fmt->bayer))
>> continue;
>>
>> - if (index == j)
>> + if (fourcc && index == k)
>> break;
>>
>> - j++;
>> + if (!code) {
>> + k++;
>> + continue;
>> + }
>> +
>> + for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>> + if (index == k)
>> + goto out;
>> +
>> + k++;
>> + }
>> }
>> if (i == ARRAY_SIZE(pixel_formats))
>> return -EINVAL;
>>
>> +out:
>> if (fourcc)
>> *fourcc = fmt->fourcc;
>> if (code)
>> - *code = fmt->codes[0];
>> + *code = fmt->codes[j];
>>
>> return 0;
>> }
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] media: imx: fix media bus format enumeration
2019-10-25 21:48 ` Steve Longerbeam
@ 2019-10-25 22:59 ` Steve Longerbeam
0 siblings, 0 replies; 10+ messages in thread
From: Steve Longerbeam @ 2019-10-25 22:59 UTC (permalink / raw)
To: Hans Verkuil, Philipp Zabel, linux-media; +Cc: kernel
Hi Hans, Philipp,
On 10/25/19 2:48 PM, Steve Longerbeam wrote:
> Hi Hans,
>
> On 9/27/19 12:33 AM, Hans Verkuil wrote:
>> On 9/12/19 6:01 PM, Philipp Zabel wrote:
>>> Iterate over all media bus formats, not just over the first format in
>>> each imx_media_pixfmt entry.
>>>
>>> Before:
>>>
>>> $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
>>> ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
>>> 0x2006: MEDIA_BUS_FMT_UYVY8_2X8
>>> 0x2008: MEDIA_BUS_FMT_YUYV8_2X8
>>> 0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
>>> 0x100a: MEDIA_BUS_FMT_RGB888_1X24
>>> 0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
>>> 0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
>>> 0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
>>> 0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
>>> 0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
>>> 0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
>>> 0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
>>> 0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
>>> 0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
>>> 0x2001: MEDIA_BUS_FMT_Y8_1X8
>>> 0x200a: MEDIA_BUS_FMT_Y10_1X10
>>>
>>> After:
>>>
>>> $ v4l2-ctl -d $(media-ctl -e ipu1_csi0) --list-subdev-mbus-codes 0
>>> ioctl: VIDIOC_SUBDEV_ENUM_MBUS_CODE (pad=0)
>>> 0x2006: MEDIA_BUS_FMT_UYVY8_2X8
>>> 0x200f: MEDIA_BUS_FMT_UYVY8_1X16
>>> 0x2008: MEDIA_BUS_FMT_YUYV8_2X8
>>> 0x2011: MEDIA_BUS_FMT_YUYV8_1X16
>>> 0x1008: MEDIA_BUS_FMT_RGB565_2X8_LE
>>> 0x100a: MEDIA_BUS_FMT_RGB888_1X24
>>> 0x100c: MEDIA_BUS_FMT_RGB888_2X12_LE
>>> 0x100d: MEDIA_BUS_FMT_ARGB8888_1X32
>>> 0x3001: MEDIA_BUS_FMT_SBGGR8_1X8
>>> 0x3013: MEDIA_BUS_FMT_SGBRG8_1X8
>>> 0x3002: MEDIA_BUS_FMT_SGRBG8_1X8
>>> 0x3014: MEDIA_BUS_FMT_SRGGB8_1X8
>>> 0x3007: MEDIA_BUS_FMT_SBGGR10_1X10
>>> 0x3008: MEDIA_BUS_FMT_SBGGR12_1X12
>>> 0x3019: MEDIA_BUS_FMT_SBGGR14_1X14
>>> 0x301d: MEDIA_BUS_FMT_SBGGR16_1X16
>>> 0x300e: MEDIA_BUS_FMT_SGBRG10_1X10
>>> 0x3010: MEDIA_BUS_FMT_SGBRG12_1X12
>>> 0x301a: MEDIA_BUS_FMT_SGBRG14_1X14
>>> 0x301e: MEDIA_BUS_FMT_SGBRG16_1X16
>>> 0x300a: MEDIA_BUS_FMT_SGRBG10_1X10
>>> 0x3011: MEDIA_BUS_FMT_SGRBG12_1X12
>>> 0x301b: MEDIA_BUS_FMT_SGRBG14_1X14
>>> 0x301f: MEDIA_BUS_FMT_SGRBG16_1X16
>>> 0x300f: MEDIA_BUS_FMT_SRGGB10_1X10
>>> 0x3012: MEDIA_BUS_FMT_SRGGB12_1X12
>>> 0x301c: MEDIA_BUS_FMT_SRGGB14_1X14
>>> 0x3020: MEDIA_BUS_FMT_SRGGB16_1X16
>>> 0x2001: MEDIA_BUS_FMT_Y8_1X8
>>> 0x200a: MEDIA_BUS_FMT_Y10_1X10
>>> 0x2013: MEDIA_BUS_FMT_Y12_1X12
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>> drivers/staging/media/imx/imx-media-utils.c | 19 +++++++++++++++----
>>> 1 file changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/imx/imx-media-utils.c
>>> b/drivers/staging/media/imx/imx-media-utils.c
>>> index d61a8f4533dc..5f8604db4dd4 100644
>>> --- a/drivers/staging/media/imx/imx-media-utils.c
>>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>>> @@ -254,7 +254,7 @@ static int enum_format(u32 *fourcc, u32 *code,
>>> u32 index,
>>> bool allow_bayer)
>>> {
>> This function is becoming confusing. I think you should add some
>> comments explaining
>> what the function does. Specifically the fourcc and code arguments.
>>
>> Can both be non-NULL? Or only one of the two? I think that if fourcc
>> is non-NULL you
>> enumerate over the V4L2 pixelformats, if code is non-NULL, then you
>> enumerate over
>> the mediabus codes.
>>
>> If so, then I think it would be easier to understand if you just make
>> two functions:
>> enum_formats and enum_codes, rather than trying to merge them into one.
>
> I don't think the function is that confusing, but I'm fine with
> splitting it into enum_formats() and enum_codes().
>
> I do agree it needs some comments describing how it works. I think my
> suggestion to rename the index that counts entries that match the
> search criteria to "match_index" will also help to follow the code.
>
I added a comment block for find_format() and enum_format() in my
media-tree github fork:
git@github.com:slongerbeam/mediatree.git, branch imx/philipp-pixfmts-cleanup
Steve
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-25 22:59 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-12 16:01 [PATCH 1/3] media: imx: enable V4L2_PIX_FMT_XBGR32, _BGRX32, and _RGBX32 Philipp Zabel
2019-09-12 16:01 ` [PATCH 2/3] media: imx: fix and simplify pixel format enumeration Philipp Zabel
2019-09-14 3:36 ` kbuild test robot
2019-09-27 8:03 ` Hans Verkuil
2019-10-25 21:43 ` Steve Longerbeam
2019-09-12 16:01 ` [PATCH 3/3] media: imx: fix media bus " Philipp Zabel
2019-09-14 3:49 ` kbuild test robot
2019-09-27 7:33 ` Hans Verkuil
2019-10-25 21:48 ` Steve Longerbeam
2019-10-25 22:59 ` Steve Longerbeam
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).