[PATCH v2 0/5] media: staging: rkisp1: cap: various fixes for capture formats

[PATCH v2 0/5] media: staging: rkisp1: cap: various fixes for capture formats

[Dafna Hirschfeld]

This patchset fixes various issues related to
the supported formats in the rkisp1 capture.

Patches summary:

patches 1,2 - fixes a wrong assignments to the register that swaps the
'u', 'v' planes in YUV semiplanar formats.

patch 3 - sets the register from patch 1 only if the format is semiplanar.

patch 4 - adds support to planar YUV formats with swapped u,v planes
by swapping the addresses of the planes

patch 5 - removes some packed YUV formats that are not supported
by the driver.

changes from v1:
- split the first patch from v1 into two separate patches, the first is a cleanup patch
the second fixes a bug.

Dafna Hirschfeld (5):
  media: staging: rkisp1: cap: cleanup in mainpath config for uv swap
    format
  media: staging: rkisp1: cap: fix value written to uv swap register in
    selfpath
  media: staging: rkisp1: cap: support uv swap only for semiplanar
    formats
  media: staging: rkisp1: cap: support uv swapped plane formats
  media: staging: rkisp1: cap: remove unsupported formats

 drivers/staging/media/rkisp1/rkisp1-capture.c | 55 ++++++++++---------
 1 file changed, 28 insertions(+), 27 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] media: staging: rkisp1: cap: cleanup in mainpath config for uv swap format

[Dafna Hirschfeld]

The value RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP equals BIT(0),
Therefore when writing it to the register there is no need to mask
it first with ~BIT(0).

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 45d237a77ca4..5700d7be2819 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -432,8 +432,7 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
 	if (cap->pix.cfg->uv_swap) {
 		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
 
-		reg = (reg & ~BIT(0)) |
-		      RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP;
+		reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP;
 		rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
 	}
 
-- 
2.17.1

[PATCH v2 2/5] media: staging: rkisp1: cap: fix value written to uv swap register in selfpath

[Dafna Hirschfeld]

The value RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP should be
set to the register instead of masking with ~BIT(1)

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 5700d7be2819..84a3cf565106 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -469,8 +469,8 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
 	if (cap->pix.cfg->uv_swap) {
 		u32 reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
 
-		rkisp1_write(rkisp1, reg & ~BIT(1),
-			     RKISP1_CIF_MI_XTD_FORMAT_CTRL);
+		reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP;
+		rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
 	}
 
 	rkisp1_mi_config_ctrl(cap);
-- 
2.17.1

[PATCH v2 3/5] media: staging: rkisp1: cap: support uv swap only for semiplanar formats

[Dafna Hirschfeld]

The register RKISP1_CIF_MI_XTD_FORMAT_CTRL is relevant only
for semiplanar formats, therefore the uv swap can be supported
through this register only for semiplanar formats.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 84a3cf565106..fa2849209433 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -37,6 +37,10 @@
 
 #define RKISP1_MIN_BUFFERS_NEEDED 3
 
+#define RKISP1_IS_SEMI_PLANAR(write_format)				\
+	(((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||	\
+	 ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
+
 enum rkisp1_plane {
 	RKISP1_PLANE_Y	= 0,
 	RKISP1_PLANE_CB	= 1,
@@ -429,7 +433,8 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
 		     cap->config->mi.cr_size_init);
 
 	rkisp1_irq_frame_end_enable(cap);
-	if (cap->pix.cfg->uv_swap) {
+	if (RKISP1_IS_SEMI_PLANAR(cap->pix.cfg->write_format) &&
+	    cap->pix.cfg->uv_swap) {
 		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
 
 		reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP;
@@ -466,7 +471,8 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
 	rkisp1_write(rkisp1, cap->sp_y_stride, RKISP1_CIF_MI_SP_Y_LLENGTH);
 
 	rkisp1_irq_frame_end_enable(cap);
-	if (cap->pix.cfg->uv_swap) {
+	if (RKISP1_IS_SEMI_PLANAR(cap->pix.cfg->write_format) &&
+	    cap->pix.cfg->uv_swap) {
 		u32 reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
 
 		reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP;
-- 
2.17.1

[PATCH v2 4/5] media: staging: rkisp1: cap: support uv swapped plane formats

[Dafna Hirschfeld]

Plane formats with the u and v planes swapped can be
supported by changing the address of the cb and cr in
the buffer.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index fa2849209433..2d274e8f565b 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -41,6 +41,10 @@
 	(((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||	\
 	 ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
 
+#define RKISP1_IS_PLANAR(write_format)					\
+	(((write_format) == RKISP1_MI_CTRL_SP_WRITE_PLA) ||		\
+	 ((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8))
+
 enum rkisp1_plane {
 	RKISP1_PLANE_Y	= 0,
 	RKISP1_PLANE_CB	= 1,
@@ -788,6 +792,19 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
 			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CB);
 	}
 
+	/*
+	 * uv swap can be supported for plane formats by switching
+	 * the address of cb and cr
+	 */
+	if (RKISP1_IS_PLANAR(cap->pix.cfg->write_format) &&
+	    cap->pix.cfg->uv_swap) {
+		ispbuf->buff_addr[RKISP1_PLANE_CR] =
+			ispbuf->buff_addr[RKISP1_PLANE_CB];
+		ispbuf->buff_addr[RKISP1_PLANE_CB] =
+			ispbuf->buff_addr[RKISP1_PLANE_CR] +
+			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR);
+	}
+
 	spin_lock_irqsave(&cap->buf.lock, flags);
 
 	/*
-- 
2.17.1

[PATCH v2 5/5] 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>
---
 drivers/staging/media/rkisp1/rkisp1-capture.c | 21 -------------------
 1 file changed, 21 deletions(-)

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 2d274e8f565b..076335193f40 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -98,15 +98,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
 		.fmt_type = RKISP1_FMT_YUV,
 		.uv_swap = 0,
 		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
-	}, {
-		.fourcc = V4L2_PIX_FMT_YVYU,
-		.fmt_type = RKISP1_FMT_YUV,
-		.uv_swap = 1,
-		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
-	}, {
-		.fourcc = V4L2_PIX_FMT_VYUY,
-		.fmt_type = RKISP1_FMT_YUV,
-		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YUV422P,
 		.fmt_type = RKISP1_FMT_YUV,
@@ -234,18 +225,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,
-		.fmt_type = RKISP1_FMT_YUV,
-		.uv_swap = 1,
-		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
-		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
-	}, {
-		.fourcc = V4L2_PIX_FMT_VYUY,
-		.fmt_type = RKISP1_FMT_YUV,
-		.uv_swap = 1,
-		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
-		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
 	}, {
 		.fourcc = V4L2_PIX_FMT_YUV422P,
 		.fmt_type = RKISP1_FMT_YUV,
-- 
2.17.1

Re: [PATCH v2 1/5] media: staging: rkisp1: cap: cleanup in mainpath config for uv swap format

[Helen Koike]

On 4/2/20 4:04 PM, Dafna Hirschfeld wrote:
> The value RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP equals BIT(0),
> Therefore when writing it to the register there is no need to mask
> it first with ~BIT(0).
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 45d237a77ca4..5700d7be2819 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -432,8 +432,7 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
>  	if (cap->pix.cfg->uv_swap) {
>  		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
>  
> -		reg = (reg & ~BIT(0)) |
> -		      RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP;
> +		reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP;
>  		rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
>  	}
>  
> 

Re: [PATCH v2 2/5] media: staging: rkisp1: cap: fix value written to uv swap register in selfpath

[Helen Koike]

On 4/2/20 4:04 PM, Dafna Hirschfeld wrote:
> The value RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP should be
> set to the register instead of masking with ~BIT(1)
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 5700d7be2819..84a3cf565106 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -469,8 +469,8 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
>  	if (cap->pix.cfg->uv_swap) {
>  		u32 reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
>  
> -		rkisp1_write(rkisp1, reg & ~BIT(1),
> -			     RKISP1_CIF_MI_XTD_FORMAT_CTRL);
> +		reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP;
> +		rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
>  	}
>  
>  	rkisp1_mi_config_ctrl(cap);
> 

Re: [PATCH v2 1/5] media: staging: rkisp1: cap: cleanup in mainpath config for uv swap format

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Thu, Apr 02, 2020 at 09:04:15PM +0200, Dafna Hirschfeld wrote:
> The value RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP equals BIT(0),
> Therefore when writing it to the register there is no need to mask
> it first with ~BIT(0).
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 45d237a77ca4..5700d7be2819 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -432,8 +432,7 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
>  	if (cap->pix.cfg->uv_swap) {
>  		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
>  
> -		reg = (reg & ~BIT(0)) |
> -		      RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP;
> +		reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP;

I'd write this

		reg |= RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP;

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

>  		rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
>  	}
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/5] media: staging: rkisp1: cap: fix value written to uv swap register in selfpath

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Thu, Apr 02, 2020 at 09:04:16PM +0200, Dafna Hirschfeld wrote:
> The value RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP should be
> set to the register instead of masking with ~BIT(1)
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 5700d7be2819..84a3cf565106 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -469,8 +469,8 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
>  	if (cap->pix.cfg->uv_swap) {
>  		u32 reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
>  
> -		rkisp1_write(rkisp1, reg & ~BIT(1),
> -			     RKISP1_CIF_MI_XTD_FORMAT_CTRL);
> +		reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP;
> +		rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL);

This indeed simplifies the code, but I think the logic is wrong in the
first place. Shouldn't this be

	reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
	if (cap->pix.cfg->uv_swap)
		reg |= RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP;
	else
		reg &= ~RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP;
	rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL);

? Same for patch 1/5.

>  	}
>  
>  	rkisp1_mi_config_ctrl(cap);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/5] media: staging: rkisp1: cap: support uv swap only for semiplanar formats

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Thu, Apr 02, 2020 at 09:04:17PM +0200, Dafna Hirschfeld wrote:
> The register RKISP1_CIF_MI_XTD_FORMAT_CTRL is relevant only
> for semiplanar formats, therefore the uv swap can be supported
> through this register only for semiplanar formats.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 84a3cf565106..fa2849209433 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -37,6 +37,10 @@
>  
>  #define RKISP1_MIN_BUFFERS_NEEDED 3
>  
> +#define RKISP1_IS_SEMI_PLANAR(write_format)				\
> +	(((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||	\
> +	 ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
> +
>  enum rkisp1_plane {
>  	RKISP1_PLANE_Y	= 0,
>  	RKISP1_PLANE_CB	= 1,
> @@ -429,7 +433,8 @@ static void rkisp1_mp_config(struct rkisp1_capture *cap)
>  		     cap->config->mi.cr_size_init);
>  
>  	rkisp1_irq_frame_end_enable(cap);
> -	if (cap->pix.cfg->uv_swap) {
> +	if (RKISP1_IS_SEMI_PLANAR(cap->pix.cfg->write_format) &&

As this is the mp config path, I suppose write_format can only be
RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA, right ? You could then write

	if (cap->pix.cfg->write_format == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA &&

> +	    cap->pix.cfg->uv_swap) {
>  		reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
>  
>  		reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_MP_CB_CR_SWAP;
> @@ -466,7 +471,8 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
>  	rkisp1_write(rkisp1, cap->sp_y_stride, RKISP1_CIF_MI_SP_Y_LLENGTH);
>  
>  	rkisp1_irq_frame_end_enable(cap);
> -	if (cap->pix.cfg->uv_swap) {
> +	if (RKISP1_IS_SEMI_PLANAR(cap->pix.cfg->write_format) &&

Same here with RKISP1_MI_CTRL_SP_WRITE_SPLA.

Another option would be to check the number of planes from
v4l2_format_info, which could lead to simpler to read code.

> +	    cap->pix.cfg->uv_swap) {
>  		u32 reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
>  
>  		reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/5] media: staging: rkisp1: cap: support uv swapped plane formats

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Thu, Apr 02, 2020 at 09:04:18PM +0200, Dafna Hirschfeld wrote:
> Plane formats with the u and v planes swapped can be
> supported by changing the address of the cb and cr in
> the buffer.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index fa2849209433..2d274e8f565b 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -41,6 +41,10 @@
>  	(((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||	\
>  	 ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
>  
> +#define RKISP1_IS_PLANAR(write_format)					\
> +	(((write_format) == RKISP1_MI_CTRL_SP_WRITE_PLA) ||		\
> +	 ((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8))
> +
>  enum rkisp1_plane {
>  	RKISP1_PLANE_Y	= 0,
>  	RKISP1_PLANE_CB	= 1,
> @@ -788,6 +792,19 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>  			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CB);
>  	}
>  
> +	/*
> +	 * uv swap can be supported for plane formats by switching
> +	 * the address of cb and cr
> +	 */
> +	if (RKISP1_IS_PLANAR(cap->pix.cfg->write_format) &&

As commented on patch 3/5, could this be checked from the data in
v4l2_format_info ?

> +	    cap->pix.cfg->uv_swap) {
> +		ispbuf->buff_addr[RKISP1_PLANE_CR] =
> +			ispbuf->buff_addr[RKISP1_PLANE_CB];
> +		ispbuf->buff_addr[RKISP1_PLANE_CB] =
> +			ispbuf->buff_addr[RKISP1_PLANE_CR] +
> +			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR);

How about

		swap(ispbuf->buff_addr[RKISP1_PLANE_CR],
		     ispbuf->buff_addr[RKISP1_PLANE_CB]);

?

> +	}
> +
>  	spin_lock_irqsave(&cap->buf.lock, flags);
>  
>  	/*

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/5] media: staging: rkisp1: cap: remove unsupported formats

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Thu, Apr 02, 2020 at 09:04:19PM +0200, Dafna Hirschfeld wrote:
> For Ycbcr packed formats only YUYV can be supported by
> the driver. This patch removes the other formats.

The RKISP1_CIF_MI_BYTE_SWAP bit could possibly help achieving other YUV
orders, but as far as I can tell, it would affect both the main path and
the self path, so it wouldn't be very convenient. At a quick glance I
haven't found a way to support those formats, but just to make sure,
have you double-checked that the nv21_self and nv21_main bits of
MI_XTD_FORMAT_CTRL don't also affect packed mode ? If they don't,

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

> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 21 -------------------
>  1 file changed, 21 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 2d274e8f565b..076335193f40 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -98,15 +98,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
>  		.fmt_type = RKISP1_FMT_YUV,
>  		.uv_swap = 0,
>  		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> -	}, {
> -		.fourcc = V4L2_PIX_FMT_YVYU,
> -		.fmt_type = RKISP1_FMT_YUV,
> -		.uv_swap = 1,
> -		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
> -	}, {
> -		.fourcc = V4L2_PIX_FMT_VYUY,
> -		.fmt_type = RKISP1_FMT_YUV,
> -		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV422P,
>  		.fmt_type = RKISP1_FMT_YUV,
> @@ -234,18 +225,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,
> -		.fmt_type = RKISP1_FMT_YUV,
> -		.uv_swap = 1,
> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
> -	}, {
> -		.fourcc = V4L2_PIX_FMT_VYUY,
> -		.fmt_type = RKISP1_FMT_YUV,
> -		.uv_swap = 1,
> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_YUV422P,
>  		.fmt_type = RKISP1_FMT_YUV,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/5] media: staging: rkisp1: cap: fix value written to uv swap register in selfpath

[Dafna Hirschfeld]

On 4/5/20 8:10 PM, Laurent Pinchart wrote:
> Hi Dafna,
> 
> Thank you for the patch.
> 
> On Thu, Apr 02, 2020 at 09:04:16PM +0200, Dafna Hirschfeld wrote:
>> The value RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP should be
>> set to the register instead of masking with ~BIT(1)
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index 5700d7be2819..84a3cf565106 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -469,8 +469,8 @@ static void rkisp1_sp_config(struct rkisp1_capture *cap)
>>   	if (cap->pix.cfg->uv_swap) {
>>   		u32 reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
>>   
>> -		rkisp1_write(rkisp1, reg & ~BIT(1),
>> -			     RKISP1_CIF_MI_XTD_FORMAT_CTRL);
>> +		reg = reg | RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP;
>> +		rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
> 
> This indeed simplifies the code, but I think the logic is wrong in the
> first place. Shouldn't this be
> 
> 	reg = rkisp1_read(rkisp1, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
> 	if (cap->pix.cfg->uv_swap)
> 		reg |= RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP;
> 	else
> 		reg &= ~RKISP1_CIF_MI_XTD_FMT_CTRL_SP_CB_CR_SWAP;
> 	rkisp1_write(rkisp1, reg, RKISP1_CIF_MI_XTD_FORMAT_CTRL);
> 
> ? Same for patch 1/5.
Right, I'll send v3.

Thanks,
Dafna
> 
>>   	}
>>   
>>   	rkisp1_mi_config_ctrl(cap);
> 

Re: [PATCH v2 4/5] media: staging: rkisp1: cap: support uv swapped plane formats

[Dafna Hirschfeld]

On 4/5/20 8:16 PM, Laurent Pinchart wrote:
> Hi Dafna,
> 
> Thank you for the patch.
> 
> On Thu, Apr 02, 2020 at 09:04:18PM +0200, Dafna Hirschfeld wrote:
>> Plane formats with the u and v planes swapped can be
>> supported by changing the address of the cb and cr in
>> the buffer.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Acked-by: Helen Koike <helen.koike@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index fa2849209433..2d274e8f565b 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -41,6 +41,10 @@
>>   	(((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||	\
>>   	 ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
>>   
>> +#define RKISP1_IS_PLANAR(write_format)					\
>> +	(((write_format) == RKISP1_MI_CTRL_SP_WRITE_PLA) ||		\
>> +	 ((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8))
>> +
>>   enum rkisp1_plane {
>>   	RKISP1_PLANE_Y	= 0,
>>   	RKISP1_PLANE_CB	= 1,
>> @@ -788,6 +792,19 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>>   			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CB);
>>   	}
>>   
>> +	/*
>> +	 * uv swap can be supported for plane formats by switching
>> +	 * the address of cb and cr
>> +	 */
>> +	if (RKISP1_IS_PLANAR(cap->pix.cfg->write_format) &&
> 
> As commented on patch 3/5, could this be checked from the data in
> v4l2_format_info ?
yes
> 
>> +	    cap->pix.cfg->uv_swap) {
>> +		ispbuf->buff_addr[RKISP1_PLANE_CR] =
>> +			ispbuf->buff_addr[RKISP1_PLANE_CB];
>> +		ispbuf->buff_addr[RKISP1_PLANE_CB] =
>> +			ispbuf->buff_addr[RKISP1_PLANE_CR] +
>> +			rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR);
> 
> How about
> 
> 		swap(ispbuf->buff_addr[RKISP1_PLANE_CR],
> 		     ispbuf->buff_addr[RKISP1_PLANE_CB]);
> 
> ?
This also works, theoretically if there was a format where the Cb, Cr planes
are not equal size then a swap will not work.

Thanks,
Dafna
> 
>> +	}
>> +
>>   	spin_lock_irqsave(&cap->buf.lock, flags);
>>   
>>   	/*
> 

Re: [PATCH v2 4/5] media: staging: rkisp1: cap: support uv swapped plane formats

[Helen Koike]

Hi,

On 4/6/20 8:56 AM, Dafna Hirschfeld wrote:
> 
> 
> On 4/5/20 8:16 PM, Laurent Pinchart wrote:
>> Hi Dafna,
>>
>> Thank you for the patch.
>>
>> On Thu, Apr 02, 2020 at 09:04:18PM +0200, Dafna Hirschfeld wrote:
>>> Plane formats with the u and v planes swapped can be
>>> supported by changing the address of the cb and cr in
>>> the buffer.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>> ---
>>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> index fa2849209433..2d274e8f565b 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> @@ -41,6 +41,10 @@
>>>       (((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||    \
>>>        ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
>>>   +#define RKISP1_IS_PLANAR(write_format)                    \
>>> +    (((write_format) == RKISP1_MI_CTRL_SP_WRITE_PLA) ||        \
>>> +     ((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8))
>>> +
>>>   enum rkisp1_plane {
>>>       RKISP1_PLANE_Y    = 0,
>>>       RKISP1_PLANE_CB    = 1,
>>> @@ -788,6 +792,19 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>>>               rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CB);
>>>       }
>>>   +    /*
>>> +     * uv swap can be supported for plane formats by switching
>>> +     * the address of cb and cr
>>> +     */
>>> +    if (RKISP1_IS_PLANAR(cap->pix.cfg->write_format) &&
>>
>> As commented on patch 3/5, could this be checked from the data in
>> v4l2_format_info ?
> yes
>>
>>> +        cap->pix.cfg->uv_swap) {
>>> +        ispbuf->buff_addr[RKISP1_PLANE_CR] =
>>> +            ispbuf->buff_addr[RKISP1_PLANE_CB];
>>> +        ispbuf->buff_addr[RKISP1_PLANE_CB] =
>>> +            ispbuf->buff_addr[RKISP1_PLANE_CR] +
>>> +            rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR);

Actually this is wrong if pixm->num_planes != 1, since they are different buffers.

>>
>> How about
>>
>>         swap(ispbuf->buff_addr[RKISP1_PLANE_CR],
>>              ispbuf->buff_addr[RKISP1_PLANE_CB]);
>>
>> ?
> This also works, theoretically if there was a format where the Cb, Cr planes
> are not equal size then a swap will not work.

If you check rkisp1_fill_pixfmt(), you'll see that they are equal size.
hdiv and vdiv applies to both.

Thank you Laurent for the review and thank you Dafna for working on this.

Regards,
Helen

> 
> Thanks,
> Dafna
>>
>>> +    }
>>> +
>>>       spin_lock_irqsave(&cap->buf.lock, flags);
>>>         /*
>>
> 

Re: [PATCH v2 4/5] media: staging: rkisp1: cap: support uv swapped plane formats

[Dafna Hirschfeld]

On 4/6/20 2:27 PM, Helen Koike wrote:
> Hi,
> 
> On 4/6/20 8:56 AM, Dafna Hirschfeld wrote:
>>
>>
>> On 4/5/20 8:16 PM, Laurent Pinchart wrote:
>>> Hi Dafna,
>>>
>>> Thank you for the patch.
>>>
>>> On Thu, Apr 02, 2020 at 09:04:18PM +0200, Dafna Hirschfeld wrote:
>>>> Plane formats with the u and v planes swapped can be
>>>> supported by changing the address of the cb and cr in
>>>> the buffer.
>>>>
>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>>> ---
>>>>    drivers/staging/media/rkisp1/rkisp1-capture.c | 17 +++++++++++++++++
>>>>    1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>> index fa2849209433..2d274e8f565b 100644
>>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>>> @@ -41,6 +41,10 @@
>>>>        (((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_SPLA) ||    \
>>>>         ((write_format) == RKISP1_MI_CTRL_SP_WRITE_SPLA))
>>>>    +#define RKISP1_IS_PLANAR(write_format)                    \
>>>> +    (((write_format) == RKISP1_MI_CTRL_SP_WRITE_PLA) ||        \
>>>> +     ((write_format) == RKISP1_MI_CTRL_MP_WRITE_YUV_PLA_OR_RAW8))
>>>> +
>>>>    enum rkisp1_plane {
>>>>        RKISP1_PLANE_Y    = 0,
>>>>        RKISP1_PLANE_CB    = 1,
>>>> @@ -788,6 +792,19 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>>>>                rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CB);
>>>>        }
>>>>    +    /*
>>>> +     * uv swap can be supported for plane formats by switching
>>>> +     * the address of cb and cr
>>>> +     */
>>>> +    if (RKISP1_IS_PLANAR(cap->pix.cfg->write_format) &&
>>>
>>> As commented on patch 3/5, could this be checked from the data in
>>> v4l2_format_info ?
>> yes
>>>
>>>> +        cap->pix.cfg->uv_swap) {
>>>> +        ispbuf->buff_addr[RKISP1_PLANE_CR] =
>>>> +            ispbuf->buff_addr[RKISP1_PLANE_CB];
>>>> +        ispbuf->buff_addr[RKISP1_PLANE_CB] =
>>>> +            ispbuf->buff_addr[RKISP1_PLANE_CR] +
>>>> +            rkisp1_pixfmt_comp_size(pixm, RKISP1_PLANE_CR);
> 
> Actually this is wrong if pixm->num_planes != 1, since they are different buffers.
Hi, right, I will change to swap

Thanks,
Dafna
> 
>>>
>>> How about
>>>
>>>          swap(ispbuf->buff_addr[RKISP1_PLANE_CR],
>>>               ispbuf->buff_addr[RKISP1_PLANE_CB]);
>>>
>>> ?
>> This also works, theoretically if there was a format where the Cb, Cr planes
>> are not equal size then a swap will not work.
> 
> If you check rkisp1_fill_pixfmt(), you'll see that they are equal size.
> hdiv and vdiv applies to both.
> 
> Thank you Laurent for the review and thank you Dafna for working on this.
> 
> Regards,
> Helen
> 
>>
>> Thanks,
>> Dafna
>>>
>>>> +    }
>>>> +
>>>>        spin_lock_irqsave(&cap->buf.lock, flags);
>>>>          /*
>>>
>>

Re: [PATCH v2 5/5] media: staging: rkisp1: cap: remove unsupported formats

[Dafna Hirschfeld]

On 4/6/20 12:43 AM, Laurent Pinchart wrote:
> Hi Dafna,
> 
> Thank you for the patch.
> 
> On Thu, Apr 02, 2020 at 09:04:19PM +0200, Dafna Hirschfeld wrote:
>> For Ycbcr packed formats only YUYV can be supported by
>> the driver. This patch removes the other formats.
> 
> The RKISP1_CIF_MI_BYTE_SWAP bit could possibly help achieving other YUV
> orders, but as far as I can tell, it would affect both the main path and
> the self path, so it wouldn't be very convenient. At a quick glance I
> haven't found a way to support those formats, but just to make sure,
> have you double-checked that the nv21_self and nv21_main bits of
> MI_XTD_FORMAT_CTRL don't also affect packed mode ? If they don't,
Hi, thanks a lot for the reviews, I'll check that.

Dafna
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Acked-by: Helen Koike <helen.koike@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 21 -------------------
>>   1 file changed, 21 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> index 2d274e8f565b..076335193f40 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>> @@ -98,15 +98,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
>>   		.fmt_type = RKISP1_FMT_YUV,
>>   		.uv_swap = 0,
>>   		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
>> -	}, {
>> -		.fourcc = V4L2_PIX_FMT_YVYU,
>> -		.fmt_type = RKISP1_FMT_YUV,
>> -		.uv_swap = 1,
>> -		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
>> -	}, {
>> -		.fourcc = V4L2_PIX_FMT_VYUY,
>> -		.fmt_type = RKISP1_FMT_YUV,
>> -		.write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
>>   	}, {
>>   		.fourcc = V4L2_PIX_FMT_YUV422P,
>>   		.fmt_type = RKISP1_FMT_YUV,
>> @@ -234,18 +225,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,
>> -		.fmt_type = RKISP1_FMT_YUV,
>> -		.uv_swap = 1,
>> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
>> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>> -	}, {
>> -		.fourcc = V4L2_PIX_FMT_VYUY,
>> -		.fmt_type = RKISP1_FMT_YUV,
>> -		.uv_swap = 1,
>> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
>> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>>   	}, {
>>   		.fourcc = V4L2_PIX_FMT_YUV422P,
>>   		.fmt_type = RKISP1_FMT_YUV,
> 

Re: [PATCH v2 5/5] media: staging: rkisp1: cap: remove unsupported formats

[Dafna Hirschfeld]

On 4/6/20 2:42 PM, Dafna Hirschfeld wrote:
> 
> 
> On 4/6/20 12:43 AM, Laurent Pinchart wrote:
>> Hi Dafna,
>>
>> Thank you for the patch.
>>
>> On Thu, Apr 02, 2020 at 09:04:19PM +0200, Dafna Hirschfeld wrote:
>>> For Ycbcr packed formats only YUYV can be supported by
>>> the driver. This patch removes the other formats.
>>
>> The RKISP1_CIF_MI_BYTE_SWAP bit could possibly help achieving other YUV
>> orders, but as far as I can tell, it would affect both the main path and
>> the self path, so it wouldn't be very convenient. At a quick glance I
>> haven't found a way to support those formats, but just to make sure,
>> have you double-checked that the nv21_self and nv21_main bits of
>> MI_XTD_FORMAT_CTRL don't also affect packed mode ? If they don't,
> Hi, thanks a lot for the reviews, I'll check that.
Hi, unfortunately MI_XTD_FORMAT_CTRL doesn't affect the the packed formats.

Dafna
> 
> Dafna
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>> ---
>>>   drivers/staging/media/rkisp1/rkisp1-capture.c | 21 -------------------
>>>   1 file changed, 21 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> index 2d274e8f565b..076335193f40 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
>>> @@ -98,15 +98,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_mp_fmts[] = {
>>>           .fmt_type = RKISP1_FMT_YUV,
>>>           .uv_swap = 0,
>>>           .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
>>> -    }, {
>>> -        .fourcc = V4L2_PIX_FMT_YVYU,
>>> -        .fmt_type = RKISP1_FMT_YUV,
>>> -        .uv_swap = 1,
>>> -        .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
>>> -    }, {
>>> -        .fourcc = V4L2_PIX_FMT_VYUY,
>>> -        .fmt_type = RKISP1_FMT_YUV,
>>> -        .write_format = RKISP1_MI_CTRL_MP_WRITE_YUVINT,
>>>       }, {
>>>           .fourcc = V4L2_PIX_FMT_YUV422P,
>>>           .fmt_type = RKISP1_FMT_YUV,
>>> @@ -234,18 +225,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,
>>> -        .fmt_type = RKISP1_FMT_YUV,
>>> -        .uv_swap = 1,
>>> -        .write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
>>> -        .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>>> -    }, {
>>> -        .fourcc = V4L2_PIX_FMT_VYUY,
>>> -        .fmt_type = RKISP1_FMT_YUV,
>>> -        .uv_swap = 1,
>>> -        .write_format = RKISP1_MI_CTRL_SP_WRITE_INT,
>>> -        .output_format = RKISP1_MI_CTRL_SP_OUTPUT_YUV422,
>>>       }, {
>>>           .fourcc = V4L2_PIX_FMT_YUV422P,
>>>           .fmt_type = RKISP1_FMT_YUV,
>>