[PATCH 0/2] media: rkisp1: Add support for UYVY output

[PATCH 0/2] media: rkisp1: Add support for UYVY output

[Paul Elder]

This series adds support for UYVY as an output format in the rkisp1
driver, for devices that support it. Notably, the i.MX8MP supports it,
while the rk3399 does not.

This series is based on branch pinchartl/v5.19/isp/1 [1] at this repo [2].

[1] https://gitlab.com/ideasonboard/nxp/linux/-/commits/pinchartl/v5.19/isp/1
[2] https://gitlab.com/ideasonboard/nxp/linux

Paul Elder (2):
  media: rkisp1: Add YC swap capability
  media: rkisp1: Add UYVY as an output format

 .../platform/rockchip/rkisp1/rkisp1-capture.c | 67 +++++++++++++++++--
 .../platform/rockchip/rkisp1/rkisp1-common.h  |  1 +
 .../platform/rockchip/rkisp1/rkisp1-dev.c     |  3 +-
 3 files changed, 63 insertions(+), 8 deletions(-)

-- 
2.30.2

[PATCH 1/2] media: rkisp1: Add YC swap capability

[Paul Elder]

The ISP version in the i.MX8MP has an MI_OUTPUT_ALIGN_FORMAT register
that the rk3399 does not have. This register allows swapping bytes,
which can be used to implement UYVY from YUYV.

Add a feature flag to signify the presence of this feature, and add it
to the i.MX8MP match data. Also add it as a flag to the format info in
the list of supported formats by the capture v4l2 devices, and update
enum_fmt and s_fmt to take it into account.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-capture.c | 27 ++++++++++++++-----
 .../platform/rockchip/rkisp1/rkisp1-common.h  |  1 +
 .../platform/rockchip/rkisp1/rkisp1-dev.c     |  3 ++-
 3 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
index c494afbc21b4..85fd85fe208c 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
@@ -47,13 +47,15 @@ enum rkisp1_plane {
  * @fourcc: pixel format
  * @fmt_type: helper filed for pixel format
  * @uv_swap: if cb cr swapped, for yuv
+ * @yc_swap: if y and cb/cr swapped, 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 uv_swap : 1;
+	u32 yc_swap : 1;
 	u32 write_format;
 	u32 output_format;
 	u32 mbus;
@@ -1150,9 +1152,13 @@ static const struct rkisp1_capture_fmt_cfg *
 rkisp1_find_fmt_cfg(const struct rkisp1_capture *cap, const u32 pixelfmt)
 {
 	unsigned int i;
+	bool yc_swap_support =
+		cap->rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN;
 
 	for (i = 0; i < cap->config->fmt_size; i++) {
-		if (cap->config->fmts[i].fourcc == pixelfmt)
+		const struct rkisp1_capture_fmt_cfg *fmt = &cap->config->fmts[i];
+		if (fmt->fourcc == pixelfmt &&
+		    (!fmt->yc_swap || yc_swap_support))
 			return &cap->config->fmts[i];
 	}
 	return NULL;
@@ -1223,22 +1229,29 @@ 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;
 	unsigned int i, n = 0;
+	bool yc_swap_support =
+		cap->rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN;
 
-	if (!f->mbus_code) {
-		if (f->index >= cap->config->fmt_size)
-			return -EINVAL;
+	if (f->index >= cap->config->fmt_size)
+		return -EINVAL;
 
+	if (!f->mbus_code && yc_swap_support) {
 		fmt = &cap->config->fmts[f->index];
 		f->pixelformat = fmt->fourcc;
 		return 0;
 	}
 
 	for (i = 0; i < cap->config->fmt_size; i++) {
-		if (cap->config->fmts[i].mbus != f->mbus_code)
+		fmt = &cap->config->fmts[i];
+
+		if (!!f->mbus_code && fmt->mbus != f->mbus_code)
+			continue;
+
+		if (!yc_swap_support && fmt->yc_swap)
 			continue;
 
 		if (n++ == f->index) {
-			f->pixelformat = cap->config->fmts[i].fourcc;
+			f->pixelformat = fmt->fourcc;
 			return 0;
 		}
 	}
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index 38be565341d6..b0f9221a1922 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -119,6 +119,7 @@ enum rkisp1_feature {
 	RKISP1_FEATURE_MAIN_STRIDE = BIT(3),
 	RKISP1_FEATURE_DMA_34BIT = BIT(4),
 	RKISP1_FEATURE_SELF_PATH = BIT(5),
+	RKISP1_FEATURE_MI_OUTPUT_ALIGN = BIT(6),
 };
 
 /*
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index 3b549c07a9bb..a475933820fd 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -520,7 +520,8 @@ static const struct rkisp1_info imx8mp_isp_info = {
 	.isp_ver = IMX8MP_V10,
 	.features = RKISP1_FEATURE_RSZ_CROP
 		  | RKISP1_FEATURE_MAIN_STRIDE
-		  | RKISP1_FEATURE_DMA_34BIT,
+		  | RKISP1_FEATURE_DMA_34BIT
+		  | RKISP1_FEATURE_MI_OUTPUT_ALIGN,
 };
 
 static const struct of_device_id rkisp1_of_match[] = {
-- 
2.30.2

[PATCH 2/2] media: rkisp1: Add UYVY as an output format

[Paul Elder]

Add support for UYVY as an output format. The uv_swap bit in the
MI_XTD_FORMAT_CTRL register that is used for the NV formats does not
work for packed YUV formats. Thus, UYVY support is implemented via
byte-swapping. This method clearly does not work for implementing
support for YVYU and VYUY.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
UYVY for the self path has not been tested because no device supports
it. The rk3399 has a self path, but does not have the
MI_OUTPUT_ALIGN_FORMAT register and thus does not support UYVY. The
i.MX8MP does support UYVY, but does not have a self path.
---
 .../platform/rockchip/rkisp1/rkisp1-capture.c | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
index 85fd85fe208c..77496ccef7ec 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
@@ -97,6 +97,12 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
 		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
+	}, {
+		.fourcc = V4L2_PIX_FMT_UYVY,
+		.uv_swap = 0,
+		.yc_swap = 1,
+		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
+		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YUV422P,
 		.uv_swap = 0,
@@ -231,6 +237,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
 		.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_UYVY,
+		.uv_swap = 0,
+		.yc_swap = 1,
+		.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,
@@ -464,6 +477,20 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
 		rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg);
 	}
 
+	/*
+	 * uv swapping with the MI_XTD_FORMAT_CTRL register only works for
+	 * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY.
+	 * YVYU and VYUY cannot be supported with this method.
+	 */
+	if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) {
+		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT);
+		if (cap->pix.cfg->yc_swap)
+			reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
+		else
+			reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
+		rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg);
+	}
+
 	rkisp1_mi_config_ctrl(cap);
 
 	reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL);
@@ -505,6 +532,19 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
 		rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg);
 	}
 
+	/*
+	 * uv swapping with the MI_XTD_FORMAT_CTRL register only works for
+	 * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY.
+	 * YVYU and VYUY cannot be supported with this method.
+	 */
+	if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) {
+		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT);
+		if (cap->pix.cfg->yc_swap)
+			reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES;
+		else
+			reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES;
+		rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg);
+	}
 	rkisp1_mi_config_ctrl(cap);
 
 	mi_ctrl = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL);
-- 
2.30.2

Re: [PATCH 1/2] media: rkisp1: Add YC swap capability

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, Jul 14, 2022 at 08:26:02PM +0900, Paul Elder wrote:
> The ISP version in the i.MX8MP has an MI_OUTPUT_ALIGN_FORMAT register
> that the rk3399 does not have. This register allows swapping bytes,
> which can be used to implement UYVY from YUYV.
> 
> Add a feature flag to signify the presence of this feature, and add it
> to the i.MX8MP match data. Also add it as a flag to the format info in
> the list of supported formats by the capture v4l2 devices, and update
> enum_fmt and s_fmt to take it into account.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-capture.c | 27 ++++++++++++++-----
>  .../platform/rockchip/rkisp1/rkisp1-common.h  |  1 +
>  .../platform/rockchip/rkisp1/rkisp1-dev.c     |  3 ++-
>  3 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> index c494afbc21b4..85fd85fe208c 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -47,13 +47,15 @@ enum rkisp1_plane {
>   * @fourcc: pixel format
>   * @fmt_type: helper filed for pixel format
>   * @uv_swap: if cb cr swapped, for yuv
> + * @yc_swap: if y and cb/cr swapped, 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 uv_swap : 1;
> +	u32 yc_swap : 1;
>  	u32 write_format;
>  	u32 output_format;
>  	u32 mbus;
> @@ -1150,9 +1152,13 @@ static const struct rkisp1_capture_fmt_cfg *
>  rkisp1_find_fmt_cfg(const struct rkisp1_capture *cap, const u32 pixelfmt)
>  {
>  	unsigned int i;
> +	bool yc_swap_support =
> +		cap->rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN;

Could you move this before the previous line ?

I'm tempted to write a patch at some point that will simplify this with
a rkisp1_has_feature() that will... scratch that,
https://lore.kernel.org/linux-media/20220719221751.4159-1-laurent.pinchart@ideasonboard.com
:-) Could you rebase for v2 ?

>  
>  	for (i = 0; i < cap->config->fmt_size; i++) {
> -		if (cap->config->fmts[i].fourcc == pixelfmt)
> +		const struct rkisp1_capture_fmt_cfg *fmt = &cap->config->fmts[i];

Missing blank line. I thought checkpatch.pl would warn about this.

> +		if (fmt->fourcc == pixelfmt &&
> +		    (!fmt->yc_swap || yc_swap_support))
>  			return &cap->config->fmts[i];
>  	}
>  	return NULL;
> @@ -1223,22 +1229,29 @@ 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;
>  	unsigned int i, n = 0;
> +	bool yc_swap_support =
> +		cap->rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN;
>  
> -	if (!f->mbus_code) {
> -		if (f->index >= cap->config->fmt_size)
> -			return -EINVAL;
> +	if (f->index >= cap->config->fmt_size)
> +		return -EINVAL;
>  
> +	if (!f->mbus_code && yc_swap_support) {
>  		fmt = &cap->config->fmts[f->index];
>  		f->pixelformat = fmt->fourcc;
>  		return 0;
>  	}

I'm tempted to drop this optimization, as it makes the code more complex
and only brings a small improvement to an ioctl that shouldn't be in a
hot path, but that's up to you.

>  
>  	for (i = 0; i < cap->config->fmt_size; i++) {
> -		if (cap->config->fmts[i].mbus != f->mbus_code)
> +		fmt = &cap->config->fmts[i];
> +
> +		if (!!f->mbus_code && fmt->mbus != f->mbus_code)

You can s/!!//

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +			continue;
> +
> +		if (!yc_swap_support && fmt->yc_swap)
>  			continue;
>  
>  		if (n++ == f->index) {
> -			f->pixelformat = cap->config->fmts[i].fourcc;
> +			f->pixelformat = fmt->fourcc;
>  			return 0;
>  		}
>  	}
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index 38be565341d6..b0f9221a1922 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -119,6 +119,7 @@ enum rkisp1_feature {
>  	RKISP1_FEATURE_MAIN_STRIDE = BIT(3),
>  	RKISP1_FEATURE_DMA_34BIT = BIT(4),
>  	RKISP1_FEATURE_SELF_PATH = BIT(5),
> +	RKISP1_FEATURE_MI_OUTPUT_ALIGN = BIT(6),
>  };
>  
>  /*
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index 3b549c07a9bb..a475933820fd 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -520,7 +520,8 @@ static const struct rkisp1_info imx8mp_isp_info = {
>  	.isp_ver = IMX8MP_V10,
>  	.features = RKISP1_FEATURE_RSZ_CROP
>  		  | RKISP1_FEATURE_MAIN_STRIDE
> -		  | RKISP1_FEATURE_DMA_34BIT,
> +		  | RKISP1_FEATURE_DMA_34BIT
> +		  | RKISP1_FEATURE_MI_OUTPUT_ALIGN,
>  };
>  
>  static const struct of_device_id rkisp1_of_match[] = {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: rkisp1: Add UYVY as an output format

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Thu, Jul 14, 2022 at 08:26:03PM +0900, Paul Elder wrote:
> Add support for UYVY as an output format. The uv_swap bit in the
> MI_XTD_FORMAT_CTRL register that is used for the NV formats does not
> work for packed YUV formats. Thus, UYVY support is implemented via
> byte-swapping. This method clearly does not work for implementing
> support for YVYU and VYUY.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> UYVY for the self path has not been tested because no device supports
> it. The rk3399 has a self path, but does not have the
> MI_OUTPUT_ALIGN_FORMAT register and thus does not support UYVY. The
> i.MX8MP does support UYVY, but does not have a self path.

I'm tempted to drop it then, as the code below isn't correct given that
you use the same register for both MP and SP. Let's address MP only for
now.

> ---
>  .../platform/rockchip/rkisp1/rkisp1-capture.c | 40 +++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> index 85fd85fe208c..77496ccef7ec 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -97,6 +97,12 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
>  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_UYVY,
> +		.uv_swap = 0,
> +		.yc_swap = 1,
> +		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV422P,
>  		.uv_swap = 0,
> @@ -231,6 +237,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.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_UYVY,
> +		.uv_swap = 0,
> +		.yc_swap = 1,
> +		.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,
> @@ -464,6 +477,20 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
>  		rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg);
>  	}
>  
> +	/*
> +	 * uv swapping with the MI_XTD_FORMAT_CTRL register only works for

s@uv@U/V@

> +	 * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY.
> +	 * YVYU and VYUY cannot be supported with this method.
> +	 */
> +	if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) {
> +		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT);
> +		if (cap->pix.cfg->yc_swap)
> +			reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
> +		else
> +			reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
> +		rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg);

As the register is not initialized anywhere, it would be better to write
it fully here instead of a read-modify-write cycle. Same comments below.

> +	}
> +
>  	rkisp1_mi_config_ctrl(cap);
>  
>  	reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL);
> @@ -505,6 +532,19 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
>  		rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg);
>  	}
>  
> +	/*
> +	 * uv swapping with the MI_XTD_FORMAT_CTRL register only works for
> +	 * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY.
> +	 * YVYU and VYUY cannot be supported with this method.
> +	 */
> +	if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) {
> +		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT);
> +		if (cap->pix.cfg->yc_swap)
> +			reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES;
> +		else
> +			reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES;
> +		rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg);
> +	}

Missing blank line.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	rkisp1_mi_config_ctrl(cap);
>  
>  	mi_ctrl = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: rkisp1: Add UYVY as an output format

[paul.elder]

Hi Laurent,

On Wed, Jul 20, 2022 at 01:29:57AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Thu, Jul 14, 2022 at 08:26:03PM +0900, Paul Elder wrote:
> > Add support for UYVY as an output format. The uv_swap bit in the
> > MI_XTD_FORMAT_CTRL register that is used for the NV formats does not
> > work for packed YUV formats. Thus, UYVY support is implemented via
> > byte-swapping. This method clearly does not work for implementing
> > support for YVYU and VYUY.
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > 
> > ---
> > UYVY for the self path has not been tested because no device supports
> > it. The rk3399 has a self path, but does not have the
> > MI_OUTPUT_ALIGN_FORMAT register and thus does not support UYVY. The
> > i.MX8MP does support UYVY, but does not have a self path.
> 
> I'm tempted to drop it then, as the code below isn't correct given that
> you use the same register for both MP and SP. Let's address MP only for
> now.

The same register is used for both MP and SP. They just have different
bits. Which is why we'd need the read-write cycle, assuming that there
exists a device that has both an SP and the MI_OUTPUT_ALIGN_FORMAT
register.


Paul

> 
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-capture.c | 40 +++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > index 85fd85fe208c..77496ccef7ec 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > @@ -97,6 +97,12 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
> >  		.uv_swap = 0,
> >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_UYVY,
> > +		.uv_swap = 0,
> > +		.yc_swap = 1,
> > +		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> > +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> >  	}, {
> >  		.fourcc = V4L2_PIX_FMT_YUV422P,
> >  		.uv_swap = 0,
> > @@ -231,6 +237,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
> >  		.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_UYVY,
> > +		.uv_swap = 0,
> > +		.yc_swap = 1,
> > +		.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,
> > @@ -464,6 +477,20 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
> >  		rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg);
> >  	}
> >  
> > +	/*
> > +	 * uv swapping with the MI_XTD_FORMAT_CTRL register only works for
> 
> s@uv@U/V@
> 
> > +	 * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY.
> > +	 * YVYU and VYUY cannot be supported with this method.
> > +	 */
> > +	if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) {
> > +		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT);
> > +		if (cap->pix.cfg->yc_swap)
> > +			reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
> > +		else
> > +			reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
> > +		rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg);
> 
> As the register is not initialized anywhere, it would be better to write
> it fully here instead of a read-modify-write cycle. Same comments below.
> 
> > +	}
> > +
> >  	rkisp1_mi_config_ctrl(cap);
> >  
> >  	reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL);
> > @@ -505,6 +532,19 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
> >  		rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg);
> >  	}
> >  
> > +	/*
> > +	 * uv swapping with the MI_XTD_FORMAT_CTRL register only works for
> > +	 * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY.
> > +	 * YVYU and VYUY cannot be supported with this method.
> > +	 */
> > +	if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) {
> > +		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT);
> > +		if (cap->pix.cfg->yc_swap)
> > +			reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES;
> > +		else
> > +			reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES;
> > +		rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg);
> > +	}
> 
> Missing blank line.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  	rkisp1_mi_config_ctrl(cap);
> >  
> >  	mi_ctrl = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL);

Re: [PATCH 2/2] media: rkisp1: Add UYVY as an output format

[Laurent Pinchart]

Hi Paul,

On Thu, Jul 28, 2022 at 09:52:59PM +0900, paul.elder@ideasonboard.com wrote:
> On Wed, Jul 20, 2022 at 01:29:57AM +0300, Laurent Pinchart wrote:
> > On Thu, Jul 14, 2022 at 08:26:03PM +0900, Paul Elder wrote:
> > > Add support for UYVY as an output format. The uv_swap bit in the
> > > MI_XTD_FORMAT_CTRL register that is used for the NV formats does not
> > > work for packed YUV formats. Thus, UYVY support is implemented via
> > > byte-swapping. This method clearly does not work for implementing
> > > support for YVYU and VYUY.
> > > 
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > 
> > > ---
> > > UYVY for the self path has not been tested because no device supports
> > > it. The rk3399 has a self path, but does not have the
> > > MI_OUTPUT_ALIGN_FORMAT register and thus does not support UYVY. The
> > > i.MX8MP does support UYVY, but does not have a self path.
> > 
> > I'm tempted to drop it then, as the code below isn't correct given that
> > you use the same register for both MP and SP. Let's address MP only for
> > now.
> 
> The same register is used for both MP and SP. They just have different
> bits. Which is why we'd need the read-write cycle, assuming that there
> exists a device that has both an SP and the MI_OUTPUT_ALIGN_FORMAT
> register.

Indeed, I had missed that. The documentation is confusing, the register
is described as "Output align format for main path", has both
mp_byte_swap and sp_byte_swap, but no sp equivalent to mp_lsb_alignment
(maybe the self path doesn't support raw outputs though ?).

I'm OK keeping support for both paths, but I think the
MI_OUTPUT_ALIGN_FORMAT register should then be initialized to a default
value somewhere.

> > > ---
> > >  .../platform/rockchip/rkisp1/rkisp1-capture.c | 40 +++++++++++++++++++
> > >  1 file changed, 40 insertions(+)
> > > 
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > index 85fd85fe208c..77496ccef7ec 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > @@ -97,6 +97,12 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
> > >  		.uv_swap = 0,
> > >  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> > >  		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > > +	}, {
> > > +		.fourcc = V4L2_PIX_FMT_UYVY,
> > > +		.uv_swap = 0,
> > > +		.yc_swap = 1,
> > > +		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> > > +		.mbus = MEDIA_BUS_FMT_YUYV8_2X8,
> > >  	}, {
> > >  		.fourcc = V4L2_PIX_FMT_YUV422P,
> > >  		.uv_swap = 0,
> > > @@ -231,6 +237,13 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
> > >  		.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_UYVY,
> > > +		.uv_swap = 0,
> > > +		.yc_swap = 1,
> > > +		.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,
> > > @@ -464,6 +477,20 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
> > >  		rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg);
> > >  	}
> > >  
> > > +	/*
> > > +	 * uv swapping with the MI_XTD_FORMAT_CTRL register only works for
> > 
> > s@uv@U/V@
> > 
> > > +	 * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY.
> > > +	 * YVYU and VYUY cannot be supported with this method.
> > > +	 */
> > > +	if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) {
> > > +		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT);
> > > +		if (cap->pix.cfg->yc_swap)
> > > +			reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
> > > +		else
> > > +			reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_MP_BYTE_SWAP_BYTES;
> > > +		rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg);
> > 
> > As the register is not initialized anywhere, it would be better to write
> > it fully here instead of a read-modify-write cycle. Same comments below.
> > 
> > > +	}
> > > +
> > >  	rkisp1_mi_config_ctrl(cap);
> > >  
> > >  	reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL);
> > > @@ -505,6 +532,19 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
> > >  		rkisp1_write(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL, reg);
> > >  	}
> > >  
> > > +	/*
> > > +	 * uv swapping with the MI_XTD_FORMAT_CTRL register only works for
> > > +	 * NV12/NV21 and NV16/NV61, so instead use byte swap to support UYVY.
> > > +	 * YVYU and VYUY cannot be supported with this method.
> > > +	 */
> > > +	if (rkisp1->info->features & RKISP1_FEATURE_MI_OUTPUT_ALIGN) {
> > > +		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT);
> > > +		if (cap->pix.cfg->yc_swap)
> > > +			reg |= RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES;
> > > +		else
> > > +			reg &= ~RKISP1_CIF_OUTPUT_ALIGN_FORMAT_SP_BYTE_SWAP_BYTES;
> > > +		rkisp1_write(rkisp1, RKISP1_CIF_MI_OUTPUT_ALIGN_FORMAT, reg);
> > > +	}
> > 
> > Missing blank line.
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > >  	rkisp1_mi_config_ctrl(cap);
> > >  
> > >  	mi_ctrl = rkisp1_read(rkisp1, RKISP1_CIF_MI_CTRL);

-- 
Regards,

Laurent Pinchart