[PATCH 0/3] media: imx: Add support for BT.709 encoding

[PATCH 0/3] media: imx: Add support for BT.709 encoding

[Steve Longerbeam]

This patchset adds support for the BT.709 encoding and inverse encoding
matrices to the ipu_ic task init functions. The imx-media driver can
now support both BT.601 and BT.709 encoding.

Steve Longerbeam (3):
  gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices
  gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding
  media: imx: Allow BT.709 encoding for IC routes

 drivers/gpu/ipu-v3/ipu-ic.c                 | 75 ++++++++++++++++++---
 drivers/gpu/ipu-v3/ipu-image-convert.c      |  1 +
 drivers/staging/media/imx/imx-ic-prpencvf.c |  4 +-
 drivers/staging/media/imx/imx-media-utils.c |  4 +-
 include/video/imx-ipu-v3.h                  |  5 +-
 5 files changed, 76 insertions(+), 13 deletions(-)

-- 
2.17.1

[PATCH 1/3] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices

[Steve Longerbeam]

The ycbcr2rgb and inverse rgb2ycbcr matrices define the BT.601 encoding
coefficients, so rename them to indicate that. And add some comments
to make clear these are BT.601 coefficients encoding between YUV limited
range and RGB full range. No functional changes.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/gpu/ipu-v3/ipu-ic.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
index 594c3cbc8291..35ae86ff0585 100644
--- a/drivers/gpu/ipu-v3/ipu-ic.c
+++ b/drivers/gpu/ipu-v3/ipu-ic.c
@@ -183,11 +183,13 @@ struct ic_csc_params {
 };
 
 /*
+ * BT.601 encoding from RGB full range to YUV limited range:
+ *
  * Y = R *  .299 + G *  .587 + B *  .114;
  * U = R * -.169 + G * -.332 + B *  .500 + 128.;
  * V = R *  .500 + G * -.419 + B * -.0813 + 128.;
  */
-static const struct ic_csc_params ic_csc_rgb2ycbcr = {
+static const struct ic_csc_params ic_csc_rgb2ycbcr_bt601 = {
 	.coeff = {
 		{ 77, 150, 29 },
 		{ 469, 427, 128 },
@@ -208,11 +210,13 @@ static const struct ic_csc_params ic_csc_rgb2rgb = {
 };
 
 /*
+ * Inverse BT.601 encoding from YUV limited range to RGB full range:
+ *
  * R = (1.164 * (Y - 16)) + (1.596 * (Cr - 128));
  * G = (1.164 * (Y - 16)) - (0.392 * (Cb - 128)) - (0.813 * (Cr - 128));
  * B = (1.164 * (Y - 16)) + (2.017 * (Cb - 128);
  */
-static const struct ic_csc_params ic_csc_ycbcr2rgb = {
+static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = {
 	.coeff = {
 		{ 149, 0, 204 },
 		{ 149, 462, 408 },
@@ -238,9 +242,9 @@ static int init_csc(struct ipu_ic *ic,
 		(priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
 
 	if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB)
-		params = &ic_csc_ycbcr2rgb;
+		params = &ic_csc_ycbcr2rgb_bt601;
 	else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV)
-		params = &ic_csc_rgb2ycbcr;
+		params = &ic_csc_rgb2ycbcr_bt601;
 	else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB)
 		params = &ic_csc_rgb2rgb;
 	else {
-- 
2.17.1

[PATCH 2/3] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding

[Steve Longerbeam]

Pass v4l2 encoding enum to the ipu_ic task init functions, and add
support for the BT.709 encoding and inverse encoding matrices.

Reported-by: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/gpu/ipu-v3/ipu-ic.c                 | 67 ++++++++++++++++++---
 drivers/gpu/ipu-v3/ipu-image-convert.c      |  1 +
 drivers/staging/media/imx/imx-ic-prpencvf.c |  4 +-
 include/video/imx-ipu-v3.h                  |  5 +-
 4 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
index 35ae86ff0585..63362b4fff81 100644
--- a/drivers/gpu/ipu-v3/ipu-ic.c
+++ b/drivers/gpu/ipu-v3/ipu-ic.c
@@ -199,6 +199,23 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr_bt601 = {
 	.scale = 1,
 };
 
+/*
+ * BT.709 encoding from RGB full range to YUV limited range:
+ *
+ * Y = R *  .2126 + G *  .7152 + B *  .0722;
+ * U = R * -.1146 + G * -.3854 + B *  .5000 + 128.;
+ * V = R *  .5000 + G * -.4542 + B * -.0458 + 128.;
+ */
+static const struct ic_csc_params ic_csc_rgb2ycbcr_bt709 = {
+	.coeff = {
+		{ 54, 183, 18 },
+		{ 483, 413, 128 },
+		{ 128, 396, 500 },
+	},
+	.offset = { 0, 512, 512 },
+	.scale = 1,
+};
+
 /* transparent RGB->RGB matrix for graphics combining */
 static const struct ic_csc_params ic_csc_rgb2rgb = {
 	.coeff = {
@@ -226,12 +243,31 @@ static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = {
 	.scale = 2,
 };
 
+/*
+ * Inverse BT.709 encoding from YUV limited range to RGB full range:
+ *
+ * R = (1. * (Y - 16)) + (1.5748 * (Cr - 128));
+ * G = (1. * (Y - 16)) - (0.1873 * (Cb - 128)) - (0.4681 * (Cr - 128));
+ * B = (1. * (Y - 16)) + (1.8556 * (Cb - 128);
+ */
+static const struct ic_csc_params ic_csc_ycbcr2rgb_bt709 = {
+	.coeff = {
+		{ 128, 0, 202 },
+		{ 128, 488, 452 },
+		{ 128, 238, 0 },
+	},
+	.offset = { -435, 136, -507 },
+	.scale = 2,
+};
+
 static int init_csc(struct ipu_ic *ic,
 		    enum ipu_color_space inf,
 		    enum ipu_color_space outf,
+		    enum v4l2_ycbcr_encoding encoding,
 		    int csc_index)
 {
 	struct ipu_ic_priv *priv = ic->priv;
+	const struct ic_csc_params *params_rgb2yuv, *params_yuv2rgb;
 	const struct ic_csc_params *params;
 	u32 __iomem *base;
 	const u16 (*c)[3];
@@ -241,10 +277,24 @@ static int init_csc(struct ipu_ic *ic,
 	base = (u32 __iomem *)
 		(priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
 
+	switch (encoding) {
+	case V4L2_YCBCR_ENC_601:
+		params_rgb2yuv =  &ic_csc_rgb2ycbcr_bt601;
+		params_yuv2rgb = &ic_csc_ycbcr2rgb_bt601;
+		break;
+	case V4L2_YCBCR_ENC_709:
+		params_rgb2yuv =  &ic_csc_rgb2ycbcr_bt709;
+		params_yuv2rgb = &ic_csc_ycbcr2rgb_bt709;
+		break;
+	default:
+		dev_err(priv->ipu->dev, "Unsupported YCbCr encoding\n");
+		return -EINVAL;
+	}
+
 	if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB)
-		params = &ic_csc_ycbcr2rgb_bt601;
+		params = params_yuv2rgb;
 	else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV)
-		params = &ic_csc_rgb2ycbcr_bt601;
+		params = params_rgb2yuv;
 	else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB)
 		params = &ic_csc_rgb2rgb;
 	else {
@@ -391,6 +441,7 @@ EXPORT_SYMBOL_GPL(ipu_ic_task_disable);
 
 int ipu_ic_task_graphics_init(struct ipu_ic *ic,
 			      enum ipu_color_space in_g_cs,
+			      enum v4l2_ycbcr_encoding encoding,
 			      bool galpha_en, u32 galpha,
 			      bool colorkey_en, u32 colorkey)
 {
@@ -409,7 +460,7 @@ int ipu_ic_task_graphics_init(struct ipu_ic *ic,
 	if (!(ic_conf & ic->bit->ic_conf_csc1_en)) {
 		/* need transparent CSC1 conversion */
 		ret = init_csc(ic, IPUV3_COLORSPACE_RGB,
-			       IPUV3_COLORSPACE_RGB, 0);
+			       IPUV3_COLORSPACE_RGB, encoding, 0);
 		if (ret)
 			goto unlock;
 	}
@@ -417,7 +468,7 @@ int ipu_ic_task_graphics_init(struct ipu_ic *ic,
 	ic->g_in_cs = in_g_cs;
 
 	if (ic->g_in_cs != ic->out_cs) {
-		ret = init_csc(ic, ic->g_in_cs, ic->out_cs, 1);
+		ret = init_csc(ic, ic->g_in_cs, ic->out_cs, encoding, 1);
 		if (ret)
 			goto unlock;
 	}
@@ -451,6 +502,7 @@ int ipu_ic_task_init_rsc(struct ipu_ic *ic,
 			 int out_width, int out_height,
 			 enum ipu_color_space in_cs,
 			 enum ipu_color_space out_cs,
+			 enum v4l2_ycbcr_encoding encoding,
 			 u32 rsc)
 {
 	struct ipu_ic_priv *priv = ic->priv;
@@ -486,7 +538,7 @@ int ipu_ic_task_init_rsc(struct ipu_ic *ic,
 	ic->out_cs = out_cs;
 
 	if (ic->in_cs != ic->out_cs) {
-		ret = init_csc(ic, ic->in_cs, ic->out_cs, 0);
+		ret = init_csc(ic, ic->in_cs, ic->out_cs, encoding, 0);
 		if (ret)
 			goto unlock;
 	}
@@ -500,10 +552,11 @@ int ipu_ic_task_init(struct ipu_ic *ic,
 		     int in_width, int in_height,
 		     int out_width, int out_height,
 		     enum ipu_color_space in_cs,
-		     enum ipu_color_space out_cs)
+		     enum ipu_color_space out_cs,
+		     enum v4l2_ycbcr_encoding encoding)
 {
 	return ipu_ic_task_init_rsc(ic, in_width, in_height, out_width,
-				    out_height, in_cs, out_cs, 0);
+				    out_height, in_cs, out_cs, encoding, 0);
 }
 EXPORT_SYMBOL_GPL(ipu_ic_task_init);
 
diff --git a/drivers/gpu/ipu-v3/ipu-image-convert.c b/drivers/gpu/ipu-v3/ipu-image-convert.c
index 13103ab86050..8b37daa99f58 100644
--- a/drivers/gpu/ipu-v3/ipu-image-convert.c
+++ b/drivers/gpu/ipu-v3/ipu-image-convert.c
@@ -1358,6 +1358,7 @@ static int convert_start(struct ipu_image_convert_run *run, unsigned int tile)
 			       dest_width,
 			       dest_height,
 			       src_cs, dest_cs,
+			       d_image->base.pix.ycbcr_enc,
 			       rsc);
 	if (ret) {
 		dev_err(priv->ipu->dev, "ipu_ic_task_init failed, %d\n", ret);
diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 3637693c2bc8..c86d5275db46 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -484,7 +484,7 @@ static int prp_setup_rotation(struct prp_priv *priv)
 	ret = ipu_ic_task_init(priv->ic,
 			       infmt->width, infmt->height,
 			       outfmt->height, outfmt->width,
-			       incc->cs, outcc->cs);
+			       incc->cs, outcc->cs, outfmt->ycbcr_enc);
 	if (ret) {
 		v4l2_err(&ic_priv->sd, "ipu_ic_task_init failed, %d\n", ret);
 		goto free_rot1;
@@ -587,7 +587,7 @@ static int prp_setup_norotation(struct prp_priv *priv)
 	ret = ipu_ic_task_init(priv->ic,
 			       infmt->width, infmt->height,
 			       outfmt->width, outfmt->height,
-			       incc->cs, outcc->cs);
+			       incc->cs, outcc->cs, outfmt->ycbcr_enc);
 	if (ret) {
 		v4l2_err(&ic_priv->sd, "ipu_ic_task_init failed, %d\n", ret);
 		return ret;
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index c887f4bee5f8..b19d1e23eece 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -391,15 +391,18 @@ int ipu_ic_task_init(struct ipu_ic *ic,
 		     int in_width, int in_height,
 		     int out_width, int out_height,
 		     enum ipu_color_space in_cs,
-		     enum ipu_color_space out_cs);
+		     enum ipu_color_space out_cs,
+		     enum v4l2_ycbcr_encoding encoding);
 int ipu_ic_task_init_rsc(struct ipu_ic *ic,
 			 int in_width, int in_height,
 			 int out_width, int out_height,
 			 enum ipu_color_space in_cs,
 			 enum ipu_color_space out_cs,
+			 enum v4l2_ycbcr_encoding encoding,
 			 u32 rsc);
 int ipu_ic_task_graphics_init(struct ipu_ic *ic,
 			      enum ipu_color_space in_g_cs,
+			      enum v4l2_ycbcr_encoding encoding,
 			      bool galpha_en, u32 galpha,
 			      bool colorkey_en, u32 colorkey);
 void ipu_ic_task_enable(struct ipu_ic *ic);
-- 
2.17.1

[PATCH 3/3] media: imx: Allow BT.709 encoding for IC routes

[Steve Longerbeam]

The IC now supports BT.709 Y'CbCr encoding, in addition to existing BT.601
encoding, so allow both, for pipelines that route through the IC.

Reported-by: Tim Harvey <tharvey@gateworks.com>
Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
 drivers/staging/media/imx/imx-media-utils.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 5f110d90a4ef..3512f09fb226 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -571,7 +571,9 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
 		tryfmt->quantization = is_rgb ?
 			V4L2_QUANTIZATION_FULL_RANGE :
 			V4L2_QUANTIZATION_LIM_RANGE;
-		tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
+		if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 &&
+		    tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709)
+			tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
 	}
 }
 EXPORT_SYMBOL_GPL(imx_media_fill_default_mbus_fields);
-- 
2.17.1

Re: [PATCH 3/3] media: imx: Allow BT.709 encoding for IC routes

[Steve Longerbeam]

Sorry this patch isn't working, it's not possible to set BT.709 
encoding, working on a fix for v2.

Steve


On 2/3/19 11:47 AM, Steve Longerbeam wrote:
> The IC now supports BT.709 Y'CbCr encoding, in addition to existing BT.601
> encoding, so allow both, for pipelines that route through the IC.
>
> Reported-by: Tim Harvey <tharvey@gateworks.com>
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
>   drivers/staging/media/imx/imx-media-utils.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 5f110d90a4ef..3512f09fb226 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -571,7 +571,9 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
>   		tryfmt->quantization = is_rgb ?
>   			V4L2_QUANTIZATION_FULL_RANGE :
>   			V4L2_QUANTIZATION_LIM_RANGE;
> -		tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +		if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 &&
> +		    tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709)
> +			tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
>   	}
>   }
>   EXPORT_SYMBOL_GPL(imx_media_fill_default_mbus_fields);

Re: [PATCH 2/3] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding

[Tim Harvey]

On Sun, Feb 3, 2019 at 11:48 AM Steve Longerbeam <slongerbeam@gmail.com> wrote:
>
> Pass v4l2 encoding enum to the ipu_ic task init functions, and add
> support for the BT.709 encoding and inverse encoding matrices.
>
> Reported-by: Tim Harvey <tharvey@gateworks.com>
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
>  drivers/gpu/ipu-v3/ipu-ic.c                 | 67 ++++++++++++++++++---
>  drivers/gpu/ipu-v3/ipu-image-convert.c      |  1 +
>  drivers/staging/media/imx/imx-ic-prpencvf.c |  4 +-
>  include/video/imx-ipu-v3.h                  |  5 +-
>  4 files changed, 67 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
> index 35ae86ff0585..63362b4fff81 100644
> --- a/drivers/gpu/ipu-v3/ipu-ic.c
> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
> @@ -199,6 +199,23 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr_bt601 = {
>         .scale = 1,
>  };
>
> +/*
> + * BT.709 encoding from RGB full range to YUV limited range:
> + *
> + * Y = R *  .2126 + G *  .7152 + B *  .0722;
> + * U = R * -.1146 + G * -.3854 + B *  .5000 + 128.;
> + * V = R *  .5000 + G * -.4542 + B * -.0458 + 128.;
> + */
> +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt709 = {
> +       .coeff = {
> +               { 54, 183, 18 },
> +               { 483, 413, 128 },
> +               { 128, 396, 500 },
> +       },
> +       .offset = { 0, 512, 512 },
> +       .scale = 1,
> +};
> +
>  /* transparent RGB->RGB matrix for graphics combining */
>  static const struct ic_csc_params ic_csc_rgb2rgb = {
>         .coeff = {
> @@ -226,12 +243,31 @@ static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = {
>         .scale = 2,
>  };
>
> +/*
> + * Inverse BT.709 encoding from YUV limited range to RGB full range:
> + *
> + * R = (1. * (Y - 16)) + (1.5748 * (Cr - 128));
> + * G = (1. * (Y - 16)) - (0.1873 * (Cb - 128)) - (0.4681 * (Cr - 128));
> + * B = (1. * (Y - 16)) + (1.8556 * (Cb - 128);
> + */
> +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt709 = {
> +       .coeff = {
> +               { 128, 0, 202 },
> +               { 128, 488, 452 },
> +               { 128, 238, 0 },
> +       },
> +       .offset = { -435, 136, -507 },
> +       .scale = 2,
> +};
> +
>  static int init_csc(struct ipu_ic *ic,
>                     enum ipu_color_space inf,
>                     enum ipu_color_space outf,
> +                   enum v4l2_ycbcr_encoding encoding,
>                     int csc_index)
>  {
>         struct ipu_ic_priv *priv = ic->priv;
> +       const struct ic_csc_params *params_rgb2yuv, *params_yuv2rgb;
>         const struct ic_csc_params *params;
>         u32 __iomem *base;
>         const u16 (*c)[3];
> @@ -241,10 +277,24 @@ static int init_csc(struct ipu_ic *ic,
>         base = (u32 __iomem *)
>                 (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
>
> +       switch (encoding) {
> +       case V4L2_YCBCR_ENC_601:
> +               params_rgb2yuv =  &ic_csc_rgb2ycbcr_bt601;
> +               params_yuv2rgb = &ic_csc_ycbcr2rgb_bt601;
> +               break;
> +       case V4L2_YCBCR_ENC_709:
> +               params_rgb2yuv =  &ic_csc_rgb2ycbcr_bt709;
> +               params_yuv2rgb = &ic_csc_ycbcr2rgb_bt709;
> +               break;
> +       default:
> +               dev_err(priv->ipu->dev, "Unsupported YCbCr encoding\n");
> +               return -EINVAL;
> +       }
> +

Steve,

This will fail for RGB to RGB with 'Unsupported YCbCr encoding. We
need to account for the RGB->RGB case.

How about something like:

 static int init_csc(struct ipu_ic *ic,
                    enum ipu_color_space inf,
                    enum ipu_color_space outf,
+                   enum v4l2_ycbcr_encoding encoding,
                    int csc_index)
 {
        struct ipu_ic_priv *priv = ic->priv;
-       const struct ic_csc_params *params;
+       const struct ic_csc_params *params = NULL;
        u32 __iomem *base;
        const u16 (*c)[3];
        const u16 *a;
@@ -241,13 +276,18 @@ static int init_csc(struct ipu_ic *ic,
        base = (u32 __iomem *)
                (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);

-       if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB)
-               params = &ic_csc_ycbcr2rgb_bt601;
-       else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV)
-               params = &ic_csc_rgb2ycbcr_bt601;
+       if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB) {
+               params = (encoding == V4L2_YCBCR_ENC_601) ?
+                       &ic_csc_ycbcr2rgb_bt601 : &ic_csc_ycbcr2rgb_bt709;
+       }
+       else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV) {
+               params = (encoding == V4L2_YCBCR_ENC_601) ?
+                       &ic_csc_rgb2ycbcr_bt601 : &ic_csc_rgb2ycbcr_bt709;
+       }
        else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB)
                params = &ic_csc_rgb2rgb;
-       else {
+
+       if (!params) {
                dev_err(priv->ipu->dev, "Unsupported color space conversion\n");
                return -EINVAL;
        }

Tim

Re: [PATCH 2/3] gpu: ipu-v3: ipu-ic: Add support for BT.709 encoding

[Steve Longerbeam]

On 2/8/19 8:24 AM, Tim Harvey wrote:
> On Sun, Feb 3, 2019 at 11:48 AM Steve Longerbeam <slongerbeam@gmail.com> wrote:
>> Pass v4l2 encoding enum to the ipu_ic task init functions, and add
>> support for the BT.709 encoding and inverse encoding matrices.
>>
>> Reported-by: Tim Harvey <tharvey@gateworks.com>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> ---
>>   drivers/gpu/ipu-v3/ipu-ic.c                 | 67 ++++++++++++++++++---
>>   drivers/gpu/ipu-v3/ipu-image-convert.c      |  1 +
>>   drivers/staging/media/imx/imx-ic-prpencvf.c |  4 +-
>>   include/video/imx-ipu-v3.h                  |  5 +-
>>   4 files changed, 67 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
>> index 35ae86ff0585..63362b4fff81 100644
>> --- a/drivers/gpu/ipu-v3/ipu-ic.c
>> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
>> @@ -199,6 +199,23 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr_bt601 = {
>>          .scale = 1,
>>   };
>>
>> +/*
>> + * BT.709 encoding from RGB full range to YUV limited range:
>> + *
>> + * Y = R *  .2126 + G *  .7152 + B *  .0722;
>> + * U = R * -.1146 + G * -.3854 + B *  .5000 + 128.;
>> + * V = R *  .5000 + G * -.4542 + B * -.0458 + 128.;
>> + */
>> +static const struct ic_csc_params ic_csc_rgb2ycbcr_bt709 = {
>> +       .coeff = {
>> +               { 54, 183, 18 },
>> +               { 483, 413, 128 },
>> +               { 128, 396, 500 },
>> +       },
>> +       .offset = { 0, 512, 512 },
>> +       .scale = 1,
>> +};
>> +
>>   /* transparent RGB->RGB matrix for graphics combining */
>>   static const struct ic_csc_params ic_csc_rgb2rgb = {
>>          .coeff = {
>> @@ -226,12 +243,31 @@ static const struct ic_csc_params ic_csc_ycbcr2rgb_bt601 = {
>>          .scale = 2,
>>   };
>>
>> +/*
>> + * Inverse BT.709 encoding from YUV limited range to RGB full range:
>> + *
>> + * R = (1. * (Y - 16)) + (1.5748 * (Cr - 128));
>> + * G = (1. * (Y - 16)) - (0.1873 * (Cb - 128)) - (0.4681 * (Cr - 128));
>> + * B = (1. * (Y - 16)) + (1.8556 * (Cb - 128);
>> + */
>> +static const struct ic_csc_params ic_csc_ycbcr2rgb_bt709 = {
>> +       .coeff = {
>> +               { 128, 0, 202 },
>> +               { 128, 488, 452 },
>> +               { 128, 238, 0 },
>> +       },
>> +       .offset = { -435, 136, -507 },
>> +       .scale = 2,
>> +};
>> +
>>   static int init_csc(struct ipu_ic *ic,
>>                      enum ipu_color_space inf,
>>                      enum ipu_color_space outf,
>> +                   enum v4l2_ycbcr_encoding encoding,
>>                      int csc_index)
>>   {
>>          struct ipu_ic_priv *priv = ic->priv;
>> +       const struct ic_csc_params *params_rgb2yuv, *params_yuv2rgb;
>>          const struct ic_csc_params *params;
>>          u32 __iomem *base;
>>          const u16 (*c)[3];
>> @@ -241,10 +277,24 @@ static int init_csc(struct ipu_ic *ic,
>>          base = (u32 __iomem *)
>>                  (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
>>
>> +       switch (encoding) {
>> +       case V4L2_YCBCR_ENC_601:
>> +               params_rgb2yuv =  &ic_csc_rgb2ycbcr_bt601;
>> +               params_yuv2rgb = &ic_csc_ycbcr2rgb_bt601;
>> +               break;
>> +       case V4L2_YCBCR_ENC_709:
>> +               params_rgb2yuv =  &ic_csc_rgb2ycbcr_bt709;
>> +               params_yuv2rgb = &ic_csc_ycbcr2rgb_bt709;
>> +               break;
>> +       default:
>> +               dev_err(priv->ipu->dev, "Unsupported YCbCr encoding\n");
>> +               return -EINVAL;
>> +       }
>> +
> Steve,
>
> This will fail for RGB to RGB with 'Unsupported YCbCr encoding. We
> need to account for the RGB->RGB case.
>
> How about something like:


Thanks for reporting Tim

I rather keep the check for supported encoding, and instead get rid of 
"Unsupported color space conversion" error, because that is the YUV->YUV 
case which can be allowed using the identity matrix.

Steve

>
>   static int init_csc(struct ipu_ic *ic,
>                      enum ipu_color_space inf,
>                      enum ipu_color_space outf,
> +                   enum v4l2_ycbcr_encoding encoding,
>                      int csc_index)
>   {
>          struct ipu_ic_priv *priv = ic->priv;
> -       const struct ic_csc_params *params;
> +       const struct ic_csc_params *params = NULL;
>          u32 __iomem *base;
>          const u16 (*c)[3];
>          const u16 *a;
> @@ -241,13 +276,18 @@ static int init_csc(struct ipu_ic *ic,
>          base = (u32 __iomem *)
>                  (priv->tpmem_base + ic->reg->tpmem_csc[csc_index]);
>
> -       if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB)
> -               params = &ic_csc_ycbcr2rgb_bt601;
> -       else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV)
> -               params = &ic_csc_rgb2ycbcr_bt601;
> +       if (inf == IPUV3_COLORSPACE_YUV && outf == IPUV3_COLORSPACE_RGB) {
> +               params = (encoding == V4L2_YCBCR_ENC_601) ?
> +                       &ic_csc_ycbcr2rgb_bt601 : &ic_csc_ycbcr2rgb_bt709;
> +       }
> +       else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV) {
> +               params = (encoding == V4L2_YCBCR_ENC_601) ?
> +                       &ic_csc_rgb2ycbcr_bt601 : &ic_csc_rgb2ycbcr_bt709;
> +       }
>          else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB)
>                  params = &ic_csc_rgb2rgb;
> -       else {
> +
> +       if (!params) {
>                  dev_err(priv->ipu->dev, "Unsupported color space conversion\n");
>                  return -EINVAL;
>          }
>
> Tim