[PATCH v3 0/4] Fixes for colorspace logic in exynos-gsc and s5p-mfc drivers

[PATCH v3 0/4] Fixes for colorspace logic in exynos-gsc and s5p-mfc drivers

[Thibault Saunier]

Hello,

This patchset fixes a few issues on the colorspace logic for the exynos-gsc
and s5p-mfc drivers.

We now handle the colorspace in those drivers, and make sure to respect user setting if
possible.

We also now set the 'v4l2_pix_format:field' if userspace passed ANY, avoiding GStreamer
spamming error at us about the driver not following the standard.

This is the third version of the patch serie.

Best regards,

Thibault Saunier

Changes in v3:
- Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'
- Do not check values in the g_fmt functions as Andrzej explained in previous review
- Set colorspace if user passed V4L2_COLORSPACE_DEFAULT in

Changes in v2:
- Fix a silly build error that slipped in while rebasing the patches

Javier Martinez Canillas (1):
  [media] exynos-gsc: Use 576p instead 720p as a threshold for
    colorspaces

Thibault Saunier (3):
  [media] exynos-gsc: Respect userspace colorspace setting in try_fmt
  [media] s5p-mfc: Set colorspace in VIDIO_{G,TRY}_FMT
  [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in
    try_fmt

 drivers/media/platform/exynos-gsc/gsc-core.c | 17 +++++++++++-----
 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 29 ++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 5 deletions(-)

-- 
2.11.1

[PATCH v3 1/4] [media] exynos-gsc: Use 576p instead 720p as a threshold for colorspaces

[Thibault Saunier]

From: Javier Martinez Canillas <javier@osg.samsung.com>

The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace
should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV. But drivers
don't agree on the display resolution that should be used as a threshold.

Some drivers set V4L2_COLORSPACE_REC709 for 720p and higher while others
set V4L2_COLORSPACE_REC709 for anything higher than 576p. Newers drivers
use the latter and that also matches what user-space multimedia programs
do (i.e: GStreamer), so change the driver logic to be aligned with this.

Also, check for the resolution in G_FMT instead unconditionally setting
the V4L2_COLORSPACE_REC709 colorspace.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
---

Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review
- Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'

Changes in v2: None

 drivers/media/platform/exynos-gsc/gsc-core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 59a634201830..db7d9883861b 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -472,7 +472,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 
 	pix_mp->num_planes = fmt->num_planes;
 
-	if (pix_mp->width >= 1280) /* HD */
+	if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
 		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
 	else /* SD */
 		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
@@ -519,9 +519,13 @@ int gsc_g_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 	pix_mp->height		= frame->f_height;
 	pix_mp->field		= V4L2_FIELD_NONE;
 	pix_mp->pixelformat	= frame->fmt->pixelformat;
-	pix_mp->colorspace	= V4L2_COLORSPACE_REC709;
 	pix_mp->num_planes	= frame->fmt->num_planes;
 
+	if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
+		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
+	else /* SD */
+		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
+
 	for (i = 0; i < pix_mp->num_planes; ++i) {
 		pix_mp->plane_fmt[i].bytesperline = (frame->f_width *
 			frame->fmt->depth[i]) / 8;
-- 
2.11.1

[PATCH v3 2/4] [media] exynos-gsc: Respect userspace colorspace setting in try_fmt

[Thibault Saunier]

If the colorspace is specified by userspace we should respect
it and not reset it ourself if we can support it.

Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>

---

Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review
- Set colorspace if user passed V4L2_COLORSPACE_DEFAULT in

Changes in v2: None

 drivers/media/platform/exynos-gsc/gsc-core.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index db7d9883861b..021598817938 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -472,10 +472,13 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
 
 	pix_mp->num_planes = fmt->num_planes;
 
-	if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
-		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
-	else /* SD */
-		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
+	if (pix_mp->colorspace != V4L2_COLORSPACE_REC709 &&
+		pix_mp->colorspace != V4L2_COLORSPACE_SMPTE170M) {
+		if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
+			pix_mp->colorspace = V4L2_COLORSPACE_REC709;
+		else /* SD */
+			pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
+	}
 
 	for (i = 0; i < pix_mp->num_planes; ++i) {
 		struct v4l2_plane_pix_format *plane_fmt = &pix_mp->plane_fmt[i];
-- 
2.11.1

[PATCH v3 3/4] [media] s5p-mfc: Set colorspace in VIDIO_{G,TRY}_FMT

[Thibault Saunier]

The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace
should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV but the driver
didn't set the colorimetry, also respect usespace setting.

Use 576p display resolution as a threshold to set this.

Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>

---

Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review
- Set colorspace if user passed V4L2_COLORSPACE_DEFAULT in

Changes in v2: None

 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 367ef8e8dbf0..16bc3eaad0ff 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -354,6 +354,11 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 		pix_mp->plane_fmt[0].sizeimage = ctx->luma_size;
 		pix_mp->plane_fmt[1].bytesperline = ctx->buf_width;
 		pix_mp->plane_fmt[1].sizeimage = ctx->chroma_size;
+
+		if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
+			pix_mp->colorspace = V4L2_COLORSPACE_REC709;
+		else /* SD */
+			pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
 	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
 		/* This is run on OUTPUT
 		   The buffer contains compressed image
@@ -378,6 +383,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
 static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 {
 	struct s5p_mfc_dev *dev = video_drvdata(file);
+	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
 	struct s5p_mfc_fmt *fmt;
 
 	mfc_debug(2, "Type is %d\n", f->type);
@@ -405,6 +411,15 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 			mfc_err("Unsupported format by this MFC version.\n");
 			return -EINVAL;
 		}
+
+		if (pix_mp->colorspace != V4L2_COLORSPACE_REC709 &&
+			pix_mp->colorspace != V4L2_COLORSPACE_SMPTE170M) {
+			if (pix_mp->width > 720 &&
+					pix_mp->height > 576) /* HD */
+				pix_mp->colorspace = V4L2_COLORSPACE_REC709;
+			else /* SD */
+				pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
+		}
 	}
 
 	return 0;
-- 
2.11.1

[PATCH v3 4/4] [media] s5p-mfc: Check and set 'v4l2_pix_format:field' field in try_fmt

[Thibault Saunier]

It is required by the standard that the field order is set by the
driver.

Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>

---

Changes in v3:
- Do not check values in the g_fmt functions as Andrzej explained in previous review

Changes in v2:
- Fix a silly build error that slipped in while rebasing the patches

 drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
index 16bc3eaad0ff..e249c5cee262 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
@@ -385,6 +385,20 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
 	struct s5p_mfc_dev *dev = video_drvdata(file);
 	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
 	struct s5p_mfc_fmt *fmt;
+	enum v4l2_field field;
+
+	field = f->fmt.pix.field;
+	if (field == V4L2_FIELD_ANY) {
+		field = V4L2_FIELD_NONE;
+	} else if (field != V4L2_FIELD_NONE) {
+		mfc_debug(2, "Not supported field order(%d)\n", pix_mp->field);
+		return -EINVAL;
+	}
+
+	/* V4L2 specification suggests the driver corrects the format struct
+	 * if any of the dimensions is unsupported
+	 */
+	f->fmt.pix.field = field;
 
 	mfc_debug(2, "Type is %d\n", f->type);
 	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
-- 
2.11.1

Re: [PATCH v3 1/4] [media] exynos-gsc: Use 576p instead 720p as a threshold for colorspaces

[Hans Verkuil]

On 02/10/2017 03:10 PM, Thibault Saunier wrote:
> From: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace
> should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV. But drivers
> don't agree on the display resolution that should be used as a threshold.
> 
> Some drivers set V4L2_COLORSPACE_REC709 for 720p and higher while others
> set V4L2_COLORSPACE_REC709 for anything higher than 576p. Newers drivers
> use the latter and that also matches what user-space multimedia programs
> do (i.e: GStreamer), so change the driver logic to be aligned with this.
> 
> Also, check for the resolution in G_FMT instead unconditionally setting
> the V4L2_COLORSPACE_REC709 colorspace.
> 
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> 
> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
> ---
> 
> Changes in v3:
> - Do not check values in the g_fmt functions as Andrzej explained in previous review
> - Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'
> 
> Changes in v2: None
> 
>  drivers/media/platform/exynos-gsc/gsc-core.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 59a634201830..db7d9883861b 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -472,7 +472,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>  
>  	pix_mp->num_planes = fmt->num_planes;
>  
> -	if (pix_mp->width >= 1280) /* HD */
> +	if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
>  		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
>  	else /* SD */
>  		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
> @@ -519,9 +519,13 @@ int gsc_g_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>  	pix_mp->height		= frame->f_height;
>  	pix_mp->field		= V4L2_FIELD_NONE;
>  	pix_mp->pixelformat	= frame->fmt->pixelformat;
> -	pix_mp->colorspace	= V4L2_COLORSPACE_REC709;
>  	pix_mp->num_planes	= frame->fmt->num_planes;
>  
> +	if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
> +		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
> +	else /* SD */
> +		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +
>  	for (i = 0; i < pix_mp->num_planes; ++i) {
>  		pix_mp->plane_fmt[i].bytesperline = (frame->f_width *
>  			frame->fmt->depth[i]) / 8;
> 

This is a mem2mem device, right? In the case of mem2mem devices the driver should never
set the colorspace, instead it just copies it from what the application provides (the
video output side) to the capture side.

After all, you are just scaling here so the input and output colorspaces are
exactly the same, and the scaler doesn't care what the colorspace is.

Regards,

	Hans

Re: [PATCH v3 3/4] [media] s5p-mfc: Set colorspace in VIDIO_{G,TRY}_FMT

[Hans Verkuil]

On 02/10/2017 03:10 PM, Thibault Saunier wrote:
> The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace
> should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV but the driver
> didn't set the colorimetry, also respect usespace setting.
> 
> Use 576p display resolution as a threshold to set this.
> 
> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
> 
> ---
> 
> Changes in v3:
> - Do not check values in the g_fmt functions as Andrzej explained in previous review
> - Set colorspace if user passed V4L2_COLORSPACE_DEFAULT in
> 
> Changes in v2: None
> 
>  drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> index 367ef8e8dbf0..16bc3eaad0ff 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_dec.c
> @@ -354,6 +354,11 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  		pix_mp->plane_fmt[0].sizeimage = ctx->luma_size;
>  		pix_mp->plane_fmt[1].bytesperline = ctx->buf_width;
>  		pix_mp->plane_fmt[1].sizeimage = ctx->chroma_size;
> +
> +		if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
> +			pix_mp->colorspace = V4L2_COLORSPACE_REC709;
> +		else /* SD */
> +			pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
>  	} else if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
>  		/* This is run on OUTPUT
>  		   The buffer contains compressed image
> @@ -378,6 +383,7 @@ static int vidioc_g_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  {
>  	struct s5p_mfc_dev *dev = video_drvdata(file);
> +	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
>  	struct s5p_mfc_fmt *fmt;
>  
>  	mfc_debug(2, "Type is %d\n", f->type);
> @@ -405,6 +411,15 @@ static int vidioc_try_fmt(struct file *file, void *priv, struct v4l2_format *f)
>  			mfc_err("Unsupported format by this MFC version.\n");
>  			return -EINVAL;
>  		}
> +
> +		if (pix_mp->colorspace != V4L2_COLORSPACE_REC709 &&
> +			pix_mp->colorspace != V4L2_COLORSPACE_SMPTE170M) {
> +			if (pix_mp->width > 720 &&
> +					pix_mp->height > 576) /* HD */
> +				pix_mp->colorspace = V4L2_COLORSPACE_REC709;
> +			else /* SD */
> +				pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
> +		}
>  	}
>  
>  	return 0;
> 

Same here: it is an mem2mem device, so it just preserves whatever colorspace the
application passes in.

Just look at what several other mem2mem device drivers do.

Regards,

	Hans

Re: [PATCH v3 1/4] [media] exynos-gsc: Use 576p instead 720p as a threshold for colorspaces

[Thibault Saunier]

On 02/10/2017 12:07 PM, Hans Verkuil wrote:
> On 02/10/2017 03:10 PM, Thibault Saunier wrote:
>> From: Javier Martinez Canillas <javier@osg.samsung.com>
>>
>> The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace
>> should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV. But drivers
>> don't agree on the display resolution that should be used as a threshold.
>>
>> Some drivers set V4L2_COLORSPACE_REC709 for 720p and higher while others
>> set V4L2_COLORSPACE_REC709 for anything higher than 576p. Newers drivers
>> use the latter and that also matches what user-space multimedia programs
>> do (i.e: GStreamer), so change the driver logic to be aligned with this.
>>
>> Also, check for the resolution in G_FMT instead unconditionally setting
>> the V4L2_COLORSPACE_REC709 colorspace.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>
>> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>> ---
>>
>> Changes in v3:
>> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>> - Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'
>>
>> Changes in v2: None
>>
>>   drivers/media/platform/exynos-gsc/gsc-core.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
>> index 59a634201830..db7d9883861b 100644
>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
>> @@ -472,7 +472,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>>   
>>   	pix_mp->num_planes = fmt->num_planes;
>>   
>> -	if (pix_mp->width >= 1280) /* HD */
>> +	if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
>>   		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
>>   	else /* SD */
>>   		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
>> @@ -519,9 +519,13 @@ int gsc_g_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>>   	pix_mp->height		= frame->f_height;
>>   	pix_mp->field		= V4L2_FIELD_NONE;
>>   	pix_mp->pixelformat	= frame->fmt->pixelformat;
>> -	pix_mp->colorspace	= V4L2_COLORSPACE_REC709;
>>   	pix_mp->num_planes	= frame->fmt->num_planes;
>>   
>> +	if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
>> +		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
>> +	else /* SD */
>> +		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
>> +
>>   	for (i = 0; i < pix_mp->num_planes; ++i) {
>>   		pix_mp->plane_fmt[i].bytesperline = (frame->f_width *
>>   			frame->fmt->depth[i]) / 8;
>>
> This is a mem2mem device, right? In the case of mem2mem devices the driver should never
> set the colorspace, instead it just copies it from what the application provides (the
> video output side) to the capture side.
>
> After all, you are just scaling here so the input and output colorspaces are
> exactly the same, and the scaler doesn't care what the colorspace is.

This device also does color conversion so I think it matters here, am I 
misunderstanding something?
Also, is that comment only for the try_fmt part as it looks like in 
g_fmt the driver should fill up the structure
with its values?

Thanks for you review.

Regards,

Thibault Saunier
> Regards,
>
> 	Hans

Re: [PATCH v3 1/4] [media] exynos-gsc: Use 576p instead 720p as a threshold for colorspaces

[Hans Verkuil]

On 02/10/2017 05:37 PM, Thibault Saunier wrote:
> On 02/10/2017 12:07 PM, Hans Verkuil wrote:
>> On 02/10/2017 03:10 PM, Thibault Saunier wrote:
>>> From: Javier Martinez Canillas <javier@osg.samsung.com>
>>>
>>> The media documentation says that the V4L2_COLORSPACE_SMPTE170M colorspace
>>> should be used for SDTV and V4L2_COLORSPACE_REC709 for HDTV. But drivers
>>> don't agree on the display resolution that should be used as a threshold.
>>>
>>> Some drivers set V4L2_COLORSPACE_REC709 for 720p and higher while others
>>> set V4L2_COLORSPACE_REC709 for anything higher than 576p. Newers drivers
>>> use the latter and that also matches what user-space multimedia programs
>>> do (i.e: GStreamer), so change the driver logic to be aligned with this.
>>>
>>> Also, check for the resolution in G_FMT instead unconditionally setting
>>> the V4L2_COLORSPACE_REC709 colorspace.
>>>
>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>>>
>>> Signed-off-by: Thibault Saunier <thibault.saunier@osg.samsung.com>
>>> ---
>>>
>>> Changes in v3:
>>> - Do not check values in the g_fmt functions as Andrzej explained in previous review
>>> - Added 'Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>'
>>>
>>> Changes in v2: None
>>>
>>>   drivers/media/platform/exynos-gsc/gsc-core.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
>>> index 59a634201830..db7d9883861b 100644
>>> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
>>> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
>>> @@ -472,7 +472,7 @@ int gsc_try_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>>>   
>>>   	pix_mp->num_planes = fmt->num_planes;
>>>   
>>> -	if (pix_mp->width >= 1280) /* HD */
>>> +	if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
>>>   		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
>>>   	else /* SD */
>>>   		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
>>> @@ -519,9 +519,13 @@ int gsc_g_fmt_mplane(struct gsc_ctx *ctx, struct v4l2_format *f)
>>>   	pix_mp->height		= frame->f_height;
>>>   	pix_mp->field		= V4L2_FIELD_NONE;
>>>   	pix_mp->pixelformat	= frame->fmt->pixelformat;
>>> -	pix_mp->colorspace	= V4L2_COLORSPACE_REC709;
>>>   	pix_mp->num_planes	= frame->fmt->num_planes;
>>>   
>>> +	if (pix_mp->width > 720 && pix_mp->height > 576) /* HD */
>>> +		pix_mp->colorspace = V4L2_COLORSPACE_REC709;
>>> +	else /* SD */
>>> +		pix_mp->colorspace = V4L2_COLORSPACE_SMPTE170M;
>>> +
>>>   	for (i = 0; i < pix_mp->num_planes; ++i) {
>>>   		pix_mp->plane_fmt[i].bytesperline = (frame->f_width *
>>>   			frame->fmt->depth[i]) / 8;
>>>
>> This is a mem2mem device, right? In the case of mem2mem devices the driver should never
>> set the colorspace, instead it just copies it from what the application provides (the
>> video output side) to the capture side.
>>
>> After all, you are just scaling here so the input and output colorspaces are
>> exactly the same, and the scaler doesn't care what the colorspace is.
> 
> This device also does color conversion so I think it matters here, am I 
> misunderstanding something?

It almost certainly only does color encoding conversion (e.g. from RGB to YCbCr or
vice versa, and from limited to full range or vice versa).

That only affects the ycbcr_enc and quantization fields (although these probably
will just remain 0). The colorspace and xfer_func values remain constant since
the HW is unlikely to support full colorspace conversions.

See https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces.html for more
information on what colorspaces really are.

> Also, is that comment only for the try_fmt part as it looks like in 
> g_fmt the driver should fill up the structure
> with its values?

Yes, same for try_fmt.

Regards,

	Hans