[PATCH v3 00/10] media: staging: rkisp1: add support to V4L2_CAP_IO_MC

[PATCH v3 00/10] media: staging: rkisp1: add support to V4L2_CAP_IO_MC

[Dafna Hirschfeld]

This patch is a v3 of the patchset

"media: staging: rkisp1: various fixes to capture formats"

https://patchwork.kernel.org/cover/11551791/

The patchset solves several problems in the rkisp1 driver:

1. Currently the resizers always return media code MEDIA_BUS_FMT_YUYV8_2X8.
The patchset adds support to other media codes on the resizer according to
the chroma subsampling.
Setting the correct media code on the source pad that matches the
chroma subsampling reflects userspace  that the resizer has downsampling
ability and also the resizer entity does not have to check the capture entity's
configuration to get the scaling ratio, the information of how to scale can be
obtained from the source media code of the resizer.

2. Add support for the V4L2_CAP_IO_MC capability on
the mainpath and selfpath captures. This helps userspace to know the
right configuration for streaming. This is especially helpful for the
RGB and Grey formats that expect media bus MEDIA_BUS_FMT_YUYV8_2X8
which is not something userspace can guess. Adding a mapping of the
expected mbus code for each pixelformat also makes the link_validation
code much simpler, it just has to check if the configuration matches the mapping.

3. Removes unsupported packed yuv formats - this patch was already part of a pull request
and was dropped due to merge conflicts.

4. Remove bayer formats on the selfpath resizer since they are not
supported on the selfpath capture.

5. Remove support to YUV444 pixel format, I was not able to find a configuration
that supports this format. I kept getting bad looking frames and
this format is not that important to support. Also the TRM says:
"
In sensor mode the MRSZ block supports only down-scaling. This is because the sensor
cannot be stopped from delivering data during one frame.
"
So it seems that the format cannot be supported.

6. Fix the configuration to support Grey format - the 'write_format' field should
be 'planar'

Dafna Hirschfeld (10):
  media: staging: rkisp1: cap: change RGB24 format to XBGR32
  media: staging: rkisp1: cap: remove unsupported formats
  media: staging: rkisp1: cap: remove unsupported format YUV444
  media: staging: rkisp1: don't support bayer format on selfpath resizer
  media: staging: rkisp1: add capability V4L2_CAP_IO_MC to capture
    devices
  media: staging: rkisp1: add a helper function to enumerate supported
    mbus formats on capture
  media: staging: rkisp1: rsz: enumerate the formats on the src pad
    according to the capture
  media: staging: rkisp1: rsz: Add support to more YUV encoded mbus
    codes on src pad
  media: rkisp1: cap: simplify the link validation by comparing the
    media bus code
  media: staging: rkisp1: fix configuration for GREY pixelformat

 drivers/staging/media/rkisp1/rkisp1-capture.c | 185 ++++++++++--------
 drivers/staging/media/rkisp1/rkisp1-common.h  |   8 +
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 108 +++++++---
 3 files changed, 195 insertions(+), 106 deletions(-)

-- 
2.17.1

[PATCH v3 01/10] media: staging: rkisp1: cap: change RGB24 format to XBGR32

[Dafna Hirschfeld]

According to the TRM [1], the YUV->RGB conversion outputs
RGB 888 format with 4 bytes, where the last byte is ignored,
using big endian representation:

________________________________
|___X___|___R___|___G___|___B___|
31      24      16      8       0

Which matches format V4L2_PIX_FMT_XBGR32 in little endian
representation, so replace it accordingly.

"24 bit word". What it means is that 4 bytes are used with
24bit for the RGB and the last byte is ignored.
This matches format V4L2_PIX_FMT_XBGR32.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index c05280950ea0..2333d2dcd2e6 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -276,7 +276,7 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
 	},
 	/* rgb */
 	{
-		.fourcc = V4L2_PIX_FMT_RGB24,
+		.fourcc = V4L2_PIX_FMT_XBGR32,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
 	}, {
-- 
2.17.1

[PATCH v3 02/10] media: staging: rkisp1: cap: remove unsupported formats

[Dafna Hirschfeld]

For Ycbcr packed formats only YUYV can be supported by
the driver. This patch removes the other formats.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 2333d2dcd2e6..470e49d5d889 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -88,13 +88,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
 		.fourcc = V4L2_PIX_FMT_YUYV,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
-	}, {
-		.fourcc = V4L2_PIX_FMT_YVYU,
-		.uv_swap = 1,
-		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
-	}, {
-		.fourcc = V4L2_PIX_FMT_VYUY,
-		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YUV422P,
 		.uv_swap = 0,
@@ -197,16 +190,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
-	}, {
-		.fourcc = V4L2_PIX_FMT_YVYU,
-		.uv_swap = 1,
-		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
-		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
-	}, {
-		.fourcc = V4L2_PIX_FMT_VYUY,
-		.uv_swap = 1,
-		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
-		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YUV422P,
 		.uv_swap = 0,
-- 
2.17.1

[PATCH v3 03/10] media: staging: rkisp1: cap: remove unsupported format YUV444

[Dafna Hirschfeld]

It is not clear if the device is able to support format
V4L2_PIX_FMT_YUV444M, and also this is not an important format
so remove it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 470e49d5d889..fd0864194203 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -131,12 +131,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
 		.uv_swap = 1,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
 	},
-	/* yuv444 */
-	{
-		.fourcc = V4L2_PIX_FMT_YUV444M,
-		.uv_swap = 0,
-		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
-	},
 	/* yuv400 */
 	{
 		.fourcc = V4L2_PIX_FMT_GREY,
@@ -243,13 +237,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
 	},
-	/* yuv444 */
-	{
-		.fourcc = V4L2_PIX_FMT_YUV444M,
-		.uv_swap = 0,
-		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
-		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV444,
-	},
 	/* yuv400 */
 	{
 		.fourcc = V4L2_PIX_FMT_GREY,
-- 
2.17.1

[PATCH v3 04/10] media: staging: rkisp1: don't support bayer format on selfpath resizer

[Dafna Hirschfeld]

The selfpath capture does not support bayer formats.
Therefore there is no reason to support bayer formats
on the selfpath resizer. The selfpath resizer should
support only MEDIA_BUS_FMT_YUYV8_2X8.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c |  7 -------
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 13 ++++++++++++-
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index fd0864194203..27efec004686 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -1186,13 +1186,6 @@ static int rkisp1_capture_link_validate(struct media_link *link)
 	struct v4l2_subdev_format sd_fmt;
 	int ret;
 
-	if (cap->id == RKISP1_SELFPATH &&
-	    isp->src_fmt->mbus_code != MEDIA_BUS_FMT_YUYV8_2X8) {
-		dev_err(cap->rkisp1->dev,
-			"selfpath only supports MEDIA_BUS_FMT_YUYV8_2X8\n");
-		return -EPIPE;
-	}
-
 	if (cap_pix_enc != isp_pix_enc &&
 	    !(isp_pix_enc == V4L2_PIXEL_ENC_YUV &&
 	      cap_pix_enc == V4L2_PIXEL_ENC_RGB)) {
diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index c66d2a52fd71..066d22096a7d 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -437,6 +437,13 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
 	u32 pad = code->pad;
 	int ret;
 
+	if (rsz->id == RKISP1_SELFPATH) {
+		if (code->index > 0)
+			return -EINVAL;
+		code->code = MEDIA_BUS_FMT_YUYV8_2X8;
+		return 0;
+	}
+
 	/* supported mbus codes are the same in isp video src pad */
 	code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
 	ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
@@ -540,7 +547,11 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
 	src_fmt = rkisp1_rsz_get_pad_fmt(rsz, cfg, RKISP1_RSZ_PAD_SRC, which);
 	sink_crop = rkisp1_rsz_get_pad_crop(rsz, cfg, RKISP1_RSZ_PAD_SINK,
 					    which);
-	sink_fmt->code = format->code;
+	if (rsz->id == RKISP1_SELFPATH)
+		sink_fmt->code = MEDIA_BUS_FMT_YUYV8_2X8;
+	else
+		sink_fmt->code = format->code;
+
 	mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
 	if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) {
 		sink_fmt->code = RKISP1_DEF_FMT;
-- 
2.17.1

[PATCH v3 05/10] media: staging: rkisp1: add capability V4L2_CAP_IO_MC to capture devices

[Dafna Hirschfeld]

The capture devices supports YUV, RGB, and Bayer formats.
Adding V4L2_CAP_IO_MC capability will reflect userspace
what format should be set on the resizer in order to stream
each of the video formats.

The patch adds a 'mbus' field to the struct
'rkisp1_capture_fmt_cfg' which maps the video format
to the needed mbus format.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 60 +++++++++++++++++--
 1 file changed, 56 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 27efec004686..5dd91b5f82a4 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -49,12 +49,14 @@ enum rkisp1_plane {
  * @uv_swap: if cb cr swaped, for yuv
  * @write_format: defines how YCbCr self picture data is written to memory
  * @output_format: defines sp output format
+ * @mbus: the mbus code on the src resizer pad that matches the pixel format
  */
 struct rkisp1_capture_fmt_cfg {
 	u32 fourcc;
 	u8 uv_swap;
 	u32 write_format;
 	u32 output_format;
+	u32 mbus;
 };
 
 struct rkisp1_capture_ops {
@@ -88,92 +90,116 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
 		.fourcc = V4L2_PIX_FMT_YUYV,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YUV422P,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV16,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV61,
 		.uv_swap = 1,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YVU422M,
 		.uv_swap = 1,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	},
 	/* yuv420 */
 	{
 		.fourcc = V4L2_PIX_FMT_NV21,
 		.uv_swap = 1,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV12,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV21M,
 		.uv_swap = 1,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV12M,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YUV420,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YVU420,
 		.uv_swap = 1,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	},
 	/* yuv400 */
 	{
 		.fourcc = V4L2_PIX_FMT_GREY,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	},
 	/* raw */
 	{
 		.fourcc = V4L2_PIX_FMT_SRGGB8,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
+		.mbus = MEDIA_BUS_FMT_SRGGB8_1X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SGRBG8,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
+		.mbus = MEDIA_BUS_FMT_SGRBG8_1X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SGBRG8,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
+		.mbus = MEDIA_BUS_FMT_SGBRG8_1X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SBGGR8,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
+		.mbus = MEDIA_BUS_FMT_SBGGR8_1X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SRGGB10,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
+		.mbus = MEDIA_BUS_FMT_SRGGB10_1X10,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SGRBG10,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
+		.mbus = MEDIA_BUS_FMT_SGRBG10_1X10,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SGBRG10,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
+		.mbus = MEDIA_BUS_FMT_SGBRG10_1X10,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SBGGR10,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
+		.mbus = MEDIA_BUS_FMT_SBGGR10_1X10,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SRGGB12,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
+		.mbus = MEDIA_BUS_FMT_SRGGB12_1X12,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SGRBG12,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
+		.mbus = MEDIA_BUS_FMT_SGRBG12_1X12,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SGBRG12,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
+		.mbus = MEDIA_BUS_FMT_SGBRG12_1X12,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SBGGR12,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
+		.mbus = MEDIA_BUS_FMT_SBGGR12_1X12,
 	},
 };
 
@@ -184,26 +210,31 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YUV422P,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV16,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV61,
 		.uv_swap = 1,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YVU422M,
 		.uv_swap = 1,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	},
 	/* yuv420 */
 	{
@@ -211,31 +242,37 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
 		.uv_swap = 1,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV12,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV21M,
 		.uv_swap = 1,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_NV12M,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YUV420,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YVU420,
 		.uv_swap = 1,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
+		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	},
 	/* yuv400 */
 	{
@@ -243,16 +280,19 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	},
 	/* rgb */
 	{
 		.fourcc = V4L2_PIX_FMT_XBGR32,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_RGB565,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	},
 };
 
@@ -1107,11 +1147,23 @@ static int rkisp1_enum_fmt_vid_cap_mplane(struct file *file, void *priv,
 	struct rkisp1_capture *cap = video_drvdata(file);
 	const struct rkisp1_capture_fmt_cfg *fmt = NULL;
 
-	if (f->index >= cap->config->fmt_size)
+	if (f->mbus_code) {
+		int i, n = 0;
+
+		for (i = 0; i < cap->config->fmt_size; i++)
+			if (cap->config->fmts[i].mbus == f->mbus_code)
+				if (n++ == f->index) {
+					f->pixelformat = cap->config->fmts[i].fourcc;
+					return 0;
+				}
 		return -EINVAL;
+	} else {
+		if (f->index >= cap->config->fmt_size)
+			return -EINVAL;
 
-	fmt = &cap->config->fmts[f->index];
-	f->pixelformat = fmt->fourcc;
+		fmt = &cap->config->fmts[f->index];
+		f->pixelformat = fmt->fourcc;
+	}
 
 	return 0;
 }
@@ -1261,7 +1313,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
 	vdev->v4l2_dev = v4l2_dev;
 	vdev->lock = &node->vlock;
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
-			    V4L2_CAP_STREAMING;
+			    V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
 	vdev->entity.ops = &rkisp1_media_ops;
 	video_set_drvdata(vdev, cap);
 	vdev->vfl_dir = VFL_DIR_RX;
-- 
2.17.1

[PATCH v3 06/10] media: staging: rkisp1: add a helper function to enumerate supported mbus formats on capture

[Dafna Hirschfeld]

Add a function 'rkisp1_cap_enum_mbus_codes' that receive
a pointer to 'v4l2_subdev_mbus_code_enum' and returns the
next supported mbus format of the capture. The function
assumes that pixel formats with identical 'mbus' are grouped
together in the hardcoded arrays, therefore the order of the
entries in the array 'rkisp1_sp_fmts' are adjusted.
This function is a helper for the media bus enumeration of
the source pad of the resizer entity.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 77 ++++++++++++-------
 drivers/staging/media/rkisp1/rkisp1-common.h  |  8 ++
 2 files changed, 58 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 5dd91b5f82a4..4dabd07a3da9 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -112,6 +112,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
 		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	},
+	/* yuv400 */
+	{
+		.fourcc = V4L2_PIX_FMT_GREY,
+		.uv_swap = 0,
+		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
+	},
 	/* yuv420 */
 	{
 		.fourcc = V4L2_PIX_FMT_NV21,
@@ -144,13 +151,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
 		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	},
-	/* yuv400 */
-	{
-		.fourcc = V4L2_PIX_FMT_GREY,
-		.uv_swap = 0,
-		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
-		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
-	},
 	/* raw */
 	{
 		.fourcc = V4L2_PIX_FMT_SRGGB8,
@@ -236,6 +236,26 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
 		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	},
+	/* yuv400 */
+	{
+		.fourcc = V4L2_PIX_FMT_GREY,
+		.uv_swap = 0,
+		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
+		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
+	},
+	/* rgb */
+	{
+		.fourcc = V4L2_PIX_FMT_XBGR32,
+		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
+		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
+	}, {
+		.fourcc = V4L2_PIX_FMT_RGB565,
+		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
+		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
+	},
 	/* yuv420 */
 	{
 		.fourcc = V4L2_PIX_FMT_NV21,
@@ -274,26 +294,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
 		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
 	},
-	/* yuv400 */
-	{
-		.fourcc = V4L2_PIX_FMT_GREY,
-		.uv_swap = 0,
-		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
-		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
-		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
-	},
-	/* rgb */
-	{
-		.fourcc = V4L2_PIX_FMT_XBGR32,
-		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
-		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
-		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
-	}, {
-		.fourcc = V4L2_PIX_FMT_RGB565,
-		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
-		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
-		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
-	},
 };
 
 static const struct rkisp1_capture_config rkisp1_capture_config_mp = {
@@ -334,6 +334,29 @@ rkisp1_vdev_to_node(struct video_device *vdev)
 	return container_of(vdev, struct rkisp1_vdev_node, vdev);
 }
 
+int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap,
+			       struct v4l2_subdev_mbus_code_enum *code)
+{
+	const struct rkisp1_capture_fmt_cfg *fmts = cap->config->fmts;
+	u32 curr_mbus = fmts[0].mbus;
+	int i, n = 0;
+
+	if (code->index == 0) {
+		code->code = fmts[0].mbus;
+		return 0;
+	}
+
+	for (i = 1; i < cap->config->fmt_size; i++)
+		if (fmts[i].mbus != curr_mbus) {
+			curr_mbus = fmts[i].mbus;
+			if (++n == code->index) {
+				code->code = curr_mbus;
+				return 0;
+			}
+		}
+	return -EINVAL;
+}
+
 /* ----------------------------------------------------------------------------
  * Stream operations for self-picture path (sp) and main-picture path (mp)
  */
diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
index 3dc51d703f73..73e1963cc47a 100644
--- a/drivers/staging/media/rkisp1/rkisp1-common.h
+++ b/drivers/staging/media/rkisp1/rkisp1-common.h
@@ -297,6 +297,14 @@ static inline u32 rkisp1_read(struct rkisp1_device *rkisp1, unsigned int addr)
 	return readl(rkisp1->base_addr + addr);
 }
 
+/*
+ * rkisp1_cap_enum_mbus_codes - A helper function that return the i'th supported mbus code
+ *				of the capture entity. This is used to enumerate the supported
+ *				mbus codes on the source pad of the resizer.
+ */
+int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap,
+			       struct v4l2_subdev_mbus_code_enum *code);
+
 void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop,
 				const struct v4l2_rect *bounds);
 
-- 
2.17.1

[PATCH v3 07/10] media: staging: rkisp1: rsz: enumerate the formats on the src pad according to the capture

[Dafna Hirschfeld]

Currently the resizer outputs the same media bus format
as the input. This is wrong since the resizer is also used
to downscale YUV formats. This patch changes the enumeration
of the supported formats. The supported formats on the sink pad
should be taken from the isp entity and the supported formats on
the source pad should be taken from the capture entity.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 41 ++++++++++++-------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index 066d22096a7d..4e87c6f3f732 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -433,24 +433,35 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
 {
 	struct rkisp1_resizer *rsz =
 		container_of(sd, struct rkisp1_resizer, sd);
-	struct v4l2_subdev_pad_config dummy_cfg;
-	u32 pad = code->pad;
 	int ret;
 
-	if (rsz->id == RKISP1_SELFPATH) {
-		if (code->index > 0)
-			return -EINVAL;
-		code->code = MEDIA_BUS_FMT_YUYV8_2X8;
-		return 0;
+	/* supported mbus codes on the sink pad are the same as isp src pad */
+	if (code->pad == RKISP1_RSZ_PAD_SINK) {
+		struct v4l2_subdev_pad_config dummy_cfg;
+		u32 pad = code->pad;
+
+		/*
+		 * the sp capture doesn't support bayer formats so the sp resizer
+		 * supports only yuv422
+		 */
+		if (rsz->id == RKISP1_SELFPATH) {
+			if (code->index > 0)
+				return -EINVAL;
+			code->code = MEDIA_BUS_FMT_YUYV8_2X8;
+			return 0;
+		}
+		code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
+		ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
+				       &dummy_cfg, code);
+
+		/* restore pad */
+		code->pad = pad;
+	} else {
+		/* supported mbus codes on the src are the same as in the capture */
+		struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
+
+		ret = rkisp1_cap_enum_mbus_codes(cap, code);
 	}
-
-	/* supported mbus codes are the same in isp video src pad */
-	code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
-	ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
-			       &dummy_cfg, code);
-
-	/* restore pad */
-	code->pad = pad;
 	return ret;
 }
 
-- 
2.17.1

[PATCH v3 08/10] media: staging: rkisp1: rsz: Add support to more YUV encoded mbus codes on src pad

[Dafna Hirschfeld]

Add support to more YUV encoded media bus formats on the resizer's
source pad. The patch defines an array rkisp1_rsz_yuv_formats[]
with the list of supported YUV media bus formats and their {hv}div
values. The {hv}div are used in the function 'rkisp1_rsz_config'
instead of the macros RKISP1_MBUS_FMT_(HV)DIV, and instead of
checking the capture format.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 66 ++++++++++++++-----
 1 file changed, 48 insertions(+), 18 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index 4e87c6f3f732..8f68a7be659f 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -16,8 +16,35 @@
 #define RKISP1_DEF_FMT MEDIA_BUS_FMT_YUYV8_2X8
 #define RKISP1_DEF_PIXEL_ENC V4L2_PIXEL_ENC_YUV
 
-#define RKISP1_MBUS_FMT_HDIV 2
-#define RKISP1_MBUS_FMT_VDIV 1
+struct rkisp1_rsz_yuv_mbus_info {
+	u32 mbus_code;
+	u32 hdiv;
+	u32 vdiv;
+};
+
+static const struct rkisp1_rsz_yuv_mbus_info rkisp1_rsz_yuv_src_formats[] = {
+	{
+		.mbus_code	= MEDIA_BUS_FMT_YUYV8_2X8, /* YUV422 */
+		.hdiv		= 2,
+		.vdiv		= 1,
+	},
+	{
+		.mbus_code	= MEDIA_BUS_FMT_YUYV8_1_5X8, /* YUV420 */
+		.hdiv		= 2,
+		.vdiv		= 2,
+	},
+};
+
+static const struct rkisp1_rsz_yuv_mbus_info *rkisp1_rsz_yuv_mbus_info(u32 mbus_code)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(rkisp1_rsz_yuv_src_formats); i++)
+		if (rkisp1_rsz_yuv_src_formats[i].mbus_code == mbus_code)
+			return &rkisp1_rsz_yuv_src_formats[i];
+
+	return NULL;
+}
 
 enum rkisp1_shadow_regs_when {
 	RKISP1_SHADOW_REGS_SYNC,
@@ -361,16 +388,19 @@ static void rkisp1_rsz_config_regs(struct rkisp1_resizer *rsz,
 static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
 			      enum rkisp1_shadow_regs_when when)
 {
-	u8 hdiv = RKISP1_MBUS_FMT_HDIV, vdiv = RKISP1_MBUS_FMT_VDIV;
+	const struct rkisp1_rsz_yuv_mbus_info *sink_yuv_info, *src_yuv_info;
 	struct v4l2_rect sink_y, sink_c, src_y, src_c;
-	struct v4l2_mbus_framefmt *src_fmt;
+	struct v4l2_mbus_framefmt *src_fmt, *sink_fmt;
 	struct v4l2_rect *sink_crop;
-	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
 
 	sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK,
 					    V4L2_SUBDEV_FORMAT_ACTIVE);
 	src_fmt = rkisp1_rsz_get_pad_fmt(rsz, NULL, RKISP1_RSZ_PAD_SRC,
 					 V4L2_SUBDEV_FORMAT_ACTIVE);
+	src_yuv_info = rkisp1_rsz_yuv_mbus_info(src_fmt->code);
+	sink_fmt = rkisp1_rsz_get_pad_fmt(rsz, NULL, RKISP1_RSZ_PAD_SINK,
+					  V4L2_SUBDEV_FORMAT_ACTIVE);
+	sink_yuv_info = rkisp1_rsz_yuv_mbus_info(sink_fmt->code);
 
 	/*
 	 * The resizer only works on yuv formats,
@@ -386,25 +416,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
 	src_y.width = src_fmt->width;
 	src_y.height = src_fmt->height;
 
-	sink_c.width = sink_y.width / RKISP1_MBUS_FMT_HDIV;
-	sink_c.height = sink_y.height / RKISP1_MBUS_FMT_VDIV;
+	sink_c.width = sink_y.width / sink_yuv_info->hdiv;
+	sink_c.height = sink_y.height / sink_yuv_info->vdiv;
 
 	/*
 	 * The resizer is used not only to change the dimensions of the frame
 	 * but also to change the scale for YUV formats,
 	 * (4:2:2 -> 4:2:0 for example). So the width/height of the CbCr
-	 * streams should be set according to the pixel format in the capture.
-	 * The resizer always gets the input as YUV422. If the capture format
-	 * is RGB then the memory input should be YUV422 so we don't change the
-	 * default hdiv, vdiv in that case.
+	 * streams should be set according to the media bus format in the src pad.
 	 */
-	if (v4l2_is_format_yuv(cap->pix.info)) {
-		hdiv = cap->pix.info->hdiv;
-		vdiv = cap->pix.info->vdiv;
-	}
-
-	src_c.width = src_y.width / hdiv;
-	src_c.height = src_y.height / vdiv;
+	src_c.width = src_y.width / src_yuv_info->hdiv;
+	src_c.height = src_y.height / src_yuv_info->vdiv;
 
 	if (sink_c.width == src_c.width && sink_c.height == src_c.height) {
 		rkisp1_rsz_disable(rsz, when);
@@ -499,6 +521,14 @@ static void rkisp1_rsz_set_src_fmt(struct rkisp1_resizer *rsz,
 	struct v4l2_mbus_framefmt *src_fmt;
 
 	src_fmt = rkisp1_rsz_get_pad_fmt(rsz, cfg, RKISP1_RSZ_PAD_SRC, which);
+	if (rsz->pixel_enc == V4L2_PIXEL_ENC_YUV) {
+		const struct rkisp1_rsz_yuv_mbus_info *fmt =
+			rkisp1_rsz_yuv_mbus_info(format->code);
+
+		if (fmt)
+			src_fmt->code = format->code;
+	}
+
 	src_fmt->width = clamp_t(u32, format->width,
 				 rsz->config->min_rsz_width,
 				 rsz->config->max_rsz_width);
-- 
2.17.1

[PATCH v3 09/10] media: rkisp1: cap: simplify the link validation by compering the media bus code

[Dafna Hirschfeld]

The capture has a mapping of the mbus code needed for each pixelformat.
This can be used to simplify the link validation by comparing the mbus
code in the capture with the code in the resizer.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 4dabd07a3da9..a5e2521577dd 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -1255,22 +1255,11 @@ static int rkisp1_capture_link_validate(struct media_link *link)
 	struct v4l2_subdev *sd =
 		media_entity_to_v4l2_subdev(link->source->entity);
 	struct rkisp1_capture *cap = video_get_drvdata(vdev);
-	struct rkisp1_isp *isp = &cap->rkisp1->isp;
-	u8 isp_pix_enc = isp->src_fmt->pixel_enc;
-	u8 cap_pix_enc = cap->pix.info->pixel_enc;
+	const struct rkisp1_capture_fmt_cfg *fmt =
+		rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
 	struct v4l2_subdev_format sd_fmt;
 	int ret;
 
-	if (cap_pix_enc != isp_pix_enc &&
-	    !(isp_pix_enc == V4L2_PIXEL_ENC_YUV &&
-	      cap_pix_enc == V4L2_PIXEL_ENC_RGB)) {
-		dev_err(cap->rkisp1->dev,
-			"format type mismatch in link '%s:%d->%s:%d'\n",
-			link->source->entity->name, link->source->index,
-			link->sink->entity->name, link->sink->index);
-		return -EPIPE;
-	}
-
 	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
 	sd_fmt.pad = link->source->index;
 	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_fmt);
@@ -1278,7 +1267,8 @@ static int rkisp1_capture_link_validate(struct media_link *link)
 		return ret;
 
 	if (sd_fmt.format.height != cap->pix.fmt.height ||
-	    sd_fmt.format.width != cap->pix.fmt.width)
+	    sd_fmt.format.width != cap->pix.fmt.width ||
+	    sd_fmt.format.code != fmt->mbus)
 		return -EPIPE;
 
 	return 0;
-- 
2.17.1

[PATCH v3 10/10] media: staging: rkisp1: fix configuration for GREY pixelformat

[Dafna Hirschfeld]

This patch changes the device configuration to support capture
of V4L2_PIX_FMT_GREY video. The 'write_format' field of the format
description should be planar.
Also the array 'pixm->plane_fmt' that describes the planes should
be memset to 0 before filling it since the the cb, cr planes should
be 0.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index a5e2521577dd..b6f9335c16e9 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -116,7 +116,7 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_GREY,
 		.uv_swap = 0,
-		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
+		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
 		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	},
 	/* yuv420 */
@@ -240,7 +240,7 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
 	{
 		.fourcc = V4L2_PIX_FMT_GREY,
 		.uv_swap = 0,
-		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
+		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
 		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	},
@@ -1050,6 +1050,7 @@ rkisp1_fill_pixfmt(struct v4l2_pix_format_mplane *pixm,
 	unsigned int i;
 	u32 stride;
 
+	memset(pixm->plane_fmt, 0, sizeof(pixm->plane_fmt));
 	info = v4l2_format_info(pixm->pixelformat);
 	pixm->num_planes = info->mem_planes;
 	stride = info->bpp[0] * pixm->width;
-- 
2.17.1

Re: [PATCH v3 01/10] media: staging: rkisp1: cap: change RGB24 format to XBGR32

[Helen Koike]

On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
> According to the TRM [1], the YUV->RGB conversion outputs
> RGB 888 format with 4 bytes, where the last byte is ignored,
> using big endian representation:
> 
> ________________________________

For some reason, it seems that patchwork ignored the rest of the message from this line

https://patchwork.linuxtv.org/project/linux-media/patch/20200723132014.4597-2-dafna.hirschfeld@collabora.com/

This is just a thing to be careful when picking from patchwork.

Regards,
Helen


> |___X___|___R___|___G___|___B___|
> 31      24      16      8       0
> 
> Which matches format V4L2_PIX_FMT_XBGR32 in little endian
> representation, so replace it accordingly.
> 
> "24 bit word". What it means is that 4 bytes are used with
> 24bit for the RGB and the last byte is ignored.
> This matches format V4L2_PIX_FMT_XBGR32.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index c05280950ea0..2333d2dcd2e6 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -276,7 +276,7 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  	},
>  	/* rgb */
>  	{
> -		.fourcc = V4L2_PIX_FMT_RGB24,
> +		.fourcc = V4L2_PIX_FMT_XBGR32,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
>  	}, {
> 

Re: [PATCH v3 03/10] media: staging: rkisp1: cap: remove unsupported format YUV444

[Helen Koike]

Hi Dafna,

On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
> It is not clear if the device is able to support format
> V4L2_PIX_FMT_YUV444M, and also this is not an important format
> so remove it.

I would just improve this a bit, how did you reach this conclusion,
what is not clear, tests you made, etc.
I know this was discussed a in previous thread, but it would be nice
to add the info here.

Regards,
Helen

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 470e49d5d889..fd0864194203 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -131,12 +131,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
>  	},
> -	/* yuv444 */
> -	{
> -		.fourcc = V4L2_PIX_FMT_YUV444M,
> -		.uv_swap = 0,
> -		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> -	},
>  	/* yuv400 */
>  	{
>  		.fourcc = V4L2_PIX_FMT_GREY,
> @@ -243,13 +237,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
>  	},
> -	/* yuv444 */
> -	{
> -		.fourcc = V4L2_PIX_FMT_YUV444M,
> -		.uv_swap = 0,
> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV444,
> -	},
>  	/* yuv400 */
>  	{
>  		.fourcc = V4L2_PIX_FMT_GREY,
> 

Re: [PATCH v3 04/10] media: staging: rkisp1: don't support bayer format on selfpath resizer

[Helen Koike]

Hi Dafna,

On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
> The selfpath capture does not support bayer formats.
> Therefore there is no reason to support bayer formats
> on the selfpath resizer. The selfpath resizer should
> support only MEDIA_BUS_FMT_YUYV8_2X8.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Acked-by: Helen Koike <helen.koike@collabora.com>

Thanks,
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c |  7 -------
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 13 ++++++++++++-
>  2 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index fd0864194203..27efec004686 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -1186,13 +1186,6 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>  	struct v4l2_subdev_format sd_fmt;
>  	int ret;
>  
> -	if (cap->id == RKISP1_SELFPATH &&
> -	    isp->src_fmt->mbus_code != MEDIA_BUS_FMT_YUYV8_2X8) {
> -		dev_err(cap->rkisp1->dev,
> -			"selfpath only supports MEDIA_BUS_FMT_YUYV8_2X8\n");
> -		return -EPIPE;
> -	}
> -
>  	if (cap_pix_enc != isp_pix_enc &&
>  	    !(isp_pix_enc == V4L2_PIXEL_ENC_YUV &&
>  	      cap_pix_enc == V4L2_PIXEL_ENC_RGB)) {
> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> index c66d2a52fd71..066d22096a7d 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -437,6 +437,13 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
>  	u32 pad = code->pad;
>  	int ret;
>  
> +	if (rsz->id == RKISP1_SELFPATH) {
> +		if (code->index > 0)
> +			return -EINVAL;
> +		code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> +		return 0;
> +	}
> +
>  	/* supported mbus codes are the same in isp video src pad */
>  	code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
>  	ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
> @@ -540,7 +547,11 @@ static void rkisp1_rsz_set_sink_fmt(struct rkisp1_resizer *rsz,
>  	src_fmt = rkisp1_rsz_get_pad_fmt(rsz, cfg, RKISP1_RSZ_PAD_SRC, which);
>  	sink_crop = rkisp1_rsz_get_pad_crop(rsz, cfg, RKISP1_RSZ_PAD_SINK,
>  					    which);
> -	sink_fmt->code = format->code;
> +	if (rsz->id == RKISP1_SELFPATH)
> +		sink_fmt->code = MEDIA_BUS_FMT_YUYV8_2X8;
> +	else
> +		sink_fmt->code = format->code;
> +
>  	mbus_info = rkisp1_isp_mbus_info_get(sink_fmt->code);
>  	if (!mbus_info || !(mbus_info->direction & RKISP1_ISP_SD_SRC)) {
>  		sink_fmt->code = RKISP1_DEF_FMT;
> 

Re: [PATCH v3 06/10] media: staging: rkisp1: add a helper function to enumerate supported mbus formats on capture

[Helen Koike]

Hi Dafna,

Thanks for the patch.

On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
> Add a function 'rkisp1_cap_enum_mbus_codes' that receive
> a pointer to 'v4l2_subdev_mbus_code_enum' and returns the
> next supported mbus format of the capture. The function
> assumes that pixel formats with identical 'mbus' are grouped
> together in the hardcoded arrays, therefore the order of the
> entries in the array 'rkisp1_sp_fmts' are adjusted.
> This function is a helper for the media bus enumeration of
> the source pad of the resizer entity.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 77 ++++++++++++-------
>  drivers/staging/media/rkisp1/rkisp1-common.h  |  8 ++
>  2 files changed, 58 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 5dd91b5f82a4..4dabd07a3da9 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -112,6 +112,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {

I would also add a comment in the beginning of this array saying that mbus
should be grouped together for the sake of enum.

>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
> +	/* yuv400 */
> +	{
> +		.fourcc = V4L2_PIX_FMT_GREY,
> +		.uv_swap = 0,
> +		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> +	},
>  	/* yuv420 */
>  	{
>  		.fourcc = V4L2_PIX_FMT_NV21,
> @@ -144,13 +151,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	},
> -	/* yuv400 */
> -	{
> -		.fourcc = V4L2_PIX_FMT_GREY,
> -		.uv_swap = 0,
> -		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> -	},
>  	/* raw */
>  	{
>  		.fourcc = V4L2_PIX_FMT_SRGGB8,
> @@ -236,6 +236,26 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
> +	/* yuv400 */
> +	{
> +		.fourcc = V4L2_PIX_FMT_GREY,
> +		.uv_swap = 0,
> +		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> +		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> +	},
> +	/* rgb */
> +	{
> +		.fourcc = V4L2_PIX_FMT_XBGR32,
> +		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> +		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_RGB565,
> +		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> +		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> +	},
>  	/* yuv420 */
>  	{
>  		.fourcc = V4L2_PIX_FMT_NV21,
> @@ -274,26 +294,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	},
> -	/* yuv400 */
> -	{
> -		.fourcc = V4L2_PIX_FMT_GREY,
> -		.uv_swap = 0,
> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> -	},
> -	/* rgb */
> -	{
> -		.fourcc = V4L2_PIX_FMT_XBGR32,
> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> -	}, {
> -		.fourcc = V4L2_PIX_FMT_RGB565,
> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> -	},
>  };
>  
>  static const struct rkisp1_capture_config rkisp1_capture_config_mp = {
> @@ -334,6 +334,29 @@ rkisp1_vdev_to_node(struct video_device *vdev)
>  	return container_of(vdev, struct rkisp1_vdev_node, vdev);
>  }
>  
> +int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap,
> +			       struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	const struct rkisp1_capture_fmt_cfg *fmts = cap->config->fmts;
> +	u32 curr_mbus = fmts[0].mbus;

Maybe you could use code->code as the current, since in case of error
-EINVAL should be returned and this value should be ignored iirc.

> +	int i, n = 0;

unsigned

> +
> +	if (code->index == 0) {

if (!code->index)

> +		code->code = fmts[0].mbus;
> +		return 0;
> +	}
> +
> +	for (i = 1; i < cap->config->fmt_size; i++)
> +		if (fmts[i].mbus != curr_mbus) {

You can decrease one indentation level with:

		if (fmts[i].mbus == curr_mbus)
				continue;

Regards,
Helen

> +			curr_mbus = fmts[i].mbus;
> +			if (++n == code->index) {
> +				code->code = curr_mbus;
> +				return 0;
> +			}
> +		}
> +	return -EINVAL;
> +}
> +
>  /* ----------------------------------------------------------------------------
>   * Stream operations for self-picture path (sp) and main-picture path (mp)
>   */
> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
> index 3dc51d703f73..73e1963cc47a 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
> @@ -297,6 +297,14 @@ static inline u32 rkisp1_read(struct rkisp1_device *rkisp1, unsigned int addr)
>  	return readl(rkisp1->base_addr + addr);
>  }
>  
> +/*
> + * rkisp1_cap_enum_mbus_codes - A helper function that return the i'th supported mbus code
> + *				of the capture entity. This is used to enumerate the supported
> + *				mbus codes on the source pad of the resizer.
> + */
> +int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap,
> +			       struct v4l2_subdev_mbus_code_enum *code);
> +
>  void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop,
>  				const struct v4l2_rect *bounds);
>  
> 

Re: [PATCH v3 05/10] media: staging: rkisp1: add capability V4L2_CAP_IO_MC to capture devices

[Helen Koike]

Hi Dafna,

Thanks for the patch.

On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
> The capture devices supports YUV, RGB, and Bayer formats.
> Adding V4L2_CAP_IO_MC capability will reflect userspace
> what format should be set on the resizer in order to stream
> each of the video formats.
> 
> The patch adds a 'mbus' field to the struct
> 'rkisp1_capture_fmt_cfg' which maps the video format
> to the needed mbus format.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 60 +++++++++++++++++--
>  1 file changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 27efec004686..5dd91b5f82a4 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -49,12 +49,14 @@ enum rkisp1_plane {
>   * @uv_swap: if cb cr swaped, for yuv
>   * @write_format: defines how YCbCr self picture data is written to memory
>   * @output_format: defines sp output format
> + * @mbus: the mbus code on the src resizer pad that matches the pixel format
>   */
>  struct rkisp1_capture_fmt_cfg {
>  	u32 fourcc;
>  	u8 uv_swap;
>  	u32 write_format;
>  	u32 output_format;
> +	u32 mbus;
>  };
>  
>  struct rkisp1_capture_ops {
> @@ -88,92 +90,116 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
>  		.fourcc = V4L2_PIX_FMT_YUYV,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV422P,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV16,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV61,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU422M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* yuv420 */
>  	{
>  		.fourcc = V4L2_PIX_FMT_NV21,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV21M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12M,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV420,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU420,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	},
>  	/* yuv400 */
>  	{
>  		.fourcc = V4L2_PIX_FMT_GREY,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* raw */
>  	{
>  		.fourcc = V4L2_PIX_FMT_SRGGB8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_SRGGB8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGRBG8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_SGRBG8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGBRG8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_SGBRG8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SBGGR8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_SBGGR8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SRGGB10,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SRGGB10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGRBG10,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SGRBG10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGBRG10,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SGBRG10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SBGGR10,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SBGGR10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SRGGB12,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SRGGB12_1X12,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGRBG12,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SGRBG12_1X12,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGBRG12,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SGBRG12_1X12,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SBGGR12,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SBGGR12_1X12,
>  	},
>  };
>  
> @@ -184,26 +210,31 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV422P,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV16,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV61,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU422M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* yuv420 */
>  	{
> @@ -211,31 +242,37 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV21M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12M,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV420,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU420,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	},
>  	/* yuv400 */
>  	{
> @@ -243,16 +280,19 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* rgb */
>  	{
>  		.fourcc = V4L2_PIX_FMT_XBGR32,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_RGB565,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  };
>  
> @@ -1107,11 +1147,23 @@ static int rkisp1_enum_fmt_vid_cap_mplane(struct file *file, void *priv,
>  	struct rkisp1_capture *cap = video_drvdata(file);
>  	const struct rkisp1_capture_fmt_cfg *fmt = NULL;
>  
> -	if (f->index >= cap->config->fmt_size)
> +	if (f->mbus_code) {
> +		int i, n = 0;

unsigned

> +
> +		for (i = 0; i < cap->config->fmt_size; i++)
> +			if (cap->config->fmts[i].mbus == f->mbus_code)
> +				if (n++ == f->index) {
> +					f->pixelformat = cap->config->fmts[i].fourcc;
> +					return 0;
> +				}
>  		return -EINVAL;
> +	} else {
> +		if (f->index >= cap->config->fmt_size)
> +			return -EINVAL;
>  
> -	fmt = &cap->config->fmts[f->index];
> -	f->pixelformat = fmt->fourcc;
> +		fmt = &cap->config->fmts[f->index];
> +		f->pixelformat = fmt->fourcc;
> +	}
>  
>  	return 0;

Maybe remove one level of indentation by checking first:

if (!f->mbus_code) {
    ...
    return 0;
}

// the else case without an else statement

Thanks,
Helen

>  }
> @@ -1261,7 +1313,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
>  	vdev->v4l2_dev = v4l2_dev;
>  	vdev->lock = &node->vlock;
>  	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> -			    V4L2_CAP_STREAMING;
> +			    V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
>  	vdev->entity.ops = &rkisp1_media_ops;
>  	video_set_drvdata(vdev, cap);
>  	vdev->vfl_dir = VFL_DIR_RX;
> 

Re: [PATCH v3 05/10] media: staging: rkisp1: add capability V4L2_CAP_IO_MC to capture devices

[Helen Koike]

Hi Dafna,

Thanks for the patch.

On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
> The capture devices supports YUV, RGB, and Bayer formats.
> Adding V4L2_CAP_IO_MC capability will reflect userspace
> what format should be set on the resizer in order to stream
> each of the video formats.
> 
> The patch adds a 'mbus' field to the struct
> 'rkisp1_capture_fmt_cfg' which maps the video format
> to the needed mbus format.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 60 +++++++++++++++++--
>  1 file changed, 56 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 27efec004686..5dd91b5f82a4 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -49,12 +49,14 @@ enum rkisp1_plane {
>   * @uv_swap: if cb cr swaped, for yuv
>   * @write_format: defines how YCbCr self picture data is written to memory
>   * @output_format: defines sp output format
> + * @mbus: the mbus code on the src resizer pad that matches the pixel format
>   */
>  struct rkisp1_capture_fmt_cfg {
>  	u32 fourcc;
>  	u8 uv_swap;
>  	u32 write_format;
>  	u32 output_format;
> +	u32 mbus;
>  };
>  
>  struct rkisp1_capture_ops {
> @@ -88,92 +90,116 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
>  		.fourcc = V4L2_PIX_FMT_YUYV,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV422P,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV16,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV61,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU422M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* yuv420 */
>  	{
>  		.fourcc = V4L2_PIX_FMT_NV21,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV21M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12M,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV420,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU420,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	},
>  	/* yuv400 */
>  	{
>  		.fourcc = V4L2_PIX_FMT_GREY,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* raw */
>  	{
>  		.fourcc = V4L2_PIX_FMT_SRGGB8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_SRGGB8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGRBG8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_SGRBG8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGBRG8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_SGBRG8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SBGGR8,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> +		.mbus = MEDIA_BUS_FMT_SBGGR8_1X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SRGGB10,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SRGGB10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGRBG10,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SGRBG10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGBRG10,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SGBRG10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SBGGR10,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SBGGR10_1X10,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SRGGB12,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SRGGB12_1X12,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGRBG12,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SGRBG12_1X12,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SGBRG12,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SGBRG12_1X12,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SBGGR12,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_RAW12,
> +		.mbus = MEDIA_BUS_FMT_SBGGR12_1X12,
>  	},
>  };
>  
> @@ -184,26 +210,31 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV422P,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV16,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV61,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU422M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* yuv420 */
>  	{
> @@ -211,31 +242,37 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV21M,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_NV12M,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_SPLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV420,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YVU420,
>  		.uv_swap = 1,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	},
>  	/* yuv400 */
>  	{
> @@ -243,16 +280,19 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  	/* rgb */
>  	{
>  		.fourcc = V4L2_PIX_FMT_XBGR32,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_RGB565,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
>  };
>  
> @@ -1107,11 +1147,23 @@ static int rkisp1_enum_fmt_vid_cap_mplane(struct file *file, void *priv,
>  	struct rkisp1_capture *cap = video_drvdata(file);
>  	const struct rkisp1_capture_fmt_cfg *fmt = NULL;
>  
> -	if (f->index >= cap->config->fmt_size)
> +	if (f->mbus_code) {
> +		int i, n = 0;

unsigned

> +
> +		for (i = 0; i < cap->config->fmt_size; i++)
> +			if (cap->config->fmts[i].mbus == f->mbus_code)
> +				if (n++ == f->index) {
> +					f->pixelformat = cap->config->fmts[i].fourcc;
> +					return 0;
> +				}
>  		return -EINVAL;
> +	} else {
> +		if (f->index >= cap->config->fmt_size)
> +			return -EINVAL;
>  
> -	fmt = &cap->config->fmts[f->index];
> -	f->pixelformat = fmt->fourcc;
> +		fmt = &cap->config->fmts[f->index];
> +		f->pixelformat = fmt->fourcc;
> +	}
>  
>  	return 0;

Maybe remove one level of indentation by checking first:

if (!f->mbus_code) {
    ...
    return 0;
}

// the else case without an else statement

Thanks,
Helen

>  }
> @@ -1261,7 +1313,7 @@ static int rkisp1_register_capture(struct rkisp1_capture *cap)
>  	vdev->v4l2_dev = v4l2_dev;
>  	vdev->lock = &node->vlock;
>  	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> -			    V4L2_CAP_STREAMING;
> +			    V4L2_CAP_STREAMING | V4L2_CAP_IO_MC;
>  	vdev->entity.ops = &rkisp1_media_ops;
>  	video_set_drvdata(vdev, cap);
>  	vdev->vfl_dir = VFL_DIR_RX;
> 

Re: [PATCH v3 07/10] media: staging: rkisp1: rsz: enumerate the formats on the src pad according to the capture

[Helen Koike]

Hi Dafna,

On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
> Currently the resizer outputs the same media bus format
> as the input. This is wrong since the resizer is also used
> to downscale YUV formats. This patch changes the enumeration
> of the supported formats. The supported formats on the sink pad
> should be taken from the isp entity and the supported formats on
> the source pad should be taken from the capture entity.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 41 ++++++++++++-------
>  1 file changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> index 066d22096a7d..4e87c6f3f732 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -433,24 +433,35 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
>  {
>  	struct rkisp1_resizer *rsz =
>  		container_of(sd, struct rkisp1_resizer, sd);
> -	struct v4l2_subdev_pad_config dummy_cfg;
> -	u32 pad = code->pad;
>  	int ret;
>  
> -	if (rsz->id == RKISP1_SELFPATH) {
> -		if (code->index > 0)
> -			return -EINVAL;
> -		code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> -		return 0;
> +	/* supported mbus codes on the sink pad are the same as isp src pad */
> +	if (code->pad == RKISP1_RSZ_PAD_SINK) {
> +		struct v4l2_subdev_pad_config dummy_cfg;
> +		u32 pad = code->pad;
> +
> +		/*
> +		 * the sp capture doesn't support bayer formats so the sp resizer
> +		 * supports only yuv422
> +		 */
> +		if (rsz->id == RKISP1_SELFPATH) {
> +			if (code->index > 0)
> +				return -EINVAL;
> +			code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> +			return 0;
> +		}
> +		code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
> +		ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
> +				       &dummy_cfg, code);
> +
> +		/* restore pad */
> +		code->pad = pad;
> +	} else {
> +		/* supported mbus codes on the src are the same as in the capture */
> +		struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
> +
> +		ret = rkisp1_cap_enum_mbus_codes(cap, code);
>  	}
> -
> -	/* supported mbus codes are the same in isp video src pad */
> -	code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
> -	ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
> -			       &dummy_cfg, code);
> -
> -	/* restore pad */
> -	code->pad = pad;
>  	return ret;
>  }
>  
> 

I think you can reduce indentation by doing:

	/* supported mbus codes on the src are the same as in the capture */
	if (code->pad == RKISP1_RSZ_PAD_SRC)
		return kisp1_cap_enum_mbus_codes(
			&rsz->rkisp1->capture_devs[rsz->id], code);

	// ... rest of the code for sink pad without an else statement

Re: [PATCH v3 06/10] media: staging: rkisp1: add a helper function to enumerate supported mbus formats on capture

[Dafna Hirschfeld]

Am 04.08.20 um 01:49 schrieb Helen Koike:
> Hi Dafna,
> 
> Thanks for the patch.
> 
> On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
>> Add a function 'rkisp1_cap_enum_mbus_codes' that receive
>> a pointer to 'v4l2_subdev_mbus_code_enum' and returns the
>> next supported mbus format of the capture. The function
>> assumes that pixel formats with identical 'mbus' are grouped
>> together in the hardcoded arrays, therefore the order of the
>> entries in the array 'rkisp1_sp_fmts' are adjusted.
>> This function is a helper for the media bus enumeration of
>> the source pad of the resizer entity.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 77 ++++++++++++-------
>>   drivers/staging/media/rkisp1/rkisp1-common.h  |  8 ++
>>   2 files changed, 58 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index 5dd91b5f82a4..4dabd07a3da9 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -112,6 +112,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
> 
> I would also add a comment in the beginning of this array saying that mbus
> should be grouped together for the sake of enum.
> 
>>   		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
>>   		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>>   	},
>> +	/* yuv400 */
>> +	{
>> +		.fourcc = V4L2_PIX_FMT_GREY,
>> +		.uv_swap = 0,
>> +		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
>> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>> +	},
>>   	/* yuv420 */
>>   	{
>>   		.fourcc = V4L2_PIX_FMT_NV21,
>> @@ -144,13 +151,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
>>   		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
>>   		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>>   	},
>> -	/* yuv400 */
>> -	{
>> -		.fourcc = V4L2_PIX_FMT_GREY,
>> -		.uv_swap = 0,
>> -		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
>> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>> -	},
>>   	/* raw */
>>   	{
>>   		.fourcc = V4L2_PIX_FMT_SRGGB8,
>> @@ -236,6 +236,26 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>>   		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>>   		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>>   	},
>> +	/* yuv400 */
>> +	{
>> +		.fourcc = V4L2_PIX_FMT_GREY,
>> +		.uv_swap = 0,
>> +		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
>> +		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
>> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>> +	},
>> +	/* rgb */
>> +	{
>> +		.fourcc = V4L2_PIX_FMT_XBGR32,
>> +		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>> +		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
>> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>> +	}, {
>> +		.fourcc = V4L2_PIX_FMT_RGB565,
>> +		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>> +		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
>> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>> +	},
>>   	/* yuv420 */
>>   	{
>>   		.fourcc = V4L2_PIX_FMT_NV21,
>> @@ -274,26 +294,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>>   		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
>>   		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>>   	},
>> -	/* yuv400 */
>> -	{
>> -		.fourcc = V4L2_PIX_FMT_GREY,
>> -		.uv_swap = 0,
>> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
>> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
>> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>> -	},
>> -	/* rgb */
>> -	{
>> -		.fourcc = V4L2_PIX_FMT_XBGR32,
>> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
>> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>> -	}, {
>> -		.fourcc = V4L2_PIX_FMT_RGB565,
>> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
>> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>> -	},
>>   };
>>   
>>   static const struct rkisp1_capture_config rkisp1_capture_config_mp = {
>> @@ -334,6 +334,29 @@ rkisp1_vdev_to_node(struct video_device *vdev)
>>   	return container_of(vdev, struct rkisp1_vdev_node, vdev);
>>   }
>>   
>> +int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap,
>> +			       struct v4l2_subdev_mbus_code_enum *code)
>> +{
>> +	const struct rkisp1_capture_fmt_cfg *fmts = cap->config->fmts;
>> +	u32 curr_mbus = fmts[0].mbus;
> 
> Maybe you could use code->code as the current, since in case of error
> -EINVAL should be returned and this value should be ignored iirc.

Hi, but code->code is an unknown value. The aim of this function is to fill it
according to code->index.

Thanks,
Dafna

> 
>> +	int i, n = 0;
> 
> unsigned
> 
>> +
>> +	if (code->index == 0) {
> 
> if (!code->index)
> 
>> +		code->code = fmts[0].mbus;
>> +		return 0;
>> +	}
>> +
>> +	for (i = 1; i < cap->config->fmt_size; i++)
>> +		if (fmts[i].mbus != curr_mbus) {
> 
> You can decrease one indentation level with:
> 
> 		if (fmts[i].mbus == curr_mbus)
> 				continue;
> 
> Regards,
> Helen
> 
>> +			curr_mbus = fmts[i].mbus;
>> +			if (++n == code->index) {
>> +				code->code = curr_mbus;
>> +				return 0;
>> +			}
>> +		}
>> +	return -EINVAL;
>> +}
>> +
>>   /* ----------------------------------------------------------------------------
>>    * Stream operations for self-picture path (sp) and main-picture path (mp)
>>    */
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-common.h b/drivers/staging/media/rkisp1/rkisp1-common.h
>> index 3dc51d703f73..73e1963cc47a 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-common.h
>> +++ b/drivers/staging/media/rkisp1/rkisp1-common.h
>> @@ -297,6 +297,14 @@ static inline u32 rkisp1_read(struct rkisp1_device *rkisp1, unsigned int addr)
>>   	return readl(rkisp1->base_addr + addr);
>>   }
>>   
>> +/*
>> + * rkisp1_cap_enum_mbus_codes - A helper function that return the i'th supported mbus code
>> + *				of the capture entity. This is used to enumerate the supported
>> + *				mbus codes on the source pad of the resizer.
>> + */
>> +int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap,
>> +			       struct v4l2_subdev_mbus_code_enum *code);
>> +
>>   void rkisp1_sd_adjust_crop_rect(struct v4l2_rect *crop,
>>   				const struct v4l2_rect *bounds);
>>   
>>

Re: [PATCH v3 07/10] media: staging: rkisp1: rsz: enumerate the formats on the src pad according to the capture

[Dafna Hirschfeld]

Am 04.08.20 um 01:50 schrieb Helen Koike:
> Hi Dafna,
> 
> On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
>> Currently the resizer outputs the same media bus format
>> as the input. This is wrong since the resizer is also used
>> to downscale YUV formats. This patch changes the enumeration
>> of the supported formats. The supported formats on the sink pad
>> should be taken from the isp entity and the supported formats on
>> the source pad should be taken from the capture entity.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 41 ++++++++++++-------
>>   1 file changed, 26 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> index 066d22096a7d..4e87c6f3f732 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> @@ -433,24 +433,35 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
>>   {
>>   	struct rkisp1_resizer *rsz =
>>   		container_of(sd, struct rkisp1_resizer, sd);
>> -	struct v4l2_subdev_pad_config dummy_cfg;
>> -	u32 pad = code->pad;
>>   	int ret;
>>   
>> -	if (rsz->id == RKISP1_SELFPATH) {
>> -		if (code->index > 0)
>> -			return -EINVAL;
>> -		code->code = MEDIA_BUS_FMT_YUYV8_2X8;
>> -		return 0;
>> +	/* supported mbus codes on the sink pad are the same as isp src pad */
>> +	if (code->pad == RKISP1_RSZ_PAD_SINK) {
>> +		struct v4l2_subdev_pad_config dummy_cfg;
>> +		u32 pad = code->pad;
>> +
>> +		/*
>> +		 * the sp capture doesn't support bayer formats so the sp resizer
>> +		 * supports only yuv422
>> +		 */
>> +		if (rsz->id == RKISP1_SELFPATH) {
>> +			if (code->index > 0)
>> +				return -EINVAL;
>> +			code->code = MEDIA_BUS_FMT_YUYV8_2X8;
>> +			return 0;
>> +		}
>> +		code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
>> +		ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
>> +				       &dummy_cfg, code);
>> +
>> +		/* restore pad */
>> +		code->pad = pad;
>> +	} else {
>> +		/* supported mbus codes on the src are the same as in the capture */
>> +		struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
>> +
>> +		ret = rkisp1_cap_enum_mbus_codes(cap, code);
>>   	}
>> -
>> -	/* supported mbus codes are the same in isp video src pad */
>> -	code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
>> -	ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
>> -			       &dummy_cfg, code);
>> -
>> -	/* restore pad */
>> -	code->pad = pad;
>>   	return ret;
>>   }
>>   
>>
> 
> I think you can reduce indentation by doing:
> 
> 	/* supported mbus codes on the src are the same as in the capture */
> 	if (code->pad == RKISP1_RSZ_PAD_SRC)
> 		return kisp1_cap_enum_mbus_codes(
> 			&rsz->rkisp1->capture_devs[rsz->id], code);
> 
> 	// ... rest of the code for sink pad without an else statement

I think it will make the code a bit less clear, since one should note that there is a return statement inside the first 'if'
and conclude that the rest is the 'else' case. With 'if-else' it is more clear what code run under which condition.

Thanks,
Dafna

> 

Re: [PATCH v3 07/10] media: staging: rkisp1: rsz: enumerate the formats on the src pad according to the capture

[Tomasz Figa]

Hi Dafna,

On Sat, Aug 29, 2020 at 7:53 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
>
>
> Am 04.08.20 um 01:50 schrieb Helen Koike:
> > Hi Dafna,
> >
> > On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
> >> Currently the resizer outputs the same media bus format
> >> as the input. This is wrong since the resizer is also used
> >> to downscale YUV formats. This patch changes the enumeration
> >> of the supported formats. The supported formats on the sink pad
> >> should be taken from the isp entity and the supported formats on
> >> the source pad should be taken from the capture entity.
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> ---
> >>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 41 ++++++++++++-------
> >>   1 file changed, 26 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >> index 066d22096a7d..4e87c6f3f732 100644
> >> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >> @@ -433,24 +433,35 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
> >>   {
> >>      struct rkisp1_resizer *rsz =
> >>              container_of(sd, struct rkisp1_resizer, sd);
> >> -    struct v4l2_subdev_pad_config dummy_cfg;
> >> -    u32 pad = code->pad;
> >>      int ret;
> >>
> >> -    if (rsz->id == RKISP1_SELFPATH) {
> >> -            if (code->index > 0)
> >> -                    return -EINVAL;
> >> -            code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> >> -            return 0;
> >> +    /* supported mbus codes on the sink pad are the same as isp src pad */
> >> +    if (code->pad == RKISP1_RSZ_PAD_SINK) {
> >> +            struct v4l2_subdev_pad_config dummy_cfg;
> >> +            u32 pad = code->pad;
> >> +
> >> +            /*
> >> +             * the sp capture doesn't support bayer formats so the sp resizer
> >> +             * supports only yuv422
> >> +             */
> >> +            if (rsz->id == RKISP1_SELFPATH) {
> >> +                    if (code->index > 0)
> >> +                            return -EINVAL;
> >> +                    code->code = MEDIA_BUS_FMT_YUYV8_2X8;
> >> +                    return 0;
> >> +            }
> >> +            code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
> >> +            ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
> >> +                                   &dummy_cfg, code);
> >> +
> >> +            /* restore pad */
> >> +            code->pad = pad;
> >> +    } else {
> >> +            /* supported mbus codes on the src are the same as in the capture */
> >> +            struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
> >> +
> >> +            ret = rkisp1_cap_enum_mbus_codes(cap, code);
> >>      }
> >> -
> >> -    /* supported mbus codes are the same in isp video src pad */
> >> -    code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
> >> -    ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
> >> -                           &dummy_cfg, code);
> >> -
> >> -    /* restore pad */
> >> -    code->pad = pad;
> >>      return ret;
> >>   }
> >>
> >>
> >
> > I think you can reduce indentation by doing:
> >
> >       /* supported mbus codes on the src are the same as in the capture */
> >       if (code->pad == RKISP1_RSZ_PAD_SRC)
> >               return kisp1_cap_enum_mbus_codes(
> >                       &rsz->rkisp1->capture_devs[rsz->id], code);
> >
> >       // ... rest of the code for sink pad without an else statement
>
> I think it will make the code a bit less clear, since one should note that there is a return statement inside the first 'if'
> and conclude that the rest is the 'else' case. With 'if-else' it is more clear what code run under which condition.

I guess it might be a bit subjective, but it is a common kernel coding
pattern and usually preferred over if/else blocks. It allows quickly
pruning various edge cases and leaving the rest (and usually most
complex part) of the function easier to follow, because of more
visible assumptions. After all, one usually reads a function from the
top to bottom.

Best regards,
Tomasz

Re: [PATCH v3 06/10] media: staging: rkisp1: add a helper function to enumerate supported mbus formats on capture

[Tomasz Figa]

Hi Dafna,

On Thu, Jul 23, 2020 at 03:20:10PM +0200, Dafna Hirschfeld wrote:
> Add a function 'rkisp1_cap_enum_mbus_codes' that receive
> a pointer to 'v4l2_subdev_mbus_code_enum' and returns the
> next supported mbus format of the capture. The function
> assumes that pixel formats with identical 'mbus' are grouped
> together in the hardcoded arrays, therefore the order of the
> entries in the array 'rkisp1_sp_fmts' are adjusted.
> This function is a helper for the media bus enumeration of
> the source pad of the resizer entity.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 77 ++++++++++++-------
>  drivers/staging/media/rkisp1/rkisp1-common.h  |  8 ++
>  2 files changed, 58 insertions(+), 27 deletions(-)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 5dd91b5f82a4..4dabd07a3da9 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -112,6 +112,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
> +	/* yuv400 */
> +	{
> +		.fourcc = V4L2_PIX_FMT_GREY,
> +		.uv_swap = 0,
> +		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> +	},
>  	/* yuv420 */
>  	{
>  		.fourcc = V4L2_PIX_FMT_NV21,
> @@ -144,13 +151,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	},
> -	/* yuv400 */
> -	{
> -		.fourcc = V4L2_PIX_FMT_GREY,
> -		.uv_swap = 0,
> -		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> -	},
>  	/* raw */
>  	{
>  		.fourcc = V4L2_PIX_FMT_SRGGB8,
> @@ -236,6 +236,26 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	},
> +	/* yuv400 */
> +	{
> +		.fourcc = V4L2_PIX_FMT_GREY,
> +		.uv_swap = 0,
> +		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> +		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> +	},
> +	/* rgb */
> +	{
> +		.fourcc = V4L2_PIX_FMT_XBGR32,
> +		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> +		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_RGB565,
> +		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> +		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> +	},

Is there any reason to move RGB formats here rather than keeping them at
the end of the list, after all YUV formats?

>  	/* yuv420 */
>  	{
>  		.fourcc = V4L2_PIX_FMT_NV21,
> @@ -274,26 +294,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
>  	},
> -	/* yuv400 */
> -	{
> -		.fourcc = V4L2_PIX_FMT_GREY,
> -		.uv_swap = 0,
> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> -	},
> -	/* rgb */
> -	{
> -		.fourcc = V4L2_PIX_FMT_XBGR32,
> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> -	}, {
> -		.fourcc = V4L2_PIX_FMT_RGB565,
> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> -		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> -	},
>  };
>  
>  static const struct rkisp1_capture_config rkisp1_capture_config_mp = {
> @@ -334,6 +334,29 @@ rkisp1_vdev_to_node(struct video_device *vdev)
>  	return container_of(vdev, struct rkisp1_vdev_node, vdev);
>  }
>  
> +int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap,
> +			       struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	const struct rkisp1_capture_fmt_cfg *fmts = cap->config->fmts;
> +	u32 curr_mbus = fmts[0].mbus;
> +	int i, n = 0;
> +
> +	if (code->index == 0) {
> +		code->code = fmts[0].mbus;
> +		return 0;
> +	}
> +
> +	for (i = 1; i < cap->config->fmt_size; i++)

How about just initializing curr_mbus to MEDIA_BUS_FMT_FIXED? This is not
supposed to be found in the array, so is a safe initial value, which would
allow removing the explicit handling of index == 0.

> +		if (fmts[i].mbus != curr_mbus) {

As Helen mentioned, we could use the continue keyword to skip the iteration
early and make it obvious that rest of the loop is entirely for the case
where mbus != curr_mbus, removing one level of nesting.

> +			curr_mbus = fmts[i].mbus;
> +			if (++n == code->index) {
> +				code->code = curr_mbus;
> +				return 0;
> +			}
> +		}

According to the kernel coding style guidelines [1], this for loop should
have braces.

[1] https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces

Best regards,
Tomasz

Re: [PATCH v3 06/10] media: staging: rkisp1: add a helper function to enumerate supported mbus formats on capture

[Dafna Hirschfeld]

(resending as plain text)

On Sun, Aug 30, 2020 at 4:43 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> Hi Dafna,
>
> On Thu, Jul 23, 2020 at 03:20:10PM +0200, Dafna Hirschfeld wrote:
> > Add a function 'rkisp1_cap_enum_mbus_codes' that receive
> > a pointer to 'v4l2_subdev_mbus_code_enum' and returns the
> > next supported mbus format of the capture. The function
> > assumes that pixel formats with identical 'mbus' are grouped
> > together in the hardcoded arrays, therefore the order of the
> > entries in the array 'rkisp1_sp_fmts' are adjusted.
> > This function is a helper for the media bus enumeration of
> > the source pad of the resizer entity.
> >
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > ---
> >  drivers/staging/media/rkisp1/rkisp1-capture.c | 77 ++++++++++++-------
> >  drivers/staging/media/rkisp1/rkisp1-common.h  |  8 ++
> >  2 files changed, 58 insertions(+), 27 deletions(-)
> >
>
> Thank you for the patch. Please see my comments inline.
>
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > index 5dd91b5f82a4..4dabd07a3da9 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > @@ -112,6 +112,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
> >               .write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> >               .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >       },
> > +     /* yuv400 */
> > +     {
> > +             .fourcc = V4L2_PIX_FMT_GREY,
> > +             .uv_swap = 0,
> > +             .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> > +             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > +     },
> >       /* yuv420 */
> >       {
> >               .fourcc = V4L2_PIX_FMT_NV21,
> > @@ -144,13 +151,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
> >               .write_format = RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8,
> >               .mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >       },
> > -     /* yuv400 */
> > -     {
> > -             .fourcc = V4L2_PIX_FMT_GREY,
> > -             .uv_swap = 0,
> > -             .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> > -             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > -     },
> >       /* raw */
> >       {
> >               .fourcc = V4L2_PIX_FMT_SRGGB8,
> > @@ -236,6 +236,26 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
> >               .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> >               .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >       },
> > +     /* yuv400 */
> > +     {
> > +             .fourcc = V4L2_PIX_FMT_GREY,
> > +             .uv_swap = 0,
> > +             .write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> > +             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> > +             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > +     },
> > +     /* rgb */
> > +     {
> > +             .fourcc = V4L2_PIX_FMT_XBGR32,
> > +             .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > +             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> > +             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > +     }, {
> > +             .fourcc = V4L2_PIX_FMT_RGB565,
> > +             .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > +             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> > +             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > +     },
>
> Is there any reason to move RGB formats here rather than keeping them at
> the end of the list, after all YUV formats?

Hi,
yes, the new function 'rkisp1_cap_enum_mbus_codes' assumes that
pixelformats with
identical mbus codes are grouped together. It uses it to iterate the
formats and increment
a counter when encountering a new mbus.

>
> >       /* yuv420 */
> >       {
> >               .fourcc = V4L2_PIX_FMT_NV21,
> > @@ -274,26 +294,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
> >               .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV420,
> >               .mbus = MEDIA_BUS_FMT_YUYV8_1_5X8,
> >       },
> > -     /* yuv400 */
> > -     {
> > -             .fourcc = V4L2_PIX_FMT_GREY,
> > -             .uv_swap = 0,
> > -             .write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> > -             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV400,
> > -             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > -     },
> > -     /* rgb */
> > -     {
> > -             .fourcc = V4L2_PIX_FMT_XBGR32,
> > -             .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > -             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
> > -             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > -     }, {
> > -             .fourcc = V4L2_PIX_FMT_RGB565,
> > -             .write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> > -             .output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> > -             .mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > -     },
> >  };
> >
> >  static const struct rkisp1_capture_config rkisp1_capture_config_mp = {
> > @@ -334,6 +334,29 @@ rkisp1_vdev_to_node(struct video_device *vdev)
> >       return container_of(vdev, struct rkisp1_vdev_node, vdev);
> >  }
> >
> > +int rkisp1_cap_enum_mbus_codes(struct rkisp1_capture *cap,
> > +                            struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > +     const struct rkisp1_capture_fmt_cfg *fmts = cap->config->fmts;
> > +     u32 curr_mbus = fmts[0].mbus;
> > +     int i, n = 0;
> > +
> > +     if (code->index == 0) {
> > +             code->code = fmts[0].mbus;
> > +             return 0;
> > +     }
> > +
> > +     for (i = 1; i < cap->config->fmt_size; i++)
>
> How about just initializing curr_mbus to MEDIA_BUS_FMT_FIXED? This is not
> supposed to be found in the array, so is a safe initial value, which would
> allow removing the explicit handling of index == 0.

yes, I'll do that.

>
> > +             if (fmts[i].mbus != curr_mbus) {
>
> As Helen mentioned, we could use the continue keyword to skip the iteration
> early and make it obvious that rest of the loop is entirely for the case
> where mbus != curr_mbus, removing one level of nesting.
>

that too,

> > +                     curr_mbus = fmts[i].mbus;
> > +                     if (++n == code->index) {
> > +                             code->code = curr_mbus;
> > +                             return 0;
> > +                     }
> > +             }
>
> According to the kernel coding style guidelines [1], this for loop should
> have braces.
>
> [1] https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces
>
thanks, I didn't know that.



Thanks,
Dafna

> Best regards,
> Tomasz

Re: [PATCH v3 01/10] media: staging: rkisp1: cap: change RGB24 format to XBGR32

[Dafna Hirschfeld]

Am 04.08.20 um 01:49 schrieb Helen Koike:
> 
> 
> On 7/23/20 10:20 AM, Dafna Hirschfeld wrote:
>> According to the TRM [1], the YUV->RGB conversion outputs
>> RGB 888 format with 4 bytes, where the last byte is ignored,
>> using big endian representation:
>>
>> ________________________________
> 
> For some reason, it seems that patchwork ignored the rest of the message from this line
> 
> https://patchwork.linuxtv.org/project/linux-media/patch/20200723132014.4597-2-dafna.hirschfeld@collabora.com/
> 
> This is just a thing to be careful when picking from patchwork.

I think this line signifies that what under it is a comment.
I'll remove it

Thanks,
Dafna


> 
> Regards,
> Helen
> 
> 
>> |___X___|___R___|___G___|___B___|
>> 31      24      16      8       0
>>
>> Which matches format V4L2_PIX_FMT_XBGR32 in little endian
>> representation, so replace it accordingly.
>>
>> "24 bit word". What it means is that 4 bytes are used with
>> 24bit for the RGB and the last byte is ignored.
>> This matches format V4L2_PIX_FMT_XBGR32.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Acked-by: Helen Koike <helen.koike@collabora.com>
>> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index c05280950ea0..2333d2dcd2e6 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -276,7 +276,7 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>>   	},
>>   	/* rgb */
>>   	{
>> -		.fourcc = V4L2_PIX_FMT_RGB24,
>> +		.fourcc = V4L2_PIX_FMT_XBGR32,
>>   		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>>   		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB888,
>>   	}, {
>>

Re: [PATCH v3 09/10] media: rkisp1: cap: simplify the link validation by compering the media bus code

[Niklas Söderlund]

Hi Dafna,

This commit is not just a simplification but a change of behavior.  The 
change is for the better but it broke capture of NV12 and NV21 formats 
in libcamera unexpectedly.

The issue at hand is that libcamera when configuring the pipeline 
retrieves the mbus code for the ISP (rkisp1_isp) source pad (2) and then 
propagates it to the resizer (rkisp_resizer_{main,self}path) sink pad 
(0) and then to the resizers source pad (1). Effectively propagating 
MEDIA_BUS_FMT_YUYV8_2X8 for all formats.

At this point if the video node (main or self) is configured with a 
YUV420 format (NV12, NV21, etc) and with this change applied the link 
validation will fail as MEDIA_BUS_FMT_YUYV8_1_5X8 !=  
MEDIA_BUS_FMT_YUYV8_2X8. Given the nature of how link validation is 
implemented it's VIDIOC_QBUF that returns a -EPIPE when it fails and 
libcamera lockup the capture session.

I will submit a fix for this to libcamera to bring it in sync with this 
change.

Would it be possible to ask that future changes to the rkisp1 driver be 
tested with libcamera so we can track and update both the kernel and 
user-space components of this driver at the same time and avoid nasty 
surprises? :-)

On 2020-07-23 15:20:13 +0200, Dafna Hirschfeld wrote:
> The capture has a mapping of the mbus code needed for each pixelformat.
> This can be used to simplify the link validation by comparing the mbus
> code in the capture with the code in the resizer.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 4dabd07a3da9..a5e2521577dd 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -1255,22 +1255,11 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>  	struct v4l2_subdev *sd =
>  		media_entity_to_v4l2_subdev(link->source->entity);
>  	struct rkisp1_capture *cap = video_get_drvdata(vdev);
> -	struct rkisp1_isp *isp = &cap->rkisp1->isp;
> -	u8 isp_pix_enc = isp->src_fmt->pixel_enc;
> -	u8 cap_pix_enc = cap->pix.info->pixel_enc;
> +	const struct rkisp1_capture_fmt_cfg *fmt =
> +		rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
>  	struct v4l2_subdev_format sd_fmt;
>  	int ret;
>  
> -	if (cap_pix_enc != isp_pix_enc &&
> -	    !(isp_pix_enc == V4L2_PIXEL_ENC_YUV &&
> -	      cap_pix_enc == V4L2_PIXEL_ENC_RGB)) {
> -		dev_err(cap->rkisp1->dev,
> -			"format type mismatch in link '%s:%d->%s:%d'\n",
> -			link->source->entity->name, link->source->index,
> -			link->sink->entity->name, link->sink->index);
> -		return -EPIPE;
> -	}
> -
>  	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>  	sd_fmt.pad = link->source->index;
>  	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_fmt);
> @@ -1278,7 +1267,8 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>  		return ret;
>  
>  	if (sd_fmt.format.height != cap->pix.fmt.height ||
> -	    sd_fmt.format.width != cap->pix.fmt.width)
> +	    sd_fmt.format.width != cap->pix.fmt.width ||
> +	    sd_fmt.format.code != fmt->mbus)
>  		return -EPIPE;
>  
>  	return 0;
> -- 
> 2.17.1
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v3 09/10] media: rkisp1: cap: simplify the link validation by compering the media bus code

[Laurent Pinchart]

Hello,

On Wed, Sep 30, 2020 at 09:00:25PM +0200, Niklas Söderlund wrote:
> Hi Dafna,
> 
> This commit is not just a simplification but a change of behavior.  The 
> change is for the better but it broke capture of NV12 and NV21 formats 
> in libcamera unexpectedly.
> 
> The issue at hand is that libcamera when configuring the pipeline 
> retrieves the mbus code for the ISP (rkisp1_isp) source pad (2) and then 
> propagates it to the resizer (rkisp_resizer_{main,self}path) sink pad 
> (0) and then to the resizers source pad (1). Effectively propagating 
> MEDIA_BUS_FMT_YUYV8_2X8 for all formats.
> 
> At this point if the video node (main or self) is configured with a 
> YUV420 format (NV12, NV21, etc) and with this change applied the link 
> validation will fail as MEDIA_BUS_FMT_YUYV8_1_5X8 !=  
> MEDIA_BUS_FMT_YUYV8_2X8. Given the nature of how link validation is 
> implemented it's VIDIOC_QBUF that returns a -EPIPE when it fails and 
> libcamera lockup the capture session.

I would be very, very surprised is the hardware really used YUYV8_1_5X8.
YUYV8_1X16 is a much more likely bus format between the resizer and the
DMA engine, as well as between the ISP and the resizer.

> I will submit a fix for this to libcamera to bring it in sync with this 
> change.
> 
> Would it be possible to ask that future changes to the rkisp1 driver be 
> tested with libcamera so we can track and update both the kernel and 
> user-space components of this driver at the same time and avoid nasty 
> surprises? :-)

I strongly second this. Drivers that are supported in libcamera should
be tested with libcamera to catch regressions, for any chance submitted
to the kernel.

> On 2020-07-23 15:20:13 +0200, Dafna Hirschfeld wrote:
> > The capture has a mapping of the mbus code needed for each pixelformat.
> > This can be used to simplify the link validation by comparing the mbus
> > code in the capture with the code in the resizer.
> > 
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > ---
> >  drivers/staging/media/rkisp1/rkisp1-capture.c | 18 ++++--------------
> >  1 file changed, 4 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > index 4dabd07a3da9..a5e2521577dd 100644
> > --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> > +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> > @@ -1255,22 +1255,11 @@ static int rkisp1_capture_link_validate(struct media_link *link)
> >  	struct v4l2_subdev *sd =
> >  		media_entity_to_v4l2_subdev(link->source->entity);
> >  	struct rkisp1_capture *cap = video_get_drvdata(vdev);
> > -	struct rkisp1_isp *isp = &cap->rkisp1->isp;
> > -	u8 isp_pix_enc = isp->src_fmt->pixel_enc;
> > -	u8 cap_pix_enc = cap->pix.info->pixel_enc;
> > +	const struct rkisp1_capture_fmt_cfg *fmt =
> > +		rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
> >  	struct v4l2_subdev_format sd_fmt;
> >  	int ret;
> >  
> > -	if (cap_pix_enc != isp_pix_enc &&
> > -	    !(isp_pix_enc == V4L2_PIXEL_ENC_YUV &&
> > -	      cap_pix_enc == V4L2_PIXEL_ENC_RGB)) {
> > -		dev_err(cap->rkisp1->dev,
> > -			"format type mismatch in link '%s:%d->%s:%d'\n",
> > -			link->source->entity->name, link->source->index,
> > -			link->sink->entity->name, link->sink->index);
> > -		return -EPIPE;
> > -	}
> > -
> >  	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >  	sd_fmt.pad = link->source->index;
> >  	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_fmt);
> > @@ -1278,7 +1267,8 @@ static int rkisp1_capture_link_validate(struct media_link *link)
> >  		return ret;
> >  
> >  	if (sd_fmt.format.height != cap->pix.fmt.height ||
> > -	    sd_fmt.format.width != cap->pix.fmt.width)
> > +	    sd_fmt.format.width != cap->pix.fmt.width ||
> > +	    sd_fmt.format.code != fmt->mbus)
> >  		return -EPIPE;
> >  
> >  	return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 09/10] media: rkisp1: cap: simplify the link validation by compering the media bus code

[Dafna Hirschfeld]

Am 01.10.20 um 04:03 schrieb Laurent Pinchart:
> Hello,
> 
> On Wed, Sep 30, 2020 at 09:00:25PM +0200, Niklas Söderlund wrote:
>> Hi Dafna,
>>
>> This commit is not just a simplification but a change of behavior.  The
>> change is for the better but it broke capture of NV12 and NV21 formats
>> in libcamera unexpectedly.
>>
>> The issue at hand is that libcamera when configuring the pipeline
>> retrieves the mbus code for the ISP (rkisp1_isp) source pad (2) and then
>> propagates it to the resizer (rkisp_resizer_{main,self}path) sink pad
>> (0) and then to the resizers source pad (1). Effectively propagating
>> MEDIA_BUS_FMT_YUYV8_2X8 for all formats.
>>
>> At this point if the video node (main or self) is configured with a
>> YUV420 format (NV12, NV21, etc) and with this change applied the link
>> validation will fail as MEDIA_BUS_FMT_YUYV8_1_5X8 !=
>> MEDIA_BUS_FMT_YUYV8_2X8. Given the nature of how link validation is
>> implemented it's VIDIOC_QBUF that returns a -EPIPE when it fails and
>> libcamera lockup the capture session.
> 
> I would be very, very surprised is the hardware really used YUYV8_1_5X8.
> YUYV8_1X16 is a much more likely bus format between the resizer and the
> DMA engine, as well as between the ISP and the resizer.

Format YUYV8_1X16 is for downsampling of 4:2:2, but the resizer has the ability
to downsample to 4:2:0.
I see there is also format YDYUYDYV8_1X16 for 4:2:0
maybe this is what I should set?

Actually according to the TRM the resizer send the stream to the DMA
engine through two separated buses, one for luma and one for chroma.

> 
>> I will submit a fix for this to libcamera to bring it in sync with this
>> change.
>>
>> Would it be possible to ask that future changes to the rkisp1 driver be
>> tested with libcamera so we can track and update both the kernel and
>> user-space components of this driver at the same time and avoid nasty
>> surprises? :-)
> 
> I strongly second this. Drivers that are supported in libcamera should
> be tested with libcamera to catch regressions, for any chance submitted
> to the kernel.

I can run several 'cam' commands with different formats and dimensions to
find regressions. I currently have unit test only in v4l-utils.

Thanks,
Dafna

> 
>> On 2020-07-23 15:20:13 +0200, Dafna Hirschfeld wrote:
>>> The capture has a mapping of the mbus code needed for each pixelformat.
>>> This can be used to simplify the link validation by comparing the mbus
>>> code in the capture with the code in the resizer.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> ---
>>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 18 ++++--------------
>>>   1 file changed, 4 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> index 4dabd07a3da9..a5e2521577dd 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> @@ -1255,22 +1255,11 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>>>   	struct v4l2_subdev *sd =
>>>   		media_entity_to_v4l2_subdev(link->source->entity);
>>>   	struct rkisp1_capture *cap = video_get_drvdata(vdev);
>>> -	struct rkisp1_isp *isp = &cap->rkisp1->isp;
>>> -	u8 isp_pix_enc = isp->src_fmt->pixel_enc;
>>> -	u8 cap_pix_enc = cap->pix.info->pixel_enc;
>>> +	const struct rkisp1_capture_fmt_cfg *fmt =
>>> +		rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
>>>   	struct v4l2_subdev_format sd_fmt;
>>>   	int ret;
>>>   
>>> -	if (cap_pix_enc != isp_pix_enc &&
>>> -	    !(isp_pix_enc == V4L2_PIXEL_ENC_YUV &&
>>> -	      cap_pix_enc == V4L2_PIXEL_ENC_RGB)) {
>>> -		dev_err(cap->rkisp1->dev,
>>> -			"format type mismatch in link '%s:%d->%s:%d'\n",
>>> -			link->source->entity->name, link->source->index,
>>> -			link->sink->entity->name, link->sink->index);
>>> -		return -EPIPE;
>>> -	}
>>> -
>>>   	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>>   	sd_fmt.pad = link->source->index;
>>>   	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_fmt);
>>> @@ -1278,7 +1267,8 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>>>   		return ret;
>>>   
>>>   	if (sd_fmt.format.height != cap->pix.fmt.height ||
>>> -	    sd_fmt.format.width != cap->pix.fmt.width)
>>> +	    sd_fmt.format.width != cap->pix.fmt.width ||
>>> +	    sd_fmt.format.code != fmt->mbus)
>>>   		return -EPIPE;
>>>   
>>>   	return 0;
> 

Re: [PATCH v3 09/10] media: rkisp1: cap: simplify the link validation by compering the media bus code

[Laurent Pinchart]

Hi Dafna,

On Thu, Oct 01, 2020 at 09:36:07PM +0200, Dafna Hirschfeld wrote:
> Am 01.10.20 um 04:03 schrieb Laurent Pinchart:
> > On Wed, Sep 30, 2020 at 09:00:25PM +0200, Niklas Söderlund wrote:
> >> Hi Dafna,
> >>
> >> This commit is not just a simplification but a change of behavior.  The
> >> change is for the better but it broke capture of NV12 and NV21 formats
> >> in libcamera unexpectedly.
> >>
> >> The issue at hand is that libcamera when configuring the pipeline
> >> retrieves the mbus code for the ISP (rkisp1_isp) source pad (2) and then
> >> propagates it to the resizer (rkisp_resizer_{main,self}path) sink pad
> >> (0) and then to the resizers source pad (1). Effectively propagating
> >> MEDIA_BUS_FMT_YUYV8_2X8 for all formats.
> >>
> >> At this point if the video node (main or self) is configured with a
> >> YUV420 format (NV12, NV21, etc) and with this change applied the link
> >> validation will fail as MEDIA_BUS_FMT_YUYV8_1_5X8 !=
> >> MEDIA_BUS_FMT_YUYV8_2X8. Given the nature of how link validation is
> >> implemented it's VIDIOC_QBUF that returns a -EPIPE when it fails and
> >> libcamera lockup the capture session.
> > 
> > I would be very, very surprised is the hardware really used YUYV8_1_5X8.
> > YUYV8_1X16 is a much more likely bus format between the resizer and the
> > DMA engine, as well as between the ISP and the resizer.
> 
> Format YUYV8_1X16 is for downsampling of 4:2:2, but the resizer has the ability
> to downsample to 4:2:0.
> I see there is also format YDYUYDYV8_1X16 for 4:2:0
> maybe this is what I should set?
> 
> Actually according to the TRM the resizer send the stream to the DMA
> engine through two separated buses, one for luma and one for chroma.

In which document is this documented ? Is this two 8-bit buses side by
side ?

Looking at the registers, the output formats are controlled by the
global MI_CTRL register, common to both the main and self paths, which
should correspond to the DMA engine. I think it would make sense to
model this at the video node level, and hardcode YUYV8_1X16 between the
resizer and the video node.

> >> I will submit a fix for this to libcamera to bring it in sync with this
> >> change.
> >>
> >> Would it be possible to ask that future changes to the rkisp1 driver be
> >> tested with libcamera so we can track and update both the kernel and
> >> user-space components of this driver at the same time and avoid nasty
> >> surprises? :-)
> > 
> > I strongly second this. Drivers that are supported in libcamera should
> > be tested with libcamera to catch regressions, for any chance submitted
> > to the kernel.
> 
> I can run several 'cam' commands with different formats and dimensions to
> find regressions. I currently have unit test only in v4l-utils.

That would be great :-) We will work on a test suite for higher-level
tests (something similar to the Android CTS) at some point, which should
also help catching regressions.

> >> On 2020-07-23 15:20:13 +0200, Dafna Hirschfeld wrote:
> >>> The capture has a mapping of the mbus code needed for each pixelformat.
> >>> This can be used to simplify the link validation by comparing the mbus
> >>> code in the capture with the code in the resizer.
> >>>
> >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >>> ---
> >>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 18 ++++--------------
> >>>   1 file changed, 4 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> >>> index 4dabd07a3da9..a5e2521577dd 100644
> >>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> >>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> >>> @@ -1255,22 +1255,11 @@ static int rkisp1_capture_link_validate(struct media_link *link)
> >>>   	struct v4l2_subdev *sd =
> >>>   		media_entity_to_v4l2_subdev(link->source->entity);
> >>>   	struct rkisp1_capture *cap = video_get_drvdata(vdev);
> >>> -	struct rkisp1_isp *isp = &cap->rkisp1->isp;
> >>> -	u8 isp_pix_enc = isp->src_fmt->pixel_enc;
> >>> -	u8 cap_pix_enc = cap->pix.info->pixel_enc;
> >>> +	const struct rkisp1_capture_fmt_cfg *fmt =
> >>> +		rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
> >>>   	struct v4l2_subdev_format sd_fmt;
> >>>   	int ret;
> >>>   
> >>> -	if (cap_pix_enc != isp_pix_enc &&
> >>> -	    !(isp_pix_enc == V4L2_PIXEL_ENC_YUV &&
> >>> -	      cap_pix_enc == V4L2_PIXEL_ENC_RGB)) {
> >>> -		dev_err(cap->rkisp1->dev,
> >>> -			"format type mismatch in link '%s:%d->%s:%d'\n",
> >>> -			link->source->entity->name, link->source->index,
> >>> -			link->sink->entity->name, link->sink->index);
> >>> -		return -EPIPE;
> >>> -	}
> >>> -
> >>>   	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> >>>   	sd_fmt.pad = link->source->index;
> >>>   	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_fmt);
> >>> @@ -1278,7 +1267,8 @@ static int rkisp1_capture_link_validate(struct media_link *link)
> >>>   		return ret;
> >>>   
> >>>   	if (sd_fmt.format.height != cap->pix.fmt.height ||
> >>> -	    sd_fmt.format.width != cap->pix.fmt.width)
> >>> +	    sd_fmt.format.width != cap->pix.fmt.width ||
> >>> +	    sd_fmt.format.code != fmt->mbus)
> >>>   		return -EPIPE;
> >>>   
> >>>   	return 0;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 09/10] media: rkisp1: cap: simplify the link validation by compering the media bus code

[Dafna Hirschfeld]

Hi,


Am 02.10.20 um 00:48 schrieb Laurent Pinchart:
> Hi Dafna,
> 
> On Thu, Oct 01, 2020 at 09:36:07PM +0200, Dafna Hirschfeld wrote:
>> Am 01.10.20 um 04:03 schrieb Laurent Pinchart:
>>> On Wed, Sep 30, 2020 at 09:00:25PM +0200, Niklas Söderlund wrote:
>>>> Hi Dafna,
>>>>
>>>> This commit is not just a simplification but a change of behavior.  The
>>>> change is for the better but it broke capture of NV12 and NV21 formats
>>>> in libcamera unexpectedly.
>>>>
>>>> The issue at hand is that libcamera when configuring the pipeline
>>>> retrieves the mbus code for the ISP (rkisp1_isp) source pad (2) and then
>>>> propagates it to the resizer (rkisp_resizer_{main,self}path) sink pad
>>>> (0) and then to the resizers source pad (1). Effectively propagating
>>>> MEDIA_BUS_FMT_YUYV8_2X8 for all formats.
>>>>
>>>> At this point if the video node (main or self) is configured with a
>>>> YUV420 format (NV12, NV21, etc) and with this change applied the link
>>>> validation will fail as MEDIA_BUS_FMT_YUYV8_1_5X8 !=
>>>> MEDIA_BUS_FMT_YUYV8_2X8. Given the nature of how link validation is
>>>> implemented it's VIDIOC_QBUF that returns a -EPIPE when it fails and
>>>> libcamera lockup the capture session.
>>>
>>> I would be very, very surprised is the hardware really used YUYV8_1_5X8.
>>> YUYV8_1X16 is a much more likely bus format between the resizer and the
>>> DMA engine, as well as between the ISP and the resizer.
>>
>> Format YUYV8_1X16 is for downsampling of 4:2:2, but the resizer has the ability
>> to downsample to 4:2:0.
>> I see there is also format YDYUYDYV8_1X16 for 4:2:0
>> maybe this is what I should set?
>>
>> Actually according to the TRM the resizer send the stream to the DMA
>> engine through two separated buses, one for luma and one for chroma.
> 
> In which document is this documented ? Is this two 8-bit buses side by
> side ?

here: https://rockchip.fr/RK3288%20TRM/rk3288-chapter-31-image-signal-processing-(isp).pdf

It is described that there is a splitter component before the
resizer. The diagram shows that those two separate streams are also
sent further to the DMA engine.

> 
> Looking at the registers, the output formats are controlled by the
> global MI_CTRL register, common to both the main and self paths, which
> should correspond to the DMA engine. I think it would make sense to
> model this at the video node level, and hardcode YUYV8_1X16 between the
> resizer and the video node.

Ok, I can change the mbus code. Is there a difference from userspace point of
view? If userspace wants to stream YUV 4:2:0, he should set the pixelformat on
the video node and should only care that the resizer and the video node agree
on the mbus code.

Thanks,
Dafna

> 
>>>> I will submit a fix for this to libcamera to bring it in sync with this
>>>> change.
>>>>
>>>> Would it be possible to ask that future changes to the rkisp1 driver be
>>>> tested with libcamera so we can track and update both the kernel and
>>>> user-space components of this driver at the same time and avoid nasty
>>>> surprises? :-)
>>>
>>> I strongly second this. Drivers that are supported in libcamera should
>>> be tested with libcamera to catch regressions, for any chance submitted
>>> to the kernel.
>>
>> I can run several 'cam' commands with different formats and dimensions to
>> find regressions. I currently have unit test only in v4l-utils.
> 
> That would be great :-) We will work on a test suite for higher-level
> tests (something similar to the Android CTS) at some point, which should
> also help catching regressions.
> 
>>>> On 2020-07-23 15:20:13 +0200, Dafna Hirschfeld wrote:
>>>>> The capture has a mapping of the mbus code needed for each pixelformat.
>>>>> This can be used to simplify the link validation by comparing the mbus
>>>>> code in the capture with the code in the resizer.
>>>>>
>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>>> ---
>>>>>    drivers/staging/media/rkisp1/rkisp1-capture.c | 18 ++++--------------
>>>>>    1 file changed, 4 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>>> index 4dabd07a3da9..a5e2521577dd 100644
>>>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>>> @@ -1255,22 +1255,11 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>>>>>    	struct v4l2_subdev *sd =
>>>>>    		media_entity_to_v4l2_subdev(link->source->entity);
>>>>>    	struct rkisp1_capture *cap = video_get_drvdata(vdev);
>>>>> -	struct rkisp1_isp *isp = &cap->rkisp1->isp;
>>>>> -	u8 isp_pix_enc = isp->src_fmt->pixel_enc;
>>>>> -	u8 cap_pix_enc = cap->pix.info->pixel_enc;
>>>>> +	const struct rkisp1_capture_fmt_cfg *fmt =
>>>>> +		rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
>>>>>    	struct v4l2_subdev_format sd_fmt;
>>>>>    	int ret;
>>>>>    
>>>>> -	if (cap_pix_enc != isp_pix_enc &&
>>>>> -	    !(isp_pix_enc == V4L2_PIXEL_ENC_YUV &&
>>>>> -	      cap_pix_enc == V4L2_PIXEL_ENC_RGB)) {
>>>>> -		dev_err(cap->rkisp1->dev,
>>>>> -			"format type mismatch in link '%s:%d->%s:%d'\n",
>>>>> -			link->source->entity->name, link->source->index,
>>>>> -			link->sink->entity->name, link->sink->index);
>>>>> -		return -EPIPE;
>>>>> -	}
>>>>> -
>>>>>    	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>>>>    	sd_fmt.pad = link->source->index;
>>>>>    	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_fmt);
>>>>> @@ -1278,7 +1267,8 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>>>>>    		return ret;
>>>>>    
>>>>>    	if (sd_fmt.format.height != cap->pix.fmt.height ||
>>>>> -	    sd_fmt.format.width != cap->pix.fmt.width)
>>>>> +	    sd_fmt.format.width != cap->pix.fmt.width ||
>>>>> +	    sd_fmt.format.code != fmt->mbus)
>>>>>    		return -EPIPE;
>>>>>    
>>>>>    	return 0;
> 

Re: [PATCH v3 09/10] media: rkisp1: cap: simplify the link validation by compering the media bus code

[Helen Koike]

Hi Laurent,

Thanks for your comments.
There is one thing that is still confusing to me (please see my question
below).

On 10/1/20 7:48 PM, Laurent Pinchart wrote:
> Hi Dafna,
> 
> On Thu, Oct 01, 2020 at 09:36:07PM +0200, Dafna Hirschfeld wrote:
>> Am 01.10.20 um 04:03 schrieb Laurent Pinchart:
>>> On Wed, Sep 30, 2020 at 09:00:25PM +0200, Niklas Söderlund wrote:
>>>> Hi Dafna,
>>>>
>>>> This commit is not just a simplification but a change of behavior.  The
>>>> change is for the better but it broke capture of NV12 and NV21 formats
>>>> in libcamera unexpectedly.
>>>>
>>>> The issue at hand is that libcamera when configuring the pipeline
>>>> retrieves the mbus code for the ISP (rkisp1_isp) source pad (2) and then
>>>> propagates it to the resizer (rkisp_resizer_{main,self}path) sink pad
>>>> (0) and then to the resizers source pad (1). Effectively propagating
>>>> MEDIA_BUS_FMT_YUYV8_2X8 for all formats.
>>>>
>>>> At this point if the video node (main or self) is configured with a
>>>> YUV420 format (NV12, NV21, etc) and with this change applied the link
>>>> validation will fail as MEDIA_BUS_FMT_YUYV8_1_5X8 !=
>>>> MEDIA_BUS_FMT_YUYV8_2X8. Given the nature of how link validation is
>>>> implemented it's VIDIOC_QBUF that returns a -EPIPE when it fails and
>>>> libcamera lockup the capture session.
>>>
>>> I would be very, very surprised is the hardware really used YUYV8_1_5X8.
>>> YUYV8_1X16 is a much more likely bus format between the resizer and the
>>> DMA engine, as well as between the ISP and the resizer.
>>
>> Format YUYV8_1X16 is for downsampling of 4:2:2, but the resizer has the ability
>> to downsample to 4:2:0.
>> I see there is also format YDYUYDYV8_1X16 for 4:2:0
>> maybe this is what I should set?
>>
>> Actually according to the TRM the resizer send the stream to the DMA
>> engine through two separated buses, one for luma and one for chroma.
> 
> In which document is this documented ? Is this two 8-bit buses side by
> side ?
> 
> Looking at the registers, the output formats are controlled by the
> global MI_CTRL register, common to both the main and self paths, which
> should correspond to the DMA engine. I think it would make sense to
> model this at the video node level, and hardcode YUYV8_1X16 between the
> resizer and the video node.

If I understand correctly, in a 4:2:0 format, we have 4 luminance
components per chrominance.

And with the YUYV8_1X16, we have 2 luminance per chrominance.

Are you suggesting that, when userspace sets 4:2:0 (NV12, NV21, YU12, YV12),
we should use MEDIA_BUS_FMT_YUYV8_1X16 between the resizer and the DMA engine?

But then, down sampling rate here won't match (this is where my confusion
comes from).

Or, are you assuming that the DMA engine receives 4:2:2 and performs the
conversion?

I would appreciate if you could help clarify this.


Just to note here, in the docs Dafna pointed, it's written:

"The Resize module is able to process luminance and chrominance data
independently, i.e. there are separate pipelines for luminance and
chrominance processing using dedicated scale factors and phase offsets.
This allows format conversion to be done by the Resize block (YCbCr 4:2:2 to
4:2:0, 4:1:1, 4:1:0)."

Thanks
Helen


> 
>>>> I will submit a fix for this to libcamera to bring it in sync with this
>>>> change.
>>>>
>>>> Would it be possible to ask that future changes to the rkisp1 driver be
>>>> tested with libcamera so we can track and update both the kernel and
>>>> user-space components of this driver at the same time and avoid nasty
>>>> surprises? :-)
>>>
>>> I strongly second this. Drivers that are supported in libcamera should
>>> be tested with libcamera to catch regressions, for any chance submitted
>>> to the kernel.
>>
>> I can run several 'cam' commands with different formats and dimensions to
>> find regressions. I currently have unit test only in v4l-utils.
> 
> That would be great :-) We will work on a test suite for higher-level
> tests (something similar to the Android CTS) at some point, which should
> also help catching regressions.
> 
>>>> On 2020-07-23 15:20:13 +0200, Dafna Hirschfeld wrote:
>>>>> The capture has a mapping of the mbus code needed for each pixelformat.
>>>>> This can be used to simplify the link validation by comparing the mbus
>>>>> code in the capture with the code in the resizer.
>>>>>
>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>>> ---
>>>>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 18 ++++--------------
>>>>>   1 file changed, 4 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>>> index 4dabd07a3da9..a5e2521577dd 100644
>>>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>>> @@ -1255,22 +1255,11 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>>>>>   	struct v4l2_subdev *sd =
>>>>>   		media_entity_to_v4l2_subdev(link->source->entity);
>>>>>   	struct rkisp1_capture *cap = video_get_drvdata(vdev);
>>>>> -	struct rkisp1_isp *isp = &cap->rkisp1->isp;
>>>>> -	u8 isp_pix_enc = isp->src_fmt->pixel_enc;
>>>>> -	u8 cap_pix_enc = cap->pix.info->pixel_enc;
>>>>> +	const struct rkisp1_capture_fmt_cfg *fmt =
>>>>> +		rkisp1_find_fmt_cfg(cap, cap->pix.fmt.pixelformat);
>>>>>   	struct v4l2_subdev_format sd_fmt;
>>>>>   	int ret;
>>>>>   
>>>>> -	if (cap_pix_enc != isp_pix_enc &&
>>>>> -	    !(isp_pix_enc == V4L2_PIXEL_ENC_YUV &&
>>>>> -	      cap_pix_enc == V4L2_PIXEL_ENC_RGB)) {
>>>>> -		dev_err(cap->rkisp1->dev,
>>>>> -			"format type mismatch in link '%s:%d->%s:%d'\n",
>>>>> -			link->source->entity->name, link->source->index,
>>>>> -			link->sink->entity->name, link->sink->index);
>>>>> -		return -EPIPE;
>>>>> -	}
>>>>> -
>>>>>   	sd_fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
>>>>>   	sd_fmt.pad = link->source->index;
>>>>>   	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &sd_fmt);
>>>>> @@ -1278,7 +1267,8 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>>>>>   		return ret;
>>>>>   
>>>>>   	if (sd_fmt.format.height != cap->pix.fmt.height ||
>>>>> -	    sd_fmt.format.width != cap->pix.fmt.width)
>>>>> +	    sd_fmt.format.width != cap->pix.fmt.width ||
>>>>> +	    sd_fmt.format.code != fmt->mbus)
>>>>>   		return -EPIPE;
>>>>>   
>>>>>   	return 0;
>