[PATCH 0/3] media: staging: rkisp1: add support for RGB formats

[PATCH 0/3] media: staging: rkisp1: add support for RGB formats

[Dafna Hirschfeld]

Add support for RGB formats in rkisp1, (currently only RGB565 is correctly captured)
The patchset is rebased on top of patch
"media: staging: rkisp1: cap: remove field fmt_type from struct rkisp1_capture_fmt_cfg"

patches summary:

patch 1: fix a redundant call to v4l2_format_info in the resizer code
patch 2: fix a bug that changes the default hdiv,vdiv in the resizer scale for RGB formats.
This bug changes the YUV ratios from 4:2:2 to 4:4:4 and the capture for RGB times out
patch 3: removes the restriction in the validation function of the capture that forces the
mbus encoding to be the same as the capture encoding. This is because for RGB formats
the mbus format should be MEDIA_BUS_FMT_YUYV8_2X8

Dafna Hirschfeld (3):
  media: staging: rkisp1: rsz: get the capture format info from the
    capture struct
  media: staging: rkisp1: rsz: change (hv)div only if capture format is
    YUV
  media: staging: rkisp1: cap: enable RGB capture format with YUV media
    bus

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

-- 
2.17.1

[PATCH 1/3] media: staging: rkisp1: rsz: get the capture format info from the capture struct

[Dafna Hirschfeld]

Currently the format info of the capture is retrieved by calling
the function  v4l2_format_info. This is not needed since it is
already save in the capture object.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index 87799fbf0363..8704267a066f 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -387,8 +387,7 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
 	if (rsz->fmt_type == RKISP1_FMT_YUV) {
 		struct rkisp1_capture *cap =
 			&rsz->rkisp1->capture_devs[rsz->id];
-		const struct v4l2_format_info *pixfmt_info =
-			v4l2_format_info(cap->pix.fmt.pixelformat);
+		const struct v4l2_format_info *pixfmt_info = cap->pix.info;
 
 		hdiv = pixfmt_info->hdiv;
 		vdiv = pixfmt_info->vdiv;
-- 
2.17.1

Re: [PATCH 1/3] media: staging: rkisp1: rsz: get the capture format info from the capture struct

[Helen Koike]

On 3/28/20 7:56 AM, Dafna Hirschfeld wrote:
> Currently the format info of the capture is retrieved by calling
> the function  v4l2_format_info. This is not needed since it is
> already save in the capture object.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

> ---
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> index 87799fbf0363..8704267a066f 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -387,8 +387,7 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>  	if (rsz->fmt_type == RKISP1_FMT_YUV) {
>  		struct rkisp1_capture *cap =
>  			&rsz->rkisp1->capture_devs[rsz->id];
> -		const struct v4l2_format_info *pixfmt_info =
> -			v4l2_format_info(cap->pix.fmt.pixelformat);
> +		const struct v4l2_format_info *pixfmt_info = cap->pix.info;
>  
>  		hdiv = pixfmt_info->hdiv;
>  		vdiv = pixfmt_info->vdiv;
> 

Re: [PATCH 1/3] media: staging: rkisp1: rsz: get the capture format info from the capture struct

[Laurent Pinchart]

Hi Dafna,

Thank you for the patch.

On Sat, Mar 28, 2020 at 11:56:04AM +0100, Dafna Hirschfeld wrote:
> Currently the format info of the capture is retrieved by calling
> the function  v4l2_format_info. This is not needed since it is
> already save in the capture object.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

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

> ---
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> index 87799fbf0363..8704267a066f 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -387,8 +387,7 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>  	if (rsz->fmt_type == RKISP1_FMT_YUV) {
>  		struct rkisp1_capture *cap =
>  			&rsz->rkisp1->capture_devs[rsz->id];
> -		const struct v4l2_format_info *pixfmt_info =
> -			v4l2_format_info(cap->pix.fmt.pixelformat);
> +		const struct v4l2_format_info *pixfmt_info = cap->pix.info;
>  
>  		hdiv = pixfmt_info->hdiv;
>  		vdiv = pixfmt_info->vdiv;

-- 
Regards,

Laurent Pinchart

[PATCH 2/3] media: staging: rkisp1: rsz: change (hv)div only if capture format is YUV

[Dafna Hirschfeld]

RGB formats in selfpath should receive input format as YUV422.
The resizer input format is always YUV422 and therefore
if the capture format is RGB, the resizer should not change
the YUV rations.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
index 8704267a066f..5721eee29ecb 100644
--- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
+++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
@@ -389,8 +389,18 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
 			&rsz->rkisp1->capture_devs[rsz->id];
 		const struct v4l2_format_info *pixfmt_info = cap->pix.info;
 
-		hdiv = pixfmt_info->hdiv;
-		vdiv = pixfmt_info->vdiv;
+		/*
+		 * The resizer always get the input as YUV422
+		 * If the capture encoding is also YUV, then the resizer should
+		 * change the 4:2:2 sampling to the sampling of the capture
+		 * format (4:2:2 -> 4:2:0 for example).
+		 * If the capture format is RGB then the memory input should
+		 * be YUV422 so we don't change the default hdiv, vdiv
+		 */
+		if (v4l2_is_format_yuv(pixfmt_info)) {
+			hdiv = pixfmt_info->hdiv;
+			vdiv = pixfmt_info->vdiv;
+		}
 	}
 	src_c.width = src_y.width / hdiv;
 	src_c.height = src_y.height / vdiv;
-- 
2.17.1

Re: [PATCH 2/3] media: staging: rkisp1: rsz: change (hv)div only if capture format is YUV

[Helen Koike]

On 3/28/20 7:56 AM, Dafna Hirschfeld wrote:
> RGB formats in selfpath should receive input format as YUV422.
> The resizer input format is always YUV422 and therefore
> if the capture format is RGB, the resizer should not change
> the YUV rations.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-resizer.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> index 8704267a066f..5721eee29ecb 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> @@ -389,8 +389,18 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>  			&rsz->rkisp1->capture_devs[rsz->id];
>  		const struct v4l2_format_info *pixfmt_info = cap->pix.info;
>  
> -		hdiv = pixfmt_info->hdiv;
> -		vdiv = pixfmt_info->vdiv;
> +		/*
> +		 * The resizer always get the input as YUV422
> +		 * If the capture encoding is also YUV, then the resizer should
> +		 * change the 4:2:2 sampling to the sampling of the capture
> +		 * format (4:2:2 -> 4:2:0 for example).
> +		 * If the capture format is RGB then the memory input should
> +		 * be YUV422 so we don't change the default hdiv, vdiv
> +		 */
> +		if (v4l2_is_format_yuv(pixfmt_info)) {

Can't this be moved with && in the outer if statement block?

Regards,
Helen

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

Re: [PATCH 2/3] media: staging: rkisp1: rsz: change (hv)div only if capture format is YUV

[Dafna Hirschfeld]

On 30.03.20 22:04, Helen Koike wrote:
> 
> 
> On 3/28/20 7:56 AM, Dafna Hirschfeld wrote:
>> RGB formats in selfpath should receive input format as YUV422.
>> The resizer input format is always YUV422 and therefore
>> if the capture format is RGB, the resizer should not change
>> the YUV rations.
>>
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 14 ++++++++++++--
>>   1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> index 8704267a066f..5721eee29ecb 100644
>> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
>> @@ -389,8 +389,18 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
>>   			&rsz->rkisp1->capture_devs[rsz->id];
>>   		const struct v4l2_format_info *pixfmt_info = cap->pix.info;
>>   
>> -		hdiv = pixfmt_info->hdiv;
>> -		vdiv = pixfmt_info->vdiv;
>> +		/*
>> +		 * The resizer always get the input as YUV422
>> +		 * If the capture encoding is also YUV, then the resizer should
>> +		 * change the 4:2:2 sampling to the sampling of the capture
>> +		 * format (4:2:2 -> 4:2:0 for example).
>> +		 * If the capture format is RGB then the memory input should
>> +		 * be YUV422 so we don't change the default hdiv, vdiv
>> +		 */
>> +		if (v4l2_is_format_yuv(pixfmt_info)) {
> 
> Can't this be moved with && in the outer if statement block?

Actually the outer statement is not needed at all and can be removed
since the code return if rsz->fmt_type is RKISP1_FMT_BAYER but
the resizer format is either bayer or yuv so there is no
need for the "if (rsz->fmt_type == RKISP1_FMT_YUV)"

Thanks,
Dafna

> 
> Regards,
> Helen
> 
>> +			hdiv = pixfmt_info->hdiv;
>> +			vdiv = pixfmt_info->vdiv;
>> +		}
>>   	}
>>   	src_c.width = src_y.width / hdiv;
>>   	src_c.height = src_y.height / vdiv;
>>

Re: [PATCH 2/3] media: staging: rkisp1: rsz: change (hv)div only if capture format is YUV

[Laurent Pinchart]

Hi Dafna,

On Sat, Apr 04, 2020 at 02:09:20PM +0200, Dafna Hirschfeld wrote:
> On 30.03.20 22:04, Helen Koike wrote:
> > On 3/28/20 7:56 AM, Dafna Hirschfeld wrote:
> >> RGB formats in selfpath should receive input format as YUV422.
> >> The resizer input format is always YUV422 and therefore
> >> if the capture format is RGB, the resizer should not change
> >> the YUV rations.
> >>
> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> >> ---
> >>   drivers/staging/media/rkisp1/rkisp1-resizer.c | 14 ++++++++++++--
> >>   1 file changed, 12 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/rkisp1/rkisp1-resizer.c b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >> index 8704267a066f..5721eee29ecb 100644
> >> --- a/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >> +++ b/drivers/staging/media/rkisp1/rkisp1-resizer.c
> >> @@ -389,8 +389,18 @@ static void rkisp1_rsz_config(struct rkisp1_resizer *rsz,
> >>   			&rsz->rkisp1->capture_devs[rsz->id];
> >>   		const struct v4l2_format_info *pixfmt_info = cap->pix.info;
> >>   
> >> -		hdiv = pixfmt_info->hdiv;
> >> -		vdiv = pixfmt_info->vdiv;
> >> +		/*
> >> +		 * The resizer always get the input as YUV422
> >> +		 * If the capture encoding is also YUV, then the resizer should
> >> +		 * change the 4:2:2 sampling to the sampling of the capture
> >> +		 * format (4:2:2 -> 4:2:0 for example).
> >> +		 * If the capture format is RGB then the memory input should
> >> +		 * be YUV422 so we don't change the default hdiv, vdiv
> >> +		 */
> >> +		if (v4l2_is_format_yuv(pixfmt_info)) {
> > 
> > Can't this be moved with && in the outer if statement block?
> 
> Actually the outer statement is not needed at all and can be removed
> since the code return if rsz->fmt_type is RKISP1_FMT_BAYER but
> the resizer format is either bayer or yuv so there is no
> need for the "if (rsz->fmt_type == RKISP1_FMT_YUV)"

This sounds reasonable.

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

-- 
Regards,

Laurent Pinchart

[PATCH 3/3] media: staging: rkisp1: cap: enable RGB capture format with YUV media bus

[Dafna Hirschfeld]

In selfpath, RGB capture formats are received in the sink pad as YUV
and are converted to RGB only when writing to memory. So the validation
function should accept YUV bus formats with RGB capture encoding.

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

diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
index b7681b806b4c..3abf38362f5a 100644
--- a/drivers/staging/media/rkisp1/rkisp1-capture.c
+++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
@@ -1227,6 +1227,9 @@ static int rkisp1_capture_link_validate(struct media_link *link)
 		media_entity_to_v4l2_subdev(link->source->entity);
 	struct rkisp1_capture *cap = video_get_drvdata(vdev);
 	struct rkisp1_isp *isp = &cap->rkisp1->isp;
+	enum rkisp1_fmt_pix_type cap_fmt =
+		rkisp1_pixel_enc_to_fmt_pix(cap->pix.info);
+	enum rkisp1_fmt_pix_type isp_fmt = isp->src_fmt->fmt_type;
 	struct v4l2_subdev_format sd_fmt;
 	int ret;
 
@@ -1237,8 +1240,8 @@ static int rkisp1_capture_link_validate(struct media_link *link)
 		return -EPIPE;
 	}
 
-	if (rkisp1_pixel_enc_to_fmt_pix(cap->pix.info) !=
-	    isp->src_fmt->fmt_type) {
+	if ((cap_fmt == RKISP1_FMT_BAYER && isp_fmt == RKISP1_FMT_YUV) ||
+	    (cap_fmt != RKISP1_FMT_BAYER && isp_fmt == RKISP1_FMT_BAYER)) {
 		dev_err(cap->rkisp1->dev,
 			"format type mismatch in link '%s:%d->%s:%d'\n",
 			link->source->entity->name, link->source->index,
-- 
2.17.1

Re: [PATCH 3/3] media: staging: rkisp1: cap: enable RGB capture format with YUV media bus

[Laurent Pinchart]

Hi Dafna

Thank you for the patch.

On Sat, Mar 28, 2020 at 11:56:06AM +0100, Dafna Hirschfeld wrote:
> In selfpath, RGB capture formats are received in the sink pad as YUV
> and are converted to RGB only when writing to memory. So the validation
> function should accept YUV bus formats with RGB capture encoding.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-capture.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-capture.c b/drivers/staging/media/rkisp1/rkisp1-capture.c
> index b7681b806b4c..3abf38362f5a 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-capture.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-capture.c
> @@ -1227,6 +1227,9 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>  		media_entity_to_v4l2_subdev(link->source->entity);
>  	struct rkisp1_capture *cap = video_get_drvdata(vdev);
>  	struct rkisp1_isp *isp = &cap->rkisp1->isp;
> +	enum rkisp1_fmt_pix_type cap_fmt =
> +		rkisp1_pixel_enc_to_fmt_pix(cap->pix.info);
> +	enum rkisp1_fmt_pix_type isp_fmt = isp->src_fmt->fmt_type;
>  	struct v4l2_subdev_format sd_fmt;
>  	int ret;
>  
> @@ -1237,8 +1240,8 @@ static int rkisp1_capture_link_validate(struct media_link *link)
>  		return -EPIPE;
>  	}
>  
> -	if (rkisp1_pixel_enc_to_fmt_pix(cap->pix.info) !=
> -	    isp->src_fmt->fmt_type) {
> +	if ((cap_fmt == RKISP1_FMT_BAYER && isp_fmt == RKISP1_FMT_YUV) ||
> +	    (cap_fmt != RKISP1_FMT_BAYER && isp_fmt == RKISP1_FMT_BAYER)) {

How about listing the supported options instead of the unsupported
options ?

	if (!(isp_fmt == cap_fmt) &&
	    !(isp_fmt == RKISP1_FMT_YUV && cap_fmt == RKISP1_FMT_RGB))

This would also reject RKISP1_FMT_JPEG (which isn't used yet, true), and
generally (in my opinion at least) be more readable.

>  		dev_err(cap->rkisp1->dev,
>  			"format type mismatch in link '%s:%d->%s:%d'\n",
>  			link->source->entity->name, link->source->index,

-- 
Regards,

Laurent Pinchart