* [PATCH v4 01/11] media: imx: utils: fix and simplify pixel format enumeration
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
@ 2020-03-29 20:59 ` Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 02/11] media: imx: utils: fix media bus " Steve Longerbeam
` (9 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-29 20:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva,
Steve Longerbeam
From: Philipp Zabel <p.zabel@pengutronix.de>
Merge yuv_formats and rgb_formats into a single array. Always loop over
all entries, skipping those that do not match the requested search
criteria. 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>
[Make loop counters unsigned]
[Decrement index instead of adding a counter]
[Return directly from within loop instead of breaking]
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[Fix colorspace comparison error]
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
drivers/staging/media/imx/imx-media-utils.c | 193 ++++++--------------
1 file changed, 59 insertions(+), 134 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index fae981698c49..39469031e510 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 ***/
{
@@ -182,33 +184,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,
@@ -246,21 +223,24 @@ static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
mbus->ycbcr_enc);
}
-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)
+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 *fmt;
- int i, j;
+ unsigned int i;
- for (i = 0; i < array_size; i++) {
- fmt = &array[i];
+ for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
+ const struct imx_media_pixfmt *fmt = &pixel_formats[i];
+ enum codespace_sel fmt_cs_sel;
+ unsigned int j;
- if ((!allow_non_mbus && !fmt->codes[0]) ||
+ fmt_cs_sel = (fmt->cs == IPUV3_COLORSPACE_YUV) ?
+ CS_SEL_YUV : CS_SEL_RGB;
+
+ if ((cs_sel != CS_SEL_ANY && fmt_cs_sel != cs_sel) ||
+ (!allow_non_mbus && !fmt->codes[0]) ||
(!allow_bayer && fmt->bayer))
continue;
@@ -270,39 +250,13 @@ 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;
}
}
- 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;
- }
+ return NULL;
}
static int enum_format(u32 *fourcc, u32 *code, u32 index,
@@ -310,61 +264,32 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
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;
- 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];
- }
+ for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
+ const struct imx_media_pixfmt *fmt = &pixel_formats[i];
+ enum codespace_sel fmt_cs_sel;
+
+ fmt_cs_sel = (fmt->cs == IPUV3_COLORSPACE_YUV) ?
+ CS_SEL_YUV : CS_SEL_RGB;
+
+ if ((cs_sel != CS_SEL_ANY && fmt_cs_sel != cs_sel) ||
+ (!allow_non_mbus && !fmt->codes[0]) ||
+ (!allow_bayer && fmt->bayer))
+ continue;
+
+ if (index == 0) {
+ if (fourcc)
+ *fourcc = fmt->fourcc;
+ if (code)
+ *code = fmt->codes[0];
+ return 0;
}
- break;
- default:
- return -EINVAL;
- }
- if (fourcc)
- *fourcc = fmt->fourcc;
- if (code)
- *code = fmt->codes[0];
+ index--;
+ }
- return 0;
+ return -EINVAL;
}
const struct imx_media_pixfmt *
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 02/11] media: imx: utils: fix media bus format enumeration
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 01/11] media: imx: utils: fix and simplify pixel format enumeration Steve Longerbeam
@ 2020-03-29 20:59 ` Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 03/11] media: imx: utils: Inline init_mbus_colorimetry() in its caller Steve Longerbeam
` (8 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-29 20:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva,
Steve Longerbeam
From: Philipp Zabel <p.zabel@pengutronix.de>
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>
[Decrement index to replace loop counter k]
[Return directly from within the loops]
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/staging/media/imx/imx-media-utils.c | 22 +++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 39469031e510..00a71f01786c 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -269,6 +269,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
const struct imx_media_pixfmt *fmt = &pixel_formats[i];
enum codespace_sel fmt_cs_sel;
+ unsigned int j;
fmt_cs_sel = (fmt->cs == IPUV3_COLORSPACE_YUV) ?
CS_SEL_YUV : CS_SEL_RGB;
@@ -278,15 +279,24 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
(!allow_bayer && fmt->bayer))
continue;
- if (index == 0) {
- if (fourcc)
- *fourcc = fmt->fourcc;
- if (code)
- *code = fmt->codes[0];
+ if (fourcc && index == 0) {
+ *fourcc = fmt->fourcc;
return 0;
}
- index--;
+ if (!code) {
+ index--;
+ continue;
+ }
+
+ for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
+ if (index == 0) {
+ *code = fmt->codes[j];
+ return 0;
+ }
+
+ index--;
+ }
}
return -EINVAL;
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 03/11] media: imx: utils: Inline init_mbus_colorimetry() in its caller
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 01/11] media: imx: utils: fix and simplify pixel format enumeration Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 02/11] media: imx: utils: fix media bus " Steve Longerbeam
@ 2020-03-29 20:59 ` Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 04/11] media: imx: utils: Handle Bayer format lookup through a selection flag Steve Longerbeam
` (7 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-29 20:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva,
Steve Longerbeam
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The init_mbus_colorimetry() function is small and used in a single
place. The code becomes easier to follow if it gets inline in its
caller. Do so.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/staging/media/imx/imx-media-utils.c | 24 +++++++++------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 00a71f01786c..cf0aba8d53ba 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -210,19 +210,6 @@ static const struct imx_media_pixfmt ipu_rgb_formats[] = {
#define NUM_IPU_RGB_FORMATS ARRAY_SIZE(ipu_rgb_formats)
-static void init_mbus_colorimetry(struct v4l2_mbus_framefmt *mbus,
- const struct imx_media_pixfmt *fmt)
-{
- mbus->colorspace = (fmt->cs == IPUV3_COLORSPACE_RGB) ?
- V4L2_COLORSPACE_SRGB : V4L2_COLORSPACE_SMPTE170M;
- mbus->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(mbus->colorspace);
- mbus->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(mbus->colorspace);
- mbus->quantization =
- V4L2_MAP_QUANTIZATION_DEFAULT(fmt->cs == IPUV3_COLORSPACE_RGB,
- mbus->colorspace,
- mbus->ycbcr_enc);
-}
-
static const struct imx_media_pixfmt *find_format(u32 fourcc,
u32 code,
enum codespace_sel cs_sel,
@@ -423,7 +410,16 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
}
mbus->code = code;
- init_mbus_colorimetry(mbus, lcc);
+
+ mbus->colorspace = (lcc->cs == IPUV3_COLORSPACE_RGB) ?
+ V4L2_COLORSPACE_SRGB : V4L2_COLORSPACE_SMPTE170M;
+ mbus->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(mbus->colorspace);
+ mbus->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(mbus->colorspace);
+ mbus->quantization =
+ V4L2_MAP_QUANTIZATION_DEFAULT(lcc->cs == IPUV3_COLORSPACE_RGB,
+ mbus->colorspace,
+ mbus->ycbcr_enc);
+
if (cc)
*cc = lcc;
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 04/11] media: imx: utils: Handle Bayer format lookup through a selection flag
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
` (2 preceding siblings ...)
2020-03-29 20:59 ` [PATCH v4 03/11] media: imx: utils: Inline init_mbus_colorimetry() in its caller Steve Longerbeam
@ 2020-03-29 20:59 ` Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 05/11] media: imx: utils: Simplify IPU format lookup and enumeration Steve Longerbeam
` (6 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-29 20:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva,
Steve Longerbeam
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The format lookup (and enumeration) functions take a boolean flag to
tell if Bayer formats should be considered. This leads to hard to read
lines such as
return enum_format(fourcc, NULL, index, cs_sel, true, false);
where the boolean parameters can easily be mixed. To make the code
clearer, add a CS_SEL_BAYER flag that can be passed through the
codespace_sel parameter of the lookup functions to replace the bool
parameter.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[Instead of declaring CS_SEL_ANY as a bitfield containing only
CS_SEL_YUV | CS_SEL_RGB, declare CS_SEL_ANY as all of the above
(YUV, RGB, BAYER). A new enum is declared for the YUV | RGB selection
as CS_SEL_YUV_RGB, and that is used by sub-devices that don't support
BAYER and only allow selecting and enumerating YUV or RGB encodings.
CS_SEL_ANY is now only used by the CSI sub-devices and the attached
capture interfaces, since only those devices support BAYER formats.]
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
drivers/staging/media/imx/imx-ic-prp.c | 8 +--
drivers/staging/media/imx/imx-ic-prpencvf.c | 9 +--
drivers/staging/media/imx/imx-media-capture.c | 14 ++--
.../staging/media/imx/imx-media-csc-scaler.c | 2 +-
drivers/staging/media/imx/imx-media-csi.c | 15 ++--
drivers/staging/media/imx/imx-media-utils.c | 68 +++++++++----------
drivers/staging/media/imx/imx-media.h | 16 ++---
drivers/staging/media/imx/imx7-media-csi.c | 12 ++--
8 files changed, 70 insertions(+), 74 deletions(-)
diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
index 2a4f77e83ed3..9c1f723972e4 100644
--- a/drivers/staging/media/imx/imx-ic-prp.c
+++ b/drivers/staging/media/imx/imx-ic-prp.c
@@ -107,7 +107,7 @@ static int prp_enum_mbus_code(struct v4l2_subdev *sd,
switch (code->pad) {
case PRP_SINK_PAD:
ret = imx_media_enum_ipu_format(&code->code, code->index,
- CS_SEL_ANY);
+ CS_SEL_YUV_RGB);
break;
case PRP_SRC_PAD_PRPENC:
case PRP_SRC_PAD_PRPVF:
@@ -180,10 +180,10 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
MIN_H, MAX_H, H_ALIGN, S_ALIGN);
cc = imx_media_find_ipu_format(sdformat->format.code,
- CS_SEL_ANY);
+ CS_SEL_YUV_RGB);
if (!cc) {
- imx_media_enum_ipu_format(&code, 0, CS_SEL_ANY);
- cc = imx_media_find_ipu_format(code, CS_SEL_ANY);
+ imx_media_enum_ipu_format(&code, 0, CS_SEL_YUV);
+ cc = imx_media_find_ipu_format(code, CS_SEL_YUV);
sdformat->format.code = cc->codes[0];
}
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 09c4e3f33807..5a22cdc7378a 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -850,7 +850,8 @@ static int prp_enum_mbus_code(struct v4l2_subdev *sd,
if (code->pad >= PRPENCVF_NUM_PADS)
return -EINVAL;
- return imx_media_enum_ipu_format(&code->code, code->index, CS_SEL_ANY);
+ return imx_media_enum_ipu_format(&code->code, code->index,
+ CS_SEL_YUV_RGB);
}
static int prp_get_fmt(struct v4l2_subdev *sd,
@@ -885,12 +886,12 @@ static void prp_try_fmt(struct prp_priv *priv,
{
struct v4l2_mbus_framefmt *infmt;
- *cc = imx_media_find_ipu_format(sdformat->format.code, CS_SEL_ANY);
+ *cc = imx_media_find_ipu_format(sdformat->format.code, CS_SEL_YUV_RGB);
if (!*cc) {
u32 code;
- imx_media_enum_ipu_format(&code, 0, CS_SEL_ANY);
- *cc = imx_media_find_ipu_format(code, CS_SEL_ANY);
+ imx_media_enum_ipu_format(&code, 0, CS_SEL_YUV);
+ *cc = imx_media_find_ipu_format(code, CS_SEL_YUV);
sdformat->format.code = (*cc)->codes[0];
}
diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index d37b776ff86d..d60b49ec4fa4 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -91,7 +91,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
};
int ret;
- cc = imx_media_find_format(fsize->pixel_format, CS_SEL_ANY, true);
+ cc = imx_media_find_format(fsize->pixel_format, CS_SEL_ANY);
if (!cc)
return -EINVAL;
@@ -133,7 +133,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
};
int ret;
- cc = imx_media_find_format(fival->pixel_format, CS_SEL_ANY, true);
+ cc = imx_media_find_format(fival->pixel_format, CS_SEL_ANY);
if (!cc)
return -EINVAL;
@@ -177,7 +177,7 @@ static int capture_enum_fmt_vid_cap(struct file *file, void *fh,
return ret;
} else {
cc_src = imx_media_find_mbus_format(fmt_src.format.code,
- CS_SEL_ANY, true);
+ CS_SEL_ANY);
if (WARN_ON(!cc_src))
return -EINVAL;
@@ -217,14 +217,14 @@ static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
CS_SEL_YUV : CS_SEL_RGB;
fourcc = f->fmt.pix.pixelformat;
- cc = imx_media_find_format(fourcc, cs_sel, false);
+ cc = imx_media_find_format(fourcc, cs_sel);
if (!cc) {
imx_media_enum_format(&fourcc, 0, cs_sel);
- cc = imx_media_find_format(fourcc, cs_sel, false);
+ cc = imx_media_find_format(fourcc, cs_sel);
}
} else {
cc_src = imx_media_find_mbus_format(fmt_src->format.code,
- CS_SEL_ANY, true);
+ CS_SEL_ANY);
if (WARN_ON(!cc_src))
return -EINVAL;
@@ -790,7 +790,7 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev)
vdev->compose.width = fmt_src.format.width;
vdev->compose.height = fmt_src.format.height;
vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat,
- CS_SEL_ANY, false);
+ CS_SEL_ANY);
v4l2_info(sd, "Registered %s as /dev/%s\n", vfd->name,
video_device_node_name(vfd));
diff --git a/drivers/staging/media/imx/imx-media-csc-scaler.c b/drivers/staging/media/imx/imx-media-csc-scaler.c
index 2cc77f6e84b6..3e1c88938e7d 100644
--- a/drivers/staging/media/imx/imx-media-csc-scaler.c
+++ b/drivers/staging/media/imx/imx-media-csc-scaler.c
@@ -164,7 +164,7 @@ static int ipu_csc_scaler_enum_fmt(struct file *file, void *fh,
u32 fourcc;
int ret;
- ret = imx_media_enum_format(&fourcc, f->index, CS_SEL_ANY);
+ ret = imx_media_enum_format(&fourcc, f->index, CS_SEL_YUV_RGB);
if (ret)
return ret;
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index e76a6a85baa3..298294b95293 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1234,12 +1234,12 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,
mutex_lock(&priv->lock);
infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, code->which);
- incc = imx_media_find_mbus_format(infmt->code, CS_SEL_ANY, true);
+ incc = imx_media_find_mbus_format(infmt->code, CS_SEL_ANY);
switch (code->pad) {
case CSI_SINK_PAD:
ret = imx_media_enum_mbus_format(&code->code, code->index,
- CS_SEL_ANY, true);
+ CS_SEL_ANY);
break;
case CSI_SRC_PAD_DIRECT:
case CSI_SRC_PAD_IDMAC:
@@ -1433,8 +1433,7 @@ static void csi_try_fmt(struct csi_priv *priv,
switch (sdformat->pad) {
case CSI_SRC_PAD_DIRECT:
case CSI_SRC_PAD_IDMAC:
- incc = imx_media_find_mbus_format(infmt->code,
- CS_SEL_ANY, true);
+ incc = imx_media_find_mbus_format(infmt->code, CS_SEL_ANY);
sdformat->format.width = compose->width;
sdformat->format.height = compose->height;
@@ -1470,12 +1469,10 @@ static void csi_try_fmt(struct csi_priv *priv,
MIN_H, MAX_H, H_ALIGN, S_ALIGN);
*cc = imx_media_find_mbus_format(sdformat->format.code,
- CS_SEL_ANY, true);
+ CS_SEL_ANY);
if (!*cc) {
- imx_media_enum_mbus_format(&code, 0,
- CS_SEL_ANY, false);
- *cc = imx_media_find_mbus_format(code,
- CS_SEL_ANY, false);
+ imx_media_enum_mbus_format(&code, 0, CS_SEL_ANY);
+ *cc = imx_media_find_mbus_format(code, CS_SEL_ANY);
sdformat->format.code = (*cc)->codes[0];
}
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index cf0aba8d53ba..6a3b0b737e5f 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -213,8 +213,7 @@ static const struct imx_media_pixfmt ipu_rgb_formats[] = {
static const struct imx_media_pixfmt *find_format(u32 fourcc,
u32 code,
enum codespace_sel cs_sel,
- bool allow_non_mbus,
- bool allow_bayer)
+ bool allow_non_mbus)
{
unsigned int i;
@@ -223,12 +222,12 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
enum codespace_sel fmt_cs_sel;
unsigned int j;
- fmt_cs_sel = (fmt->cs == IPUV3_COLORSPACE_YUV) ?
- CS_SEL_YUV : CS_SEL_RGB;
+ fmt_cs_sel = fmt->bayer ? CS_SEL_BAYER :
+ ((fmt->cs == IPUV3_COLORSPACE_YUV) ?
+ CS_SEL_YUV : CS_SEL_RGB);
- if ((cs_sel != CS_SEL_ANY && fmt_cs_sel != cs_sel) ||
- (!allow_non_mbus && !fmt->codes[0]) ||
- (!allow_bayer && fmt->bayer))
+ if (!(fmt_cs_sel & cs_sel) ||
+ (!allow_non_mbus && !fmt->codes[0]))
continue;
if (fourcc && fmt->fourcc == fourcc)
@@ -248,8 +247,7 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
static int enum_format(u32 *fourcc, u32 *code, u32 index,
enum codespace_sel cs_sel,
- bool allow_non_mbus,
- bool allow_bayer)
+ bool allow_non_mbus)
{
unsigned int i;
@@ -258,12 +256,12 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
enum codespace_sel fmt_cs_sel;
unsigned int j;
- fmt_cs_sel = (fmt->cs == IPUV3_COLORSPACE_YUV) ?
- CS_SEL_YUV : CS_SEL_RGB;
+ fmt_cs_sel = fmt->bayer ? CS_SEL_BAYER :
+ ((fmt->cs == IPUV3_COLORSPACE_YUV) ?
+ CS_SEL_YUV : CS_SEL_RGB);
- if ((cs_sel != CS_SEL_ANY && fmt_cs_sel != cs_sel) ||
- (!allow_non_mbus && !fmt->codes[0]) ||
- (!allow_bayer && fmt->bayer))
+ if (!(fmt_cs_sel & cs_sel) ||
+ (!allow_non_mbus && !fmt->codes[0]))
continue;
if (fourcc && index == 0) {
@@ -290,30 +288,28 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
}
const struct imx_media_pixfmt *
-imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel, bool allow_bayer)
+imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel)
{
- return find_format(fourcc, 0, cs_sel, true, allow_bayer);
+ return find_format(fourcc, 0, cs_sel, true);
}
EXPORT_SYMBOL_GPL(imx_media_find_format);
int imx_media_enum_format(u32 *fourcc, u32 index, enum codespace_sel cs_sel)
{
- return enum_format(fourcc, NULL, index, cs_sel, true, false);
+ return enum_format(fourcc, NULL, index, cs_sel, true);
}
EXPORT_SYMBOL_GPL(imx_media_enum_format);
const struct imx_media_pixfmt *
-imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel,
- bool allow_bayer)
+imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel)
{
- return find_format(0, code, cs_sel, false, allow_bayer);
+ return find_format(0, code, cs_sel, false);
}
EXPORT_SYMBOL_GPL(imx_media_find_mbus_format);
-int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel,
- bool allow_bayer)
+int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel)
{
- return enum_format(NULL, code, index, cs_sel, false, allow_bayer);
+ return enum_format(NULL, code, index, cs_sel, false);
}
EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
@@ -324,6 +320,8 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
u32 array_size;
int i, j;
+ cs_sel &= ~CS_SEL_BAYER;
+
switch (cs_sel) {
case CS_SEL_YUV:
array_size = NUM_IPU_YUV_FORMATS;
@@ -333,7 +331,7 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
array_size = NUM_IPU_RGB_FORMATS;
array = ipu_rgb_formats;
break;
- case CS_SEL_ANY:
+ case CS_SEL_YUV_RGB:
array_size = NUM_IPU_YUV_FORMATS + NUM_IPU_RGB_FORMATS;
array = ipu_yuv_formats;
break;
@@ -342,7 +340,7 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
}
for (i = 0; i < array_size; i++) {
- if (cs_sel == CS_SEL_ANY && i >= NUM_IPU_YUV_FORMATS)
+ if (cs_sel == CS_SEL_YUV_RGB && i >= NUM_IPU_YUV_FORMATS)
fmt = &ipu_rgb_formats[i - NUM_IPU_YUV_FORMATS];
else
fmt = &array[i];
@@ -362,6 +360,8 @@ EXPORT_SYMBOL_GPL(imx_media_find_ipu_format);
int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
{
+ cs_sel &= ~CS_SEL_BAYER;
+
switch (cs_sel) {
case CS_SEL_YUV:
if (index >= NUM_IPU_YUV_FORMATS)
@@ -373,7 +373,7 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
return -EINVAL;
*code = ipu_rgb_formats[index].codes[0];
break;
- case CS_SEL_ANY:
+ case CS_SEL_YUV_RGB:
if (index >= NUM_IPU_YUV_FORMATS + NUM_IPU_RGB_FORMATS)
return -EINVAL;
if (index >= NUM_IPU_YUV_FORMATS) {
@@ -401,8 +401,8 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
mbus->height = height;
mbus->field = field;
if (code == 0)
- imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV, false);
- lcc = imx_media_find_mbus_format(code, CS_SEL_ANY, false);
+ imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV);
+ lcc = imx_media_find_mbus_format(code, CS_SEL_ANY);
if (!lcc) {
lcc = imx_media_find_ipu_format(code, CS_SEL_ANY);
if (!lcc)
@@ -473,7 +473,7 @@ void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
const struct imx_media_pixfmt *cc;
bool is_rgb = false;
- cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY, true);
+ cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY);
if (!cc)
cc = imx_media_find_ipu_format(tryfmt->code, CS_SEL_ANY);
if (cc && cc->cs == IPUV3_COLORSPACE_RGB)
@@ -527,8 +527,8 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
if (!cc) {
cc = imx_media_find_ipu_format(mbus->code, CS_SEL_ANY);
if (!cc)
- cc = imx_media_find_mbus_format(mbus->code, CS_SEL_ANY,
- true);
+ cc = imx_media_find_mbus_format(mbus->code,
+ CS_SEL_ANY);
if (!cc)
return -EINVAL;
}
@@ -540,8 +540,8 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
if (cc->ipufmt && cc->cs == IPUV3_COLORSPACE_YUV) {
u32 code;
- imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV, false);
- cc = imx_media_find_mbus_format(code, CS_SEL_YUV, false);
+ imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV);
+ cc = imx_media_find_mbus_format(code, CS_SEL_YUV);
}
/* Round up width for minimum burst size */
@@ -592,7 +592,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
{
const struct imx_media_pixfmt *fmt;
- fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY, true);
+ fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
if (!fmt)
return -EINVAL;
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index 11861191324a..652673a703cd 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -150,20 +150,20 @@ struct imx_media_dev {
};
enum codespace_sel {
- CS_SEL_YUV = 0,
- CS_SEL_RGB,
- CS_SEL_ANY,
+ CS_SEL_YUV = BIT(0),
+ CS_SEL_RGB = BIT(1),
+ CS_SEL_BAYER = BIT(2),
+ CS_SEL_YUV_RGB = CS_SEL_YUV | CS_SEL_RGB,
+ CS_SEL_ANY = CS_SEL_YUV | CS_SEL_RGB | CS_SEL_BAYER,
};
/* imx-media-utils.c */
const struct imx_media_pixfmt *
-imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel, bool allow_bayer);
+imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel);
int imx_media_enum_format(u32 *fourcc, u32 index, enum codespace_sel cs_sel);
const struct imx_media_pixfmt *
-imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel,
- bool allow_bayer);
-int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel,
- bool allow_bayer);
+imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel);
+int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel);
const struct imx_media_pixfmt *
imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel);
int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel);
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index acbdffb77668..a469dc76a787 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -959,7 +959,7 @@ static int imx7_csi_enum_mbus_code(struct v4l2_subdev *sd,
switch (code->pad) {
case IMX7_CSI_PAD_SINK:
ret = imx_media_enum_mbus_format(&code->code, code->index,
- CS_SEL_ANY, true);
+ CS_SEL_ANY);
break;
case IMX7_CSI_PAD_SRC:
if (code->index != 0) {
@@ -1019,8 +1019,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
switch (sdformat->pad) {
case IMX7_CSI_PAD_SRC:
- in_cc = imx_media_find_mbus_format(in_fmt->code, CS_SEL_ANY,
- true);
+ in_cc = imx_media_find_mbus_format(in_fmt->code, CS_SEL_ANY);
sdformat->format.width = in_fmt->width;
sdformat->format.height = in_fmt->height;
@@ -1035,11 +1034,10 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
break;
case IMX7_CSI_PAD_SINK:
*cc = imx_media_find_mbus_format(sdformat->format.code,
- CS_SEL_ANY, true);
+ CS_SEL_ANY);
if (!*cc) {
- imx_media_enum_mbus_format(&code, 0, CS_SEL_ANY, false);
- *cc = imx_media_find_mbus_format(code, CS_SEL_ANY,
- false);
+ imx_media_enum_mbus_format(&code, 0, CS_SEL_ANY);
+ *cc = imx_media_find_mbus_format(code, CS_SEL_ANY);
sdformat->format.code = (*cc)->codes[0];
}
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 05/11] media: imx: utils: Simplify IPU format lookup and enumeration
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
` (3 preceding siblings ...)
2020-03-29 20:59 ` [PATCH v4 04/11] media: imx: utils: Handle Bayer format lookup through a selection flag Steve Longerbeam
@ 2020-03-29 20:59 ` Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes Steve Longerbeam
` (5 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-29 20:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva,
Steve Longerbeam
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The IPU formats are stored in two separate tables, one for YUV and one
for RGB formats. This complicates the lookup and enumeration function
without really increasing efficiency, as both tables contain a single
element. Merge the two tables and simplify the functions, and move the
resulting table next to the functions that use it.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/staging/media/imx/imx-media-utils.c | 134 ++++++++------------
1 file changed, 54 insertions(+), 80 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 6a3b0b737e5f..981a8b540a3c 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -186,30 +186,6 @@ static const struct imx_media_pixfmt pixel_formats[] = {
},
};
-static const struct imx_media_pixfmt ipu_yuv_formats[] = {
- {
- .fourcc = V4L2_PIX_FMT_YUV32,
- .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
- .cs = IPUV3_COLORSPACE_YUV,
- .bpp = 32,
- .ipufmt = true,
- },
-};
-
-#define NUM_IPU_YUV_FORMATS ARRAY_SIZE(ipu_yuv_formats)
-
-static const struct imx_media_pixfmt ipu_rgb_formats[] = {
- {
- .fourcc = V4L2_PIX_FMT_XRGB32,
- .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
- .cs = IPUV3_COLORSPACE_RGB,
- .bpp = 32,
- .ipufmt = true,
- },
-};
-
-#define NUM_IPU_RGB_FORMATS ARRAY_SIZE(ipu_rgb_formats)
-
static const struct imx_media_pixfmt *find_format(u32 fourcc,
u32 code,
enum codespace_sel cs_sel,
@@ -313,81 +289,79 @@ int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel)
}
EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
+/* -----------------------------------------------------------------------------
+ * IPU Formats Lookup and Enumeration
+ */
+
+static const struct imx_media_pixfmt ipu_formats[] = {
+ {
+ .fourcc = V4L2_PIX_FMT_YUV32,
+ .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
+ .cs = IPUV3_COLORSPACE_YUV,
+ .bpp = 32,
+ .ipufmt = true,
+ }, {
+ .fourcc = V4L2_PIX_FMT_XRGB32,
+ .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
+ .cs = IPUV3_COLORSPACE_RGB,
+ .bpp = 32,
+ .ipufmt = true,
+ },
+};
+
const struct imx_media_pixfmt *
imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
{
- const struct imx_media_pixfmt *array, *fmt, *ret = NULL;
- u32 array_size;
- int i, j;
-
- cs_sel &= ~CS_SEL_BAYER;
+ bool accept_yuv = cs_sel & CS_SEL_YUV;
+ bool accept_rgb = cs_sel & CS_SEL_RGB;
+ unsigned int i;
- switch (cs_sel) {
- case CS_SEL_YUV:
- array_size = NUM_IPU_YUV_FORMATS;
- array = ipu_yuv_formats;
- break;
- case CS_SEL_RGB:
- array_size = NUM_IPU_RGB_FORMATS;
- array = ipu_rgb_formats;
- break;
- case CS_SEL_YUV_RGB:
- array_size = NUM_IPU_YUV_FORMATS + NUM_IPU_RGB_FORMATS;
- array = ipu_yuv_formats;
- break;
- default:
+ if (!code)
return NULL;
- }
- for (i = 0; i < array_size; i++) {
- if (cs_sel == CS_SEL_YUV_RGB && i >= NUM_IPU_YUV_FORMATS)
- fmt = &ipu_rgb_formats[i - NUM_IPU_YUV_FORMATS];
- else
- fmt = &array[i];
+ for (i = 0; i < ARRAY_SIZE(ipu_formats); i++) {
+ const struct imx_media_pixfmt *fmt = &ipu_formats[i];
+ unsigned int j;
- for (j = 0; code && fmt->codes[j]; j++) {
- if (code == fmt->codes[j]) {
- ret = fmt;
- goto out;
- }
+ if ((!accept_yuv && fmt->cs == IPUV3_COLORSPACE_YUV) ||
+ (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
+ continue;
+
+ for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
+ if (code == fmt->codes[j])
+ return fmt;
}
}
-out:
- return ret;
+ return NULL;
}
EXPORT_SYMBOL_GPL(imx_media_find_ipu_format);
int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
{
- cs_sel &= ~CS_SEL_BAYER;
+ bool accept_yuv = cs_sel & CS_SEL_YUV;
+ bool accept_rgb = cs_sel & CS_SEL_RGB;
+ unsigned int i;
- switch (cs_sel) {
- case CS_SEL_YUV:
- if (index >= NUM_IPU_YUV_FORMATS)
- return -EINVAL;
- *code = ipu_yuv_formats[index].codes[0];
- break;
- case CS_SEL_RGB:
- if (index >= NUM_IPU_RGB_FORMATS)
- return -EINVAL;
- *code = ipu_rgb_formats[index].codes[0];
- break;
- case CS_SEL_YUV_RGB:
- if (index >= NUM_IPU_YUV_FORMATS + NUM_IPU_RGB_FORMATS)
- return -EINVAL;
- if (index >= NUM_IPU_YUV_FORMATS) {
- index -= NUM_IPU_YUV_FORMATS;
- *code = ipu_rgb_formats[index].codes[0];
- } else {
- *code = ipu_yuv_formats[index].codes[0];
+ for (i = 0; i < ARRAY_SIZE(ipu_formats); i++) {
+ const struct imx_media_pixfmt *fmt = &ipu_formats[i];
+ unsigned int j;
+
+ if ((!accept_yuv && fmt->cs == IPUV3_COLORSPACE_YUV) ||
+ (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
+ continue;
+
+ for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
+ if (index == 0) {
+ *code = fmt->codes[j];
+ return 0;
+ }
+
+ index--;
}
- break;
- default:
- return -EINVAL;
}
- return 0;
+ return -EINVAL;
}
EXPORT_SYMBOL_GPL(imx_media_enum_ipu_format);
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
` (4 preceding siblings ...)
2020-03-29 20:59 ` [PATCH v4 05/11] media: imx: utils: Simplify IPU format lookup and enumeration Steve Longerbeam
@ 2020-03-29 20:59 ` Steve Longerbeam
2020-03-31 13:45 ` Hans Verkuil
2020-03-29 20:59 ` [PATCH v4 07/11] media: imx: utils: Remove unneeded argument to (find|enum)_format() Steve Longerbeam
` (4 subsequent siblings)
10 siblings, 1 reply; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-29 20:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva,
Steve Longerbeam
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The imx_media_pixfmt structures includes a codes member that stores
media bus codes as a fixed array of 4 integers. The functions dealing
with the imx_media_pixfmt structures assume that the array of codes is
terminated by a 0 elements. This mechanism is fragile, as demonstrated
by several instances of the structure contained 4 non-zero codes.
Fix this by turning the array into a pointer, and providing an
IMX_BUS_FMTS macro to initialize the codes member with a guaranteed 0
element at the end.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[Fix a NULL deref when derefencing a NULL cc->codes on return from
several calls to imx_media_find_format()]
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
drivers/staging/media/imx/imx-media-capture.c | 4 +-
drivers/staging/media/imx/imx-media-utils.c | 88 +++++++++++--------
drivers/staging/media/imx/imx-media.h | 2 +-
3 files changed, 53 insertions(+), 41 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index d60b49ec4fa4..650c53289f6b 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -95,7 +95,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
if (!cc)
return -EINVAL;
- fse.code = cc->codes[0];
+ fse.code = cc->codes ? cc->codes[0] : 0;
ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
if (ret)
@@ -137,7 +137,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
if (!cc)
return -EINVAL;
- fie.code = cc->codes[0];
+ fie.code = cc->codes ? cc->codes[0] : 0;
ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval,
NULL, &fie);
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 981a8b540a3c..da010eef0ae6 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -7,6 +7,12 @@
#include <linux/module.h>
#include "imx-media.h"
+#define IMX_BUS_FMTS(fmts...) \
+ (const u32[]) { \
+ fmts, \
+ 0 \
+ }
+
/*
* List of supported pixel formats for the subdevs.
*/
@@ -14,18 +20,18 @@ static const struct imx_media_pixfmt pixel_formats[] = {
/*** YUV formats start here ***/
{
.fourcc = V4L2_PIX_FMT_UYVY,
- .codes = {
+ .codes = IMX_BUS_FMTS(
MEDIA_BUS_FMT_UYVY8_2X8,
MEDIA_BUS_FMT_UYVY8_1X16
- },
+ ),
.cs = IPUV3_COLORSPACE_YUV,
.bpp = 16,
}, {
.fourcc = V4L2_PIX_FMT_YUYV,
- .codes = {
+ .codes = IMX_BUS_FMTS(
MEDIA_BUS_FMT_YUYV8_2X8,
MEDIA_BUS_FMT_YUYV8_1X16
- },
+ ),
.cs = IPUV3_COLORSPACE_YUV,
.bpp = 16,
}, {
@@ -57,16 +63,16 @@ static const struct imx_media_pixfmt pixel_formats[] = {
/*** RGB formats start here ***/
{
.fourcc = V4L2_PIX_FMT_RGB565,
- .codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
+ .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 16,
.cycles = 2,
}, {
.fourcc = V4L2_PIX_FMT_RGB24,
- .codes = {
+ .codes = IMX_BUS_FMTS(
MEDIA_BUS_FMT_RGB888_1X24,
MEDIA_BUS_FMT_RGB888_2X12_LE
- },
+ ),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 24,
}, {
@@ -75,7 +81,7 @@ static const struct imx_media_pixfmt pixel_formats[] = {
.bpp = 24,
}, {
.fourcc = V4L2_PIX_FMT_XRGB32,
- .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
+ .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 32,
.ipufmt = true,
@@ -95,91 +101,91 @@ static const struct imx_media_pixfmt pixel_formats[] = {
/*** raw bayer and grayscale formats start here ***/
{
.fourcc = V4L2_PIX_FMT_SBGGR8,
- .codes = {MEDIA_BUS_FMT_SBGGR8_1X8},
+ .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 8,
.bayer = true,
}, {
.fourcc = V4L2_PIX_FMT_SGBRG8,
- .codes = {MEDIA_BUS_FMT_SGBRG8_1X8},
+ .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 8,
.bayer = true,
}, {
.fourcc = V4L2_PIX_FMT_SGRBG8,
- .codes = {MEDIA_BUS_FMT_SGRBG8_1X8},
+ .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 8,
.bayer = true,
}, {
.fourcc = V4L2_PIX_FMT_SRGGB8,
- .codes = {MEDIA_BUS_FMT_SRGGB8_1X8},
+ .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 8,
.bayer = true,
}, {
.fourcc = V4L2_PIX_FMT_SBGGR16,
- .codes = {
+ .codes = IMX_BUS_FMTS(
MEDIA_BUS_FMT_SBGGR10_1X10,
MEDIA_BUS_FMT_SBGGR12_1X12,
MEDIA_BUS_FMT_SBGGR14_1X14,
MEDIA_BUS_FMT_SBGGR16_1X16
- },
+ ),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 16,
.bayer = true,
}, {
.fourcc = V4L2_PIX_FMT_SGBRG16,
- .codes = {
+ .codes = IMX_BUS_FMTS(
MEDIA_BUS_FMT_SGBRG10_1X10,
MEDIA_BUS_FMT_SGBRG12_1X12,
MEDIA_BUS_FMT_SGBRG14_1X14,
- MEDIA_BUS_FMT_SGBRG16_1X16,
- },
+ MEDIA_BUS_FMT_SGBRG16_1X16
+ ),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 16,
.bayer = true,
}, {
.fourcc = V4L2_PIX_FMT_SGRBG16,
- .codes = {
+ .codes = IMX_BUS_FMTS(
MEDIA_BUS_FMT_SGRBG10_1X10,
MEDIA_BUS_FMT_SGRBG12_1X12,
MEDIA_BUS_FMT_SGRBG14_1X14,
- MEDIA_BUS_FMT_SGRBG16_1X16,
- },
+ MEDIA_BUS_FMT_SGRBG16_1X16
+ ),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 16,
.bayer = true,
}, {
.fourcc = V4L2_PIX_FMT_SRGGB16,
- .codes = {
+ .codes = IMX_BUS_FMTS(
MEDIA_BUS_FMT_SRGGB10_1X10,
MEDIA_BUS_FMT_SRGGB12_1X12,
MEDIA_BUS_FMT_SRGGB14_1X14,
- MEDIA_BUS_FMT_SRGGB16_1X16,
- },
+ MEDIA_BUS_FMT_SRGGB16_1X16
+ ),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 16,
.bayer = true,
}, {
.fourcc = V4L2_PIX_FMT_GREY,
- .codes = {
+ .codes = IMX_BUS_FMTS(
MEDIA_BUS_FMT_Y8_1X8,
MEDIA_BUS_FMT_Y10_1X10,
- MEDIA_BUS_FMT_Y12_1X12,
- },
+ MEDIA_BUS_FMT_Y12_1X12
+ ),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 8,
.bayer = true,
}, {
.fourcc = V4L2_PIX_FMT_Y10,
- .codes = {MEDIA_BUS_FMT_Y10_1X10},
+ .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y10_1X10),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 16,
.bayer = true,
}, {
.fourcc = V4L2_PIX_FMT_Y12,
- .codes = {MEDIA_BUS_FMT_Y12_1X12},
+ .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y12_1X12),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 16,
.bayer = true,
@@ -203,16 +209,16 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
CS_SEL_YUV : CS_SEL_RGB);
if (!(fmt_cs_sel & cs_sel) ||
- (!allow_non_mbus && !fmt->codes[0]))
+ (!allow_non_mbus && !fmt->codes))
continue;
if (fourcc && fmt->fourcc == fourcc)
return fmt;
- if (!code)
+ if (!code || !fmt->codes)
continue;
- for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
+ for (j = 0; fmt->codes[j]; j++) {
if (code == fmt->codes[j])
return fmt;
}
@@ -237,7 +243,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
CS_SEL_YUV : CS_SEL_RGB);
if (!(fmt_cs_sel & cs_sel) ||
- (!allow_non_mbus && !fmt->codes[0]))
+ (!allow_non_mbus && !fmt->codes))
continue;
if (fourcc && index == 0) {
@@ -250,7 +256,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
continue;
}
- for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
+ for (j = 0; fmt->codes[j]; j++) {
if (index == 0) {
*code = fmt->codes[j];
return 0;
@@ -296,13 +302,13 @@ EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
static const struct imx_media_pixfmt ipu_formats[] = {
{
.fourcc = V4L2_PIX_FMT_YUV32,
- .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
+ .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_AYUV8_1X32),
.cs = IPUV3_COLORSPACE_YUV,
.bpp = 32,
.ipufmt = true,
}, {
.fourcc = V4L2_PIX_FMT_XRGB32,
- .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
+ .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
.cs = IPUV3_COLORSPACE_RGB,
.bpp = 32,
.ipufmt = true,
@@ -327,7 +333,10 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
(!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
continue;
- for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
+ if (!fmt->codes)
+ continue;
+
+ for (j = 0; fmt->codes[j]; j++) {
if (code == fmt->codes[j])
return fmt;
}
@@ -351,7 +360,10 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
(!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
continue;
- for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
+ if (!fmt->codes)
+ continue;
+
+ for (j = 0; fmt->codes[j]; j++) {
if (index == 0) {
*code = fmt->codes[j];
return 0;
@@ -567,7 +579,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
const struct imx_media_pixfmt *fmt;
fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
- if (!fmt)
+ if (!fmt || !fmt->codes)
return -EINVAL;
memset(mbus, 0, sizeof(*mbus));
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index 652673a703cd..917b4db02985 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -69,7 +69,7 @@ enum {
struct imx_media_pixfmt {
u32 fourcc;
- u32 codes[4];
+ const u32 *codes;
int bpp; /* total bpp */
/* cycles per pixel for generic (bayer) formats for the parallel bus */
int cycles;
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes
2020-03-29 20:59 ` [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes Steve Longerbeam
@ 2020-03-31 13:45 ` Hans Verkuil
2020-03-31 23:04 ` Steve Longerbeam
0 siblings, 1 reply; 20+ messages in thread
From: Hans Verkuil @ 2020-03-31 13:45 UTC (permalink / raw)
To: Steve Longerbeam, linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva
On 3/29/20 10:59 PM, Steve Longerbeam wrote:
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> The imx_media_pixfmt structures includes a codes member that stores
> media bus codes as a fixed array of 4 integers. The functions dealing
> with the imx_media_pixfmt structures assume that the array of codes is
> terminated by a 0 elements. This mechanism is fragile, as demonstrated
elements -> element
> by several instances of the structure contained 4 non-zero codes.
contained -> that contained
>
> Fix this by turning the array into a pointer, and providing an
> IMX_BUS_FMTS macro to initialize the codes member with a guaranteed 0
> element at the end.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> [Fix a NULL deref when derefencing a NULL cc->codes on return from
> several calls to imx_media_find_format()]
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
> drivers/staging/media/imx/imx-media-capture.c | 4 +-
> drivers/staging/media/imx/imx-media-utils.c | 88 +++++++++++--------
> drivers/staging/media/imx/imx-media.h | 2 +-
> 3 files changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index d60b49ec4fa4..650c53289f6b 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -95,7 +95,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
> if (!cc)
> return -EINVAL;
>
> - fse.code = cc->codes[0];
> + fse.code = cc->codes ? cc->codes[0] : 0;
I'm wondering: wouldn't it be better to have a format without codes point to
an empty code list containing just 0? That would avoid all the cc->codes checks,
which are a bit fragile IMHO.
It would be nice in that case if there is a WARN_ON or something during probe
time that checks that the codes field in pixel_formats[] is never NULL.
As an aside: 'cc'? The struct is called imx_media_pixfmt, so I don't offhand see
what 'cc' stands for.
Regards,
Hans
>
> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
> if (ret)
> @@ -137,7 +137,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
> if (!cc)
> return -EINVAL;
>
> - fie.code = cc->codes[0];
> + fie.code = cc->codes ? cc->codes[0] : 0;
>
> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval,
> NULL, &fie);
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 981a8b540a3c..da010eef0ae6 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -7,6 +7,12 @@
> #include <linux/module.h>
> #include "imx-media.h"
>
> +#define IMX_BUS_FMTS(fmts...) \
> + (const u32[]) { \
> + fmts, \
> + 0 \
> + }
> +
> /*
> * List of supported pixel formats for the subdevs.
> */
> @@ -14,18 +20,18 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> /*** YUV formats start here ***/
> {
> .fourcc = V4L2_PIX_FMT_UYVY,
> - .codes = {
> + .codes = IMX_BUS_FMTS(
> MEDIA_BUS_FMT_UYVY8_2X8,
> MEDIA_BUS_FMT_UYVY8_1X16
> - },
> + ),
> .cs = IPUV3_COLORSPACE_YUV,
> .bpp = 16,
> }, {
> .fourcc = V4L2_PIX_FMT_YUYV,
> - .codes = {
> + .codes = IMX_BUS_FMTS(
> MEDIA_BUS_FMT_YUYV8_2X8,
> MEDIA_BUS_FMT_YUYV8_1X16
> - },
> + ),
> .cs = IPUV3_COLORSPACE_YUV,
> .bpp = 16,
> }, {
> @@ -57,16 +63,16 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> /*** RGB formats start here ***/
> {
> .fourcc = V4L2_PIX_FMT_RGB565,
> - .codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 16,
> .cycles = 2,
> }, {
> .fourcc = V4L2_PIX_FMT_RGB24,
> - .codes = {
> + .codes = IMX_BUS_FMTS(
> MEDIA_BUS_FMT_RGB888_1X24,
> MEDIA_BUS_FMT_RGB888_2X12_LE
> - },
> + ),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 24,
> }, {
> @@ -75,7 +81,7 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> .bpp = 24,
> }, {
> .fourcc = V4L2_PIX_FMT_XRGB32,
> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 32,
> .ipufmt = true,
> @@ -95,91 +101,91 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> /*** raw bayer and grayscale formats start here ***/
> {
> .fourcc = V4L2_PIX_FMT_SBGGR8,
> - .codes = {MEDIA_BUS_FMT_SBGGR8_1X8},
> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 8,
> .bayer = true,
> }, {
> .fourcc = V4L2_PIX_FMT_SGBRG8,
> - .codes = {MEDIA_BUS_FMT_SGBRG8_1X8},
> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 8,
> .bayer = true,
> }, {
> .fourcc = V4L2_PIX_FMT_SGRBG8,
> - .codes = {MEDIA_BUS_FMT_SGRBG8_1X8},
> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 8,
> .bayer = true,
> }, {
> .fourcc = V4L2_PIX_FMT_SRGGB8,
> - .codes = {MEDIA_BUS_FMT_SRGGB8_1X8},
> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 8,
> .bayer = true,
> }, {
> .fourcc = V4L2_PIX_FMT_SBGGR16,
> - .codes = {
> + .codes = IMX_BUS_FMTS(
> MEDIA_BUS_FMT_SBGGR10_1X10,
> MEDIA_BUS_FMT_SBGGR12_1X12,
> MEDIA_BUS_FMT_SBGGR14_1X14,
> MEDIA_BUS_FMT_SBGGR16_1X16
> - },
> + ),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 16,
> .bayer = true,
> }, {
> .fourcc = V4L2_PIX_FMT_SGBRG16,
> - .codes = {
> + .codes = IMX_BUS_FMTS(
> MEDIA_BUS_FMT_SGBRG10_1X10,
> MEDIA_BUS_FMT_SGBRG12_1X12,
> MEDIA_BUS_FMT_SGBRG14_1X14,
> - MEDIA_BUS_FMT_SGBRG16_1X16,
> - },
> + MEDIA_BUS_FMT_SGBRG16_1X16
> + ),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 16,
> .bayer = true,
> }, {
> .fourcc = V4L2_PIX_FMT_SGRBG16,
> - .codes = {
> + .codes = IMX_BUS_FMTS(
> MEDIA_BUS_FMT_SGRBG10_1X10,
> MEDIA_BUS_FMT_SGRBG12_1X12,
> MEDIA_BUS_FMT_SGRBG14_1X14,
> - MEDIA_BUS_FMT_SGRBG16_1X16,
> - },
> + MEDIA_BUS_FMT_SGRBG16_1X16
> + ),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 16,
> .bayer = true,
> }, {
> .fourcc = V4L2_PIX_FMT_SRGGB16,
> - .codes = {
> + .codes = IMX_BUS_FMTS(
> MEDIA_BUS_FMT_SRGGB10_1X10,
> MEDIA_BUS_FMT_SRGGB12_1X12,
> MEDIA_BUS_FMT_SRGGB14_1X14,
> - MEDIA_BUS_FMT_SRGGB16_1X16,
> - },
> + MEDIA_BUS_FMT_SRGGB16_1X16
> + ),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 16,
> .bayer = true,
> }, {
> .fourcc = V4L2_PIX_FMT_GREY,
> - .codes = {
> + .codes = IMX_BUS_FMTS(
> MEDIA_BUS_FMT_Y8_1X8,
> MEDIA_BUS_FMT_Y10_1X10,
> - MEDIA_BUS_FMT_Y12_1X12,
> - },
> + MEDIA_BUS_FMT_Y12_1X12
> + ),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 8,
> .bayer = true,
> }, {
> .fourcc = V4L2_PIX_FMT_Y10,
> - .codes = {MEDIA_BUS_FMT_Y10_1X10},
> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y10_1X10),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 16,
> .bayer = true,
> }, {
> .fourcc = V4L2_PIX_FMT_Y12,
> - .codes = {MEDIA_BUS_FMT_Y12_1X12},
> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y12_1X12),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 16,
> .bayer = true,
> @@ -203,16 +209,16 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
> CS_SEL_YUV : CS_SEL_RGB);
>
> if (!(fmt_cs_sel & cs_sel) ||
> - (!allow_non_mbus && !fmt->codes[0]))
> + (!allow_non_mbus && !fmt->codes))
> continue;
>
> if (fourcc && fmt->fourcc == fourcc)
> return fmt;
>
> - if (!code)
> + if (!code || !fmt->codes)
> continue;
>
> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> + for (j = 0; fmt->codes[j]; j++) {
> if (code == fmt->codes[j])
> return fmt;
> }
> @@ -237,7 +243,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
> CS_SEL_YUV : CS_SEL_RGB);
>
> if (!(fmt_cs_sel & cs_sel) ||
> - (!allow_non_mbus && !fmt->codes[0]))
> + (!allow_non_mbus && !fmt->codes))
> continue;
>
> if (fourcc && index == 0) {
> @@ -250,7 +256,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
> continue;
> }
>
> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> + for (j = 0; fmt->codes[j]; j++) {
> if (index == 0) {
> *code = fmt->codes[j];
> return 0;
> @@ -296,13 +302,13 @@ EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
> static const struct imx_media_pixfmt ipu_formats[] = {
> {
> .fourcc = V4L2_PIX_FMT_YUV32,
> - .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_AYUV8_1X32),
> .cs = IPUV3_COLORSPACE_YUV,
> .bpp = 32,
> .ipufmt = true,
> }, {
> .fourcc = V4L2_PIX_FMT_XRGB32,
> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 32,
> .ipufmt = true,
> @@ -327,7 +333,10 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
> continue;
>
> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> + if (!fmt->codes)
> + continue;
> +
> + for (j = 0; fmt->codes[j]; j++) {
> if (code == fmt->codes[j])
> return fmt;
> }
> @@ -351,7 +360,10 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
> continue;
>
> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> + if (!fmt->codes)
> + continue;
> +
> + for (j = 0; fmt->codes[j]; j++) {
> if (index == 0) {
> *code = fmt->codes[j];
> return 0;
> @@ -567,7 +579,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
> const struct imx_media_pixfmt *fmt;
>
> fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
> - if (!fmt)
> + if (!fmt || !fmt->codes)
> return -EINVAL;
>
> memset(mbus, 0, sizeof(*mbus));
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index 652673a703cd..917b4db02985 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -69,7 +69,7 @@ enum {
>
> struct imx_media_pixfmt {
> u32 fourcc;
> - u32 codes[4];
> + const u32 *codes;
> int bpp; /* total bpp */
> /* cycles per pixel for generic (bayer) formats for the parallel bus */
> int cycles;
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes
2020-03-31 13:45 ` Hans Verkuil
@ 2020-03-31 23:04 ` Steve Longerbeam
2020-04-01 0:31 ` Laurent Pinchart
0 siblings, 1 reply; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-31 23:04 UTC (permalink / raw)
To: Hans Verkuil, linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva
Hi Hans,
On 3/31/20 6:45 AM, Hans Verkuil wrote:
> On 3/29/20 10:59 PM, Steve Longerbeam wrote:
>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> The imx_media_pixfmt structures includes a codes member that stores
>> media bus codes as a fixed array of 4 integers. The functions dealing
>> with the imx_media_pixfmt structures assume that the array of codes is
>> terminated by a 0 elements. This mechanism is fragile, as demonstrated
> elements -> element
>
>> by several instances of the structure contained 4 non-zero codes.
> contained -> that contained
Will fix above language.
>
>> Fix this by turning the array into a pointer, and providing an
>> IMX_BUS_FMTS macro to initialize the codes member with a guaranteed 0
>> element at the end.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> [Fix a NULL deref when derefencing a NULL cc->codes on return from
>> several calls to imx_media_find_format()]
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> ---
>> drivers/staging/media/imx/imx-media-capture.c | 4 +-
>> drivers/staging/media/imx/imx-media-utils.c | 88 +++++++++++--------
>> drivers/staging/media/imx/imx-media.h | 2 +-
>> 3 files changed, 53 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
>> index d60b49ec4fa4..650c53289f6b 100644
>> --- a/drivers/staging/media/imx/imx-media-capture.c
>> +++ b/drivers/staging/media/imx/imx-media-capture.c
>> @@ -95,7 +95,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
>> if (!cc)
>> return -EINVAL;
>>
>> - fse.code = cc->codes[0];
>> + fse.code = cc->codes ? cc->codes[0] : 0;
> I'm wondering: wouldn't it be better to have a format without codes point to
> an empty code list containing just 0? That would avoid all the cc->codes checks,
> which are a bit fragile IMHO.
I'll modify this patch to declare a const codes array 'no_bus_fmts',
containing a single 0 value, that the in-memory-only imx_media_pixfmt
entries can point to.
>
> It would be nice in that case if there is a WARN_ON or something during probe
> time that checks that the codes field in pixel_formats[] is never NULL.
I'm not so keen on that idea. How about a comment that the
in-memory-only entries should point to no_bus_fmts ?
>
> As an aside: 'cc'? The struct is called imx_media_pixfmt, so I don't offhand see
> what 'cc' stands for.
It's an abbreviation of FourCC. I know, it's obscure and not really
accurate. I will add a non-functional patch that makes the naming
consistent, using 'fmt' for imx_media_pixfmt pointers throughout.
Steve
>>
>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
>> if (ret)
>> @@ -137,7 +137,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
>> if (!cc)
>> return -EINVAL;
>>
>> - fie.code = cc->codes[0];
>> + fie.code = cc->codes ? cc->codes[0] : 0;
>>
>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval,
>> NULL, &fie);
>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>> index 981a8b540a3c..da010eef0ae6 100644
>> --- a/drivers/staging/media/imx/imx-media-utils.c
>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>> @@ -7,6 +7,12 @@
>> #include <linux/module.h>
>> #include "imx-media.h"
>>
>> +#define IMX_BUS_FMTS(fmts...) \
>> + (const u32[]) { \
>> + fmts, \
>> + 0 \
>> + }
>> +
>> /*
>> * List of supported pixel formats for the subdevs.
>> */
>> @@ -14,18 +20,18 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>> /*** YUV formats start here ***/
>> {
>> .fourcc = V4L2_PIX_FMT_UYVY,
>> - .codes = {
>> + .codes = IMX_BUS_FMTS(
>> MEDIA_BUS_FMT_UYVY8_2X8,
>> MEDIA_BUS_FMT_UYVY8_1X16
>> - },
>> + ),
>> .cs = IPUV3_COLORSPACE_YUV,
>> .bpp = 16,
>> }, {
>> .fourcc = V4L2_PIX_FMT_YUYV,
>> - .codes = {
>> + .codes = IMX_BUS_FMTS(
>> MEDIA_BUS_FMT_YUYV8_2X8,
>> MEDIA_BUS_FMT_YUYV8_1X16
>> - },
>> + ),
>> .cs = IPUV3_COLORSPACE_YUV,
>> .bpp = 16,
>> }, {
>> @@ -57,16 +63,16 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>> /*** RGB formats start here ***/
>> {
>> .fourcc = V4L2_PIX_FMT_RGB565,
>> - .codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 16,
>> .cycles = 2,
>> }, {
>> .fourcc = V4L2_PIX_FMT_RGB24,
>> - .codes = {
>> + .codes = IMX_BUS_FMTS(
>> MEDIA_BUS_FMT_RGB888_1X24,
>> MEDIA_BUS_FMT_RGB888_2X12_LE
>> - },
>> + ),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 24,
>> }, {
>> @@ -75,7 +81,7 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>> .bpp = 24,
>> }, {
>> .fourcc = V4L2_PIX_FMT_XRGB32,
>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 32,
>> .ipufmt = true,
>> @@ -95,91 +101,91 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>> /*** raw bayer and grayscale formats start here ***/
>> {
>> .fourcc = V4L2_PIX_FMT_SBGGR8,
>> - .codes = {MEDIA_BUS_FMT_SBGGR8_1X8},
>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 8,
>> .bayer = true,
>> }, {
>> .fourcc = V4L2_PIX_FMT_SGBRG8,
>> - .codes = {MEDIA_BUS_FMT_SGBRG8_1X8},
>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 8,
>> .bayer = true,
>> }, {
>> .fourcc = V4L2_PIX_FMT_SGRBG8,
>> - .codes = {MEDIA_BUS_FMT_SGRBG8_1X8},
>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 8,
>> .bayer = true,
>> }, {
>> .fourcc = V4L2_PIX_FMT_SRGGB8,
>> - .codes = {MEDIA_BUS_FMT_SRGGB8_1X8},
>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 8,
>> .bayer = true,
>> }, {
>> .fourcc = V4L2_PIX_FMT_SBGGR16,
>> - .codes = {
>> + .codes = IMX_BUS_FMTS(
>> MEDIA_BUS_FMT_SBGGR10_1X10,
>> MEDIA_BUS_FMT_SBGGR12_1X12,
>> MEDIA_BUS_FMT_SBGGR14_1X14,
>> MEDIA_BUS_FMT_SBGGR16_1X16
>> - },
>> + ),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 16,
>> .bayer = true,
>> }, {
>> .fourcc = V4L2_PIX_FMT_SGBRG16,
>> - .codes = {
>> + .codes = IMX_BUS_FMTS(
>> MEDIA_BUS_FMT_SGBRG10_1X10,
>> MEDIA_BUS_FMT_SGBRG12_1X12,
>> MEDIA_BUS_FMT_SGBRG14_1X14,
>> - MEDIA_BUS_FMT_SGBRG16_1X16,
>> - },
>> + MEDIA_BUS_FMT_SGBRG16_1X16
>> + ),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 16,
>> .bayer = true,
>> }, {
>> .fourcc = V4L2_PIX_FMT_SGRBG16,
>> - .codes = {
>> + .codes = IMX_BUS_FMTS(
>> MEDIA_BUS_FMT_SGRBG10_1X10,
>> MEDIA_BUS_FMT_SGRBG12_1X12,
>> MEDIA_BUS_FMT_SGRBG14_1X14,
>> - MEDIA_BUS_FMT_SGRBG16_1X16,
>> - },
>> + MEDIA_BUS_FMT_SGRBG16_1X16
>> + ),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 16,
>> .bayer = true,
>> }, {
>> .fourcc = V4L2_PIX_FMT_SRGGB16,
>> - .codes = {
>> + .codes = IMX_BUS_FMTS(
>> MEDIA_BUS_FMT_SRGGB10_1X10,
>> MEDIA_BUS_FMT_SRGGB12_1X12,
>> MEDIA_BUS_FMT_SRGGB14_1X14,
>> - MEDIA_BUS_FMT_SRGGB16_1X16,
>> - },
>> + MEDIA_BUS_FMT_SRGGB16_1X16
>> + ),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 16,
>> .bayer = true,
>> }, {
>> .fourcc = V4L2_PIX_FMT_GREY,
>> - .codes = {
>> + .codes = IMX_BUS_FMTS(
>> MEDIA_BUS_FMT_Y8_1X8,
>> MEDIA_BUS_FMT_Y10_1X10,
>> - MEDIA_BUS_FMT_Y12_1X12,
>> - },
>> + MEDIA_BUS_FMT_Y12_1X12
>> + ),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 8,
>> .bayer = true,
>> }, {
>> .fourcc = V4L2_PIX_FMT_Y10,
>> - .codes = {MEDIA_BUS_FMT_Y10_1X10},
>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y10_1X10),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 16,
>> .bayer = true,
>> }, {
>> .fourcc = V4L2_PIX_FMT_Y12,
>> - .codes = {MEDIA_BUS_FMT_Y12_1X12},
>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y12_1X12),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 16,
>> .bayer = true,
>> @@ -203,16 +209,16 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
>> CS_SEL_YUV : CS_SEL_RGB);
>>
>> if (!(fmt_cs_sel & cs_sel) ||
>> - (!allow_non_mbus && !fmt->codes[0]))
>> + (!allow_non_mbus && !fmt->codes))
>> continue;
>>
>> if (fourcc && fmt->fourcc == fourcc)
>> return fmt;
>>
>> - if (!code)
>> + if (!code || !fmt->codes)
>> continue;
>>
>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>> + for (j = 0; fmt->codes[j]; j++) {
>> if (code == fmt->codes[j])
>> return fmt;
>> }
>> @@ -237,7 +243,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>> CS_SEL_YUV : CS_SEL_RGB);
>>
>> if (!(fmt_cs_sel & cs_sel) ||
>> - (!allow_non_mbus && !fmt->codes[0]))
>> + (!allow_non_mbus && !fmt->codes))
>> continue;
>>
>> if (fourcc && index == 0) {
>> @@ -250,7 +256,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>> continue;
>> }
>>
>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>> + for (j = 0; fmt->codes[j]; j++) {
>> if (index == 0) {
>> *code = fmt->codes[j];
>> return 0;
>> @@ -296,13 +302,13 @@ EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
>> static const struct imx_media_pixfmt ipu_formats[] = {
>> {
>> .fourcc = V4L2_PIX_FMT_YUV32,
>> - .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_AYUV8_1X32),
>> .cs = IPUV3_COLORSPACE_YUV,
>> .bpp = 32,
>> .ipufmt = true,
>> }, {
>> .fourcc = V4L2_PIX_FMT_XRGB32,
>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
>> .cs = IPUV3_COLORSPACE_RGB,
>> .bpp = 32,
>> .ipufmt = true,
>> @@ -327,7 +333,10 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
>> continue;
>>
>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>> + if (!fmt->codes)
>> + continue;
>> +
>> + for (j = 0; fmt->codes[j]; j++) {
>> if (code == fmt->codes[j])
>> return fmt;
>> }
>> @@ -351,7 +360,10 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
>> continue;
>>
>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>> + if (!fmt->codes)
>> + continue;
>> +
>> + for (j = 0; fmt->codes[j]; j++) {
>> if (index == 0) {
>> *code = fmt->codes[j];
>> return 0;
>> @@ -567,7 +579,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>> const struct imx_media_pixfmt *fmt;
>>
>> fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
>> - if (!fmt)
>> + if (!fmt || !fmt->codes)
>> return -EINVAL;
>>
>> memset(mbus, 0, sizeof(*mbus));
>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
>> index 652673a703cd..917b4db02985 100644
>> --- a/drivers/staging/media/imx/imx-media.h
>> +++ b/drivers/staging/media/imx/imx-media.h
>> @@ -69,7 +69,7 @@ enum {
>>
>> struct imx_media_pixfmt {
>> u32 fourcc;
>> - u32 codes[4];
>> + const u32 *codes;
>> int bpp; /* total bpp */
>> /* cycles per pixel for generic (bayer) formats for the parallel bus */
>> int cycles;
>>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes
2020-03-31 23:04 ` Steve Longerbeam
@ 2020-04-01 0:31 ` Laurent Pinchart
2020-04-01 7:39 ` Hans Verkuil
2020-04-01 15:20 ` Steve Longerbeam
0 siblings, 2 replies; 20+ messages in thread
From: Laurent Pinchart @ 2020-04-01 0:31 UTC (permalink / raw)
To: Steve Longerbeam
Cc: Hans Verkuil, linux-media, Philipp Zabel, Rui Miguel Silva
Hello,
On Tue, Mar 31, 2020 at 04:04:18PM -0700, Steve Longerbeam wrote:
> On 3/31/20 6:45 AM, Hans Verkuil wrote:
> > On 3/29/20 10:59 PM, Steve Longerbeam wrote:
> >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>
> >> The imx_media_pixfmt structures includes a codes member that stores
> >> media bus codes as a fixed array of 4 integers. The functions dealing
> >> with the imx_media_pixfmt structures assume that the array of codes is
> >> terminated by a 0 elements. This mechanism is fragile, as demonstrated
> >
> > elements -> element
> >
> >> by several instances of the structure contained 4 non-zero codes.
> >
> > contained -> that contained
>
> Will fix above language.
>
> >> Fix this by turning the array into a pointer, and providing an
> >> IMX_BUS_FMTS macro to initialize the codes member with a guaranteed 0
> >> element at the end.
> >>
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>
> >> [Fix a NULL deref when derefencing a NULL cc->codes on return from
> >> several calls to imx_media_find_format()]
> >> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> >> ---
> >> drivers/staging/media/imx/imx-media-capture.c | 4 +-
> >> drivers/staging/media/imx/imx-media-utils.c | 88 +++++++++++--------
> >> drivers/staging/media/imx/imx-media.h | 2 +-
> >> 3 files changed, 53 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> >> index d60b49ec4fa4..650c53289f6b 100644
> >> --- a/drivers/staging/media/imx/imx-media-capture.c
> >> +++ b/drivers/staging/media/imx/imx-media-capture.c
> >> @@ -95,7 +95,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
> >> if (!cc)
> >> return -EINVAL;
> >>
> >> - fse.code = cc->codes[0];
> >> + fse.code = cc->codes ? cc->codes[0] : 0;
> > I'm wondering: wouldn't it be better to have a format without codes point to
> > an empty code list containing just 0? That would avoid all the cc->codes checks,
> > which are a bit fragile IMHO.
>
> I'll modify this patch to declare a const codes array 'no_bus_fmts',
> containing a single 0 value, that the in-memory-only imx_media_pixfmt
> entries can point to.
>
> > It would be nice in that case if there is a WARN_ON or something during probe
> > time that checks that the codes field in pixel_formats[] is never NULL.
>
> I'm not so keen on that idea. How about a comment that the
> in-memory-only entries should point to no_bus_fmts ?
I don't like either of those options I'm afraid. Someone will one day
forget to point to that array when adding a new format, and we'll have a
bug. With code in just a few locations that checks for NULL, we're safe
as the compiler will initialize the pointer to NULL for us. Let's use
the compiler when it helps us, I don't see what removing the NULL check
in the code could bring us except issues :-)
> > As an aside: 'cc'? The struct is called imx_media_pixfmt, so I don't offhand see
> > what 'cc' stands for.
>
> It's an abbreviation of FourCC. I know, it's obscure and not really
> accurate. I will add a non-functional patch that makes the naming
> consistent, using 'fmt' for imx_media_pixfmt pointers throughout.
>
> >> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
> >> if (ret)
> >> @@ -137,7 +137,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
> >> if (!cc)
> >> return -EINVAL;
> >>
> >> - fie.code = cc->codes[0];
> >> + fie.code = cc->codes ? cc->codes[0] : 0;
> >>
> >> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval,
> >> NULL, &fie);
> >> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> >> index 981a8b540a3c..da010eef0ae6 100644
> >> --- a/drivers/staging/media/imx/imx-media-utils.c
> >> +++ b/drivers/staging/media/imx/imx-media-utils.c
> >> @@ -7,6 +7,12 @@
> >> #include <linux/module.h>
> >> #include "imx-media.h"
> >>
> >> +#define IMX_BUS_FMTS(fmts...) \
> >> + (const u32[]) { \
> >> + fmts, \
> >> + 0 \
> >> + }
> >> +
> >> /*
> >> * List of supported pixel formats for the subdevs.
> >> */
> >> @@ -14,18 +20,18 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >> /*** YUV formats start here ***/
> >> {
> >> .fourcc = V4L2_PIX_FMT_UYVY,
> >> - .codes = {
> >> + .codes = IMX_BUS_FMTS(
> >> MEDIA_BUS_FMT_UYVY8_2X8,
> >> MEDIA_BUS_FMT_UYVY8_1X16
> >> - },
> >> + ),
> >> .cs = IPUV3_COLORSPACE_YUV,
> >> .bpp = 16,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_YUYV,
> >> - .codes = {
> >> + .codes = IMX_BUS_FMTS(
> >> MEDIA_BUS_FMT_YUYV8_2X8,
> >> MEDIA_BUS_FMT_YUYV8_1X16
> >> - },
> >> + ),
> >> .cs = IPUV3_COLORSPACE_YUV,
> >> .bpp = 16,
> >> }, {
> >> @@ -57,16 +63,16 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >> /*** RGB formats start here ***/
> >> {
> >> .fourcc = V4L2_PIX_FMT_RGB565,
> >> - .codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
> >> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 16,
> >> .cycles = 2,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_RGB24,
> >> - .codes = {
> >> + .codes = IMX_BUS_FMTS(
> >> MEDIA_BUS_FMT_RGB888_1X24,
> >> MEDIA_BUS_FMT_RGB888_2X12_LE
> >> - },
> >> + ),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 24,
> >> }, {
> >> @@ -75,7 +81,7 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >> .bpp = 24,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_XRGB32,
> >> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> >> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 32,
> >> .ipufmt = true,
> >> @@ -95,91 +101,91 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >> /*** raw bayer and grayscale formats start here ***/
> >> {
> >> .fourcc = V4L2_PIX_FMT_SBGGR8,
> >> - .codes = {MEDIA_BUS_FMT_SBGGR8_1X8},
> >> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 8,
> >> .bayer = true,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_SGBRG8,
> >> - .codes = {MEDIA_BUS_FMT_SGBRG8_1X8},
> >> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 8,
> >> .bayer = true,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_SGRBG8,
> >> - .codes = {MEDIA_BUS_FMT_SGRBG8_1X8},
> >> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 8,
> >> .bayer = true,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_SRGGB8,
> >> - .codes = {MEDIA_BUS_FMT_SRGGB8_1X8},
> >> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 8,
> >> .bayer = true,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_SBGGR16,
> >> - .codes = {
> >> + .codes = IMX_BUS_FMTS(
> >> MEDIA_BUS_FMT_SBGGR10_1X10,
> >> MEDIA_BUS_FMT_SBGGR12_1X12,
> >> MEDIA_BUS_FMT_SBGGR14_1X14,
> >> MEDIA_BUS_FMT_SBGGR16_1X16
> >> - },
> >> + ),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 16,
> >> .bayer = true,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_SGBRG16,
> >> - .codes = {
> >> + .codes = IMX_BUS_FMTS(
> >> MEDIA_BUS_FMT_SGBRG10_1X10,
> >> MEDIA_BUS_FMT_SGBRG12_1X12,
> >> MEDIA_BUS_FMT_SGBRG14_1X14,
> >> - MEDIA_BUS_FMT_SGBRG16_1X16,
> >> - },
> >> + MEDIA_BUS_FMT_SGBRG16_1X16
> >> + ),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 16,
> >> .bayer = true,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_SGRBG16,
> >> - .codes = {
> >> + .codes = IMX_BUS_FMTS(
> >> MEDIA_BUS_FMT_SGRBG10_1X10,
> >> MEDIA_BUS_FMT_SGRBG12_1X12,
> >> MEDIA_BUS_FMT_SGRBG14_1X14,
> >> - MEDIA_BUS_FMT_SGRBG16_1X16,
> >> - },
> >> + MEDIA_BUS_FMT_SGRBG16_1X16
> >> + ),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 16,
> >> .bayer = true,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_SRGGB16,
> >> - .codes = {
> >> + .codes = IMX_BUS_FMTS(
> >> MEDIA_BUS_FMT_SRGGB10_1X10,
> >> MEDIA_BUS_FMT_SRGGB12_1X12,
> >> MEDIA_BUS_FMT_SRGGB14_1X14,
> >> - MEDIA_BUS_FMT_SRGGB16_1X16,
> >> - },
> >> + MEDIA_BUS_FMT_SRGGB16_1X16
> >> + ),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 16,
> >> .bayer = true,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_GREY,
> >> - .codes = {
> >> + .codes = IMX_BUS_FMTS(
> >> MEDIA_BUS_FMT_Y8_1X8,
> >> MEDIA_BUS_FMT_Y10_1X10,
> >> - MEDIA_BUS_FMT_Y12_1X12,
> >> - },
> >> + MEDIA_BUS_FMT_Y12_1X12
> >> + ),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 8,
> >> .bayer = true,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_Y10,
> >> - .codes = {MEDIA_BUS_FMT_Y10_1X10},
> >> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y10_1X10),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 16,
> >> .bayer = true,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_Y12,
> >> - .codes = {MEDIA_BUS_FMT_Y12_1X12},
> >> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y12_1X12),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 16,
> >> .bayer = true,
> >> @@ -203,16 +209,16 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
> >> CS_SEL_YUV : CS_SEL_RGB);
> >>
> >> if (!(fmt_cs_sel & cs_sel) ||
> >> - (!allow_non_mbus && !fmt->codes[0]))
> >> + (!allow_non_mbus && !fmt->codes))
> >> continue;
> >>
> >> if (fourcc && fmt->fourcc == fourcc)
> >> return fmt;
> >>
> >> - if (!code)
> >> + if (!code || !fmt->codes)
> >> continue;
> >>
> >> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >> + for (j = 0; fmt->codes[j]; j++) {
> >> if (code == fmt->codes[j])
> >> return fmt;
> >> }
> >> @@ -237,7 +243,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
> >> CS_SEL_YUV : CS_SEL_RGB);
> >>
> >> if (!(fmt_cs_sel & cs_sel) ||
> >> - (!allow_non_mbus && !fmt->codes[0]))
> >> + (!allow_non_mbus && !fmt->codes))
> >> continue;
> >>
> >> if (fourcc && index == 0) {
> >> @@ -250,7 +256,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
> >> continue;
> >> }
> >>
> >> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >> + for (j = 0; fmt->codes[j]; j++) {
> >> if (index == 0) {
> >> *code = fmt->codes[j];
> >> return 0;
> >> @@ -296,13 +302,13 @@ EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
> >> static const struct imx_media_pixfmt ipu_formats[] = {
> >> {
> >> .fourcc = V4L2_PIX_FMT_YUV32,
> >> - .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
> >> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_AYUV8_1X32),
> >> .cs = IPUV3_COLORSPACE_YUV,
> >> .bpp = 32,
> >> .ipufmt = true,
> >> }, {
> >> .fourcc = V4L2_PIX_FMT_XRGB32,
> >> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> >> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
> >> .cs = IPUV3_COLORSPACE_RGB,
> >> .bpp = 32,
> >> .ipufmt = true,
> >> @@ -327,7 +333,10 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
> >> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
> >> continue;
> >>
> >> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >> + if (!fmt->codes)
> >> + continue;
> >> +
> >> + for (j = 0; fmt->codes[j]; j++) {
> >> if (code == fmt->codes[j])
> >> return fmt;
> >> }
> >> @@ -351,7 +360,10 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
> >> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
> >> continue;
> >>
> >> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >> + if (!fmt->codes)
> >> + continue;
> >> +
> >> + for (j = 0; fmt->codes[j]; j++) {
> >> if (index == 0) {
> >> *code = fmt->codes[j];
> >> return 0;
> >> @@ -567,7 +579,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
> >> const struct imx_media_pixfmt *fmt;
> >>
> >> fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
> >> - if (!fmt)
> >> + if (!fmt || !fmt->codes)
> >> return -EINVAL;
> >>
> >> memset(mbus, 0, sizeof(*mbus));
> >> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> >> index 652673a703cd..917b4db02985 100644
> >> --- a/drivers/staging/media/imx/imx-media.h
> >> +++ b/drivers/staging/media/imx/imx-media.h
> >> @@ -69,7 +69,7 @@ enum {
> >>
> >> struct imx_media_pixfmt {
> >> u32 fourcc;
> >> - u32 codes[4];
> >> + const u32 *codes;
> >> int bpp; /* total bpp */
> >> /* cycles per pixel for generic (bayer) formats for the parallel bus */
> >> int cycles;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes
2020-04-01 0:31 ` Laurent Pinchart
@ 2020-04-01 7:39 ` Hans Verkuil
2020-04-01 15:20 ` Steve Longerbeam
1 sibling, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2020-04-01 7:39 UTC (permalink / raw)
To: Laurent Pinchart, Steve Longerbeam
Cc: linux-media, Philipp Zabel, Rui Miguel Silva
On 4/1/20 2:31 AM, Laurent Pinchart wrote:
> Hello,
>
> On Tue, Mar 31, 2020 at 04:04:18PM -0700, Steve Longerbeam wrote:
>> On 3/31/20 6:45 AM, Hans Verkuil wrote:
>>> On 3/29/20 10:59 PM, Steve Longerbeam wrote:
>>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>
>>>> The imx_media_pixfmt structures includes a codes member that stores
>>>> media bus codes as a fixed array of 4 integers. The functions dealing
>>>> with the imx_media_pixfmt structures assume that the array of codes is
>>>> terminated by a 0 elements. This mechanism is fragile, as demonstrated
>>>
>>> elements -> element
>>>
>>>> by several instances of the structure contained 4 non-zero codes.
>>>
>>> contained -> that contained
>>
>> Will fix above language.
>>
>>>> Fix this by turning the array into a pointer, and providing an
>>>> IMX_BUS_FMTS macro to initialize the codes member with a guaranteed 0
>>>> element at the end.
>>>>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>
>>>> [Fix a NULL deref when derefencing a NULL cc->codes on return from
>>>> several calls to imx_media_find_format()]
>>>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>>>> ---
>>>> drivers/staging/media/imx/imx-media-capture.c | 4 +-
>>>> drivers/staging/media/imx/imx-media-utils.c | 88 +++++++++++--------
>>>> drivers/staging/media/imx/imx-media.h | 2 +-
>>>> 3 files changed, 53 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
>>>> index d60b49ec4fa4..650c53289f6b 100644
>>>> --- a/drivers/staging/media/imx/imx-media-capture.c
>>>> +++ b/drivers/staging/media/imx/imx-media-capture.c
>>>> @@ -95,7 +95,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
>>>> if (!cc)
>>>> return -EINVAL;
>>>>
>>>> - fse.code = cc->codes[0];
>>>> + fse.code = cc->codes ? cc->codes[0] : 0;
>>> I'm wondering: wouldn't it be better to have a format without codes point to
>>> an empty code list containing just 0? That would avoid all the cc->codes checks,
>>> which are a bit fragile IMHO.
>>
>> I'll modify this patch to declare a const codes array 'no_bus_fmts',
>> containing a single 0 value, that the in-memory-only imx_media_pixfmt
>> entries can point to.
>>
>>> It would be nice in that case if there is a WARN_ON or something during probe
>>> time that checks that the codes field in pixel_formats[] is never NULL.
>>
>> I'm not so keen on that idea. How about a comment that the
>> in-memory-only entries should point to no_bus_fmts ?
>
> I don't like either of those options I'm afraid. Someone will one day
> forget to point to that array when adding a new format, and we'll have a
> bug. With code in just a few locations that checks for NULL, we're safe
> as the compiler will initialize the pointer to NULL for us. Let's use
> the compiler when it helps us, I don't see what removing the NULL check
> in the code could bring us except issues :-)
With the current patch there is a chance that someone adds new code that
forgets to check if 'codes' is NULL. By always setting 'codes' to a valid
pointer that problem is solved, but now you need to check somewhere in probe()
that nobody forgot to set 'codes'. Although this might not be necessary if
there is already code that walks the full array and uses 'codes'. In that
case adding a new entry and forgetting to set 'codes' will automatically
cause a crash, thus making sure that nobody forgets.
I think the second approach is more robust in the end.
Just to clarify, this is not a blocking issue since in the end it is mostly a
matter of taste.
Regards,
Hans
>
>>> As an aside: 'cc'? The struct is called imx_media_pixfmt, so I don't offhand see
>>> what 'cc' stands for.
>>
>> It's an abbreviation of FourCC. I know, it's obscure and not really
>> accurate. I will add a non-functional patch that makes the naming
>> consistent, using 'fmt' for imx_media_pixfmt pointers throughout.
>>
>>>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
>>>> if (ret)
>>>> @@ -137,7 +137,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
>>>> if (!cc)
>>>> return -EINVAL;
>>>>
>>>> - fie.code = cc->codes[0];
>>>> + fie.code = cc->codes ? cc->codes[0] : 0;
>>>>
>>>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval,
>>>> NULL, &fie);
>>>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>>>> index 981a8b540a3c..da010eef0ae6 100644
>>>> --- a/drivers/staging/media/imx/imx-media-utils.c
>>>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>>>> @@ -7,6 +7,12 @@
>>>> #include <linux/module.h>
>>>> #include "imx-media.h"
>>>>
>>>> +#define IMX_BUS_FMTS(fmts...) \
>>>> + (const u32[]) { \
>>>> + fmts, \
>>>> + 0 \
>>>> + }
>>>> +
>>>> /*
>>>> * List of supported pixel formats for the subdevs.
>>>> */
>>>> @@ -14,18 +20,18 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>> /*** YUV formats start here ***/
>>>> {
>>>> .fourcc = V4L2_PIX_FMT_UYVY,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_UYVY8_2X8,
>>>> MEDIA_BUS_FMT_UYVY8_1X16
>>>> - },
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_YUV,
>>>> .bpp = 16,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_YUYV,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_YUYV8_2X8,
>>>> MEDIA_BUS_FMT_YUYV8_1X16
>>>> - },
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_YUV,
>>>> .bpp = 16,
>>>> }, {
>>>> @@ -57,16 +63,16 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>> /*** RGB formats start here ***/
>>>> {
>>>> .fourcc = V4L2_PIX_FMT_RGB565,
>>>> - .codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .cycles = 2,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_RGB24,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_RGB888_1X24,
>>>> MEDIA_BUS_FMT_RGB888_2X12_LE
>>>> - },
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 24,
>>>> }, {
>>>> @@ -75,7 +81,7 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>> .bpp = 24,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_XRGB32,
>>>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 32,
>>>> .ipufmt = true,
>>>> @@ -95,91 +101,91 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>> /*** raw bayer and grayscale formats start here ***/
>>>> {
>>>> .fourcc = V4L2_PIX_FMT_SBGGR8,
>>>> - .codes = {MEDIA_BUS_FMT_SBGGR8_1X8},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 8,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SGBRG8,
>>>> - .codes = {MEDIA_BUS_FMT_SGBRG8_1X8},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 8,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SGRBG8,
>>>> - .codes = {MEDIA_BUS_FMT_SGRBG8_1X8},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 8,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SRGGB8,
>>>> - .codes = {MEDIA_BUS_FMT_SRGGB8_1X8},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 8,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SBGGR16,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_SBGGR10_1X10,
>>>> MEDIA_BUS_FMT_SBGGR12_1X12,
>>>> MEDIA_BUS_FMT_SBGGR14_1X14,
>>>> MEDIA_BUS_FMT_SBGGR16_1X16
>>>> - },
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SGBRG16,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_SGBRG10_1X10,
>>>> MEDIA_BUS_FMT_SGBRG12_1X12,
>>>> MEDIA_BUS_FMT_SGBRG14_1X14,
>>>> - MEDIA_BUS_FMT_SGBRG16_1X16,
>>>> - },
>>>> + MEDIA_BUS_FMT_SGBRG16_1X16
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SGRBG16,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_SGRBG10_1X10,
>>>> MEDIA_BUS_FMT_SGRBG12_1X12,
>>>> MEDIA_BUS_FMT_SGRBG14_1X14,
>>>> - MEDIA_BUS_FMT_SGRBG16_1X16,
>>>> - },
>>>> + MEDIA_BUS_FMT_SGRBG16_1X16
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SRGGB16,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_SRGGB10_1X10,
>>>> MEDIA_BUS_FMT_SRGGB12_1X12,
>>>> MEDIA_BUS_FMT_SRGGB14_1X14,
>>>> - MEDIA_BUS_FMT_SRGGB16_1X16,
>>>> - },
>>>> + MEDIA_BUS_FMT_SRGGB16_1X16
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_GREY,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_Y8_1X8,
>>>> MEDIA_BUS_FMT_Y10_1X10,
>>>> - MEDIA_BUS_FMT_Y12_1X12,
>>>> - },
>>>> + MEDIA_BUS_FMT_Y12_1X12
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 8,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_Y10,
>>>> - .codes = {MEDIA_BUS_FMT_Y10_1X10},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y10_1X10),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_Y12,
>>>> - .codes = {MEDIA_BUS_FMT_Y12_1X12},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y12_1X12),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> @@ -203,16 +209,16 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
>>>> CS_SEL_YUV : CS_SEL_RGB);
>>>>
>>>> if (!(fmt_cs_sel & cs_sel) ||
>>>> - (!allow_non_mbus && !fmt->codes[0]))
>>>> + (!allow_non_mbus && !fmt->codes))
>>>> continue;
>>>>
>>>> if (fourcc && fmt->fourcc == fourcc)
>>>> return fmt;
>>>>
>>>> - if (!code)
>>>> + if (!code || !fmt->codes)
>>>> continue;
>>>>
>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>> if (code == fmt->codes[j])
>>>> return fmt;
>>>> }
>>>> @@ -237,7 +243,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>>>> CS_SEL_YUV : CS_SEL_RGB);
>>>>
>>>> if (!(fmt_cs_sel & cs_sel) ||
>>>> - (!allow_non_mbus && !fmt->codes[0]))
>>>> + (!allow_non_mbus && !fmt->codes))
>>>> continue;
>>>>
>>>> if (fourcc && index == 0) {
>>>> @@ -250,7 +256,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>>>> continue;
>>>> }
>>>>
>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>> if (index == 0) {
>>>> *code = fmt->codes[j];
>>>> return 0;
>>>> @@ -296,13 +302,13 @@ EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
>>>> static const struct imx_media_pixfmt ipu_formats[] = {
>>>> {
>>>> .fourcc = V4L2_PIX_FMT_YUV32,
>>>> - .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_AYUV8_1X32),
>>>> .cs = IPUV3_COLORSPACE_YUV,
>>>> .bpp = 32,
>>>> .ipufmt = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_XRGB32,
>>>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 32,
>>>> .ipufmt = true,
>>>> @@ -327,7 +333,10 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
>>>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
>>>> continue;
>>>>
>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>> + if (!fmt->codes)
>>>> + continue;
>>>> +
>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>> if (code == fmt->codes[j])
>>>> return fmt;
>>>> }
>>>> @@ -351,7 +360,10 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
>>>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
>>>> continue;
>>>>
>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>> + if (!fmt->codes)
>>>> + continue;
>>>> +
>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>> if (index == 0) {
>>>> *code = fmt->codes[j];
>>>> return 0;
>>>> @@ -567,7 +579,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>>>> const struct imx_media_pixfmt *fmt;
>>>>
>>>> fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
>>>> - if (!fmt)
>>>> + if (!fmt || !fmt->codes)
>>>> return -EINVAL;
>>>>
>>>> memset(mbus, 0, sizeof(*mbus));
>>>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
>>>> index 652673a703cd..917b4db02985 100644
>>>> --- a/drivers/staging/media/imx/imx-media.h
>>>> +++ b/drivers/staging/media/imx/imx-media.h
>>>> @@ -69,7 +69,7 @@ enum {
>>>>
>>>> struct imx_media_pixfmt {
>>>> u32 fourcc;
>>>> - u32 codes[4];
>>>> + const u32 *codes;
>>>> int bpp; /* total bpp */
>>>> /* cycles per pixel for generic (bayer) formats for the parallel bus */
>>>> int cycles;
>
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes
2020-04-01 0:31 ` Laurent Pinchart
2020-04-01 7:39 ` Hans Verkuil
@ 2020-04-01 15:20 ` Steve Longerbeam
2020-04-01 15:31 ` Laurent Pinchart
1 sibling, 1 reply; 20+ messages in thread
From: Steve Longerbeam @ 2020-04-01 15:20 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Hans Verkuil, linux-media, Philipp Zabel, Rui Miguel Silva
Hi Laurent,
On 3/31/20 5:31 PM, Laurent Pinchart wrote:
> Hello,
>
> On Tue, Mar 31, 2020 at 04:04:18PM -0700, Steve Longerbeam wrote:
>> On 3/31/20 6:45 AM, Hans Verkuil wrote:
>>> On 3/29/20 10:59 PM, Steve Longerbeam wrote:
>>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>
>>>> The imx_media_pixfmt structures includes a codes member that stores
>>>> media bus codes as a fixed array of 4 integers. The functions dealing
>>>> with the imx_media_pixfmt structures assume that the array of codes is
>>>> terminated by a 0 elements. This mechanism is fragile, as demonstrated
>>> elements -> element
>>>
>>>> by several instances of the structure contained 4 non-zero codes.
>>> contained -> that contained
>> Will fix above language.
>>
>>>> Fix this by turning the array into a pointer, and providing an
>>>> IMX_BUS_FMTS macro to initialize the codes member with a guaranteed 0
>>>> element at the end.
>>>>
>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>
>>>> [Fix a NULL deref when derefencing a NULL cc->codes on return from
>>>> several calls to imx_media_find_format()]
>>>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>>>> ---
>>>> drivers/staging/media/imx/imx-media-capture.c | 4 +-
>>>> drivers/staging/media/imx/imx-media-utils.c | 88 +++++++++++--------
>>>> drivers/staging/media/imx/imx-media.h | 2 +-
>>>> 3 files changed, 53 insertions(+), 41 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
>>>> index d60b49ec4fa4..650c53289f6b 100644
>>>> --- a/drivers/staging/media/imx/imx-media-capture.c
>>>> +++ b/drivers/staging/media/imx/imx-media-capture.c
>>>> @@ -95,7 +95,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
>>>> if (!cc)
>>>> return -EINVAL;
>>>>
>>>> - fse.code = cc->codes[0];
>>>> + fse.code = cc->codes ? cc->codes[0] : 0;
>>> I'm wondering: wouldn't it be better to have a format without codes point to
>>> an empty code list containing just 0? That would avoid all the cc->codes checks,
>>> which are a bit fragile IMHO.
>> I'll modify this patch to declare a const codes array 'no_bus_fmts',
>> containing a single 0 value, that the in-memory-only imx_media_pixfmt
>> entries can point to.
>>
>>> It would be nice in that case if there is a WARN_ON or something during probe
>>> time that checks that the codes field in pixel_formats[] is never NULL.
>> I'm not so keen on that idea. How about a comment that the
>> in-memory-only entries should point to no_bus_fmts ?
> I don't like either of those options I'm afraid. Someone will one day
> forget to point to that array when adding a new format, and we'll have a
> bug. With code in just a few locations that checks for NULL, we're safe
> as the compiler will initialize the pointer to NULL for us. Let's use
> the compiler when it helps us, I don't see what removing the NULL check
> in the code could bring us except issues :-)
I wasn't planning on removing the NULL checks, only to add the comment
to advise setting .codes = no_bus_fmts. Although I don't really think
that's necessary either.
Steve
>
>>> As an aside: 'cc'? The struct is called imx_media_pixfmt, so I don't offhand see
>>> what 'cc' stands for.
>> It's an abbreviation of FourCC. I know, it's obscure and not really
>> accurate. I will add a non-functional patch that makes the naming
>> consistent, using 'fmt' for imx_media_pixfmt pointers throughout.
>>
>>>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
>>>> if (ret)
>>>> @@ -137,7 +137,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
>>>> if (!cc)
>>>> return -EINVAL;
>>>>
>>>> - fie.code = cc->codes[0];
>>>> + fie.code = cc->codes ? cc->codes[0] : 0;
>>>>
>>>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval,
>>>> NULL, &fie);
>>>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>>>> index 981a8b540a3c..da010eef0ae6 100644
>>>> --- a/drivers/staging/media/imx/imx-media-utils.c
>>>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>>>> @@ -7,6 +7,12 @@
>>>> #include <linux/module.h>
>>>> #include "imx-media.h"
>>>>
>>>> +#define IMX_BUS_FMTS(fmts...) \
>>>> + (const u32[]) { \
>>>> + fmts, \
>>>> + 0 \
>>>> + }
>>>> +
>>>> /*
>>>> * List of supported pixel formats for the subdevs.
>>>> */
>>>> @@ -14,18 +20,18 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>> /*** YUV formats start here ***/
>>>> {
>>>> .fourcc = V4L2_PIX_FMT_UYVY,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_UYVY8_2X8,
>>>> MEDIA_BUS_FMT_UYVY8_1X16
>>>> - },
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_YUV,
>>>> .bpp = 16,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_YUYV,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_YUYV8_2X8,
>>>> MEDIA_BUS_FMT_YUYV8_1X16
>>>> - },
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_YUV,
>>>> .bpp = 16,
>>>> }, {
>>>> @@ -57,16 +63,16 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>> /*** RGB formats start here ***/
>>>> {
>>>> .fourcc = V4L2_PIX_FMT_RGB565,
>>>> - .codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .cycles = 2,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_RGB24,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_RGB888_1X24,
>>>> MEDIA_BUS_FMT_RGB888_2X12_LE
>>>> - },
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 24,
>>>> }, {
>>>> @@ -75,7 +81,7 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>> .bpp = 24,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_XRGB32,
>>>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 32,
>>>> .ipufmt = true,
>>>> @@ -95,91 +101,91 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>> /*** raw bayer and grayscale formats start here ***/
>>>> {
>>>> .fourcc = V4L2_PIX_FMT_SBGGR8,
>>>> - .codes = {MEDIA_BUS_FMT_SBGGR8_1X8},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 8,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SGBRG8,
>>>> - .codes = {MEDIA_BUS_FMT_SGBRG8_1X8},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 8,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SGRBG8,
>>>> - .codes = {MEDIA_BUS_FMT_SGRBG8_1X8},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 8,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SRGGB8,
>>>> - .codes = {MEDIA_BUS_FMT_SRGGB8_1X8},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 8,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SBGGR16,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_SBGGR10_1X10,
>>>> MEDIA_BUS_FMT_SBGGR12_1X12,
>>>> MEDIA_BUS_FMT_SBGGR14_1X14,
>>>> MEDIA_BUS_FMT_SBGGR16_1X16
>>>> - },
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SGBRG16,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_SGBRG10_1X10,
>>>> MEDIA_BUS_FMT_SGBRG12_1X12,
>>>> MEDIA_BUS_FMT_SGBRG14_1X14,
>>>> - MEDIA_BUS_FMT_SGBRG16_1X16,
>>>> - },
>>>> + MEDIA_BUS_FMT_SGBRG16_1X16
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SGRBG16,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_SGRBG10_1X10,
>>>> MEDIA_BUS_FMT_SGRBG12_1X12,
>>>> MEDIA_BUS_FMT_SGRBG14_1X14,
>>>> - MEDIA_BUS_FMT_SGRBG16_1X16,
>>>> - },
>>>> + MEDIA_BUS_FMT_SGRBG16_1X16
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_SRGGB16,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_SRGGB10_1X10,
>>>> MEDIA_BUS_FMT_SRGGB12_1X12,
>>>> MEDIA_BUS_FMT_SRGGB14_1X14,
>>>> - MEDIA_BUS_FMT_SRGGB16_1X16,
>>>> - },
>>>> + MEDIA_BUS_FMT_SRGGB16_1X16
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_GREY,
>>>> - .codes = {
>>>> + .codes = IMX_BUS_FMTS(
>>>> MEDIA_BUS_FMT_Y8_1X8,
>>>> MEDIA_BUS_FMT_Y10_1X10,
>>>> - MEDIA_BUS_FMT_Y12_1X12,
>>>> - },
>>>> + MEDIA_BUS_FMT_Y12_1X12
>>>> + ),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 8,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_Y10,
>>>> - .codes = {MEDIA_BUS_FMT_Y10_1X10},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y10_1X10),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_Y12,
>>>> - .codes = {MEDIA_BUS_FMT_Y12_1X12},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y12_1X12),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 16,
>>>> .bayer = true,
>>>> @@ -203,16 +209,16 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
>>>> CS_SEL_YUV : CS_SEL_RGB);
>>>>
>>>> if (!(fmt_cs_sel & cs_sel) ||
>>>> - (!allow_non_mbus && !fmt->codes[0]))
>>>> + (!allow_non_mbus && !fmt->codes))
>>>> continue;
>>>>
>>>> if (fourcc && fmt->fourcc == fourcc)
>>>> return fmt;
>>>>
>>>> - if (!code)
>>>> + if (!code || !fmt->codes)
>>>> continue;
>>>>
>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>> if (code == fmt->codes[j])
>>>> return fmt;
>>>> }
>>>> @@ -237,7 +243,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>>>> CS_SEL_YUV : CS_SEL_RGB);
>>>>
>>>> if (!(fmt_cs_sel & cs_sel) ||
>>>> - (!allow_non_mbus && !fmt->codes[0]))
>>>> + (!allow_non_mbus && !fmt->codes))
>>>> continue;
>>>>
>>>> if (fourcc && index == 0) {
>>>> @@ -250,7 +256,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>>>> continue;
>>>> }
>>>>
>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>> if (index == 0) {
>>>> *code = fmt->codes[j];
>>>> return 0;
>>>> @@ -296,13 +302,13 @@ EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
>>>> static const struct imx_media_pixfmt ipu_formats[] = {
>>>> {
>>>> .fourcc = V4L2_PIX_FMT_YUV32,
>>>> - .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_AYUV8_1X32),
>>>> .cs = IPUV3_COLORSPACE_YUV,
>>>> .bpp = 32,
>>>> .ipufmt = true,
>>>> }, {
>>>> .fourcc = V4L2_PIX_FMT_XRGB32,
>>>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>> .bpp = 32,
>>>> .ipufmt = true,
>>>> @@ -327,7 +333,10 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
>>>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
>>>> continue;
>>>>
>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>> + if (!fmt->codes)
>>>> + continue;
>>>> +
>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>> if (code == fmt->codes[j])
>>>> return fmt;
>>>> }
>>>> @@ -351,7 +360,10 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
>>>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
>>>> continue;
>>>>
>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>> + if (!fmt->codes)
>>>> + continue;
>>>> +
>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>> if (index == 0) {
>>>> *code = fmt->codes[j];
>>>> return 0;
>>>> @@ -567,7 +579,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>>>> const struct imx_media_pixfmt *fmt;
>>>>
>>>> fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
>>>> - if (!fmt)
>>>> + if (!fmt || !fmt->codes)
>>>> return -EINVAL;
>>>>
>>>> memset(mbus, 0, sizeof(*mbus));
>>>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
>>>> index 652673a703cd..917b4db02985 100644
>>>> --- a/drivers/staging/media/imx/imx-media.h
>>>> +++ b/drivers/staging/media/imx/imx-media.h
>>>> @@ -69,7 +69,7 @@ enum {
>>>>
>>>> struct imx_media_pixfmt {
>>>> u32 fourcc;
>>>> - u32 codes[4];
>>>> + const u32 *codes;
>>>> int bpp; /* total bpp */
>>>> /* cycles per pixel for generic (bayer) formats for the parallel bus */
>>>> int cycles;
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes
2020-04-01 15:20 ` Steve Longerbeam
@ 2020-04-01 15:31 ` Laurent Pinchart
2020-04-03 22:29 ` Steve Longerbeam
0 siblings, 1 reply; 20+ messages in thread
From: Laurent Pinchart @ 2020-04-01 15:31 UTC (permalink / raw)
To: Steve Longerbeam
Cc: Hans Verkuil, linux-media, Philipp Zabel, Rui Miguel Silva
On Wed, Apr 01, 2020 at 08:20:59AM -0700, Steve Longerbeam wrote:
> On 3/31/20 5:31 PM, Laurent Pinchart wrote:
> > On Tue, Mar 31, 2020 at 04:04:18PM -0700, Steve Longerbeam wrote:
> >> On 3/31/20 6:45 AM, Hans Verkuil wrote:
> >>> On 3/29/20 10:59 PM, Steve Longerbeam wrote:
> >>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>
> >>>> The imx_media_pixfmt structures includes a codes member that stores
> >>>> media bus codes as a fixed array of 4 integers. The functions dealing
> >>>> with the imx_media_pixfmt structures assume that the array of codes is
> >>>> terminated by a 0 elements. This mechanism is fragile, as demonstrated
> >>>
> >>> elements -> element
> >>>
> >>>> by several instances of the structure contained 4 non-zero codes.
> >>>
> >>> contained -> that contained
> >>
> >> Will fix above language.
> >>
> >>>> Fix this by turning the array into a pointer, and providing an
> >>>> IMX_BUS_FMTS macro to initialize the codes member with a guaranteed 0
> >>>> element at the end.
> >>>>
> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>
> >>>> [Fix a NULL deref when derefencing a NULL cc->codes on return from
> >>>> several calls to imx_media_find_format()]
> >>>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> >>>> ---
> >>>> drivers/staging/media/imx/imx-media-capture.c | 4 +-
> >>>> drivers/staging/media/imx/imx-media-utils.c | 88 +++++++++++--------
> >>>> drivers/staging/media/imx/imx-media.h | 2 +-
> >>>> 3 files changed, 53 insertions(+), 41 deletions(-)
> >>>>
> >>>> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> >>>> index d60b49ec4fa4..650c53289f6b 100644
> >>>> --- a/drivers/staging/media/imx/imx-media-capture.c
> >>>> +++ b/drivers/staging/media/imx/imx-media-capture.c
> >>>> @@ -95,7 +95,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
> >>>> if (!cc)
> >>>> return -EINVAL;
> >>>>
> >>>> - fse.code = cc->codes[0];
> >>>> + fse.code = cc->codes ? cc->codes[0] : 0;
> >>>>
> >>> I'm wondering: wouldn't it be better to have a format without codes point to
> >>> an empty code list containing just 0? That would avoid all the cc->codes checks,
> >>> which are a bit fragile IMHO.
> >>
> >> I'll modify this patch to declare a const codes array 'no_bus_fmts',
> >> containing a single 0 value, that the in-memory-only imx_media_pixfmt
> >> entries can point to.
> >>
> >>> It would be nice in that case if there is a WARN_ON or something during probe
> >>> time that checks that the codes field in pixel_formats[] is never NULL.
> >>
> >> I'm not so keen on that idea. How about a comment that the
> >> in-memory-only entries should point to no_bus_fmts ?
> >
> > I don't like either of those options I'm afraid. Someone will one day
> > forget to point to that array when adding a new format, and we'll have a
> > bug. With code in just a few locations that checks for NULL, we're safe
> > as the compiler will initialize the pointer to NULL for us. Let's use
> > the compiler when it helps us, I don't see what removing the NULL check
> > in the code could bring us except issues :-)
>
> I wasn't planning on removing the NULL checks, only to add the comment
> to advise setting .codes = no_bus_fmts. Although I don't really think
> that's necessary either.
But if we keep the NULL checks, do we need no_bus_fmts ?
As Hans doesn't consider this as a blocker, I'll let you decice which
option you like best and use that.
> >>> As an aside: 'cc'? The struct is called imx_media_pixfmt, so I don't offhand see
> >>> what 'cc' stands for.
> >> It's an abbreviation of FourCC. I know, it's obscure and not really
> >> accurate. I will add a non-functional patch that makes the naming
> >> consistent, using 'fmt' for imx_media_pixfmt pointers throughout.
> >>
> >>>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
> >>>> if (ret)
> >>>> @@ -137,7 +137,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
> >>>> if (!cc)
> >>>> return -EINVAL;
> >>>>
> >>>> - fie.code = cc->codes[0];
> >>>> + fie.code = cc->codes ? cc->codes[0] : 0;
> >>>>
> >>>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval,
> >>>> NULL, &fie);
> >>>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> >>>> index 981a8b540a3c..da010eef0ae6 100644
> >>>> --- a/drivers/staging/media/imx/imx-media-utils.c
> >>>> +++ b/drivers/staging/media/imx/imx-media-utils.c
> >>>> @@ -7,6 +7,12 @@
> >>>> #include <linux/module.h>
> >>>> #include "imx-media.h"
> >>>>
> >>>> +#define IMX_BUS_FMTS(fmts...) \
> >>>> + (const u32[]) { \
> >>>> + fmts, \
> >>>> + 0 \
> >>>> + }
> >>>> +
> >>>> /*
> >>>> * List of supported pixel formats for the subdevs.
> >>>> */
> >>>> @@ -14,18 +20,18 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >>>> /*** YUV formats start here ***/
> >>>> {
> >>>> .fourcc = V4L2_PIX_FMT_UYVY,
> >>>> - .codes = {
> >>>> + .codes = IMX_BUS_FMTS(
> >>>> MEDIA_BUS_FMT_UYVY8_2X8,
> >>>> MEDIA_BUS_FMT_UYVY8_1X16
> >>>> - },
> >>>> + ),
> >>>> .cs = IPUV3_COLORSPACE_YUV,
> >>>> .bpp = 16,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_YUYV,
> >>>> - .codes = {
> >>>> + .codes = IMX_BUS_FMTS(
> >>>> MEDIA_BUS_FMT_YUYV8_2X8,
> >>>> MEDIA_BUS_FMT_YUYV8_1X16
> >>>> - },
> >>>> + ),
> >>>> .cs = IPUV3_COLORSPACE_YUV,
> >>>> .bpp = 16,
> >>>> }, {
> >>>> @@ -57,16 +63,16 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >>>> /*** RGB formats start here ***/
> >>>> {
> >>>> .fourcc = V4L2_PIX_FMT_RGB565,
> >>>> - .codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
> >>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 16,
> >>>> .cycles = 2,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_RGB24,
> >>>> - .codes = {
> >>>> + .codes = IMX_BUS_FMTS(
> >>>> MEDIA_BUS_FMT_RGB888_1X24,
> >>>> MEDIA_BUS_FMT_RGB888_2X12_LE
> >>>> - },
> >>>> + ),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 24,
> >>>> }, {
> >>>> @@ -75,7 +81,7 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >>>> .bpp = 24,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_XRGB32,
> >>>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> >>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 32,
> >>>> .ipufmt = true,
> >>>> @@ -95,91 +101,91 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >>>> /*** raw bayer and grayscale formats start here ***/
> >>>> {
> >>>> .fourcc = V4L2_PIX_FMT_SBGGR8,
> >>>> - .codes = {MEDIA_BUS_FMT_SBGGR8_1X8},
> >>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 8,
> >>>> .bayer = true,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_SGBRG8,
> >>>> - .codes = {MEDIA_BUS_FMT_SGBRG8_1X8},
> >>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 8,
> >>>> .bayer = true,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_SGRBG8,
> >>>> - .codes = {MEDIA_BUS_FMT_SGRBG8_1X8},
> >>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 8,
> >>>> .bayer = true,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_SRGGB8,
> >>>> - .codes = {MEDIA_BUS_FMT_SRGGB8_1X8},
> >>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 8,
> >>>> .bayer = true,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_SBGGR16,
> >>>> - .codes = {
> >>>> + .codes = IMX_BUS_FMTS(
> >>>> MEDIA_BUS_FMT_SBGGR10_1X10,
> >>>> MEDIA_BUS_FMT_SBGGR12_1X12,
> >>>> MEDIA_BUS_FMT_SBGGR14_1X14,
> >>>> MEDIA_BUS_FMT_SBGGR16_1X16
> >>>> - },
> >>>> + ),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 16,
> >>>> .bayer = true,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_SGBRG16,
> >>>> - .codes = {
> >>>> + .codes = IMX_BUS_FMTS(
> >>>> MEDIA_BUS_FMT_SGBRG10_1X10,
> >>>> MEDIA_BUS_FMT_SGBRG12_1X12,
> >>>> MEDIA_BUS_FMT_SGBRG14_1X14,
> >>>> - MEDIA_BUS_FMT_SGBRG16_1X16,
> >>>> - },
> >>>> + MEDIA_BUS_FMT_SGBRG16_1X16
> >>>> + ),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 16,
> >>>> .bayer = true,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_SGRBG16,
> >>>> - .codes = {
> >>>> + .codes = IMX_BUS_FMTS(
> >>>> MEDIA_BUS_FMT_SGRBG10_1X10,
> >>>> MEDIA_BUS_FMT_SGRBG12_1X12,
> >>>> MEDIA_BUS_FMT_SGRBG14_1X14,
> >>>> - MEDIA_BUS_FMT_SGRBG16_1X16,
> >>>> - },
> >>>> + MEDIA_BUS_FMT_SGRBG16_1X16
> >>>> + ),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 16,
> >>>> .bayer = true,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_SRGGB16,
> >>>> - .codes = {
> >>>> + .codes = IMX_BUS_FMTS(
> >>>> MEDIA_BUS_FMT_SRGGB10_1X10,
> >>>> MEDIA_BUS_FMT_SRGGB12_1X12,
> >>>> MEDIA_BUS_FMT_SRGGB14_1X14,
> >>>> - MEDIA_BUS_FMT_SRGGB16_1X16,
> >>>> - },
> >>>> + MEDIA_BUS_FMT_SRGGB16_1X16
> >>>> + ),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 16,
> >>>> .bayer = true,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_GREY,
> >>>> - .codes = {
> >>>> + .codes = IMX_BUS_FMTS(
> >>>> MEDIA_BUS_FMT_Y8_1X8,
> >>>> MEDIA_BUS_FMT_Y10_1X10,
> >>>> - MEDIA_BUS_FMT_Y12_1X12,
> >>>> - },
> >>>> + MEDIA_BUS_FMT_Y12_1X12
> >>>> + ),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 8,
> >>>> .bayer = true,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_Y10,
> >>>> - .codes = {MEDIA_BUS_FMT_Y10_1X10},
> >>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y10_1X10),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 16,
> >>>> .bayer = true,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_Y12,
> >>>> - .codes = {MEDIA_BUS_FMT_Y12_1X12},
> >>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y12_1X12),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 16,
> >>>> .bayer = true,
> >>>> @@ -203,16 +209,16 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
> >>>> CS_SEL_YUV : CS_SEL_RGB);
> >>>>
> >>>> if (!(fmt_cs_sel & cs_sel) ||
> >>>> - (!allow_non_mbus && !fmt->codes[0]))
> >>>> + (!allow_non_mbus && !fmt->codes))
> >>>> continue;
> >>>>
> >>>> if (fourcc && fmt->fourcc == fourcc)
> >>>> return fmt;
> >>>>
> >>>> - if (!code)
> >>>> + if (!code || !fmt->codes)
> >>>> continue;
> >>>>
> >>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >>>> + for (j = 0; fmt->codes[j]; j++) {
> >>>> if (code == fmt->codes[j])
> >>>> return fmt;
> >>>> }
> >>>> @@ -237,7 +243,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
> >>>> CS_SEL_YUV : CS_SEL_RGB);
> >>>>
> >>>> if (!(fmt_cs_sel & cs_sel) ||
> >>>> - (!allow_non_mbus && !fmt->codes[0]))
> >>>> + (!allow_non_mbus && !fmt->codes))
> >>>> continue;
> >>>>
> >>>> if (fourcc && index == 0) {
> >>>> @@ -250,7 +256,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
> >>>> continue;
> >>>> }
> >>>>
> >>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >>>> + for (j = 0; fmt->codes[j]; j++) {
> >>>> if (index == 0) {
> >>>> *code = fmt->codes[j];
> >>>> return 0;
> >>>> @@ -296,13 +302,13 @@ EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
> >>>> static const struct imx_media_pixfmt ipu_formats[] = {
> >>>> {
> >>>> .fourcc = V4L2_PIX_FMT_YUV32,
> >>>> - .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
> >>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_AYUV8_1X32),
> >>>> .cs = IPUV3_COLORSPACE_YUV,
> >>>> .bpp = 32,
> >>>> .ipufmt = true,
> >>>> }, {
> >>>> .fourcc = V4L2_PIX_FMT_XRGB32,
> >>>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> >>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
> >>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>> .bpp = 32,
> >>>> .ipufmt = true,
> >>>> @@ -327,7 +333,10 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
> >>>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
> >>>> continue;
> >>>>
> >>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >>>> + if (!fmt->codes)
> >>>> + continue;
> >>>> +
> >>>> + for (j = 0; fmt->codes[j]; j++) {
> >>>> if (code == fmt->codes[j])
> >>>> return fmt;
> >>>> }
> >>>> @@ -351,7 +360,10 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
> >>>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
> >>>> continue;
> >>>>
> >>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >>>> + if (!fmt->codes)
> >>>> + continue;
> >>>> +
> >>>> + for (j = 0; fmt->codes[j]; j++) {
> >>>> if (index == 0) {
> >>>> *code = fmt->codes[j];
> >>>> return 0;
> >>>> @@ -567,7 +579,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
> >>>> const struct imx_media_pixfmt *fmt;
> >>>>
> >>>> fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
> >>>> - if (!fmt)
> >>>> + if (!fmt || !fmt->codes)
> >>>> return -EINVAL;
> >>>>
> >>>> memset(mbus, 0, sizeof(*mbus));
> >>>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> >>>> index 652673a703cd..917b4db02985 100644
> >>>> --- a/drivers/staging/media/imx/imx-media.h
> >>>> +++ b/drivers/staging/media/imx/imx-media.h
> >>>> @@ -69,7 +69,7 @@ enum {
> >>>>
> >>>> struct imx_media_pixfmt {
> >>>> u32 fourcc;
> >>>> - u32 codes[4];
> >>>> + const u32 *codes;
> >>>> int bpp; /* total bpp */
> >>>> /* cycles per pixel for generic (bayer) formats for the parallel bus */
> >>>> int cycles;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes
2020-04-01 15:31 ` Laurent Pinchart
@ 2020-04-03 22:29 ` Steve Longerbeam
2020-04-03 22:38 ` Laurent Pinchart
0 siblings, 1 reply; 20+ messages in thread
From: Steve Longerbeam @ 2020-04-03 22:29 UTC (permalink / raw)
To: Laurent Pinchart, Hans Verkuil
Cc: linux-media, Philipp Zabel, Rui Miguel Silva
Hi Hans, Laurent,
On 4/1/20 8:31 AM, Laurent Pinchart wrote:
> On Wed, Apr 01, 2020 at 08:20:59AM -0700, Steve Longerbeam wrote:
>> On 3/31/20 5:31 PM, Laurent Pinchart wrote:
>>> On Tue, Mar 31, 2020 at 04:04:18PM -0700, Steve Longerbeam wrote:
>>>> On 3/31/20 6:45 AM, Hans Verkuil wrote:
>>>>> On 3/29/20 10:59 PM, Steve Longerbeam wrote:
>>>>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>>>
>>>>>> The imx_media_pixfmt structures includes a codes member that stores
>>>>>> media bus codes as a fixed array of 4 integers. The functions dealing
>>>>>> with the imx_media_pixfmt structures assume that the array of codes is
>>>>>> terminated by a 0 elements. This mechanism is fragile, as demonstrated
>>>>> elements -> element
>>>>>
>>>>>> by several instances of the structure contained 4 non-zero codes.
>>>>> contained -> that contained
>>>> Will fix above language.
Oops, in my v5 just posted, I forgot to fix above text. Should I just
resubmit this one patch or resnd the whole series again as v6?
>>>>
>>>>>> Fix this by turning the array into a pointer, and providing an
>>>>>> IMX_BUS_FMTS macro to initialize the codes member with a guaranteed 0
>>>>>> element at the end.
>>>>>>
>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>>>>>
>>>>>> [Fix a NULL deref when derefencing a NULL cc->codes on return from
>>>>>> several calls to imx_media_find_format()]
>>>>>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>>>>>> ---
>>>>>> drivers/staging/media/imx/imx-media-capture.c | 4 +-
>>>>>> drivers/staging/media/imx/imx-media-utils.c | 88 +++++++++++--------
>>>>>> drivers/staging/media/imx/imx-media.h | 2 +-
>>>>>> 3 files changed, 53 insertions(+), 41 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
>>>>>> index d60b49ec4fa4..650c53289f6b 100644
>>>>>> --- a/drivers/staging/media/imx/imx-media-capture.c
>>>>>> +++ b/drivers/staging/media/imx/imx-media-capture.c
>>>>>> @@ -95,7 +95,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
>>>>>> if (!cc)
>>>>>> return -EINVAL;
>>>>>>
>>>>>> - fse.code = cc->codes[0];
>>>>>> + fse.code = cc->codes ? cc->codes[0] : 0;
>>>>>>
>>>>> I'm wondering: wouldn't it be better to have a format without codes point to
>>>>> an empty code list containing just 0? That would avoid all the cc->codes checks,
>>>>> which are a bit fragile IMHO.
>>>> I'll modify this patch to declare a const codes array 'no_bus_fmts',
>>>> containing a single 0 value, that the in-memory-only imx_media_pixfmt
>>>> entries can point to.
>>>>
>>>>> It would be nice in that case if there is a WARN_ON or something during probe
>>>>> time that checks that the codes field in pixel_formats[] is never NULL.
>>>> I'm not so keen on that idea. How about a comment that the
>>>> in-memory-only entries should point to no_bus_fmts ?
>>> I don't like either of those options I'm afraid. Someone will one day
>>> forget to point to that array when adding a new format, and we'll have a
>>> bug. With code in just a few locations that checks for NULL, we're safe
>>> as the compiler will initialize the pointer to NULL for us. Let's use
>>> the compiler when it helps us, I don't see what removing the NULL check
>>> in the code could bring us except issues :-)
>> I wasn't planning on removing the NULL checks, only to add the comment
>> to advise setting .codes = no_bus_fmts. Although I don't really think
>> that's necessary either.
> But if we keep the NULL checks, do we need no_bus_fmts ?
Correct, no_bus_fmts is not really needed.
>
> As Hans doesn't consider this as a blocker, I'll let you decice which
> option you like best and use that.
I've decided in the v5 series just posted, to add a NOTE! comment in the
description of the struct imx_media_pixfmt members, warning that codes
pointer is NULL for the in-memory-only formats.
>
>>>>> As an aside: 'cc'? The struct is called imx_media_pixfmt, so I don't offhand see
>>>>> what 'cc' stands for.
>>>> It's an abbreviation of FourCC. I know, it's obscure and not really
>>>> accurate. I will add a non-functional patch that makes the naming
>>>> consistent, using 'fmt' for imx_media_pixfmt pointers throughout.
Hans, I decided to skip this for now, if really desired I can send a
patch later.
Steve
>>>>
>>>>>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
>>>>>> if (ret)
>>>>>> @@ -137,7 +137,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
>>>>>> if (!cc)
>>>>>> return -EINVAL;
>>>>>>
>>>>>> - fie.code = cc->codes[0];
>>>>>> + fie.code = cc->codes ? cc->codes[0] : 0;
>>>>>>
>>>>>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval,
>>>>>> NULL, &fie);
>>>>>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
>>>>>> index 981a8b540a3c..da010eef0ae6 100644
>>>>>> --- a/drivers/staging/media/imx/imx-media-utils.c
>>>>>> +++ b/drivers/staging/media/imx/imx-media-utils.c
>>>>>> @@ -7,6 +7,12 @@
>>>>>> #include <linux/module.h>
>>>>>> #include "imx-media.h"
>>>>>>
>>>>>> +#define IMX_BUS_FMTS(fmts...) \
>>>>>> + (const u32[]) { \
>>>>>> + fmts, \
>>>>>> + 0 \
>>>>>> + }
>>>>>> +
>>>>>> /*
>>>>>> * List of supported pixel formats for the subdevs.
>>>>>> */
>>>>>> @@ -14,18 +20,18 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>>>> /*** YUV formats start here ***/
>>>>>> {
>>>>>> .fourcc = V4L2_PIX_FMT_UYVY,
>>>>>> - .codes = {
>>>>>> + .codes = IMX_BUS_FMTS(
>>>>>> MEDIA_BUS_FMT_UYVY8_2X8,
>>>>>> MEDIA_BUS_FMT_UYVY8_1X16
>>>>>> - },
>>>>>> + ),
>>>>>> .cs = IPUV3_COLORSPACE_YUV,
>>>>>> .bpp = 16,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_YUYV,
>>>>>> - .codes = {
>>>>>> + .codes = IMX_BUS_FMTS(
>>>>>> MEDIA_BUS_FMT_YUYV8_2X8,
>>>>>> MEDIA_BUS_FMT_YUYV8_1X16
>>>>>> - },
>>>>>> + ),
>>>>>> .cs = IPUV3_COLORSPACE_YUV,
>>>>>> .bpp = 16,
>>>>>> }, {
>>>>>> @@ -57,16 +63,16 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>>>> /*** RGB formats start here ***/
>>>>>> {
>>>>>> .fourcc = V4L2_PIX_FMT_RGB565,
>>>>>> - .codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
>>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 16,
>>>>>> .cycles = 2,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_RGB24,
>>>>>> - .codes = {
>>>>>> + .codes = IMX_BUS_FMTS(
>>>>>> MEDIA_BUS_FMT_RGB888_1X24,
>>>>>> MEDIA_BUS_FMT_RGB888_2X12_LE
>>>>>> - },
>>>>>> + ),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 24,
>>>>>> }, {
>>>>>> @@ -75,7 +81,7 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>>>> .bpp = 24,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_XRGB32,
>>>>>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
>>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 32,
>>>>>> .ipufmt = true,
>>>>>> @@ -95,91 +101,91 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>>>>>> /*** raw bayer and grayscale formats start here ***/
>>>>>> {
>>>>>> .fourcc = V4L2_PIX_FMT_SBGGR8,
>>>>>> - .codes = {MEDIA_BUS_FMT_SBGGR8_1X8},
>>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 8,
>>>>>> .bayer = true,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_SGBRG8,
>>>>>> - .codes = {MEDIA_BUS_FMT_SGBRG8_1X8},
>>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 8,
>>>>>> .bayer = true,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_SGRBG8,
>>>>>> - .codes = {MEDIA_BUS_FMT_SGRBG8_1X8},
>>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 8,
>>>>>> .bayer = true,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_SRGGB8,
>>>>>> - .codes = {MEDIA_BUS_FMT_SRGGB8_1X8},
>>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 8,
>>>>>> .bayer = true,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_SBGGR16,
>>>>>> - .codes = {
>>>>>> + .codes = IMX_BUS_FMTS(
>>>>>> MEDIA_BUS_FMT_SBGGR10_1X10,
>>>>>> MEDIA_BUS_FMT_SBGGR12_1X12,
>>>>>> MEDIA_BUS_FMT_SBGGR14_1X14,
>>>>>> MEDIA_BUS_FMT_SBGGR16_1X16
>>>>>> - },
>>>>>> + ),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 16,
>>>>>> .bayer = true,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_SGBRG16,
>>>>>> - .codes = {
>>>>>> + .codes = IMX_BUS_FMTS(
>>>>>> MEDIA_BUS_FMT_SGBRG10_1X10,
>>>>>> MEDIA_BUS_FMT_SGBRG12_1X12,
>>>>>> MEDIA_BUS_FMT_SGBRG14_1X14,
>>>>>> - MEDIA_BUS_FMT_SGBRG16_1X16,
>>>>>> - },
>>>>>> + MEDIA_BUS_FMT_SGBRG16_1X16
>>>>>> + ),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 16,
>>>>>> .bayer = true,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_SGRBG16,
>>>>>> - .codes = {
>>>>>> + .codes = IMX_BUS_FMTS(
>>>>>> MEDIA_BUS_FMT_SGRBG10_1X10,
>>>>>> MEDIA_BUS_FMT_SGRBG12_1X12,
>>>>>> MEDIA_BUS_FMT_SGRBG14_1X14,
>>>>>> - MEDIA_BUS_FMT_SGRBG16_1X16,
>>>>>> - },
>>>>>> + MEDIA_BUS_FMT_SGRBG16_1X16
>>>>>> + ),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 16,
>>>>>> .bayer = true,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_SRGGB16,
>>>>>> - .codes = {
>>>>>> + .codes = IMX_BUS_FMTS(
>>>>>> MEDIA_BUS_FMT_SRGGB10_1X10,
>>>>>> MEDIA_BUS_FMT_SRGGB12_1X12,
>>>>>> MEDIA_BUS_FMT_SRGGB14_1X14,
>>>>>> - MEDIA_BUS_FMT_SRGGB16_1X16,
>>>>>> - },
>>>>>> + MEDIA_BUS_FMT_SRGGB16_1X16
>>>>>> + ),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 16,
>>>>>> .bayer = true,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_GREY,
>>>>>> - .codes = {
>>>>>> + .codes = IMX_BUS_FMTS(
>>>>>> MEDIA_BUS_FMT_Y8_1X8,
>>>>>> MEDIA_BUS_FMT_Y10_1X10,
>>>>>> - MEDIA_BUS_FMT_Y12_1X12,
>>>>>> - },
>>>>>> + MEDIA_BUS_FMT_Y12_1X12
>>>>>> + ),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 8,
>>>>>> .bayer = true,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_Y10,
>>>>>> - .codes = {MEDIA_BUS_FMT_Y10_1X10},
>>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y10_1X10),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 16,
>>>>>> .bayer = true,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_Y12,
>>>>>> - .codes = {MEDIA_BUS_FMT_Y12_1X12},
>>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y12_1X12),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 16,
>>>>>> .bayer = true,
>>>>>> @@ -203,16 +209,16 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
>>>>>> CS_SEL_YUV : CS_SEL_RGB);
>>>>>>
>>>>>> if (!(fmt_cs_sel & cs_sel) ||
>>>>>> - (!allow_non_mbus && !fmt->codes[0]))
>>>>>> + (!allow_non_mbus && !fmt->codes))
>>>>>> continue;
>>>>>>
>>>>>> if (fourcc && fmt->fourcc == fourcc)
>>>>>> return fmt;
>>>>>>
>>>>>> - if (!code)
>>>>>> + if (!code || !fmt->codes)
>>>>>> continue;
>>>>>>
>>>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>>>> if (code == fmt->codes[j])
>>>>>> return fmt;
>>>>>> }
>>>>>> @@ -237,7 +243,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>>>>>> CS_SEL_YUV : CS_SEL_RGB);
>>>>>>
>>>>>> if (!(fmt_cs_sel & cs_sel) ||
>>>>>> - (!allow_non_mbus && !fmt->codes[0]))
>>>>>> + (!allow_non_mbus && !fmt->codes))
>>>>>> continue;
>>>>>>
>>>>>> if (fourcc && index == 0) {
>>>>>> @@ -250,7 +256,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
>>>>>> continue;
>>>>>> }
>>>>>>
>>>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>>>> if (index == 0) {
>>>>>> *code = fmt->codes[j];
>>>>>> return 0;
>>>>>> @@ -296,13 +302,13 @@ EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
>>>>>> static const struct imx_media_pixfmt ipu_formats[] = {
>>>>>> {
>>>>>> .fourcc = V4L2_PIX_FMT_YUV32,
>>>>>> - .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
>>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_AYUV8_1X32),
>>>>>> .cs = IPUV3_COLORSPACE_YUV,
>>>>>> .bpp = 32,
>>>>>> .ipufmt = true,
>>>>>> }, {
>>>>>> .fourcc = V4L2_PIX_FMT_XRGB32,
>>>>>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
>>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
>>>>>> .cs = IPUV3_COLORSPACE_RGB,
>>>>>> .bpp = 32,
>>>>>> .ipufmt = true,
>>>>>> @@ -327,7 +333,10 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
>>>>>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
>>>>>> continue;
>>>>>>
>>>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>>>> + if (!fmt->codes)
>>>>>> + continue;
>>>>>> +
>>>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>>>> if (code == fmt->codes[j])
>>>>>> return fmt;
>>>>>> }
>>>>>> @@ -351,7 +360,10 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
>>>>>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
>>>>>> continue;
>>>>>>
>>>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
>>>>>> + if (!fmt->codes)
>>>>>> + continue;
>>>>>> +
>>>>>> + for (j = 0; fmt->codes[j]; j++) {
>>>>>> if (index == 0) {
>>>>>> *code = fmt->codes[j];
>>>>>> return 0;
>>>>>> @@ -567,7 +579,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
>>>>>> const struct imx_media_pixfmt *fmt;
>>>>>>
>>>>>> fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
>>>>>> - if (!fmt)
>>>>>> + if (!fmt || !fmt->codes)
>>>>>> return -EINVAL;
>>>>>>
>>>>>> memset(mbus, 0, sizeof(*mbus));
>>>>>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
>>>>>> index 652673a703cd..917b4db02985 100644
>>>>>> --- a/drivers/staging/media/imx/imx-media.h
>>>>>> +++ b/drivers/staging/media/imx/imx-media.h
>>>>>> @@ -69,7 +69,7 @@ enum {
>>>>>>
>>>>>> struct imx_media_pixfmt {
>>>>>> u32 fourcc;
>>>>>> - u32 codes[4];
>>>>>> + const u32 *codes;
>>>>>> int bpp; /* total bpp */
>>>>>> /* cycles per pixel for generic (bayer) formats for the parallel bus */
>>>>>> int cycles;
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes
2020-04-03 22:29 ` Steve Longerbeam
@ 2020-04-03 22:38 ` Laurent Pinchart
0 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2020-04-03 22:38 UTC (permalink / raw)
To: Steve Longerbeam
Cc: Hans Verkuil, linux-media, Philipp Zabel, Rui Miguel Silva
Hi Steve,
On Fri, Apr 03, 2020 at 03:29:51PM -0700, Steve Longerbeam wrote:
> On 4/1/20 8:31 AM, Laurent Pinchart wrote:
> > On Wed, Apr 01, 2020 at 08:20:59AM -0700, Steve Longerbeam wrote:
> >> On 3/31/20 5:31 PM, Laurent Pinchart wrote:
> >>> On Tue, Mar 31, 2020 at 04:04:18PM -0700, Steve Longerbeam wrote:
> >>>> On 3/31/20 6:45 AM, Hans Verkuil wrote:
> >>>>> On 3/29/20 10:59 PM, Steve Longerbeam wrote:
> >>>>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>>>
> >>>>>> The imx_media_pixfmt structures includes a codes member that stores
> >>>>>> media bus codes as a fixed array of 4 integers. The functions dealing
> >>>>>> with the imx_media_pixfmt structures assume that the array of codes is
> >>>>>> terminated by a 0 elements. This mechanism is fragile, as demonstrated
> >>>>> elements -> element
> >>>>>
> >>>>>> by several instances of the structure contained 4 non-zero codes.
> >>>>> contained -> that contained
> >>>> Will fix above language.
>
> Oops, in my v5 just posted, I forgot to fix above text. Should I just
> resubmit this one patch or resnd the whole series again as v6?
When that happens, I usually send a v5.1 for just that patch, in a reply
to the original v5 patch.
> >>>>>> Fix this by turning the array into a pointer, and providing an
> >>>>>> IMX_BUS_FMTS macro to initialize the codes member with a guaranteed 0
> >>>>>> element at the end.
> >>>>>>
> >>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>>>
> >>>>>> [Fix a NULL deref when derefencing a NULL cc->codes on return from
> >>>>>> several calls to imx_media_find_format()]
> >>>>>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> >>>>>> ---
> >>>>>> drivers/staging/media/imx/imx-media-capture.c | 4 +-
> >>>>>> drivers/staging/media/imx/imx-media-utils.c | 88 +++++++++++--------
> >>>>>> drivers/staging/media/imx/imx-media.h | 2 +-
> >>>>>> 3 files changed, 53 insertions(+), 41 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> >>>>>> index d60b49ec4fa4..650c53289f6b 100644
> >>>>>> --- a/drivers/staging/media/imx/imx-media-capture.c
> >>>>>> +++ b/drivers/staging/media/imx/imx-media-capture.c
> >>>>>> @@ -95,7 +95,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
> >>>>>> if (!cc)
> >>>>>> return -EINVAL;
> >>>>>>
> >>>>>> - fse.code = cc->codes[0];
> >>>>>> + fse.code = cc->codes ? cc->codes[0] : 0;
> >>>>>>
> >>>>>
> >>>>> I'm wondering: wouldn't it be better to have a format without codes point to
> >>>>> an empty code list containing just 0? That would avoid all the cc->codes checks,
> >>>>> which are a bit fragile IMHO.
> >>>>
> >>>> I'll modify this patch to declare a const codes array 'no_bus_fmts',
> >>>> containing a single 0 value, that the in-memory-only imx_media_pixfmt
> >>>> entries can point to.
> >>>>
> >>>>> It would be nice in that case if there is a WARN_ON or something during probe
> >>>>> time that checks that the codes field in pixel_formats[] is never NULL.
> >>>>
> >>>> I'm not so keen on that idea. How about a comment that the
> >>>> in-memory-only entries should point to no_bus_fmts ?
> >>>
> >>> I don't like either of those options I'm afraid. Someone will one day
> >>> forget to point to that array when adding a new format, and we'll have a
> >>> bug. With code in just a few locations that checks for NULL, we're safe
> >>> as the compiler will initialize the pointer to NULL for us. Let's use
> >>> the compiler when it helps us, I don't see what removing the NULL check
> >>> in the code could bring us except issues :-)
> >>
> >> I wasn't planning on removing the NULL checks, only to add the comment
> >> to advise setting .codes = no_bus_fmts. Although I don't really think
> >> that's necessary either.
> >
> > But if we keep the NULL checks, do we need no_bus_fmts ?
>
> Correct, no_bus_fmts is not really needed.
>
> > As Hans doesn't consider this as a blocker, I'll let you decice which
> > option you like best and use that.
>
> I've decided in the v5 series just posted, to add a NOTE! comment in the
> description of the struct imx_media_pixfmt members, warning that codes
> pointer is NULL for the in-memory-only formats.
>
> >>>>> As an aside: 'cc'? The struct is called imx_media_pixfmt, so I don't offhand see
> >>>>> what 'cc' stands for.
> >>>>
> >>>> It's an abbreviation of FourCC. I know, it's obscure and not really
> >>>> accurate. I will add a non-functional patch that makes the naming
> >>>> consistent, using 'fmt' for imx_media_pixfmt pointers throughout.
>
> Hans, I decided to skip this for now, if really desired I can send a
> patch later.
>
> >>>>>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_size, NULL, &fse);
> >>>>>> if (ret)
> >>>>>> @@ -137,7 +137,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
> >>>>>> if (!cc)
> >>>>>> return -EINVAL;
> >>>>>>
> >>>>>> - fie.code = cc->codes[0];
> >>>>>> + fie.code = cc->codes ? cc->codes[0] : 0;
> >>>>>>
> >>>>>> ret = v4l2_subdev_call(priv->src_sd, pad, enum_frame_interval,
> >>>>>> NULL, &fie);
> >>>>>> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> >>>>>> index 981a8b540a3c..da010eef0ae6 100644
> >>>>>> --- a/drivers/staging/media/imx/imx-media-utils.c
> >>>>>> +++ b/drivers/staging/media/imx/imx-media-utils.c
> >>>>>> @@ -7,6 +7,12 @@
> >>>>>> #include <linux/module.h>
> >>>>>> #include "imx-media.h"
> >>>>>>
> >>>>>> +#define IMX_BUS_FMTS(fmts...) \
> >>>>>> + (const u32[]) { \
> >>>>>> + fmts, \
> >>>>>> + 0 \
> >>>>>> + }
> >>>>>> +
> >>>>>> /*
> >>>>>> * List of supported pixel formats for the subdevs.
> >>>>>> */
> >>>>>> @@ -14,18 +20,18 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >>>>>> /*** YUV formats start here ***/
> >>>>>> {
> >>>>>> .fourcc = V4L2_PIX_FMT_UYVY,
> >>>>>> - .codes = {
> >>>>>> + .codes = IMX_BUS_FMTS(
> >>>>>> MEDIA_BUS_FMT_UYVY8_2X8,
> >>>>>> MEDIA_BUS_FMT_UYVY8_1X16
> >>>>>> - },
> >>>>>> + ),
> >>>>>> .cs = IPUV3_COLORSPACE_YUV,
> >>>>>> .bpp = 16,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_YUYV,
> >>>>>> - .codes = {
> >>>>>> + .codes = IMX_BUS_FMTS(
> >>>>>> MEDIA_BUS_FMT_YUYV8_2X8,
> >>>>>> MEDIA_BUS_FMT_YUYV8_1X16
> >>>>>> - },
> >>>>>> + ),
> >>>>>> .cs = IPUV3_COLORSPACE_YUV,
> >>>>>> .bpp = 16,
> >>>>>> }, {
> >>>>>> @@ -57,16 +63,16 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >>>>>> /*** RGB formats start here ***/
> >>>>>> {
> >>>>>> .fourcc = V4L2_PIX_FMT_RGB565,
> >>>>>> - .codes = {MEDIA_BUS_FMT_RGB565_2X8_LE},
> >>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_RGB565_2X8_LE),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 16,
> >>>>>> .cycles = 2,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_RGB24,
> >>>>>> - .codes = {
> >>>>>> + .codes = IMX_BUS_FMTS(
> >>>>>> MEDIA_BUS_FMT_RGB888_1X24,
> >>>>>> MEDIA_BUS_FMT_RGB888_2X12_LE
> >>>>>> - },
> >>>>>> + ),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 24,
> >>>>>> }, {
> >>>>>> @@ -75,7 +81,7 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >>>>>> .bpp = 24,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_XRGB32,
> >>>>>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> >>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 32,
> >>>>>> .ipufmt = true,
> >>>>>> @@ -95,91 +101,91 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> >>>>>> /*** raw bayer and grayscale formats start here ***/
> >>>>>> {
> >>>>>> .fourcc = V4L2_PIX_FMT_SBGGR8,
> >>>>>> - .codes = {MEDIA_BUS_FMT_SBGGR8_1X8},
> >>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR8_1X8),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 8,
> >>>>>> .bayer = true,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_SGBRG8,
> >>>>>> - .codes = {MEDIA_BUS_FMT_SGBRG8_1X8},
> >>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG8_1X8),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 8,
> >>>>>> .bayer = true,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_SGRBG8,
> >>>>>> - .codes = {MEDIA_BUS_FMT_SGRBG8_1X8},
> >>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG8_1X8),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 8,
> >>>>>> .bayer = true,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_SRGGB8,
> >>>>>> - .codes = {MEDIA_BUS_FMT_SRGGB8_1X8},
> >>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB8_1X8),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 8,
> >>>>>> .bayer = true,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_SBGGR16,
> >>>>>> - .codes = {
> >>>>>> + .codes = IMX_BUS_FMTS(
> >>>>>> MEDIA_BUS_FMT_SBGGR10_1X10,
> >>>>>> MEDIA_BUS_FMT_SBGGR12_1X12,
> >>>>>> MEDIA_BUS_FMT_SBGGR14_1X14,
> >>>>>> MEDIA_BUS_FMT_SBGGR16_1X16
> >>>>>> - },
> >>>>>> + ),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 16,
> >>>>>> .bayer = true,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_SGBRG16,
> >>>>>> - .codes = {
> >>>>>> + .codes = IMX_BUS_FMTS(
> >>>>>> MEDIA_BUS_FMT_SGBRG10_1X10,
> >>>>>> MEDIA_BUS_FMT_SGBRG12_1X12,
> >>>>>> MEDIA_BUS_FMT_SGBRG14_1X14,
> >>>>>> - MEDIA_BUS_FMT_SGBRG16_1X16,
> >>>>>> - },
> >>>>>> + MEDIA_BUS_FMT_SGBRG16_1X16
> >>>>>> + ),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 16,
> >>>>>> .bayer = true,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_SGRBG16,
> >>>>>> - .codes = {
> >>>>>> + .codes = IMX_BUS_FMTS(
> >>>>>> MEDIA_BUS_FMT_SGRBG10_1X10,
> >>>>>> MEDIA_BUS_FMT_SGRBG12_1X12,
> >>>>>> MEDIA_BUS_FMT_SGRBG14_1X14,
> >>>>>> - MEDIA_BUS_FMT_SGRBG16_1X16,
> >>>>>> - },
> >>>>>> + MEDIA_BUS_FMT_SGRBG16_1X16
> >>>>>> + ),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 16,
> >>>>>> .bayer = true,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_SRGGB16,
> >>>>>> - .codes = {
> >>>>>> + .codes = IMX_BUS_FMTS(
> >>>>>> MEDIA_BUS_FMT_SRGGB10_1X10,
> >>>>>> MEDIA_BUS_FMT_SRGGB12_1X12,
> >>>>>> MEDIA_BUS_FMT_SRGGB14_1X14,
> >>>>>> - MEDIA_BUS_FMT_SRGGB16_1X16,
> >>>>>> - },
> >>>>>> + MEDIA_BUS_FMT_SRGGB16_1X16
> >>>>>> + ),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 16,
> >>>>>> .bayer = true,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_GREY,
> >>>>>> - .codes = {
> >>>>>> + .codes = IMX_BUS_FMTS(
> >>>>>> MEDIA_BUS_FMT_Y8_1X8,
> >>>>>> MEDIA_BUS_FMT_Y10_1X10,
> >>>>>> - MEDIA_BUS_FMT_Y12_1X12,
> >>>>>> - },
> >>>>>> + MEDIA_BUS_FMT_Y12_1X12
> >>>>>> + ),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 8,
> >>>>>> .bayer = true,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_Y10,
> >>>>>> - .codes = {MEDIA_BUS_FMT_Y10_1X10},
> >>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y10_1X10),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 16,
> >>>>>> .bayer = true,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_Y12,
> >>>>>> - .codes = {MEDIA_BUS_FMT_Y12_1X12},
> >>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_Y12_1X12),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 16,
> >>>>>> .bayer = true,
> >>>>>> @@ -203,16 +209,16 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
> >>>>>> CS_SEL_YUV : CS_SEL_RGB);
> >>>>>>
> >>>>>> if (!(fmt_cs_sel & cs_sel) ||
> >>>>>> - (!allow_non_mbus && !fmt->codes[0]))
> >>>>>> + (!allow_non_mbus && !fmt->codes))
> >>>>>> continue;
> >>>>>>
> >>>>>> if (fourcc && fmt->fourcc == fourcc)
> >>>>>> return fmt;
> >>>>>>
> >>>>>> - if (!code)
> >>>>>> + if (!code || !fmt->codes)
> >>>>>> continue;
> >>>>>>
> >>>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >>>>>> + for (j = 0; fmt->codes[j]; j++) {
> >>>>>> if (code == fmt->codes[j])
> >>>>>> return fmt;
> >>>>>> }
> >>>>>> @@ -237,7 +243,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
> >>>>>> CS_SEL_YUV : CS_SEL_RGB);
> >>>>>>
> >>>>>> if (!(fmt_cs_sel & cs_sel) ||
> >>>>>> - (!allow_non_mbus && !fmt->codes[0]))
> >>>>>> + (!allow_non_mbus && !fmt->codes))
> >>>>>> continue;
> >>>>>>
> >>>>>> if (fourcc && index == 0) {
> >>>>>> @@ -250,7 +256,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
> >>>>>> continue;
> >>>>>> }
> >>>>>>
> >>>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >>>>>> + for (j = 0; fmt->codes[j]; j++) {
> >>>>>> if (index == 0) {
> >>>>>> *code = fmt->codes[j];
> >>>>>> return 0;
> >>>>>> @@ -296,13 +302,13 @@ EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
> >>>>>> static const struct imx_media_pixfmt ipu_formats[] = {
> >>>>>> {
> >>>>>> .fourcc = V4L2_PIX_FMT_YUV32,
> >>>>>> - .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
> >>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_AYUV8_1X32),
> >>>>>> .cs = IPUV3_COLORSPACE_YUV,
> >>>>>> .bpp = 32,
> >>>>>> .ipufmt = true,
> >>>>>> }, {
> >>>>>> .fourcc = V4L2_PIX_FMT_XRGB32,
> >>>>>> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> >>>>>> + .codes = IMX_BUS_FMTS(MEDIA_BUS_FMT_ARGB8888_1X32),
> >>>>>> .cs = IPUV3_COLORSPACE_RGB,
> >>>>>> .bpp = 32,
> >>>>>> .ipufmt = true,
> >>>>>> @@ -327,7 +333,10 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
> >>>>>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
> >>>>>> continue;
> >>>>>>
> >>>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >>>>>> + if (!fmt->codes)
> >>>>>> + continue;
> >>>>>> +
> >>>>>> + for (j = 0; fmt->codes[j]; j++) {
> >>>>>> if (code == fmt->codes[j])
> >>>>>> return fmt;
> >>>>>> }
> >>>>>> @@ -351,7 +360,10 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
> >>>>>> (!accept_rgb && fmt->cs == IPUV3_COLORSPACE_RGB))
> >>>>>> continue;
> >>>>>>
> >>>>>> - for (j = 0; j < ARRAY_SIZE(fmt->codes) && fmt->codes[j]; j++) {
> >>>>>> + if (!fmt->codes)
> >>>>>> + continue;
> >>>>>> +
> >>>>>> + for (j = 0; fmt->codes[j]; j++) {
> >>>>>> if (index == 0) {
> >>>>>> *code = fmt->codes[j];
> >>>>>> return 0;
> >>>>>> @@ -567,7 +579,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
> >>>>>> const struct imx_media_pixfmt *fmt;
> >>>>>>
> >>>>>> fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
> >>>>>> - if (!fmt)
> >>>>>> + if (!fmt || !fmt->codes)
> >>>>>> return -EINVAL;
> >>>>>>
> >>>>>> memset(mbus, 0, sizeof(*mbus));
> >>>>>> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> >>>>>> index 652673a703cd..917b4db02985 100644
> >>>>>> --- a/drivers/staging/media/imx/imx-media.h
> >>>>>> +++ b/drivers/staging/media/imx/imx-media.h
> >>>>>> @@ -69,7 +69,7 @@ enum {
> >>>>>>
> >>>>>> struct imx_media_pixfmt {
> >>>>>> u32 fourcc;
> >>>>>> - u32 codes[4];
> >>>>>> + const u32 *codes;
> >>>>>> int bpp; /* total bpp */
> >>>>>> /* cycles per pixel for generic (bayer) formats for the parallel bus */
> >>>>>> int cycles;
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4 07/11] media: imx: utils: Remove unneeded argument to (find|enum)_format()
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
` (5 preceding siblings ...)
2020-03-29 20:59 ` [PATCH v4 06/11] media: imx: utils: Make imx_media_pixfmt handle variable number of codes Steve Longerbeam
@ 2020-03-29 20:59 ` Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 08/11] media: imx: utils: Rename format lookup and enumeration functions Steve Longerbeam
` (3 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-29 20:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva,
Steve Longerbeam
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The find_format() and enum_format() functions take an argument that
tells whether to take into account formats that don't have associated
media bus codes. The same information can be deduced from the fourcc
argument passed to these functions. Remove the allow_non_mbus argument
and use fourcc instead internally.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/staging/media/imx/imx-media-utils.c | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index da010eef0ae6..bf187f6d87fe 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -194,8 +194,7 @@ static const struct imx_media_pixfmt pixel_formats[] = {
static const struct imx_media_pixfmt *find_format(u32 fourcc,
u32 code,
- enum codespace_sel cs_sel,
- bool allow_non_mbus)
+ enum codespace_sel cs_sel)
{
unsigned int i;
@@ -208,8 +207,7 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
((fmt->cs == IPUV3_COLORSPACE_YUV) ?
CS_SEL_YUV : CS_SEL_RGB);
- if (!(fmt_cs_sel & cs_sel) ||
- (!allow_non_mbus && !fmt->codes))
+ if (!(fmt_cs_sel & cs_sel) || (!fourcc && !fmt->codes))
continue;
if (fourcc && fmt->fourcc == fourcc)
@@ -228,8 +226,7 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
}
static int enum_format(u32 *fourcc, u32 *code, u32 index,
- enum codespace_sel cs_sel,
- bool allow_non_mbus)
+ enum codespace_sel cs_sel)
{
unsigned int i;
@@ -242,8 +239,7 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
((fmt->cs == IPUV3_COLORSPACE_YUV) ?
CS_SEL_YUV : CS_SEL_RGB);
- if (!(fmt_cs_sel & cs_sel) ||
- (!allow_non_mbus && !fmt->codes))
+ if (!(fmt_cs_sel & cs_sel) || (!fourcc && !fmt->codes))
continue;
if (fourcc && index == 0) {
@@ -272,26 +268,26 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
const struct imx_media_pixfmt *
imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel)
{
- return find_format(fourcc, 0, cs_sel, true);
+ return find_format(fourcc, 0, cs_sel);
}
EXPORT_SYMBOL_GPL(imx_media_find_format);
int imx_media_enum_format(u32 *fourcc, u32 index, enum codespace_sel cs_sel)
{
- return enum_format(fourcc, NULL, index, cs_sel, true);
+ return enum_format(fourcc, NULL, index, cs_sel);
}
EXPORT_SYMBOL_GPL(imx_media_enum_format);
const struct imx_media_pixfmt *
imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel)
{
- return find_format(0, code, cs_sel, false);
+ return find_format(0, code, cs_sel);
}
EXPORT_SYMBOL_GPL(imx_media_find_mbus_format);
int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel)
{
- return enum_format(NULL, code, index, cs_sel, false);
+ return enum_format(NULL, code, index, cs_sel);
}
EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 08/11] media: imx: utils: Rename format lookup and enumeration functions
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
` (6 preceding siblings ...)
2020-03-29 20:59 ` [PATCH v4 07/11] media: imx: utils: Remove unneeded argument to (find|enum)_format() Steve Longerbeam
@ 2020-03-29 20:59 ` Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 09/11] media: imx: utils: Constify mbus argument to imx_media_mbus_fmt_to_* Steve Longerbeam
` (2 subsequent siblings)
10 siblings, 0 replies; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-29 20:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva,
Steve Longerbeam
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Rename the format lookup and enumeration functions according to their
usage:
- Rename imx_media_(find|enum)_format() to *_pixel_format() to
explicitly state on what formats the functions operate. This aligns
the naming scheme with the media bus and IPU format functions that
already end with *_mbus_format() and *_ipu_formats().
- Rename all enumeration functions to pluralize 'formats' at the end, as
they enumerate multiple formats.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
drivers/staging/media/imx/imx-ic-prp.c | 8 ++---
drivers/staging/media/imx/imx-ic-prpencvf.c | 8 ++---
drivers/staging/media/imx/imx-media-capture.c | 16 +++++-----
.../staging/media/imx/imx-media-csc-scaler.c | 3 +-
drivers/staging/media/imx/imx-media-csi.c | 15 +++++----
drivers/staging/media/imx/imx-media-utils.c | 31 ++++++++++---------
drivers/staging/media/imx/imx-media-vdic.c | 6 ++--
drivers/staging/media/imx/imx-media.h | 11 ++++---
drivers/staging/media/imx/imx7-media-csi.c | 6 ++--
9 files changed, 54 insertions(+), 50 deletions(-)
diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.c
index 9c1f723972e4..50471541f4bc 100644
--- a/drivers/staging/media/imx/imx-ic-prp.c
+++ b/drivers/staging/media/imx/imx-ic-prp.c
@@ -106,8 +106,8 @@ static int prp_enum_mbus_code(struct v4l2_subdev *sd,
switch (code->pad) {
case PRP_SINK_PAD:
- ret = imx_media_enum_ipu_format(&code->code, code->index,
- CS_SEL_YUV_RGB);
+ ret = imx_media_enum_ipu_formats(&code->code, code->index,
+ CS_SEL_YUV_RGB);
break;
case PRP_SRC_PAD_PRPENC:
case PRP_SRC_PAD_PRPVF:
@@ -182,7 +182,7 @@ static int prp_set_fmt(struct v4l2_subdev *sd,
cc = imx_media_find_ipu_format(sdformat->format.code,
CS_SEL_YUV_RGB);
if (!cc) {
- imx_media_enum_ipu_format(&code, 0, CS_SEL_YUV);
+ imx_media_enum_ipu_formats(&code, 0, CS_SEL_YUV);
cc = imx_media_find_ipu_format(code, CS_SEL_YUV);
sdformat->format.code = cc->codes[0];
}
@@ -438,7 +438,7 @@ static int prp_registered(struct v4l2_subdev *sd)
priv->frame_interval.denominator = 30;
/* set a default mbus format */
- imx_media_enum_ipu_format(&code, 0, CS_SEL_YUV);
+ imx_media_enum_ipu_formats(&code, 0, CS_SEL_YUV);
return imx_media_init_mbus_fmt(&priv->format_mbus, 640, 480, code,
V4L2_FIELD_NONE, NULL);
}
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 5a22cdc7378a..003ace29ccaf 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -850,8 +850,8 @@ static int prp_enum_mbus_code(struct v4l2_subdev *sd,
if (code->pad >= PRPENCVF_NUM_PADS)
return -EINVAL;
- return imx_media_enum_ipu_format(&code->code, code->index,
- CS_SEL_YUV_RGB);
+ return imx_media_enum_ipu_formats(&code->code, code->index,
+ CS_SEL_YUV_RGB);
}
static int prp_get_fmt(struct v4l2_subdev *sd,
@@ -890,7 +890,7 @@ static void prp_try_fmt(struct prp_priv *priv,
if (!*cc) {
u32 code;
- imx_media_enum_ipu_format(&code, 0, CS_SEL_YUV);
+ imx_media_enum_ipu_formats(&code, 0, CS_SEL_YUV);
*cc = imx_media_find_ipu_format(code, CS_SEL_YUV);
sdformat->format.code = (*cc)->codes[0];
}
@@ -1249,7 +1249,7 @@ static int prp_registered(struct v4l2_subdev *sd)
u32 code;
/* set a default mbus format */
- imx_media_enum_ipu_format(&code, 0, CS_SEL_YUV);
+ imx_media_enum_ipu_formats(&code, 0, CS_SEL_YUV);
for (i = 0; i < PRPENCVF_NUM_PADS; i++) {
ret = imx_media_init_mbus_fmt(&priv->format_mbus[i],
640, 480, code, V4L2_FIELD_NONE,
diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index 650c53289f6b..386c61342234 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -91,7 +91,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,
};
int ret;
- cc = imx_media_find_format(fsize->pixel_format, CS_SEL_ANY);
+ cc = imx_media_find_pixel_format(fsize->pixel_format, CS_SEL_ANY);
if (!cc)
return -EINVAL;
@@ -133,7 +133,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,
};
int ret;
- cc = imx_media_find_format(fival->pixel_format, CS_SEL_ANY);
+ cc = imx_media_find_pixel_format(fival->pixel_format, CS_SEL_ANY);
if (!cc)
return -EINVAL;
@@ -172,7 +172,7 @@ static int capture_enum_fmt_vid_cap(struct file *file, void *fh,
u32 cs_sel = (cc_src->cs == IPUV3_COLORSPACE_YUV) ?
CS_SEL_YUV : CS_SEL_RGB;
- ret = imx_media_enum_format(&fourcc, f->index, cs_sel);
+ ret = imx_media_enum_pixel_formats(&fourcc, f->index, cs_sel);
if (ret)
return ret;
} else {
@@ -217,10 +217,10 @@ static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
CS_SEL_YUV : CS_SEL_RGB;
fourcc = f->fmt.pix.pixelformat;
- cc = imx_media_find_format(fourcc, cs_sel);
+ cc = imx_media_find_pixel_format(fourcc, cs_sel);
if (!cc) {
- imx_media_enum_format(&fourcc, 0, cs_sel);
- cc = imx_media_find_format(fourcc, cs_sel);
+ imx_media_enum_pixel_formats(&fourcc, 0, cs_sel);
+ cc = imx_media_find_pixel_format(fourcc, cs_sel);
}
} else {
cc_src = imx_media_find_mbus_format(fmt_src->format.code,
@@ -789,8 +789,8 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev)
&fmt_src.format, NULL);
vdev->compose.width = fmt_src.format.width;
vdev->compose.height = fmt_src.format.height;
- vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat,
- CS_SEL_ANY);
+ vdev->cc = imx_media_find_pixel_format(vdev->fmt.fmt.pix.pixelformat,
+ CS_SEL_ANY);
v4l2_info(sd, "Registered %s as /dev/%s\n", vfd->name,
video_device_node_name(vfd));
diff --git a/drivers/staging/media/imx/imx-media-csc-scaler.c b/drivers/staging/media/imx/imx-media-csc-scaler.c
index 3e1c88938e7d..0aa2493cfc47 100644
--- a/drivers/staging/media/imx/imx-media-csc-scaler.c
+++ b/drivers/staging/media/imx/imx-media-csc-scaler.c
@@ -164,7 +164,8 @@ static int ipu_csc_scaler_enum_fmt(struct file *file, void *fh,
u32 fourcc;
int ret;
- ret = imx_media_enum_format(&fourcc, f->index, CS_SEL_YUV_RGB);
+ ret = imx_media_enum_pixel_formats(&fourcc, f->index,
+ CS_SEL_YUV_RGB);
if (ret)
return ret;
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 298294b95293..caab4d0cd5ce 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1238,8 +1238,8 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,
switch (code->pad) {
case CSI_SINK_PAD:
- ret = imx_media_enum_mbus_format(&code->code, code->index,
- CS_SEL_ANY);
+ ret = imx_media_enum_mbus_formats(&code->code, code->index,
+ CS_SEL_ANY);
break;
case CSI_SRC_PAD_DIRECT:
case CSI_SRC_PAD_IDMAC:
@@ -1258,9 +1258,8 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,
} else {
u32 cs_sel = (incc->cs == IPUV3_COLORSPACE_YUV) ?
CS_SEL_YUV : CS_SEL_RGB;
- ret = imx_media_enum_ipu_format(&code->code,
- code->index,
- cs_sel);
+ ret = imx_media_enum_ipu_formats(&code->code,
+ code->index, cs_sel);
}
break;
default:
@@ -1448,7 +1447,7 @@ static void csi_try_fmt(struct csi_priv *priv,
*cc = imx_media_find_ipu_format(sdformat->format.code,
cs_sel);
if (!*cc) {
- imx_media_enum_ipu_format(&code, 0, cs_sel);
+ imx_media_enum_ipu_formats(&code, 0, cs_sel);
*cc = imx_media_find_ipu_format(code, cs_sel);
sdformat->format.code = (*cc)->codes[0];
}
@@ -1471,7 +1470,7 @@ static void csi_try_fmt(struct csi_priv *priv,
*cc = imx_media_find_mbus_format(sdformat->format.code,
CS_SEL_ANY);
if (!*cc) {
- imx_media_enum_mbus_format(&code, 0, CS_SEL_ANY);
+ imx_media_enum_mbus_formats(&code, 0, CS_SEL_ANY);
*cc = imx_media_find_mbus_format(code, CS_SEL_ANY);
sdformat->format.code = (*cc)->codes[0];
}
@@ -1758,7 +1757,7 @@ static int csi_registered(struct v4l2_subdev *sd)
for (i = 0; i < CSI_NUM_PADS; i++) {
code = 0;
if (i != CSI_SINK_PAD)
- imx_media_enum_ipu_format(&code, 0, CS_SEL_YUV);
+ imx_media_enum_ipu_formats(&code, 0, CS_SEL_YUV);
/* set a default mbus format */
ret = imx_media_init_mbus_fmt(&priv->format_mbus[i],
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index bf187f6d87fe..5631b01510ef 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -225,8 +225,8 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
return NULL;
}
-static int enum_format(u32 *fourcc, u32 *code, u32 index,
- enum codespace_sel cs_sel)
+static int enum_formats(u32 *fourcc, u32 *code, u32 index,
+ enum codespace_sel cs_sel)
{
unsigned int i;
@@ -266,17 +266,18 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
}
const struct imx_media_pixfmt *
-imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel)
+imx_media_find_pixel_format(u32 fourcc, enum codespace_sel cs_sel)
{
return find_format(fourcc, 0, cs_sel);
}
-EXPORT_SYMBOL_GPL(imx_media_find_format);
+EXPORT_SYMBOL_GPL(imx_media_find_pixel_format);
-int imx_media_enum_format(u32 *fourcc, u32 index, enum codespace_sel cs_sel)
+int imx_media_enum_pixel_formats(u32 *fourcc, u32 index,
+ enum codespace_sel cs_sel)
{
- return enum_format(fourcc, NULL, index, cs_sel);
+ return enum_formats(fourcc, NULL, index, cs_sel);
}
-EXPORT_SYMBOL_GPL(imx_media_enum_format);
+EXPORT_SYMBOL_GPL(imx_media_enum_pixel_formats);
const struct imx_media_pixfmt *
imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel)
@@ -285,11 +286,11 @@ imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel)
}
EXPORT_SYMBOL_GPL(imx_media_find_mbus_format);
-int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel)
+int imx_media_enum_mbus_formats(u32 *code, u32 index, enum codespace_sel cs_sel)
{
- return enum_format(NULL, code, index, cs_sel);
+ return enum_formats(NULL, code, index, cs_sel);
}
-EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
+EXPORT_SYMBOL_GPL(imx_media_enum_mbus_formats);
/* -----------------------------------------------------------------------------
* IPU Formats Lookup and Enumeration
@@ -342,7 +343,7 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)
}
EXPORT_SYMBOL_GPL(imx_media_find_ipu_format);
-int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
+int imx_media_enum_ipu_formats(u32 *code, u32 index, enum codespace_sel cs_sel)
{
bool accept_yuv = cs_sel & CS_SEL_YUV;
bool accept_rgb = cs_sel & CS_SEL_RGB;
@@ -371,7 +372,7 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)
return -EINVAL;
}
-EXPORT_SYMBOL_GPL(imx_media_enum_ipu_format);
+EXPORT_SYMBOL_GPL(imx_media_enum_ipu_formats);
int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
u32 width, u32 height, u32 code, u32 field,
@@ -383,7 +384,7 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
mbus->height = height;
mbus->field = field;
if (code == 0)
- imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV);
+ imx_media_enum_mbus_formats(&code, 0, CS_SEL_YUV);
lcc = imx_media_find_mbus_format(code, CS_SEL_ANY);
if (!lcc) {
lcc = imx_media_find_ipu_format(code, CS_SEL_ANY);
@@ -522,7 +523,7 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
if (cc->ipufmt && cc->cs == IPUV3_COLORSPACE_YUV) {
u32 code;
- imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV);
+ imx_media_enum_mbus_formats(&code, 0, CS_SEL_YUV);
cc = imx_media_find_mbus_format(code, CS_SEL_YUV);
}
@@ -574,7 +575,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
{
const struct imx_media_pixfmt *fmt;
- fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY);
+ fmt = imx_media_find_pixel_format(image->pix.pixelformat, CS_SEL_ANY);
if (!fmt || !fmt->codes)
return -EINVAL;
diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
index 0d83c2c41606..9dbf63796806 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -548,7 +548,7 @@ static int vdic_enum_mbus_code(struct v4l2_subdev *sd,
if (code->pad >= VDIC_NUM_PADS)
return -EINVAL;
- return imx_media_enum_ipu_format(&code->code, code->index, CS_SEL_YUV);
+ return imx_media_enum_ipu_formats(&code->code, code->index, CS_SEL_YUV);
}
static int vdic_get_fmt(struct v4l2_subdev *sd,
@@ -587,7 +587,7 @@ static void vdic_try_fmt(struct vdic_priv *priv,
if (!*cc) {
u32 code;
- imx_media_enum_ipu_format(&code, 0, CS_SEL_YUV);
+ imx_media_enum_ipu_formats(&code, 0, CS_SEL_YUV);
*cc = imx_media_find_ipu_format(code, CS_SEL_YUV);
sdformat->format.code = (*cc)->codes[0];
}
@@ -850,7 +850,7 @@ static int vdic_registered(struct v4l2_subdev *sd)
for (i = 0; i < VDIC_NUM_PADS; i++) {
code = 0;
if (i != VDIC_SINK_PAD_IDMAC)
- imx_media_enum_ipu_format(&code, 0, CS_SEL_YUV);
+ imx_media_enum_ipu_formats(&code, 0, CS_SEL_YUV);
/* set a default mbus format */
ret = imx_media_init_mbus_fmt(&priv->format_mbus[i],
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index 917b4db02985..67983a26e5ff 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -159,14 +159,17 @@ enum codespace_sel {
/* imx-media-utils.c */
const struct imx_media_pixfmt *
-imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel);
-int imx_media_enum_format(u32 *fourcc, u32 index, enum codespace_sel cs_sel);
+imx_media_find_pixel_format(u32 fourcc, enum codespace_sel cs_sel);
+int imx_media_enum_pixel_formats(u32 *fourcc, u32 index,
+ enum codespace_sel cs_sel);
const struct imx_media_pixfmt *
imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel);
-int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel);
+int imx_media_enum_mbus_formats(u32 *code, u32 index,
+ enum codespace_sel cs_sel);
const struct imx_media_pixfmt *
imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel);
-int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel);
+int imx_media_enum_ipu_formats(u32 *code, u32 index, enum codespace_sel cs_sel);
+
int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
u32 width, u32 height, u32 code, u32 field,
const struct imx_media_pixfmt **cc);
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index a469dc76a787..df4dd41b23de 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -958,8 +958,8 @@ static int imx7_csi_enum_mbus_code(struct v4l2_subdev *sd,
switch (code->pad) {
case IMX7_CSI_PAD_SINK:
- ret = imx_media_enum_mbus_format(&code->code, code->index,
- CS_SEL_ANY);
+ ret = imx_media_enum_mbus_formats(&code->code, code->index,
+ CS_SEL_ANY);
break;
case IMX7_CSI_PAD_SRC:
if (code->index != 0) {
@@ -1036,7 +1036,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
*cc = imx_media_find_mbus_format(sdformat->format.code,
CS_SEL_ANY);
if (!*cc) {
- imx_media_enum_mbus_format(&code, 0, CS_SEL_ANY);
+ imx_media_enum_mbus_formats(&code, 0, CS_SEL_ANY);
*cc = imx_media_find_mbus_format(code, CS_SEL_ANY);
sdformat->format.code = (*cc)->codes[0];
}
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 09/11] media: imx: utils: Constify mbus argument to imx_media_mbus_fmt_to_*
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
` (7 preceding siblings ...)
2020-03-29 20:59 ` [PATCH v4 08/11] media: imx: utils: Rename format lookup and enumeration functions Steve Longerbeam
@ 2020-03-29 20:59 ` Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 10/11] media: imx: utils: Constify ipu_image argument to imx_media_ipu_image_to_mbus_fmt() Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 11/11] media: imx: utils: Split find|enum_format into fourcc and mbus functions Steve Longerbeam
10 siblings, 0 replies; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-29 20:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva,
Steve Longerbeam
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
The imx_media_mbus_fmt_to_pix_fmt() and imx_media_mbus_fmt_to_ipu_image()
functions do not need to modify their mbus argument. Make them const.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
[Constified mbus arg to imx_media_mbus_fmt_to_ipu_image() as well]
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
drivers/staging/media/imx/imx-media-utils.c | 4 ++--
drivers/staging/media/imx/imx-media.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 5631b01510ef..b30045221841 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -501,7 +501,7 @@ void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
- struct v4l2_mbus_framefmt *mbus,
+ const struct v4l2_mbus_framefmt *mbus,
const struct imx_media_pixfmt *cc)
{
u32 width;
@@ -553,7 +553,7 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
EXPORT_SYMBOL_GPL(imx_media_mbus_fmt_to_pix_fmt);
int imx_media_mbus_fmt_to_ipu_image(struct ipu_image *image,
- struct v4l2_mbus_framefmt *mbus)
+ const struct v4l2_mbus_framefmt *mbus)
{
int ret;
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index 67983a26e5ff..f4b0fe508553 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -178,10 +178,10 @@ int imx_media_init_cfg(struct v4l2_subdev *sd,
void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
bool ic_route);
int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
- struct v4l2_mbus_framefmt *mbus,
+ const struct v4l2_mbus_framefmt *mbus,
const struct imx_media_pixfmt *cc);
int imx_media_mbus_fmt_to_ipu_image(struct ipu_image *image,
- struct v4l2_mbus_framefmt *mbus);
+ const struct v4l2_mbus_framefmt *mbus);
int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
struct ipu_image *image);
void imx_media_grp_id_to_sd_name(char *sd_name, int sz,
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 10/11] media: imx: utils: Constify ipu_image argument to imx_media_ipu_image_to_mbus_fmt()
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
` (8 preceding siblings ...)
2020-03-29 20:59 ` [PATCH v4 09/11] media: imx: utils: Constify mbus argument to imx_media_mbus_fmt_to_* Steve Longerbeam
@ 2020-03-29 20:59 ` Steve Longerbeam
2020-03-29 20:59 ` [PATCH v4 11/11] media: imx: utils: Split find|enum_format into fourcc and mbus functions Steve Longerbeam
10 siblings, 0 replies; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-29 20:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva,
Steve Longerbeam
The imx_media_ipu_image_to_mbus_fmt() function doesn't need to modify its
ipu_image argument. Make it const.
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
drivers/staging/media/imx/imx-media-utils.c | 2 +-
drivers/staging/media/imx/imx-media.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index b30045221841..1b0cadb601cd 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -571,7 +571,7 @@ int imx_media_mbus_fmt_to_ipu_image(struct ipu_image *image,
EXPORT_SYMBOL_GPL(imx_media_mbus_fmt_to_ipu_image);
int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
- struct ipu_image *image)
+ const struct ipu_image *image)
{
const struct imx_media_pixfmt *fmt;
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index f4b0fe508553..a8985966568a 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -183,7 +183,7 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
int imx_media_mbus_fmt_to_ipu_image(struct ipu_image *image,
const struct v4l2_mbus_framefmt *mbus);
int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,
- struct ipu_image *image);
+ const struct ipu_image *image);
void imx_media_grp_id_to_sd_name(char *sd_name, int sz,
u32 grp_id, int ipu_id);
struct v4l2_subdev *
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v4 11/11] media: imx: utils: Split find|enum_format into fourcc and mbus functions
2020-03-29 20:59 [PATCH v4 00/11] media: imx: Miscellaneous format-related cleanups Steve Longerbeam
` (9 preceding siblings ...)
2020-03-29 20:59 ` [PATCH v4 10/11] media: imx: utils: Constify ipu_image argument to imx_media_ipu_image_to_mbus_fmt() Steve Longerbeam
@ 2020-03-29 20:59 ` Steve Longerbeam
10 siblings, 0 replies; 20+ messages in thread
From: Steve Longerbeam @ 2020-03-29 20:59 UTC (permalink / raw)
To: linux-media
Cc: Laurent Pinchart, Philipp Zabel, Rui Miguel Silva,
Steve Longerbeam
To make the code easier to follow, split up find_format() into separate
search functions for pixel formats and media-bus codes. In the process
inline the code into the exported functions imx_media_find_pixel_format()
and imx_media_find_mbus_format(). Do the equivalent for enum_formats().
Also add comment blocks for the exported find|enum functions.
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
drivers/staging/media/imx/imx-media-utils.c | 131 +++++++++++++-------
1 file changed, 88 insertions(+), 43 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 1b0cadb601cd..4be588313681 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -192,28 +192,58 @@ static const struct imx_media_pixfmt pixel_formats[] = {
},
};
-static const struct imx_media_pixfmt *find_format(u32 fourcc,
- u32 code,
- enum codespace_sel cs_sel)
+/*
+ * Search for and return an entry in the pixel_formats[] array that matches
+ * the requested search criteria.
+ *
+ * @fourcc: Search for an entry with the given fourcc pixel format.
+ * @cs_sel: Search for entries with the given codespace encodings
+ * (YUV, RGB, and/or BAYER).
+ */
+const struct imx_media_pixfmt *
+imx_media_find_pixel_format(u32 fourcc, enum codespace_sel cs_sel)
{
unsigned int i;
for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
const struct imx_media_pixfmt *fmt = &pixel_formats[i];
enum codespace_sel fmt_cs_sel;
- unsigned int j;
fmt_cs_sel = fmt->bayer ? CS_SEL_BAYER :
((fmt->cs == IPUV3_COLORSPACE_YUV) ?
CS_SEL_YUV : CS_SEL_RGB);
- if (!(fmt_cs_sel & cs_sel) || (!fourcc && !fmt->codes))
- continue;
-
- if (fourcc && fmt->fourcc == fourcc)
+ if ((fmt_cs_sel & cs_sel) && fmt->fourcc == fourcc)
return fmt;
+ }
+
+ return NULL;
+}
+EXPORT_SYMBOL_GPL(imx_media_find_pixel_format);
+
+/*
+ * Search for and return an entry in the pixel_formats[] array that matches
+ * the requested search criteria.
+ *
+ * @code: Search for an entry with the given media-bus code.
+ * @cs_sel: Search for entries with the given codespace encodings
+ * (YUV, RGB, and/or BAYER).
+ */
+const struct imx_media_pixfmt *
+imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel)
+{
+ unsigned int i;
- if (!code || !fmt->codes)
+ for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
+ const struct imx_media_pixfmt *fmt = &pixel_formats[i];
+ enum codespace_sel fmt_cs_sel;
+ unsigned int j;
+
+ fmt_cs_sel = fmt->bayer ? CS_SEL_BAYER :
+ ((fmt->cs == IPUV3_COLORSPACE_YUV) ?
+ CS_SEL_YUV : CS_SEL_RGB);
+
+ if (!(fmt_cs_sel & cs_sel) || !fmt->codes)
continue;
for (j = 0; fmt->codes[j]; j++) {
@@ -224,33 +254,74 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,
return NULL;
}
+EXPORT_SYMBOL_GPL(imx_media_find_mbus_format);
-static int enum_formats(u32 *fourcc, u32 *code, u32 index,
- enum codespace_sel cs_sel)
+/*
+ * Enumerate entries in the pixel_formats[] array that match the
+ * requested search criteria. Returns the fourcc that matches the
+ * search criteria at the requested match index.
+ *
+ * @fourcc: The returned fourcc that matches the search criteria at
+ * the requested match index.
+ * @index: The requested match index.
+ * @cs_sel: Include in the enumeration entries with the given codespace
+ * encodings (YUV, RGB, and/or BAYER).
+ */
+int imx_media_enum_pixel_formats(u32 *fourcc, u32 index,
+ enum codespace_sel cs_sel)
{
unsigned int i;
for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
const struct imx_media_pixfmt *fmt = &pixel_formats[i];
enum codespace_sel fmt_cs_sel;
- unsigned int j;
fmt_cs_sel = fmt->bayer ? CS_SEL_BAYER :
((fmt->cs == IPUV3_COLORSPACE_YUV) ?
CS_SEL_YUV : CS_SEL_RGB);
- if (!(fmt_cs_sel & cs_sel) || (!fourcc && !fmt->codes))
+ if (!(fmt_cs_sel & cs_sel))
continue;
- if (fourcc && index == 0) {
+ if (index == 0) {
*fourcc = fmt->fourcc;
return 0;
}
- if (!code) {
- index--;
+ index--;
+ }
+
+ return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(imx_media_enum_pixel_formats);
+
+/*
+ * Enumerate entries in the pixel_formats[] array that match the
+ * requested search criteria. Returns the media-bus code that matches
+ * the search criteria at the requested match index.
+ *
+ * @code: The returned media-bus code that matches the search criteria at
+ * the requested match index.
+ * @index: The requested match index.
+ * @cs_sel: Include in the enumeration entries with the given codespace
+ * encodings (YUV, RGB, and/or BAYER).
+ */
+int imx_media_enum_mbus_formats(u32 *code, u32 index,
+ enum codespace_sel cs_sel)
+{
+ unsigned int i;
+
+ for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
+ const struct imx_media_pixfmt *fmt = &pixel_formats[i];
+ enum codespace_sel fmt_cs_sel;
+ unsigned int j;
+
+ fmt_cs_sel = fmt->bayer ? CS_SEL_BAYER :
+ ((fmt->cs == IPUV3_COLORSPACE_YUV) ?
+ CS_SEL_YUV : CS_SEL_RGB);
+
+ if (!(fmt_cs_sel & cs_sel) || !fmt->codes)
continue;
- }
for (j = 0; fmt->codes[j]; j++) {
if (index == 0) {
@@ -264,32 +335,6 @@ static int enum_formats(u32 *fourcc, u32 *code, u32 index,
return -EINVAL;
}
-
-const struct imx_media_pixfmt *
-imx_media_find_pixel_format(u32 fourcc, enum codespace_sel cs_sel)
-{
- return find_format(fourcc, 0, cs_sel);
-}
-EXPORT_SYMBOL_GPL(imx_media_find_pixel_format);
-
-int imx_media_enum_pixel_formats(u32 *fourcc, u32 index,
- enum codespace_sel cs_sel)
-{
- return enum_formats(fourcc, NULL, index, cs_sel);
-}
-EXPORT_SYMBOL_GPL(imx_media_enum_pixel_formats);
-
-const struct imx_media_pixfmt *
-imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel)
-{
- return find_format(0, code, cs_sel);
-}
-EXPORT_SYMBOL_GPL(imx_media_find_mbus_format);
-
-int imx_media_enum_mbus_formats(u32 *code, u32 index, enum codespace_sel cs_sel)
-{
- return enum_formats(NULL, code, index, cs_sel);
-}
EXPORT_SYMBOL_GPL(imx_media_enum_mbus_formats);
/* -----------------------------------------------------------------------------
--
2.17.1
^ permalink raw reply related [flat|nested] 20+ messages in thread