Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/4] media: staging: rkisp1: various fixes to capture formats
@ 2020-05-15 14:29 Dafna Hirschfeld
  2020-05-15 14:29 ` [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32 Dafna Hirschfeld
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Dafna Hirschfeld @ 2020-05-15 14:29 UTC (permalink / raw)
  To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, sakari.ailus, linux-rockchip, mchehab,
	laurent.pinchart

This patchset fixes some issues in the supported capture formats
of rkisp1.

Patches Summary:

1. Replaces the format V4L2_PIX_FMT_RGB24 with format V4L2_PIX_FMT_XBGR32 which
is the format the device supports.

2. Uses the vdiv, hdiv of the yuv422 explicitly instead of defining macros.
This is a cleanup patch to make it clear where those values are taken from.

3. In case of YUV444 format, the memory input format should stay YUV422
and not be converted by the resizer. This patch fixes it

4. remove support of BGR666 format since it is not supported by the device.

The fixes to formats V4L2_PIX_FMT_XBGR32, V4L2_PIX_FMT_YUV444M
were tested.

Changes from v1:
In v1 I sent a single patch
"media: staging: rkisp1: cap: change RGB24 format to XBGR32"
This patchset reword the commit log of that patch and includes
the 3 other patches.


Dafna Hirschfeld (4):
  media: staging: rkisp1: cap: change RGB24 format to XBGR32
  media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of
    macros
  media: staging: rkisp1: rsz: set output format to YUV422 if cap format
    is YUV444
  media: staging: rkisp1: cap: remove support of BGR666 format

 drivers/staging/media/rkisp1/rkisp1-capture.c |  6 +---
 drivers/staging/media/rkisp1/rkisp1-resizer.c | 28 +++++++++----------
 2 files changed, 15 insertions(+), 19 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32
  2020-05-15 14:29 [PATCH v2 0/4] media: staging: rkisp1: various fixes to capture formats Dafna Hirschfeld
@ 2020-05-15 14:29 ` Dafna Hirschfeld
  2020-05-20 21:50   ` Helen Koike
  2020-05-15 14:29 ` [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros Dafna Hirschfeld
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 18+ messages in thread
From: Dafna Hirschfeld @ 2020-05-15 14:29 UTC (permalink / raw)
  To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, sakari.ailus, linux-rockchip, mchehab,
	laurent.pinchart

According to the TRM, the YUV->RGB conversion outputs
"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>
---
 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 f69235f82c45..61b9ebe577b2 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


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
  2020-05-15 14:29 [PATCH v2 0/4] media: staging: rkisp1: various fixes to capture formats Dafna Hirschfeld
  2020-05-15 14:29 ` [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32 Dafna Hirschfeld
@ 2020-05-15 14:29 ` Dafna Hirschfeld
  2020-05-20 21:54   ` Helen Koike
  2020-05-15 14:29 ` [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444 Dafna Hirschfeld
  2020-05-15 14:29 ` [PATCH v2 4/4] media: staging: rkisp1: cap: remove support of BGR666 format Dafna Hirschfeld
  3 siblings, 1 reply; 18+ messages in thread
From: Dafna Hirschfeld @ 2020-05-15 14:29 UTC (permalink / raw)
  To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, sakari.ailus, linux-rockchip, mchehab,
	laurent.pinchart

The resize block of rkisp1 always get the input as yuv422.
Instead of defining the default hdiv, vdiv as macros, the
code is more clear if it takes the (hv)div from the YUV422P
info. This makes it clear where those values come from.
The patch also adds documentation to explain that.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index d049374413dc..04a29af8cc92 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -16,9 +16,6 @@
 #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
-
 enum rkisp1_shadow_regs_when {
 	RKISP1_SHADOW_REGS_SYNC,
 	RKISP1_SHADOW_REGS_ASYNC,
@@ -361,11 +358,12 @@ 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;
 	struct v4l2_rect sink_y, sink_c, src_y, src_c;
 	struct v4l2_mbus_framefmt *src_fmt;
 	struct v4l2_rect *sink_crop;
 	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
+	const struct v4l2_format_info *yuv422_info =
+		v4l2_format_info(V4L2_PIX_FMT_YUV422P);
 
 	sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK,
 					    V4L2_SUBDEV_FORMAT_ACTIVE);
@@ -386,8 +384,9 @@ 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;
+	/* The input format of the resizer is always yuv422 */
+	sink_c.width = sink_y.width / yuv422_info->hdiv;
+	sink_c.height = sink_y.height / yuv422_info->vdiv;
 
 	/*
 	 * The resizer is used not only to change the dimensions of the frame
@@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
 	 * (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.
+	 * is RGB then the memory input (the resizer output) should be YUV422
+	 * so we use the hdiv, vdiv of the YUV422 info in this case.
 	 */
 	if (v4l2_is_format_yuv(cap->pix.info)) {
-		hdiv = cap->pix.info->hdiv;
-		vdiv = cap->pix.info->vdiv;
+		src_c.width = cap->pix.info->hdiv;
+		src_c.height = cap->pix.info->vdiv;
+	} else {
+		src_c.width = src_y.width / yuv422_info->hdiv;
+		src_c.height = src_y.height / yuv422_info->vdiv;
 	}
 
-	src_c.width = src_y.width / hdiv;
-	src_c.height = src_y.height / vdiv;
-
 	if (sink_c.width == src_c.width && sink_c.height == src_c.height) {
 		rkisp1_rsz_disable(rsz, when);
 		return;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444
  2020-05-15 14:29 [PATCH v2 0/4] media: staging: rkisp1: various fixes to capture formats Dafna Hirschfeld
  2020-05-15 14:29 ` [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32 Dafna Hirschfeld
  2020-05-15 14:29 ` [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros Dafna Hirschfeld
@ 2020-05-15 14:29 ` Dafna Hirschfeld
  2020-05-20 22:23   ` Helen Koike
  2020-05-15 14:29 ` [PATCH v2 4/4] media: staging: rkisp1: cap: remove support of BGR666 format Dafna Hirschfeld
  3 siblings, 1 reply; 18+ messages in thread
From: Dafna Hirschfeld @ 2020-05-15 14:29 UTC (permalink / raw)
  To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, sakari.ailus, linux-rockchip, mchehab,
	laurent.pinchart

If the capture format is YUV444M then the memory input format
should be YUV422, so the resizer should not change the default
hdiv, vdiv in that case.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index 04a29af8cc92..5f9740ddd558 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -394,10 +394,11 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
 	 * (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 (the resizer output) should be YUV422
-	 * so we use the hdiv, vdiv of the YUV422 info in this case.
+	 * is RGB or YUV444 then the memory input (the resizer output) should
+	 * be YUV422 so we use the hdiv, vdiv of the YUV422 info in this case.
 	 */
-	if (v4l2_is_format_yuv(cap->pix.info)) {
+	if (v4l2_is_format_yuv(cap->pix.info) &&
+	    cap->pix.info->format != V4L2_PIX_FMT_YUV444M) {
 		src_c.width = cap->pix.info->hdiv;
 		src_c.height = cap->pix.info->vdiv;
 	} else {
-- 
2.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 4/4] media: staging: rkisp1: cap: remove support of BGR666 format
  2020-05-15 14:29 [PATCH v2 0/4] media: staging: rkisp1: various fixes to capture formats Dafna Hirschfeld
                   ` (2 preceding siblings ...)
  2020-05-15 14:29 ` [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444 Dafna Hirschfeld
@ 2020-05-15 14:29 ` Dafna Hirschfeld
  2020-05-20 22:34   ` Helen Koike
  3 siblings, 1 reply; 18+ messages in thread
From: Dafna Hirschfeld @ 2020-05-15 14:29 UTC (permalink / raw)
  To: linux-media, dafna.hirschfeld, helen.koike, ezequiel, hverkuil,
	kernel, dafna3, sakari.ailus, linux-rockchip, mchehab,
	laurent.pinchart

The rkisp1 supports RGB encoding with 6 bits per
color with the following format:
- - b5 b4 b3 b2 b1 b0 - - g5 g4 g3 g2 g1 g0 - - r5 r4 r3 r2 r1 r0 - - - - - - - -

This is not how V4L2_PIX_FMT_BGR666 is defined, so remove
this format from the driver's formats list.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index 61b9ebe577b2..2660e44eda88 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -283,10 +283,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
 		.fourcc = V4L2_PIX_FMT_RGB565,
 		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
 		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
-	}, {
-		.fourcc = V4L2_PIX_FMT_BGR666,
-		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
-		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB666,
 	},
 };
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32
  2020-05-15 14:29 ` [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32 Dafna Hirschfeld
@ 2020-05-20 21:50   ` Helen Koike
  0 siblings, 0 replies; 18+ messages in thread
From: Helen Koike @ 2020-05-20 21:50 UTC (permalink / raw)
  To: Dafna Hirschfeld, linux-media, ezequiel, hverkuil, kernel,
	dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart

Hi Dafna,

On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
> According to the TRM, the YUV->RGB conversion outputs
> "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.

I would just improve this a bit, since you only mention the number of bytes, but
doesn't mention why the colors are swapped.

My suggestion:

According to the TRM, 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.


With this clarification:

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

Thanks,
Helen

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  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 f69235f82c45..61b9ebe577b2 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,
>  	}, {
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
  2020-05-15 14:29 ` [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros Dafna Hirschfeld
@ 2020-05-20 21:54   ` Helen Koike
  2020-05-20 22:08     ` Helen Koike
  0 siblings, 1 reply; 18+ messages in thread
From: Helen Koike @ 2020-05-20 21:54 UTC (permalink / raw)
  To: Dafna Hirschfeld, linux-media, ezequiel, hverkuil, kernel,
	dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart

Hi Dafna,

On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
> The resize block of rkisp1 always get the input as yuv422.
> Instead of defining the default hdiv, vdiv as macros, the
> code is more clear if it takes the (hv)div from the YUV422P
> info. This makes it clear where those values come from.
> The patch also adds documentation to explain that.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

Thanks!
Helen

> ---
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> index d049374413dc..04a29af8cc92 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -16,9 +16,6 @@
>  #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
> -
>  enum rkisp1_shadow_regs_when {
>  	RKISP1_SHADOW_REGS_SYNC,
>  	RKISP1_SHADOW_REGS_ASYNC,
> @@ -361,11 +358,12 @@ 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;
>  	struct v4l2_rect sink_y, sink_c, src_y, src_c;
>  	struct v4l2_mbus_framefmt *src_fmt;
>  	struct v4l2_rect *sink_crop;
>  	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
> +	const struct v4l2_format_info *yuv422_info =
> +		v4l2_format_info(V4L2_PIX_FMT_YUV422P);
>  
>  	sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK,
>  					    V4L2_SUBDEV_FORMAT_ACTIVE);
> @@ -386,8 +384,9 @@ 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;
> +	/* The input format of the resizer is always yuv422 */
> +	sink_c.width = sink_y.width / yuv422_info->hdiv;
> +	sink_c.height = sink_y.height / yuv422_info->vdiv;
>  
>  	/*
>  	 * The resizer is used not only to change the dimensions of the frame
> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>  	 * (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.
> +	 * is RGB then the memory input (the resizer output) should be YUV422
> +	 * so we use the hdiv, vdiv of the YUV422 info in this case.
>  	 */
>  	if (v4l2_is_format_yuv(cap->pix.info)) {
> -		hdiv = cap->pix.info->hdiv;
> -		vdiv = cap->pix.info->vdiv;
> +		src_c.width = cap->pix.info->hdiv;
> +		src_c.height = cap->pix.info->vdiv;
> +	} else {
> +		src_c.width = src_y.width / yuv422_info->hdiv;
> +		src_c.height = src_y.height / yuv422_info->vdiv;
>  	}
>  
> -	src_c.width = src_y.width / hdiv;
> -	src_c.height = src_y.height / vdiv;
> -
>  	if (sink_c.width == src_c.width && sink_c.height == src_c.height) {
>  		rkisp1_rsz_disable(rsz, when);
>  		return;
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
  2020-05-20 21:54   ` Helen Koike
@ 2020-05-20 22:08     ` Helen Koike
  2020-05-22 12:11       ` Dafna Hirschfeld
  0 siblings, 1 reply; 18+ messages in thread
From: Helen Koike @ 2020-05-20 22:08 UTC (permalink / raw)
  To: Dafna Hirschfeld, linux-media, ezequiel, hverkuil, kernel,
	dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart



On 5/20/20 6:54 PM, Helen Koike wrote:
> Hi Dafna,
> 
> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
>> The resize block of rkisp1 always get the input as yuv422.
>> Instead of defining the default hdiv, vdiv as macros, the
>> code is more clear if it takes the (hv)div from the YUV422P
>> info. This makes it clear where those values come from.
>> The patch also adds documentation to explain that.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> 
> Acked-by: Helen Koike <helen.koike@collabora.com>
> 
> Thanks!
> Helen
> 
>> ---
>>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
>>  1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> index d049374413dc..04a29af8cc92 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> @@ -16,9 +16,6 @@
>>  #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
>> -
>>  enum rkisp1_shadow_regs_when {
>>  	RKISP1_SHADOW_REGS_SYNC,
>>  	RKISP1_SHADOW_REGS_ASYNC,
>> @@ -361,11 +358,12 @@ 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;
>>  	struct v4l2_rect sink_y, sink_c, src_y, src_c;
>>  	struct v4l2_mbus_framefmt *src_fmt;
>>  	struct v4l2_rect *sink_crop;
>>  	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
>> +	const struct v4l2_format_info *yuv422_info =
>> +		v4l2_format_info(V4L2_PIX_FMT_YUV422P);
>>  
>>  	sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK,
>>  					    V4L2_SUBDEV_FORMAT_ACTIVE);
>> @@ -386,8 +384,9 @@ 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;
>> +	/* The input format of the resizer is always yuv422 */
>> +	sink_c.width = sink_y.width / yuv422_info->hdiv;
>> +	sink_c.height = sink_y.height / yuv422_info->vdiv;
>>  
>>  	/*
>>  	 * The resizer is used not only to change the dimensions of the frame
>> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>>  	 * (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.
>> +	 * is RGB then the memory input (the resizer output) should be YUV422
>> +	 * so we use the hdiv, vdiv of the YUV422 info in this case.
>>  	 */
>>  	if (v4l2_is_format_yuv(cap->pix.info)) {
>> -		hdiv = cap->pix.info->hdiv;
>> -		vdiv = cap->pix.info->vdiv;
>> +		src_c.width = cap->pix.info->hdiv;
>> +		src_c.height = cap->pix.info->vdiv;

Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong.

It should be:

src_c.width = src_y.width / cap->pix.info->hdiv;
src_c.height = src_y.height / cap->pix.info->vdiv;

Regards,
Helen

>> +	} else {
>> +		src_c.width = src_y.width / yuv422_info->hdiv;
>> +		src_c.height = src_y.height / yuv422_info->vdiv;
>>  	}
>>  
>> -	src_c.width = src_y.width / hdiv;
>> -	src_c.height = src_y.height / vdiv;
>> -
>>  	if (sink_c.width == src_c.width && sink_c.height == src_c.height) {
>>  		rkisp1_rsz_disable(rsz, when);
>>  		return;
>>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444
  2020-05-15 14:29 ` [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444 Dafna Hirschfeld
@ 2020-05-20 22:23   ` Helen Koike
  0 siblings, 0 replies; 18+ messages in thread
From: Helen Koike @ 2020-05-20 22:23 UTC (permalink / raw)
  To: Dafna Hirschfeld, linux-media, ezequiel, hverkuil, kernel,
	dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart

Hi Dafna,

On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
> If the capture format is YUV444M then the memory input format
> should be YUV422, so the resizer should not change the default
> hdiv, vdiv in that case.

I didn't understand why YUV444M is special, do you mind elaborating a bit
more to help me understand please?

Thanks
Helen

> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> index 04a29af8cc92..5f9740ddd558 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -394,10 +394,11 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>  	 * (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 (the resizer output) should be YUV422
> -	 * so we use the hdiv, vdiv of the YUV422 info in this case.
> +	 * is RGB or YUV444 then the memory input (the resizer output) should
> +	 * be YUV422 so we use the hdiv, vdiv of the YUV422 info in this case.
>  	 */
> -	if (v4l2_is_format_yuv(cap->pix.info)) {
> +	if (v4l2_is_format_yuv(cap->pix.info) &&
> +	    cap->pix.info->format != V4L2_PIX_FMT_YUV444M) {
>  		src_c.width = cap->pix.info->hdiv;
>  		src_c.height = cap->pix.info->vdiv;
>  	} else {
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 4/4] media: staging: rkisp1: cap: remove support of BGR666 format
  2020-05-15 14:29 ` [PATCH v2 4/4] media: staging: rkisp1: cap: remove support of BGR666 format Dafna Hirschfeld
@ 2020-05-20 22:34   ` Helen Koike
  0 siblings, 0 replies; 18+ messages in thread
From: Helen Koike @ 2020-05-20 22:34 UTC (permalink / raw)
  To: Dafna Hirschfeld, linux-media, ezequiel, hverkuil, kernel,
	dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart



On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
> The rkisp1 supports RGB encoding with 6 bits per
> color with the following format:
> - - b5 b4 b3 b2 b1 b0 - - g5 g4 g3 g2 g1 g0 - - r5 r4 r3 r2 r1 r0 - - - - - - - -

Is this the same as V4L2_PIX_FMT_XBGR32 format, but with colors range from 0 to 63 ?

I was wondering what is the usage of such a format. If it is useful, then I'm sad
to see this being removed from the driver, since this is a v4l2 api limitation, and not
a hw limitation.

Regards,
Helen

> 
> This is not how V4L2_PIX_FMT_BGR666 is defined, so remove
> this format from the driver's formats list.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index 61b9ebe577b2..2660e44eda88 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -283,10 +283,6 @@ static const struct rkisp1_capture_fmt_cfg rkisp1_sp_fmts[] = {
>  		.fourcc = V4L2_PIX_FMT_RGB565,
>  		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
>  		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB565,
> -	}, {
> -		.fourcc = V4L2_PIX_FMT_BGR666,
> -		.write_format = RKISP1_MI_CTRL_SP_WRITE_PLA,
> -		.output_format = RKISP1_MI_CTRL_SP_OUTPUT_RGB666,
>  	},
>  };
>  
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
  2020-05-20 22:08     ` Helen Koike
@ 2020-05-22 12:11       ` Dafna Hirschfeld
  2020-05-22 13:31         ` Ezequiel Garcia
  2020-05-22 13:57         ` Laurent Pinchart
  0 siblings, 2 replies; 18+ messages in thread
From: Dafna Hirschfeld @ 2020-05-22 12:11 UTC (permalink / raw)
  To: Helen Koike, linux-media, ezequiel, hverkuil, kernel, dafna3,
	sakari.ailus, linux-rockchip, mchehab, laurent.pinchart,
	Tomasz Figa



On 21.05.20 00:08, Helen Koike wrote:
> 
> 
> On 5/20/20 6:54 PM, Helen Koike wrote:
>> Hi Dafna,
>>
>> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
>>> The resize block of rkisp1 always get the input as yuv422.
>>> Instead of defining the default hdiv, vdiv as macros, the
>>> code is more clear if it takes the (hv)div from the YUV422P
>>> info. This makes it clear where those values come from.
>>> The patch also adds documentation to explain that.
>>>
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>
>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>
>> Thanks!
>> Helen
>>
>>> ---
>>>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
>>>   1 file changed, 12 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>> index d049374413dc..04a29af8cc92 100644
>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>> @@ -16,9 +16,6 @@
>>>   #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
>>> -
>>>   enum rkisp1_shadow_regs_when {
>>>   	RKISP1_SHADOW_REGS_SYNC,
>>>   	RKISP1_SHADOW_REGS_ASYNC,
>>> @@ -361,11 +358,12 @@ 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;
>>>   	struct v4l2_rect sink_y, sink_c, src_y, src_c;
>>>   	struct v4l2_mbus_framefmt *src_fmt;
>>>   	struct v4l2_rect *sink_crop;
>>>   	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
>>> +	const struct v4l2_format_info *yuv422_info =
>>> +		v4l2_format_info(V4L2_PIX_FMT_YUV422P);
>>>   
>>>   	sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK,
>>>   					    V4L2_SUBDEV_FORMAT_ACTIVE);
>>> @@ -386,8 +384,9 @@ 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;
>>> +	/* The input format of the resizer is always yuv422 */
>>> +	sink_c.width = sink_y.width / yuv422_info->hdiv;
>>> +	sink_c.height = sink_y.height / yuv422_info->vdiv;
>>>   
>>>   	/*
>>>   	 * The resizer is used not only to change the dimensions of the frame
>>> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>>>   	 * (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.
>>> +	 * is RGB then the memory input (the resizer output) should be YUV422
>>> +	 * so we use the hdiv, vdiv of the YUV422 info in this case.
>>>   	 */
>>>   	if (v4l2_is_format_yuv(cap->pix.info)) {
>>> -		hdiv = cap->pix.info->hdiv;
>>> -		vdiv = cap->pix.info->vdiv;
>>> +		src_c.width = cap->pix.info->hdiv;
>>> +		src_c.height = cap->pix.info->vdiv;
> 
> Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong.
> 
> It should be:
> 
> src_c.width = src_y.width / cap->pix.info->hdiv;
> src_c.height = src_y.height / cap->pix.info->vdiv;

autch, thanks for finding it,  I probably concentrated only on testing the new RGB formats

Thanks,
Dafna

> 
> Regards,
> Helen
> 
>>> +	} else {
>>> +		src_c.width = src_y.width / yuv422_info->hdiv;
>>> +		src_c.height = src_y.height / yuv422_info->vdiv;
>>>   	}
>>>   
>>> -	src_c.width = src_y.width / hdiv;
>>> -	src_c.height = src_y.height / vdiv;
>>> -
>>>   	if (sink_c.width == src_c.width && sink_c.height == src_c.height) {
>>>   		rkisp1_rsz_disable(rsz, when);
>>>   		return;
>>>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
  2020-05-22 12:11       ` Dafna Hirschfeld
@ 2020-05-22 13:31         ` Ezequiel Garcia
  2020-05-22 14:15           ` Dafna Hirschfeld
  2020-05-22 13:57         ` Laurent Pinchart
  1 sibling, 1 reply; 18+ messages in thread
From: Ezequiel Garcia @ 2020-05-22 13:31 UTC (permalink / raw)
  To: Dafna Hirschfeld, Helen Koike, linux-media, hverkuil, kernel,
	dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart,
	Tomasz Figa

Hi Dafna, Helen,

On Fri, 2020-05-22 at 14:11 +0200, Dafna Hirschfeld wrote:
> 
> On 21.05.20 00:08, Helen Koike wrote:
> > 
> > On 5/20/20 6:54 PM, Helen Koike wrote:
> > > Hi Dafna,
> > > 
> > > On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
> > > > The resize block of rkisp1 always get the input as yuv422.
> > > > Instead of defining the default hdiv, vdiv as macros, the
> > > > code is more clear if it takes the (hv)div from the YUV422P
> > > > info. This makes it clear where those values come from.
> > > > The patch also adds documentation to explain that.
> > > > 
> > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > > 
> > > Acked-by: Helen Koike <helen.koike@collabora.com>
> > > 
> > > Thanks!
> > > Helen
> > > 
> > > > ---
> > > >   drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
> > > >   1 file changed, 12 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> > > > index d049374413dc..04a29af8cc92 100644
> > > > --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> > > > +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> > > > @@ -16,9 +16,6 @@
> > > >   #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
> > > > -
> > > >   enum rkisp1_shadow_regs_when {
> > > >   	RKISP1_SHADOW_REGS_SYNC,
> > > >   	RKISP1_SHADOW_REGS_ASYNC,
> > > > @@ -361,11 +358,12 @@ 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;
> > > >   	struct v4l2_rect sink_y, sink_c, src_y, src_c;
> > > >   	struct v4l2_mbus_framefmt *src_fmt;
> > > >   	struct v4l2_rect *sink_crop;
> > > >   	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
> > > > +	const struct v4l2_format_info *yuv422_info =
> > > > +		v4l2_format_info(V4L2_PIX_FMT_YUV422P);
> > > >   

Instead of hardcoding this fourcc, is there any way we can
retrieve it from a configured format?

Thanks,
Ezequiel


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
  2020-05-22 12:11       ` Dafna Hirschfeld
  2020-05-22 13:31         ` Ezequiel Garcia
@ 2020-05-22 13:57         ` Laurent Pinchart
  2020-05-22 14:54           ` Dafna Hirschfeld
  1 sibling, 1 reply; 18+ messages in thread
From: Laurent Pinchart @ 2020-05-22 13:57 UTC (permalink / raw)
  To: Dafna Hirschfeld
  Cc: Helen Koike, linux-media, ezequiel, hverkuil, kernel, dafna3,
	sakari.ailus, linux-rockchip, mchehab, Tomasz Figa

Hi Dafna,

On Fri, May 22, 2020 at 02:11:22PM +0200, Dafna Hirschfeld wrote:
> On 21.05.20 00:08, Helen Koike wrote:
> > On 5/20/20 6:54 PM, Helen Koike wrote:
> >> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
> >>> The resize block of rkisp1 always get the input as yuv422.
> >>> Instead of defining the default hdiv, vdiv as macros, the
> >>> code is more clear if it takes the (hv)div from the YUV422P
> >>> info. This makes it clear where those values come from.
> >>> The patch also adds documentation to explain that.
> >>>
> >>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >>
> >> Acked-by: Helen Koike <helen.koike@collabora.com>
> >>
> >>> ---
> >>>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
> >>>   1 file changed, 12 insertions(+), 13 deletions(-)
> >>>
> >>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>> index d049374413dc..04a29af8cc92 100644
> >>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>> @@ -16,9 +16,6 @@
> >>>   #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
> >>> -
> >>>   enum rkisp1_shadow_regs_when {
> >>>   	RKISP1_SHADOW_REGS_SYNC,
> >>>   	RKISP1_SHADOW_REGS_ASYNC,
> >>> @@ -361,11 +358,12 @@ 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;
> >>>   	struct v4l2_rect sink_y, sink_c, src_y, src_c;
> >>>   	struct v4l2_mbus_framefmt *src_fmt;
> >>>   	struct v4l2_rect *sink_crop;
> >>>   	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
> >>> +	const struct v4l2_format_info *yuv422_info =
> >>> +		v4l2_format_info(V4L2_PIX_FMT_YUV422P);
> >>>   
> >>>   	sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK,
> >>>   					    V4L2_SUBDEV_FORMAT_ACTIVE);
> >>> @@ -386,8 +384,9 @@ 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;
> >>> +	/* The input format of the resizer is always yuv422 */
> >>> +	sink_c.width = sink_y.width / yuv422_info->hdiv;
> >>> +	sink_c.height = sink_y.height / yuv422_info->vdiv;
> >>>   
> >>>   	/*
> >>>   	 * The resizer is used not only to change the dimensions of the frame
> >>> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
> >>>   	 * (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.
> >>> +	 * is RGB then the memory input (the resizer output) should be YUV422
> >>> +	 * so we use the hdiv, vdiv of the YUV422 info in this case.
> >>>   	 */
> >>>   	if (v4l2_is_format_yuv(cap->pix.info)) {
> >>> -		hdiv = cap->pix.info->hdiv;
> >>> -		vdiv = cap->pix.info->vdiv;
> >>> +		src_c.width = cap->pix.info->hdiv;
> >>> +		src_c.height = cap->pix.info->vdiv;
> > 
> > Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong.
> > 
> > It should be:
> > 
> > src_c.width = src_y.width / cap->pix.info->hdiv;
> > src_c.height = src_y.height / cap->pix.info->vdiv;
> 
> autch, thanks for finding it,  I probably concentrated only on testing the new RGB formats

Please make sure to test all supported formats :-)

I really, really recommend writing a small set of test scripts that will
automate the tests for you. It can be as simple as wrapping media-ctl +
yavta (or v4l2-ctl), or the libcamera cam utility if it offers all the
features you need, and run them for all supported formats.

> >>> +	} else {
> >>> +		src_c.width = src_y.width / yuv422_info->hdiv;
> >>> +		src_c.height = src_y.height / yuv422_info->vdiv;
> >>>   	}
> >>>   
> >>> -	src_c.width = src_y.width / hdiv;
> >>> -	src_c.height = src_y.height / vdiv;
> >>> -
> >>>   	if (sink_c.width == src_c.width && sink_c.height == src_c.height) {
> >>>   		rkisp1_rsz_disable(rsz, when);
> >>>   		return;
> >>>

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
  2020-05-22 13:31         ` Ezequiel Garcia
@ 2020-05-22 14:15           ` Dafna Hirschfeld
  2020-05-22 15:04             ` Ezequiel Garcia
  0 siblings, 1 reply; 18+ messages in thread
From: Dafna Hirschfeld @ 2020-05-22 14:15 UTC (permalink / raw)
  To: Ezequiel Garcia, Helen Koike, linux-media, hverkuil, kernel,
	dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart,
	Tomasz Figa



On 22.05.20 15:31, Ezequiel Garcia wrote:
> Hi Dafna, Helen,
> 
> On Fri, 2020-05-22 at 14:11 +0200, Dafna Hirschfeld wrote:
>>
>> On 21.05.20 00:08, Helen Koike wrote:
>>>
>>> On 5/20/20 6:54 PM, Helen Koike wrote:
>>>> Hi Dafna,
>>>>
>>>> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
>>>>> The resize block of rkisp1 always get the input as yuv422.
>>>>> Instead of defining the default hdiv, vdiv as macros, the
>>>>> code is more clear if it takes the (hv)div from the YUV422P
>>>>> info. This makes it clear where those values come from.
>>>>> The patch also adds documentation to explain that.
>>>>>
>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>>
>>>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>>>
>>>> Thanks!
>>>> Helen
>>>>
>>>>> ---
>>>>>    drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
>>>>>    1 file changed, 12 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>>>> index d049374413dc..04a29af8cc92 100644
>>>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>>>> @@ -16,9 +16,6 @@
>>>>>    #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
>>>>> -
>>>>>    enum rkisp1_shadow_regs_when {
>>>>>    	RKISP1_SHADOW_REGS_SYNC,
>>>>>    	RKISP1_SHADOW_REGS_ASYNC,
>>>>> @@ -361,11 +358,12 @@ 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;
>>>>>    	struct v4l2_rect sink_y, sink_c, src_y, src_c;
>>>>>    	struct v4l2_mbus_framefmt *src_fmt;
>>>>>    	struct v4l2_rect *sink_crop;
>>>>>    	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
>>>>> +	const struct v4l2_format_info *yuv422_info =
>>>>> +		v4l2_format_info(V4L2_PIX_FMT_YUV422P);
>>>>>    
> 
> Instead of hardcoding this fourcc, is there any way we can
> retrieve it from a configured format?
> 
What do you mean?
If the configured format is bayer then the resizer is disabled.
Otherwise the resizer always get the input as yuv422, this is why it is hard coded.

Thanks,
Dafna

> Thanks,
> Ezequiel
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
  2020-05-22 13:57         ` Laurent Pinchart
@ 2020-05-22 14:54           ` Dafna Hirschfeld
  2020-05-22 14:59             ` Laurent Pinchart
  0 siblings, 1 reply; 18+ messages in thread
From: Dafna Hirschfeld @ 2020-05-22 14:54 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Helen Koike, linux-media, ezequiel, hverkuil, kernel, dafna3,
	sakari.ailus, linux-rockchip, mchehab, Tomasz Figa



On 22.05.20 15:57, Laurent Pinchart wrote:
> Hi Dafna,
> 
> On Fri, May 22, 2020 at 02:11:22PM +0200, Dafna Hirschfeld wrote:
>> On 21.05.20 00:08, Helen Koike wrote:
>>> On 5/20/20 6:54 PM, Helen Koike wrote:
>>>> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
>>>>> The resize block of rkisp1 always get the input as yuv422.
>>>>> Instead of defining the default hdiv, vdiv as macros, the
>>>>> code is more clear if it takes the (hv)div from the YUV422P
>>>>> info. This makes it clear where those values come from.
>>>>> The patch also adds documentation to explain that.
>>>>>
>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>>
>>>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>>>
>>>>> ---
>>>>>    drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
>>>>>    1 file changed, 12 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>>>> index d049374413dc..04a29af8cc92 100644
>>>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>>>> @@ -16,9 +16,6 @@
>>>>>    #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
>>>>> -
>>>>>    enum rkisp1_shadow_regs_when {
>>>>>    	RKISP1_SHADOW_REGS_SYNC,
>>>>>    	RKISP1_SHADOW_REGS_ASYNC,
>>>>> @@ -361,11 +358,12 @@ 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;
>>>>>    	struct v4l2_rect sink_y, sink_c, src_y, src_c;
>>>>>    	struct v4l2_mbus_framefmt *src_fmt;
>>>>>    	struct v4l2_rect *sink_crop;
>>>>>    	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
>>>>> +	const struct v4l2_format_info *yuv422_info =
>>>>> +		v4l2_format_info(V4L2_PIX_FMT_YUV422P);
>>>>>    
>>>>>    	sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK,
>>>>>    					    V4L2_SUBDEV_FORMAT_ACTIVE);
>>>>> @@ -386,8 +384,9 @@ 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;
>>>>> +	/* The input format of the resizer is always yuv422 */
>>>>> +	sink_c.width = sink_y.width / yuv422_info->hdiv;
>>>>> +	sink_c.height = sink_y.height / yuv422_info->vdiv;
>>>>>    
>>>>>    	/*
>>>>>    	 * The resizer is used not only to change the dimensions of the frame
>>>>> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>>>>>    	 * (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.
>>>>> +	 * is RGB then the memory input (the resizer output) should be YUV422
>>>>> +	 * so we use the hdiv, vdiv of the YUV422 info in this case.
>>>>>    	 */
>>>>>    	if (v4l2_is_format_yuv(cap->pix.info)) {
>>>>> -		hdiv = cap->pix.info->hdiv;
>>>>> -		vdiv = cap->pix.info->vdiv;
>>>>> +		src_c.width = cap->pix.info->hdiv;
>>>>> +		src_c.height = cap->pix.info->vdiv;
>>>
>>> Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong.
>>>
>>> It should be:
>>>
>>> src_c.width = src_y.width / cap->pix.info->hdiv;
>>> src_c.height = src_y.height / cap->pix.info->vdiv;
>>
>> autch, thanks for finding it,  I probably concentrated only on testing the new RGB formats
> 
> Please make sure to test all supported formats :-)Yes, I don't how I missed that :/

> 
> I really, really recommend writing a small set of test scripts that will
> automate the tests for you. It can be as simple as wrapping media-ctl +
> yavta (or v4l2-ctl), or the libcamera cam utility if it offers all the
> features you need, and run them for all supported formats.

We already have python scripts that wrap media-ctl/v4l2-ctl
https://gitlab.collabora.com/dafna/v4l-utils/-/blob/rkisp1-simult-stream/contrib/test/test-rkisp1.py

For example, you can run
python3 test-rkisp1.py -t CUSTOM_RKISP1_TEST_stream -o /root -m YUYV8_2X8 -p sp -P YU12 -S  -v -c --isp-dim 1300x1500 --resizer-dim 1200x700

Then:
'-m YUYV8_2X8' is the media bus for the output of the isp
'-p sp' is for streaming from selfpath video node
'-P YU12' is for format yuv420p
'--sip-dim 1300x1500' is the values to set the sensor and the crop of the sink pad of the isp entity
'--resizer-dim 1200x700' is the output dimensions of the resizer

Thanks,
Dafna

> 
>>>>> +	} else {
>>>>> +		src_c.width = src_y.width / yuv422_info->hdiv;
>>>>> +		src_c.height = src_y.height / yuv422_info->vdiv;
>>>>>    	}
>>>>>    
>>>>> -	src_c.width = src_y.width / hdiv;
>>>>> -	src_c.height = src_y.height / vdiv;
>>>>> -
>>>>>    	if (sink_c.width == src_c.width && sink_c.height == src_c.height) {
>>>>>    		rkisp1_rsz_disable(rsz, when);
>>>>>    		return;
>>>>>
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
  2020-05-22 14:54           ` Dafna Hirschfeld
@ 2020-05-22 14:59             ` Laurent Pinchart
  0 siblings, 0 replies; 18+ messages in thread
From: Laurent Pinchart @ 2020-05-22 14:59 UTC (permalink / raw)
  To: Dafna Hirschfeld
  Cc: Helen Koike, linux-media, ezequiel, hverkuil, kernel, dafna3,
	sakari.ailus, linux-rockchip, mchehab, Tomasz Figa

Hi Dafna,

On Fri, May 22, 2020 at 04:54:24PM +0200, Dafna Hirschfeld wrote:
> On 22.05.20 15:57, Laurent Pinchart wrote:
> > On Fri, May 22, 2020 at 02:11:22PM +0200, Dafna Hirschfeld wrote:
> >> On 21.05.20 00:08, Helen Koike wrote:
> >>> On 5/20/20 6:54 PM, Helen Koike wrote:
> >>>> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
> >>>>> The resize block of rkisp1 always get the input as yuv422.
> >>>>> Instead of defining the default hdiv, vdiv as macros, the
> >>>>> code is more clear if it takes the (hv)div from the YUV422P
> >>>>> info. This makes it clear where those values come from.
> >>>>> The patch also adds documentation to explain that.
> >>>>>
> >>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >>>>
> >>>> Acked-by: Helen Koike <helen.koike@collabora.com>
> >>>>
> >>>>> ---
> >>>>>    drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
> >>>>>    1 file changed, 12 insertions(+), 13 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>>>> index d049374413dc..04a29af8cc92 100644
> >>>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >>>>> @@ -16,9 +16,6 @@
> >>>>>    #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
> >>>>> -
> >>>>>    enum rkisp1_shadow_regs_when {
> >>>>>    	RKISP1_SHADOW_REGS_SYNC,
> >>>>>    	RKISP1_SHADOW_REGS_ASYNC,
> >>>>> @@ -361,11 +358,12 @@ 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;
> >>>>>    	struct v4l2_rect sink_y, sink_c, src_y, src_c;
> >>>>>    	struct v4l2_mbus_framefmt *src_fmt;
> >>>>>    	struct v4l2_rect *sink_crop;
> >>>>>    	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
> >>>>> +	const struct v4l2_format_info *yuv422_info =
> >>>>> +		v4l2_format_info(V4L2_PIX_FMT_YUV422P);
> >>>>>    
> >>>>>    	sink_crop = rkisp1_rsz_get_pad_crop(rsz, NULL, RKISP1_RSZ_PAD_SINK,
> >>>>>    					    V4L2_SUBDEV_FORMAT_ACTIVE);
> >>>>> @@ -386,8 +384,9 @@ 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;
> >>>>> +	/* The input format of the resizer is always yuv422 */
> >>>>> +	sink_c.width = sink_y.width / yuv422_info->hdiv;
> >>>>> +	sink_c.height = sink_y.height / yuv422_info->vdiv;
> >>>>>    
> >>>>>    	/*
> >>>>>    	 * The resizer is used not only to change the dimensions of the frame
> >>>>> @@ -395,17 +394,17 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
> >>>>>    	 * (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.
> >>>>> +	 * is RGB then the memory input (the resizer output) should be YUV422
> >>>>> +	 * so we use the hdiv, vdiv of the YUV422 info in this case.
> >>>>>    	 */
> >>>>>    	if (v4l2_is_format_yuv(cap->pix.info)) {
> >>>>> -		hdiv = cap->pix.info->hdiv;
> >>>>> -		vdiv = cap->pix.info->vdiv;
> >>>>> +		src_c.width = cap->pix.info->hdiv;
> >>>>> +		src_c.height = cap->pix.info->vdiv;
> >>>
> >>> Sorry, I just noticed you are assigning hdiv and vdiv directly to width and height, which looks wrong.
> >>>
> >>> It should be:
> >>>
> >>> src_c.width = src_y.width / cap->pix.info->hdiv;
> >>> src_c.height = src_y.height / cap->pix.info->vdiv;
> >>
> >> autch, thanks for finding it,  I probably concentrated only on testing the new RGB formats
> > 
> > Please make sure to test all supported formats :-)
> 
> Yes, I don't how I missed that :/
> 
> > I really, really recommend writing a small set of test scripts that will
> > automate the tests for you. It can be as simple as wrapping media-ctl +
> > yavta (or v4l2-ctl), or the libcamera cam utility if it offers all the
> > features you need, and run them for all supported formats.
> 
> We already have python scripts that wrap media-ctl/v4l2-ctl
> https://gitlab.collabora.com/dafna/v4l-utils/-/blob/rkisp1-simult-stream/contrib/test/test-rkisp1.py
> 
> For example, you can run
> python3 test-rkisp1.py -t CUSTOM_RKISP1_TEST_stream -o /root -m YUYV8_2X8 -p sp -P YU12 -S  -v -c --isp-dim 1300x1500 --resizer-dim 1200x700
> 
> Then:
> '-m YUYV8_2X8' is the media bus for the output of the isp
> '-p sp' is for streaming from selfpath video node
> '-P YU12' is for format yuv420p
> '--sip-dim 1300x1500' is the values to set the sensor and the crop of the sink pad of the isp entity
> '--resizer-dim 1200x700' is the output dimensions of the resizer

I recommend wrapping that into a script that will test all supported
formats, and give a pass/fail result. For instance, we have developed
unit test scripts for the Renesas VSP (a memory-to-memory video
processing engine supported by a V4L2 driver), available at
http://git.ideasonboard.com/renesas/vsp-tests.git, that test lots of
different pipelines, including checks on the output image.

Checking the output image is more difficult in your case, as the input
to the rkisp1 is a camera sensor and not memory, but maybe there's a
test pattern mode that could be leveraged ?

In any case, validation of the image content is likely a long term
project, short term I recommend test cases that will try all the
supported formats in various resolutions, to ensure there's no crash,
and you can limit the verification to the captured buffer size for
instance.

> >>>>> +	} else {
> >>>>> +		src_c.width = src_y.width / yuv422_info->hdiv;
> >>>>> +		src_c.height = src_y.height / yuv422_info->vdiv;
> >>>>>    	}
> >>>>>    
> >>>>> -	src_c.width = src_y.width / hdiv;
> >>>>> -	src_c.height = src_y.height / vdiv;
> >>>>> -
> >>>>>    	if (sink_c.width == src_c.width && sink_c.height == src_c.height) {
> >>>>>    		rkisp1_rsz_disable(rsz, when);
> >>>>>    		return;
> >>>>>

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
  2020-05-22 14:15           ` Dafna Hirschfeld
@ 2020-05-22 15:04             ` Ezequiel Garcia
  2020-05-25  9:51               ` Dafna Hirschfeld
  0 siblings, 1 reply; 18+ messages in thread
From: Ezequiel Garcia @ 2020-05-22 15:04 UTC (permalink / raw)
  To: Dafna Hirschfeld, Helen Koike, linux-media, hverkuil, kernel,
	dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart,
	Tomasz Figa

On Fri, 2020-05-22 at 16:15 +0200, Dafna Hirschfeld wrote:
> 
> On 22.05.20 15:31, Ezequiel Garcia wrote:
> > Hi Dafna, Helen,
> > 
> > On Fri, 2020-05-22 at 14:11 +0200, Dafna Hirschfeld wrote:
> > > On 21.05.20 00:08, Helen Koike wrote:
> > > > On 5/20/20 6:54 PM, Helen Koike wrote:
> > > > > Hi Dafna,
> > > > > 
> > > > > On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
> > > > > > The resize block of rkisp1 always get the input as yuv422.
> > > > > > Instead of defining the default hdiv, vdiv as macros, the
> > > > > > code is more clear if it takes the (hv)div from the YUV422P
> > > > > > info. This makes it clear where those values come from.
> > > > > > The patch also adds documentation to explain that.
> > > > > > 
> > > > > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > > > > 
> > > > > Acked-by: Helen Koike <helen.koike@collabora.com>
> > > > > 
> > > > > Thanks!
> > > > > Helen
> > > > > 
> > > > > > ---
> > > > > >    drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
> > > > > >    1 file changed, 12 insertions(+), 13 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> > > > > > index d049374413dc..04a29af8cc92 100644
> > > > > > --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> > > > > > +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> > > > > > @@ -16,9 +16,6 @@
> > > > > >    #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
> > > > > > -
> > > > > >    enum rkisp1_shadow_regs_when {
> > > > > >    	RKISP1_SHADOW_REGS_SYNC,
> > > > > >    	RKISP1_SHADOW_REGS_ASYNC,
> > > > > > @@ -361,11 +358,12 @@ 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;
> > > > > >    	struct v4l2_rect sink_y, sink_c, src_y, src_c;
> > > > > >    	struct v4l2_mbus_framefmt *src_fmt;
> > > > > >    	struct v4l2_rect *sink_crop;
> > > > > >    	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
> > > > > > +	const struct v4l2_format_info *yuv422_info =
> > > > > > +		v4l2_format_info(V4L2_PIX_FMT_YUV422P);
> > > > > >    
> > 
> > Instead of hardcoding this fourcc, is there any way we can
> > retrieve it from a configured format?
> > 
> What do you mean?
> If the configured format is bayer then the resizer is disabled.
> Otherwise the resizer always get the input as yuv422, this is why it is hard coded.
> 

I don't like to rely on these assumptions/knowledge.
It's much cleaner to retrieve the format, and avoiding
hardcoding things as much as you can.

Hope I'm making sense :-)

Ezequiel


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros
  2020-05-22 15:04             ` Ezequiel Garcia
@ 2020-05-25  9:51               ` Dafna Hirschfeld
  0 siblings, 0 replies; 18+ messages in thread
From: Dafna Hirschfeld @ 2020-05-25  9:51 UTC (permalink / raw)
  To: Ezequiel Garcia, Helen Koike, linux-media, hverkuil, kernel,
	dafna3, sakari.ailus, linux-rockchip, mchehab, laurent.pinchart,
	Tomasz Figa



On 22.05.20 17:04, Ezequiel Garcia wrote:
> On Fri, 2020-05-22 at 16:15 +0200, Dafna Hirschfeld wrote:
>>
>> On 22.05.20 15:31, Ezequiel Garcia wrote:
>>> Hi Dafna, Helen,
>>>
>>> On Fri, 2020-05-22 at 14:11 +0200, Dafna Hirschfeld wrote:
>>>> On 21.05.20 00:08, Helen Koike wrote:
>>>>> On 5/20/20 6:54 PM, Helen Koike wrote:
>>>>>> Hi Dafna,
>>>>>>
>>>>>> On 5/15/20 11:29 AM, Dafna Hirschfeld wrote:
>>>>>>> The resize block of rkisp1 always get the input as yuv422.
>>>>>>> Instead of defining the default hdiv, vdiv as macros, the
>>>>>>> code is more clear if it takes the (hv)div from the YUV422P
>>>>>>> info. This makes it clear where those values come from.
>>>>>>> The patch also adds documentation to explain that.
>>>>>>>
>>>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>>>>
>>>>>> Acked-by: Helen Koike <helen.koike@collabora.com>
>>>>>>
>>>>>> Thanks!
>>>>>> Helen
>>>>>>
>>>>>>> ---
>>>>>>>     drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
>>>>>>>     1 file changed, 12 insertions(+), 13 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>>>>>> index d049374413dc..04a29af8cc92 100644
>>>>>>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>>>>>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>>>>>>> @@ -16,9 +16,6 @@
>>>>>>>     #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
>>>>>>> -
>>>>>>>     enum rkisp1_shadow_regs_when {
>>>>>>>     	RKISP1_SHADOW_REGS_SYNC,
>>>>>>>     	RKISP1_SHADOW_REGS_ASYNC,
>>>>>>> @@ -361,11 +358,12 @@ 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;
>>>>>>>     	struct v4l2_rect sink_y, sink_c, src_y, src_c;
>>>>>>>     	struct v4l2_mbus_framefmt *src_fmt;
>>>>>>>     	struct v4l2_rect *sink_crop;
>>>>>>>     	struct rkisp1_capture *cap = &rsz->rkisp1->capture_devs[rsz->id];
>>>>>>> +	const struct v4l2_format_info *yuv422_info =
>>>>>>> +		v4l2_format_info(V4L2_PIX_FMT_YUV422P);
>>>>>>>     
>>>
>>> Instead of hardcoding this fourcc, is there any way we can
>>> retrieve it from a configured format?
>>>
>> What do you mean?
>> If the configured format is bayer then the resizer is disabled.
>> Otherwise the resizer always get the input as yuv422, this is why it is hard coded.
>>
> 
> I don't like to rely on these assumptions/knowledge.> It's much cleaner to retrieve the format, and avoiding
> hardcoding things as much as you can.
> 
> Hope I'm making sense :-)
hmm, not yet, If the input is a constant why not hardcode it?
Not sure what kind of code would you expect to replace it?
You mean a function that get the sink mbus as an input and returns "v4l2_format_info(V4L2_PIX_FMT_YUV422P)" if
the mbus is MEDIA_BUS_FMT_YUYV8_2X8?


Thanks,
Dafna

> 
> Ezequiel
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, back to index

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-15 14:29 [PATCH v2 0/4] media: staging: rkisp1: various fixes to capture formats Dafna Hirschfeld
2020-05-15 14:29 ` [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32 Dafna Hirschfeld
2020-05-20 21:50   ` Helen Koike
2020-05-15 14:29 ` [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros Dafna Hirschfeld
2020-05-20 21:54   ` Helen Koike
2020-05-20 22:08     ` Helen Koike
2020-05-22 12:11       ` Dafna Hirschfeld
2020-05-22 13:31         ` Ezequiel Garcia
2020-05-22 14:15           ` Dafna Hirschfeld
2020-05-22 15:04             ` Ezequiel Garcia
2020-05-25  9:51               ` Dafna Hirschfeld
2020-05-22 13:57         ` Laurent Pinchart
2020-05-22 14:54           ` Dafna Hirschfeld
2020-05-22 14:59             ` Laurent Pinchart
2020-05-15 14:29 ` [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444 Dafna Hirschfeld
2020-05-20 22:23   ` Helen Koike
2020-05-15 14:29 ` [PATCH v2 4/4] media: staging: rkisp1: cap: remove support of BGR666 format Dafna Hirschfeld
2020-05-20 22:34   ` Helen Koike

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git