[PATCH v2 0/4] media: staging: rkisp1: various fixes to capture formats

[PATCH v2 0/4] media: staging: rkisp1: various fixes to capture formats

[Dafna Hirschfeld]

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

[PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32

[Dafna Hirschfeld]

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

Re: [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32

[Helen Koike]

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,
>  	}, {
> 

Re: [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32

[Tomasz Figa]

Hi Dafna,

On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> 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.

I don't see this mentioned in the datasheets I have. On the other
hand, XBGR32 indeed makes much more sense, as the 3-byte RGB isn't a
very popular format. Have you validated that the hardware behavior
indeed matches that?

Best regards,
Tomasz

> 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
>

Re: [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32

[Dafna Hirschfeld]

On 27.05.20 01:04, Tomasz Figa wrote:
> Hi Dafna,
> 
> On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> 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.
> 
> I don't see this mentioned in the datasheets I have. On the other
> hand, XBGR32 indeed makes much more sense, as the 3-byte RGB isn't a
> very popular format. Have you validated that the hardware behavior
> indeed matches that?

Hi, yes I validated it, I also found it mentioned here:

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

under section "31.3.9 YCbCr to RGB Conversion"

Thanks,
Dafna
> 
> Best regards,
> Tomasz
> 
>> 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
>>

Re: [PATCH v2 1/4] media: staging: rkisp1: cap: change RGB24 format to XBGR32

[Tomasz Figa]

On Wed, Jun 10, 2020 at 6:11 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
>
>
> On 27.05.20 01:04, Tomasz Figa wrote:
> > Hi Dafna,
> >
> > On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
> > <dafna.hirschfeld@collabora.com> 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.
> >
> > I don't see this mentioned in the datasheets I have. On the other
> > hand, XBGR32 indeed makes much more sense, as the 3-byte RGB isn't a
> > very popular format. Have you validated that the hardware behavior
> > indeed matches that?
>
> Hi, yes I validated it, I also found it mentioned here:
>
> http://rockchip.fr/RK3288%20TRM/rk3288-chapter-31-image-signal-processing-(isp).pdf
>
> under section "31.3.9 YCbCr to RGB Conversion"

Okay, great. Thanks!

Feel free to add my Reviewed-by after addressing Helen's comments.

Best regards,
Tomasz

[PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Dafna Hirschfeld]

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

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Helen Koike]

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;
> 

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Helen Koike]

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;
>>

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Dafna Hirschfeld]

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;
>>>

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Ezequiel Garcia]

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

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Dafna Hirschfeld]

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
> 

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Ezequiel Garcia]

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

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Dafna Hirschfeld]

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
> 

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Tomasz Figa]

On Fri, May 22, 2020 at 5:05 PM Ezequiel Garcia <ezequiel@collabora.com> 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.

It would indeed be cleaner if we could retrieve this format from
somewhere, but where would that be? In theory we could assign a
YUV4:2:2 mbus format to the resizer input pad, but I think there is no
similar data available for mbs formats, is there?

Actually, if we look at this a bit more strictly, V4L2_PIX_FMT_YUV422P
is not exactly what the resizer gets at its input.
V4L2_PIX_FMT_YUV422P is a specific memory representation and the
corresponding v4l2_format_info struct contains data about the memory
layout. The resizer gets an unspecified YUV 4:2:2 pixel stream. Making
the code suggest that it's V4L2_PIX_FMT_YUV422P might make it more
confusing in another way.

Perhaps the way forward would be to simply add a comment explaining
where the 2 and 1 dividers come from?

Best regards,
Tomasz

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Laurent Pinchart]

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

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Dafna Hirschfeld]

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;
>>>>>
> 

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Laurent Pinchart]

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

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Dafna Hirschfeld]

On 22.05.20 16:59, Laurent Pinchart wrote:
> 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.

Thanks for the suggestion, I wrote this simple script for now that just
iterates all video formats supported by selfpath and mainpath and streams
them and then test that the resulted file matches the expected size.
I will extend the tests in the future.

https://gitlab.collabora.com/dafna/v4l-utils/-/blob/rkisp1-unit-tests/contrib/test/rkisp1-unit-tests.sh

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;
>>>>>>>
> 

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Tomasz Figa]

Hi Dafna,

On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> 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>
> ---
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
>  1 file changed, 12 insertions(+), 13 deletions(-)
>

Thank you for the effort on improving this driver! Please see my comments below.

> 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);

As I mentioned in another reply, this is a memory format, but we're
using it to configure a local (i.e. non-DMA) input.

>
>         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;

I'd honestly go the opposite way and just make it:

/* The resizer input is always YUV 4:2:2 and so horizontally subsampled by 2. */
sink_c.width = sink_y.width / 2;
sink_c.height = sink_y.height;

>
>         /*
>          * 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

It could be just me, but "memory input" sounds like there was an input
DMA involved, which is not the case. How about "Memory Interface
input"?

Also, if already amending this, I would also fix the overuse of
"should". How about the following?

"According to the hardware pipeline, the resizer input is always YUV
4:2:2. For YUV formats, the Memory Interface requires its input to be
already properly subsampled. We can achieve subsampling factors other
than YUV 4:2:2 by scaling the planes appropriately. For RGB formats,
the Memory Interface requires YUV 4:2:2 input, so no additional
scaling is needed."

> +        * 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 {

How about making it an explicit else if (v4l2_is_format_rgb(...))?

> +               src_c.width = src_y.width / yuv422_info->hdiv;
> +               src_c.height = src_y.height / yuv422_info->vdiv;

And then:

/* YUV 4:2:2 output to Memory Interface */
src_c.width = src_y.width / 2;
src_c.height = src_y.height;

>         }
>
> -       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
>

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Dafna Hirschfeld]

On 27.05.20 00:44, Tomasz Figa wrote:
> Hi Dafna,
> 
> On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> 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>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
>>   1 file changed, 12 insertions(+), 13 deletions(-)
>>
> 
> Thank you for the effort on improving this driver! Please see my comments below.
> 
>> 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);
> 
> As I mentioned in another reply, this is a memory format, but we're
> using it to configure a local (i.e. non-DMA) input.
> 
>>
>>          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;
> 
> I'd honestly go the opposite way and just make it:
> 
> /* The resizer input is always YUV 4:2:2 and so horizontally subsampled by 2. */
> sink_c.width = sink_y.width / 2;
> sink_c.height = sink_y.height;
> 
>>
>>          /*
>>           * 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
> 
> It could be just me, but "memory input" sounds like there was an input
> DMA involved, which is not the case. How about "Memory Interface
> input"?
> 
> Also, if already amending this, I would also fix the overuse of
> "should". How about the following?
> 
> "According to the hardware pipeline, the resizer input is always YUV
> 4:2:2. For YUV formats, the Memory Interface requires its input to be
> already properly subsampled. We can achieve subsampling factors other
> than YUV 4:2:2 by scaling the planes appropriately. For RGB formats,
> the Memory Interface requires YUV 4:2:2 input, so no additional
> scaling is needed."
> 
>> +        * 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 {
> 
> How about making it an explicit else if (v4l2_is_format_rgb(...))?
Actually the right way to deal with YUV downscaling is to support
MEDIA_BUS_FMT_YUYV8_1_5X8 (which is YUV420) on the src format
and not to look on what is configured on the capture.

Thanks,
Dafna
> 
>> +               src_c.width = src_y.width / yuv422_info->hdiv;
>> +               src_c.height = src_y.height / yuv422_info->vdiv;
> 
> And then:
> 
> /* YUV 4:2:2 output to Memory Interface */
> src_c.width = src_y.width / 2;
> src_c.height = src_y.height;
> 
>>          }
>>
>> -       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
>>

Re: [PATCH v2 2/4] media: staging: rkisp1: rsz: use hdiv, vdiv of yuv422 instead of macros

[Tomasz Figa]

On Wed, Jun 10, 2020 at 7:01 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> wrote:
>
>
>
> On 27.05.20 00:44, Tomasz Figa wrote:
> > Hi Dafna,
> >
> > On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
> > <dafna.hirschfeld@collabora.com> 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>
> >> ---
> >>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 25 +++++++++----------
> >>   1 file changed, 12 insertions(+), 13 deletions(-)
> >>
> >
> > Thank you for the effort on improving this driver! Please see my comments below.
> >
> >> 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);
> >
> > As I mentioned in another reply, this is a memory format, but we're
> > using it to configure a local (i.e. non-DMA) input.
> >
> >>
> >>          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;
> >
> > I'd honestly go the opposite way and just make it:
> >
> > /* The resizer input is always YUV 4:2:2 and so horizontally subsampled by 2. */
> > sink_c.width = sink_y.width / 2;
> > sink_c.height = sink_y.height;
> >
> >>
> >>          /*
> >>           * 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
> >
> > It could be just me, but "memory input" sounds like there was an input
> > DMA involved, which is not the case. How about "Memory Interface
> > input"?
> >
> > Also, if already amending this, I would also fix the overuse of
> > "should". How about the following?
> >
> > "According to the hardware pipeline, the resizer input is always YUV
> > 4:2:2. For YUV formats, the Memory Interface requires its input to be
> > already properly subsampled. We can achieve subsampling factors other
> > than YUV 4:2:2 by scaling the planes appropriately. For RGB formats,
> > the Memory Interface requires YUV 4:2:2 input, so no additional
> > scaling is needed."
> >
> >> +        * 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 {
> >
> > How about making it an explicit else if (v4l2_is_format_rgb(...))?
> Actually the right way to deal with YUV downscaling is to support
> MEDIA_BUS_FMT_YUYV8_1_5X8 (which is YUV420) on the src format
> and not to look on what is configured on the capture.

Good point. Since we're dealing with the resizer configuration, it
should be contained within the resizer subdev. My mind was still stuck
with our downstream topology.

Best regards,
Tomasz

[PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Dafna Hirschfeld]

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

Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Helen Koike]

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 {
> 

Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Tomasz Figa]

On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
<dafna.hirschfeld@collabora.com> 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.
>
> 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;

As pointed out in another thread, this should have been the original
size divided by the divisor and not just the latter alone.

It seems a bit suspicious to me that we don't need to upscale the
chroma planes here, because it would mean that the MI itself would be
doing some horizontal pixel doubling. The hardware documentation
doesn't really explain this, though.

Have you been able to validate that the setting without upscaling
indeed produces correct output?

Best regards,
Tomasz

Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Dafna Hirschfeld]

On 27.05.20 01:09, Tomasz Figa wrote:
> On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
> <dafna.hirschfeld@collabora.com> 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.
>>
>> 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;
> 
> As pointed out in another thread, this should have been the original
> size divided by the divisor and not just the latter alone.
> 
> It seems a bit suspicious to me that we don't need to upscale the
> chroma planes here, because it would mean that the MI itself would be
> doing some horizontal pixel doubling. The hardware documentation
> doesn't really explain this, though.
> 
> Have you been able to validate that the setting without upscaling
> indeed produces correct output?

Hi,
I investigated it again, without this patch, the frames on mainpath for YUV444 look good: https://imgur.com/a/NtazWhY
but there is a problem on selfpath: https://imgur.com/a/vQJPqAn

This patch somehow solves the problem on selfpath but ruins the frames on mainpath.

I think the bug is actually in another place in the code. The function 'rkisp1_sp_config'
sets a constant value RKISP1_MI_CTRL_SP_INPUT_YUV422 as the input format on the RKISP1_CIF_MI_CTRL register.
But the value should be set according to the configuration. For some reason the problem arises only
when trying to capture yuv444. When I capture yuv420 on the selfpath the frame looks good although the
value RKISP1_MI_CTRL_SP_INPUT_YUV422 is set.
Setting the the SP_INPUT_* according to the configuration seems to solve the problem.

Thanks,
Dafna


> 
> Best regards,
> Tomasz
> 

Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Tomasz Figa]

On Fri, Jun 12, 2020 at 02:45:00PM +0200, Dafna Hirschfeld wrote:
> 
> 
> On 27.05.20 01:09, Tomasz Figa wrote:
> > On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
> > <dafna.hirschfeld@collabora.com> 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.
> > > 
> > > 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;
> > 
> > As pointed out in another thread, this should have been the original
> > size divided by the divisor and not just the latter alone.
> > 
> > It seems a bit suspicious to me that we don't need to upscale the
> > chroma planes here, because it would mean that the MI itself would be
> > doing some horizontal pixel doubling. The hardware documentation
> > doesn't really explain this, though.
> > 
> > Have you been able to validate that the setting without upscaling
> > indeed produces correct output?
> 
> Hi,
> I investigated it again, without this patch, the frames on mainpath for YUV444 look good: https://imgur.com/a/NtazWhY
> but there is a problem on selfpath: https://imgur.com/a/vQJPqAn
> 
> This patch somehow solves the problem on selfpath but ruins the frames on mainpath.
> 
> I think the bug is actually in another place in the code. The function 'rkisp1_sp_config'
> sets a constant value RKISP1_MI_CTRL_SP_INPUT_YUV422 as the input format on the RKISP1_CIF_MI_CTRL register.
> But the value should be set according to the configuration. For some reason the problem arises only
> when trying to capture yuv444. When I capture yuv420 on the selfpath the frame looks good although the
> value RKISP1_MI_CTRL_SP_INPUT_YUV422 is set.
> Setting the the SP_INPUT_* according to the configuration seems to solve the problem.

Looking at the datasheet, SP seems to have some internal format
conversion capability - one can set SP_OUTPUT_FORMAT and SP_INPUT_FORMAT
to different YUV subsampling modes or even some RGB formats. MP doesn't
have this capability - it can only write whatever it receives.

I think this needs to be reflected in the driver, if it isn't yet.
Depending on the resizer source format, the MP video node must allow
only formats with matching subsampling mode.

Best regards,
Tomasz

Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Dafna Hirschfeld]

On 18.06.20 19:47, Tomasz Figa wrote:
> On Fri, Jun 12, 2020 at 02:45:00PM +0200, Dafna Hirschfeld wrote:
>>
>>
>> On 27.05.20 01:09, Tomasz Figa wrote:
>>> On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
>>> <dafna.hirschfeld@collabora.com> 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.
>>>>
>>>> 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;
>>>
>>> As pointed out in another thread, this should have been the original
>>> size divided by the divisor and not just the latter alone.
>>>
>>> It seems a bit suspicious to me that we don't need to upscale the
>>> chroma planes here, because it would mean that the MI itself would be
>>> doing some horizontal pixel doubling. The hardware documentation
>>> doesn't really explain this, though.
>>>
>>> Have you been able to validate that the setting without upscaling
>>> indeed produces correct output?
>>
>> Hi,
>> I investigated it again, without this patch, the frames on mainpath for YUV444 look good: https://imgur.com/a/NtazWhY
>> but there is a problem on selfpath: https://imgur.com/a/vQJPqAn
>>
>> This patch somehow solves the problem on selfpath but ruins the frames on mainpath.
>>
>> I think the bug is actually in another place in the code. The function 'rkisp1_sp_config'
>> sets a constant value RKISP1_MI_CTRL_SP_INPUT_YUV422 as the input format on the RKISP1_CIF_MI_CTRL register.
>> But the value should be set according to the configuration. For some reason the problem arises only
>> when trying to capture yuv444. When I capture yuv420 on the selfpath the frame looks good although the
>> value RKISP1_MI_CTRL_SP_INPUT_YUV422 is set.
>> Setting the the SP_INPUT_* according to the configuration seems to solve the problem.
> 
> Looking at the datasheet, SP seems to have some internal format
> conversion capability - one can set SP_OUTPUT_FORMAT and SP_INPUT_FORMAT
> to different YUV subsampling modes or even some RGB formats. MP doesn't
> have this capability - it can only write whatever it receives.
> 
> I think this needs to be reflected in the driver, if it isn't yet.
> Depending on the resizer source format, the MP video node must allow
> only formats with matching subsampling mode.

Hi,
I work on v3 now that does that, it supports several yuv mbus codes on the source pads of the resizers
and then in the link_validation callback of the capture it checks that the subsampling matches.

It is not clear to me what is the meaning of the input/output capapility of the selfpath except of RGB convertion.
Since yuv subsampling is what the resizer does.

Thanks,
Dafna


> 
> Best regards,
> Tomasz
> 

Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Tomasz Figa]

On Thu, Jun 18, 2020 at 08:00:37PM +0200, Dafna Hirschfeld wrote:
> 
> 
> On 18.06.20 19:47, Tomasz Figa wrote:
> > On Fri, Jun 12, 2020 at 02:45:00PM +0200, Dafna Hirschfeld wrote:
> > > 
> > > 
> > > On 27.05.20 01:09, Tomasz Figa wrote:
> > > > On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
> > > > <dafna.hirschfeld@collabora.com> 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.
> > > > > 
> > > > > 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;
> > > > 
> > > > As pointed out in another thread, this should have been the original
> > > > size divided by the divisor and not just the latter alone.
> > > > 
> > > > It seems a bit suspicious to me that we don't need to upscale the
> > > > chroma planes here, because it would mean that the MI itself would be
> > > > doing some horizontal pixel doubling. The hardware documentation
> > > > doesn't really explain this, though.
> > > > 
> > > > Have you been able to validate that the setting without upscaling
> > > > indeed produces correct output?
> > > 
> > > Hi,
> > > I investigated it again, without this patch, the frames on mainpath for YUV444 look good: https://imgur.com/a/NtazWhY
> > > but there is a problem on selfpath: https://imgur.com/a/vQJPqAn
> > > 
> > > This patch somehow solves the problem on selfpath but ruins the frames on mainpath.
> > > 
> > > I think the bug is actually in another place in the code. The function 'rkisp1_sp_config'
> > > sets a constant value RKISP1_MI_CTRL_SP_INPUT_YUV422 as the input format on the RKISP1_CIF_MI_CTRL register.
> > > But the value should be set according to the configuration. For some reason the problem arises only
> > > when trying to capture yuv444. When I capture yuv420 on the selfpath the frame looks good although the
> > > value RKISP1_MI_CTRL_SP_INPUT_YUV422 is set.
> > > Setting the the SP_INPUT_* according to the configuration seems to solve the problem.
> > 
> > Looking at the datasheet, SP seems to have some internal format
> > conversion capability - one can set SP_OUTPUT_FORMAT and SP_INPUT_FORMAT
> > to different YUV subsampling modes or even some RGB formats. MP doesn't
> > have this capability - it can only write whatever it receives.
> > 
> > I think this needs to be reflected in the driver, if it isn't yet.
> > Depending on the resizer source format, the MP video node must allow
> > only formats with matching subsampling mode.
> 
> Hi,
> I work on v3 now that does that, it supports several yuv mbus codes on the source pads of the resizers
> and then in the link_validation callback of the capture it checks that the subsampling matches.

Is it enough? I believe the principle is that within the same entity the
state needs to stay consistent, i.e. if the sink pad changes, the source
pad or video node must be updated to expose only correct formats,
including resetting the current format.

> 
> It is not clear to me what is the meaning of the input/output capapility of the selfpath except of RGB convertion.
> Since yuv subsampling is what the resizer does.

Could be some historical legacy, a byproduct of some feature (e.g.
RGB could be bundled with support for other formats) or could be there
on purpose.

It would be worth checking whether there are any quality or
performance/power differences depending on whether the resizer or the SP
is used for resampling. For example, one could be using some
interpolation algorithm, while the other just a simple pixel doubling or
decimation.

Best regards,
Tomasz

Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Dafna Hirschfeld]

On 18.06.20 20:12, Tomasz Figa wrote:
> On Thu, Jun 18, 2020 at 08:00:37PM +0200, Dafna Hirschfeld wrote:
>>
>>
>> On 18.06.20 19:47, Tomasz Figa wrote:
>>> On Fri, Jun 12, 2020 at 02:45:00PM +0200, Dafna Hirschfeld wrote:
>>>>
>>>>
>>>> On 27.05.20 01:09, Tomasz Figa wrote:
>>>>> On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
>>>>> <dafna.hirschfeld@collabora.com> 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.
>>>>>>
>>>>>> 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;
>>>>>
>>>>> As pointed out in another thread, this should have been the original
>>>>> size divided by the divisor and not just the latter alone.
>>>>>
>>>>> It seems a bit suspicious to me that we don't need to upscale the
>>>>> chroma planes here, because it would mean that the MI itself would be
>>>>> doing some horizontal pixel doubling. The hardware documentation
>>>>> doesn't really explain this, though.
>>>>>
>>>>> Have you been able to validate that the setting without upscaling
>>>>> indeed produces correct output?
>>>>
>>>> Hi,
>>>> I investigated it again, without this patch, the frames on mainpath for YUV444 look good: https://imgur.com/a/NtazWhY
>>>> but there is a problem on selfpath: https://imgur.com/a/vQJPqAn
>>>>
>>>> This patch somehow solves the problem on selfpath but ruins the frames on mainpath.
>>>>
>>>> I think the bug is actually in another place in the code. The function 'rkisp1_sp_config'
>>>> sets a constant value RKISP1_MI_CTRL_SP_INPUT_YUV422 as the input format on the RKISP1_CIF_MI_CTRL register.
>>>> But the value should be set according to the configuration. For some reason the problem arises only
>>>> when trying to capture yuv444. When I capture yuv420 on the selfpath the frame looks good although the
>>>> value RKISP1_MI_CTRL_SP_INPUT_YUV422 is set.
>>>> Setting the the SP_INPUT_* according to the configuration seems to solve the problem.
>>>
>>> Looking at the datasheet, SP seems to have some internal format
>>> conversion capability - one can set SP_OUTPUT_FORMAT and SP_INPUT_FORMAT
>>> to different YUV subsampling modes or even some RGB formats. MP doesn't
>>> have this capability - it can only write whatever it receives.
>>>
>>> I think this needs to be reflected in the driver, if it isn't yet.
>>> Depending on the resizer source format, the MP video node must allow
>>> only formats with matching subsampling mode.
>>
>> Hi,
>> I work on v3 now that does that, it supports several yuv mbus codes on the source pads of the resizers
>> and then in the link_validation callback of the capture it checks that the subsampling matches.
> 
> Is it enough? I believe the principle is that within the same entity the
> state needs to stay consistent, i.e. if the sink pad changes, the source
> pad or video node must be updated to expose only correct formats,
> including resetting the current format.
Yes, but video devices have only a sink pad. Also, the sink pad of
a video device is not associated with an mbus code. The video format configuration
is not related to the media controller API.

> 
>>
>> It is not clear to me what is the meaning of the input/output capapility of the selfpath except of RGB convertion.
>> Since yuv subsampling is what the resizer does.
> 
> Could be some historical legacy, a byproduct of some feature (e.g.
> RGB could be bundled with support for other formats) or could be there
> on purpose.
> 
> It would be worth checking whether there are any quality or
> performance/power differences depending on whether the resizer or the SP
> is used for resampling. For example, one could be using some
> interpolation algorithm, while the other just a simple pixel doubling or
> decimation.

I can experiment with that.

Thanks,
Dafna

> 
> Best regards,
> Tomasz
> 

Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Tomasz Figa]

On Thu, Jun 18, 2020 at 08:50:23PM +0200, Dafna Hirschfeld wrote:
> 
> 
> On 18.06.20 20:12, Tomasz Figa wrote:
> > On Thu, Jun 18, 2020 at 08:00:37PM +0200, Dafna Hirschfeld wrote:
> > > 
> > > 
> > > On 18.06.20 19:47, Tomasz Figa wrote:
> > > > On Fri, Jun 12, 2020 at 02:45:00PM +0200, Dafna Hirschfeld wrote:
> > > > > 
> > > > > 
> > > > > On 27.05.20 01:09, Tomasz Figa wrote:
> > > > > > On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
> > > > > > <dafna.hirschfeld@collabora.com> 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.
> > > > > > > 
> > > > > > > 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;
> > > > > > 
> > > > > > As pointed out in another thread, this should have been the original
> > > > > > size divided by the divisor and not just the latter alone.
> > > > > > 
> > > > > > It seems a bit suspicious to me that we don't need to upscale the
> > > > > > chroma planes here, because it would mean that the MI itself would be
> > > > > > doing some horizontal pixel doubling. The hardware documentation
> > > > > > doesn't really explain this, though.
> > > > > > 
> > > > > > Have you been able to validate that the setting without upscaling
> > > > > > indeed produces correct output?
> > > > > 
> > > > > Hi,
> > > > > I investigated it again, without this patch, the frames on mainpath for YUV444 look good: https://imgur.com/a/NtazWhY
> > > > > but there is a problem on selfpath: https://imgur.com/a/vQJPqAn
> > > > > 
> > > > > This patch somehow solves the problem on selfpath but ruins the frames on mainpath.
> > > > > 
> > > > > I think the bug is actually in another place in the code. The function 'rkisp1_sp_config'
> > > > > sets a constant value RKISP1_MI_CTRL_SP_INPUT_YUV422 as the input format on the RKISP1_CIF_MI_CTRL register.
> > > > > But the value should be set according to the configuration. For some reason the problem arises only
> > > > > when trying to capture yuv444. When I capture yuv420 on the selfpath the frame looks good although the
> > > > > value RKISP1_MI_CTRL_SP_INPUT_YUV422 is set.
> > > > > Setting the the SP_INPUT_* according to the configuration seems to solve the problem.
> > > > 
> > > > Looking at the datasheet, SP seems to have some internal format
> > > > conversion capability - one can set SP_OUTPUT_FORMAT and SP_INPUT_FORMAT
> > > > to different YUV subsampling modes or even some RGB formats. MP doesn't
> > > > have this capability - it can only write whatever it receives.
> > > > 
> > > > I think this needs to be reflected in the driver, if it isn't yet.
> > > > Depending on the resizer source format, the MP video node must allow
> > > > only formats with matching subsampling mode.
> > > 
> > > Hi,
> > > I work on v3 now that does that, it supports several yuv mbus codes on the source pads of the resizers
> > > and then in the link_validation callback of the capture it checks that the subsampling matches.
> > 
> > Is it enough? I believe the principle is that within the same entity the
> > state needs to stay consistent, i.e. if the sink pad changes, the source
> > pad or video node must be updated to expose only correct formats,
> > including resetting the current format.
> Yes, but video devices have only a sink pad. Also, the sink pad of
> a video device is not associated with an mbus code. The video format configuration
> is not related to the media controller API.

The video device interface and the entity it is linked to belong to the
same driver. The V4L2 video device interface requires that the state is
always valid and the implementation of the video device needs to take
into account its links to external entities.

Best regards,
Tomasz

Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Dafna Hirschfeld]

On 18.06.20 21:18, Tomasz Figa wrote:
> On Thu, Jun 18, 2020 at 08:50:23PM +0200, Dafna Hirschfeld wrote:
>>
>>
>> On 18.06.20 20:12, Tomasz Figa wrote:
>>> On Thu, Jun 18, 2020 at 08:00:37PM +0200, Dafna Hirschfeld wrote:
>>>>
>>>>
>>>> On 18.06.20 19:47, Tomasz Figa wrote:
>>>>> On Fri, Jun 12, 2020 at 02:45:00PM +0200, Dafna Hirschfeld wrote:
>>>>>>
>>>>>>
>>>>>> On 27.05.20 01:09, Tomasz Figa wrote:
>>>>>>> On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
>>>>>>> <dafna.hirschfeld@collabora.com> 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.
>>>>>>>>
>>>>>>>> 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;
>>>>>>>
>>>>>>> As pointed out in another thread, this should have been the original
>>>>>>> size divided by the divisor and not just the latter alone.
>>>>>>>
>>>>>>> It seems a bit suspicious to me that we don't need to upscale the
>>>>>>> chroma planes here, because it would mean that the MI itself would be
>>>>>>> doing some horizontal pixel doubling. The hardware documentation
>>>>>>> doesn't really explain this, though.
>>>>>>>
>>>>>>> Have you been able to validate that the setting without upscaling
>>>>>>> indeed produces correct output?
>>>>>>
>>>>>> Hi,
>>>>>> I investigated it again, without this patch, the frames on mainpath for YUV444 look good: https://imgur.com/a/NtazWhY
>>>>>> but there is a problem on selfpath: https://imgur.com/a/vQJPqAn
>>>>>>
>>>>>> This patch somehow solves the problem on selfpath but ruins the frames on mainpath.
>>>>>>
>>>>>> I think the bug is actually in another place in the code. The function 'rkisp1_sp_config'
>>>>>> sets a constant value RKISP1_MI_CTRL_SP_INPUT_YUV422 as the input format on the RKISP1_CIF_MI_CTRL register.
>>>>>> But the value should be set according to the configuration. For some reason the problem arises only
>>>>>> when trying to capture yuv444. When I capture yuv420 on the selfpath the frame looks good although the
>>>>>> value RKISP1_MI_CTRL_SP_INPUT_YUV422 is set.
>>>>>> Setting the the SP_INPUT_* according to the configuration seems to solve the problem.
>>>>>
>>>>> Looking at the datasheet, SP seems to have some internal format
>>>>> conversion capability - one can set SP_OUTPUT_FORMAT and SP_INPUT_FORMAT
>>>>> to different YUV subsampling modes or even some RGB formats. MP doesn't
>>>>> have this capability - it can only write whatever it receives.
>>>>>
>>>>> I think this needs to be reflected in the driver, if it isn't yet.
>>>>> Depending on the resizer source format, the MP video node must allow
>>>>> only formats with matching subsampling mode.
>>>>
>>>> Hi,
>>>> I work on v3 now that does that, it supports several yuv mbus codes on the source pads of the resizers
>>>> and then in the link_validation callback of the capture it checks that the subsampling matches.
>>>
>>> Is it enough? I believe the principle is that within the same entity the
>>> state needs to stay consistent, i.e. if the sink pad changes, the source
>>> pad or video node must be updated to expose only correct formats,
>>> including resetting the current format.
>> Yes, but video devices have only a sink pad. Also, the sink pad of
>> a video device is not associated with an mbus code. The video format configuration
>> is not related to the media controller API.
> 
> The video device interface and the entity it is linked to belong to the
> same driver. The V4L2 video device interface requires that the state is
> always valid and the implementation of the video device needs to take
> into account its links to external entities.

But if the pad on the other side of the link is configured after the video device
then the state might become invalid.

Thanks,
Dafna

> 
> Best regards,
> Tomasz
> 

Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Tomasz Figa]

On Fri, Jun 19, 2020 at 09:30:50AM +0200, Dafna Hirschfeld wrote:
> 
> 
> On 18.06.20 21:18, Tomasz Figa wrote:
> > On Thu, Jun 18, 2020 at 08:50:23PM +0200, Dafna Hirschfeld wrote:
> > > 
> > > 
> > > On 18.06.20 20:12, Tomasz Figa wrote:
> > > > On Thu, Jun 18, 2020 at 08:00:37PM +0200, Dafna Hirschfeld wrote:
> > > > > 
> > > > > 
> > > > > On 18.06.20 19:47, Tomasz Figa wrote:
> > > > > > On Fri, Jun 12, 2020 at 02:45:00PM +0200, Dafna Hirschfeld wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 27.05.20 01:09, Tomasz Figa wrote:
> > > > > > > > On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
> > > > > > > > <dafna.hirschfeld@collabora.com> 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.
> > > > > > > > > 
> > > > > > > > > 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;
> > > > > > > > 
> > > > > > > > As pointed out in another thread, this should have been the original
> > > > > > > > size divided by the divisor and not just the latter alone.
> > > > > > > > 
> > > > > > > > It seems a bit suspicious to me that we don't need to upscale the
> > > > > > > > chroma planes here, because it would mean that the MI itself would be
> > > > > > > > doing some horizontal pixel doubling. The hardware documentation
> > > > > > > > doesn't really explain this, though.
> > > > > > > > 
> > > > > > > > Have you been able to validate that the setting without upscaling
> > > > > > > > indeed produces correct output?
> > > > > > > 
> > > > > > > Hi,
> > > > > > > I investigated it again, without this patch, the frames on mainpath for YUV444 look good: https://imgur.com/a/NtazWhY
> > > > > > > but there is a problem on selfpath: https://imgur.com/a/vQJPqAn
> > > > > > > 
> > > > > > > This patch somehow solves the problem on selfpath but ruins the frames on mainpath.
> > > > > > > 
> > > > > > > I think the bug is actually in another place in the code. The function 'rkisp1_sp_config'
> > > > > > > sets a constant value RKISP1_MI_CTRL_SP_INPUT_YUV422 as the input format on the RKISP1_CIF_MI_CTRL register.
> > > > > > > But the value should be set according to the configuration. For some reason the problem arises only
> > > > > > > when trying to capture yuv444. When I capture yuv420 on the selfpath the frame looks good although the
> > > > > > > value RKISP1_MI_CTRL_SP_INPUT_YUV422 is set.
> > > > > > > Setting the the SP_INPUT_* according to the configuration seems to solve the problem.
> > > > > > 
> > > > > > Looking at the datasheet, SP seems to have some internal format
> > > > > > conversion capability - one can set SP_OUTPUT_FORMAT and SP_INPUT_FORMAT
> > > > > > to different YUV subsampling modes or even some RGB formats. MP doesn't
> > > > > > have this capability - it can only write whatever it receives.
> > > > > > 
> > > > > > I think this needs to be reflected in the driver, if it isn't yet.
> > > > > > Depending on the resizer source format, the MP video node must allow
> > > > > > only formats with matching subsampling mode.
> > > > > 
> > > > > Hi,
> > > > > I work on v3 now that does that, it supports several yuv mbus codes on the source pads of the resizers
> > > > > and then in the link_validation callback of the capture it checks that the subsampling matches.
> > > > 
> > > > Is it enough? I believe the principle is that within the same entity the
> > > > state needs to stay consistent, i.e. if the sink pad changes, the source
> > > > pad or video node must be updated to expose only correct formats,
> > > > including resetting the current format.
> > > Yes, but video devices have only a sink pad. Also, the sink pad of
> > > a video device is not associated with an mbus code. The video format configuration
> > > is not related to the media controller API.
> > 
> > The video device interface and the entity it is linked to belong to the
> > same driver. The V4L2 video device interface requires that the state is
> > always valid and the implementation of the video device needs to take
> > into account its links to external entities.
> 
> But if the pad on the other side of the link is configured after the video device
> then the state might become invalid.

The pad on the other side of the link is already outside of the scope.
Basically, the entity that interfaces with the video node must have
state consistent with the video node.

To make sure we're on the same page, let me give some examples below:

Initial state:
ISP source pad: YUV 4:2:2
Resizer sink pad: YUV 4:2:2
Resizer source pad: YUV 4:2:0
Video CAPTURE format: NV12

Scenario 1:
- Userspace changes ISP source pad to RAW.
- Userspace starts streaming and link validation fails.
- Still, the state is consistent between the resizer and the video node.

Scenario 2:
- Userspace changes ISP source pad to RAW.
- Userspace changes Resizer sink pad to RAW.
- Kernel updates Resizer source pad to RAW.
- Kernel updates video CAPTURE format to a supported RAW pixelformat.
- All the state is consistent and streaming can start.
- If the userspace calls ENUM_FMT on video CAPTURE, it only gets RAW
  pixelformats.

Scenario 3:
- Userspace changes Resizer source pad to YUV 4:2:2.
- Kernel updates video CAPTURE to a supported YUV 4:2:2 pixelformat
  (e.g. YUYV).
- All the state is consistent and streaming can start.
- If the userspace calls ENUM_FMT on video CAPTURE, it only gets YUV
  4:2:2 pixelformats.

Scenario 4:
- Userspace calls ENUM_FMT on video CAPTURE and only gets YUV 4:2:0
  pixelformats.
- It's not possible to make the state inconsistent between the resizer
  and the video node by using V4L2 video ioctls.

Am I missing something?

Best regards,
Tomasz

Re: [PATCH v2 3/4] media: staging: rkisp1: rsz: set output format to YUV422 if cap format is YUV444

[Dafna Hirschfeld]

On 19.06.20 14:04, Tomasz Figa wrote:
> On Fri, Jun 19, 2020 at 09:30:50AM +0200, Dafna Hirschfeld wrote:
>>
>>
>> On 18.06.20 21:18, Tomasz Figa wrote:
>>> On Thu, Jun 18, 2020 at 08:50:23PM +0200, Dafna Hirschfeld wrote:
>>>>
>>>>
>>>> On 18.06.20 20:12, Tomasz Figa wrote:
>>>>> On Thu, Jun 18, 2020 at 08:00:37PM +0200, Dafna Hirschfeld wrote:
>>>>>>
>>>>>>
>>>>>> On 18.06.20 19:47, Tomasz Figa wrote:
>>>>>>> On Fri, Jun 12, 2020 at 02:45:00PM +0200, Dafna Hirschfeld wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 27.05.20 01:09, Tomasz Figa wrote:
>>>>>>>>> On Fri, May 15, 2020 at 4:30 PM Dafna Hirschfeld
>>>>>>>>> <dafna.hirschfeld@collabora.com> 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.
>>>>>>>>>>
>>>>>>>>>> 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;
>>>>>>>>>
>>>>>>>>> As pointed out in another thread, this should have been the original
>>>>>>>>> size divided by the divisor and not just the latter alone.
>>>>>>>>>
>>>>>>>>> It seems a bit suspicious to me that we don't need to upscale the
>>>>>>>>> chroma planes here, because it would mean that the MI itself would be
>>>>>>>>> doing some horizontal pixel doubling. The hardware documentation
>>>>>>>>> doesn't really explain this, though.
>>>>>>>>>
>>>>>>>>> Have you been able to validate that the setting without upscaling
>>>>>>>>> indeed produces correct output?
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>> I investigated it again, without this patch, the frames on mainpath for YUV444 look good: https://imgur.com/a/NtazWhY
>>>>>>>> but there is a problem on selfpath: https://imgur.com/a/vQJPqAn
>>>>>>>>
>>>>>>>> This patch somehow solves the problem on selfpath but ruins the frames on mainpath.
>>>>>>>>
>>>>>>>> I think the bug is actually in another place in the code. The function 'rkisp1_sp_config'
>>>>>>>> sets a constant value RKISP1_MI_CTRL_SP_INPUT_YUV422 as the input format on the RKISP1_CIF_MI_CTRL register.
>>>>>>>> But the value should be set according to the configuration. For some reason the problem arises only
>>>>>>>> when trying to capture yuv444. When I capture yuv420 on the selfpath the frame looks good although the
>>>>>>>> value RKISP1_MI_CTRL_SP_INPUT_YUV422 is set.
>>>>>>>> Setting the the SP_INPUT_* according to the configuration seems to solve the problem.
>>>>>>>
>>>>>>> Looking at the datasheet, SP seems to have some internal format
>>>>>>> conversion capability - one can set SP_OUTPUT_FORMAT and SP_INPUT_FORMAT
>>>>>>> to different YUV subsampling modes or even some RGB formats. MP doesn't
>>>>>>> have this capability - it can only write whatever it receives.
>>>>>>>
>>>>>>> I think this needs to be reflected in the driver, if it isn't yet.
>>>>>>> Depending on the resizer source format, the MP video node must allow
>>>>>>> only formats with matching subsampling mode.
>>>>>>
>>>>>> Hi,
>>>>>> I work on v3 now that does that, it supports several yuv mbus codes on the source pads of the resizers
>>>>>> and then in the link_validation callback of the capture it checks that the subsampling matches.
>>>>>
>>>>> Is it enough? I believe the principle is that within the same entity the
>>>>> state needs to stay consistent, i.e. if the sink pad changes, the source
>>>>> pad or video node must be updated to expose only correct formats,
>>>>> including resetting the current format.
>>>> Yes, but video devices have only a sink pad. Also, the sink pad of
>>>> a video device is not associated with an mbus code. The video format configuration
>>>> is not related to the media controller API.
>>>
>>> The video device interface and the entity it is linked to belong to the
>>> same driver. The V4L2 video device interface requires that the state is
>>> always valid and the implementation of the video device needs to take
>>> into account its links to external entities.
>>
>> But if the pad on the other side of the link is configured after the video device
>> then the state might become invalid.
> 
> The pad on the other side of the link is already outside of the scope.
> Basically, the entity that interfaces with the video node must have
> state consistent with the video node.
> 
> To make sure we're on the same page, let me give some examples below:
> 
> Initial state:
> ISP source pad: YUV 4:2:2
> Resizer sink pad: YUV 4:2:2
> Resizer source pad: YUV 4:2:0
> Video CAPTURE format: NV12
> 
> Scenario 1:
> - Userspace changes ISP source pad to RAW.
> - Userspace starts streaming and link validation fails.
> - Still, the state is consistent between the resizer and the video node.
> 
> Scenario 2:
> - Userspace changes ISP source pad to RAW.
> - Userspace changes Resizer sink pad to RAW.
> - Kernel updates Resizer source pad to RAW.
> - Kernel updates video CAPTURE format to a supported RAW pixelformat.

I think this kernel update should not run, quoting Laurant:
"
In MC-based drivers, kernel drivers should not propagate those values
between subdevs or from subdev to video node. Userspace handles the
propagation.
"


> - All the state is consistent and streaming can start.
> - If the userspace calls ENUM_FMT on video CAPTURE, it only gets RAW
>    pixelformats.

According to the spec: "Drivers shall enumerate all image formats."

I think adding support to V4L2_CAP_IO_MC will be helpful for this driver.
So userspace see which pixel format are supported for each media bus on the resizer.

Thanks,
Dafna

> 
> Scenario 3:
> - Userspace changes Resizer source pad to YUV 4:2:2.
> - Kernel updates video CAPTURE to a supported YUV 4:2:2 pixelformat
>    (e.g. YUYV).
> - All the state is consistent and streaming can start.
> - If the userspace calls ENUM_FMT on video CAPTURE, it only gets YUV
>    4:2:2 pixelformats.
> 
> Scenario 4:
> - Userspace calls ENUM_FMT on video CAPTURE and only gets YUV 4:2:0
>    pixelformats.
> - It's not possible to make the state inconsistent between the resizer
>    and the video node by using V4L2 video ioctls.
> 
> Am I missing something?
> 
> Best regards,
> Tomasz
> 

[PATCH v2 4/4] media: staging: rkisp1: cap: remove support of BGR666 format

[Dafna Hirschfeld]

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

Re: [PATCH v2 4/4] media: staging: rkisp1: cap: remove support of BGR666 format

[Helen Koike]

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,
>  	},
>  };
>  
> 

Re: [PATCH v2 4/4] media: staging: rkisp1: cap: remove support of BGR666 format

[Tomasz Figa]

On Thu, May 21, 2020 at 12:35 AM Helen Koike <helen.koike@collabora.com> wrote:
>
>
>
> 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.

Sounds like some relic of the past. ;)

In general, RGB formats are not very useful in this hardware. One
wastes the precious bandwidth, but doesn't get more color resolution,
since the pipeline uses YUV 4:2:2 internally anyway. The typical usage
scenarios with video or JPEG encoding and preview are much better off
with the available YUV formats.

>
> 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,
> >       },
> >  };
> >
> >

Re: [PATCH v2 4/4] media: staging: rkisp1: cap: remove support of BGR666 format

[Helen Koike]

On 5/26/20 7:57 PM, Tomasz Figa wrote:
> On Thu, May 21, 2020 at 12:35 AM Helen Koike <helen.koike@collabora.com> wrote:
>>
>>
>>
>> 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.
> 
> Sounds like some relic of the past. ;)
> 
> In general, RGB formats are not very useful in this hardware. One
> wastes the precious bandwidth, but doesn't get more color resolution,
> since the pipeline uses YUV 4:2:2 internally anyway. The typical usage
> scenarios with video or JPEG encoding and preview are much better off
> with the available YUV formats.

Thanks for your input.

Right, then in this case, I don't oppose removing it.

> 
>>
>> 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>

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

Thanks
Helen

>>> ---
>>>  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,
>>>       },
>>>  };
>>>
>>>