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

[PATCH v4 1/4] 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. The ic_csc_rgb2rgb matrix is just an identity
matrix, so rename to ic_csc_identity. No functional changes.

Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
---
Changes in v2:
- rename ic_csc_rgb2rgb matrix to ic_csc_identity.
---
 drivers/gpu/ipu-v3/ipu-ic.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
index 594c3cbc8291..3ef61f0b509b 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 },
@@ -197,8 +199,11 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr = {
 	.scale = 1,
 };
 
-/* transparent RGB->RGB matrix for graphics combining */
-static const struct ic_csc_params ic_csc_rgb2rgb = {
+/*
+ * identity matrix, used for transparent RGB->RGB graphics
+ * combining.
+ */
+static const struct ic_csc_params ic_csc_identity = {
 	.coeff = {
 		{ 128, 0, 0 },
 		{ 0, 128, 0 },
@@ -208,11 +213,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,11 +245,11 @@ 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;
+		params = &ic_csc_identity;
 	else {
 		dev_err(priv->ipu->dev, "Unsupported color space conversion\n");
 		return -EINVAL;
-- 
2.17.1

Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices

[Philipp Zabel]

On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote:
> 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. The ic_csc_rgb2rgb matrix is just an identity
> matrix, so rename to ic_csc_identity. No functional changes.
> 
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> ---
> Changes in v2:
> - rename ic_csc_rgb2rgb matrix to ic_csc_identity.
> ---
>  drivers/gpu/ipu-v3/ipu-ic.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
> index 594c3cbc8291..3ef61f0b509b 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.;

Hm, this is a conversion to full range BT.601. For limited range, the
matrix coefficients

   0.2990  0.5870  0.1140
  -0.1687 -0.3313  0.5000
   0.5000 -0.4187 -0.0813

should be multiplied with 219/255 (Y) and 224/255 (U,V), respectively:

  Y = R *  .2568 + G *  .5041 + B *  .0979 + 16;
  U = R * -.1482 + G * -.2910 + B *  .4392 + 128;
  V = R *  .4392 + G * -.3678 + B * -.0714 + 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 },
> @@ -197,8 +199,11 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr = {
>  	.scale = 1,
>  };
>  
> -/* transparent RGB->RGB matrix for graphics combining */
> -static const struct ic_csc_params ic_csc_rgb2rgb = {
> +/*
> + * identity matrix, used for transparent RGB->RGB graphics
> + * combining.
> + */
> +static const struct ic_csc_params ic_csc_identity = {
>  	.coeff = {
>  		{ 128, 0, 0 },
>  		{ 0, 128, 0 },
> @@ -208,11 +213,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);
>   */

This looks correct.

> -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,11 +245,11 @@ 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;
> +		params = &ic_csc_identity;
>  	else {
>  		dev_err(priv->ipu->dev, "Unsupported color space conversion\n");
>  		return -EINVAL;

regards
Philipp

Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices

[Steve Longerbeam]

Hi Philipp,

On 2/11/19 1:58 AM, Philipp Zabel wrote:
> On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote:
>> 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. The ic_csc_rgb2rgb matrix is just an identity
>> matrix, so rename to ic_csc_identity. No functional changes.
>>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> ---
>> Changes in v2:
>> - rename ic_csc_rgb2rgb matrix to ic_csc_identity.
>> ---
>>   drivers/gpu/ipu-v3/ipu-ic.c | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
>> index 594c3cbc8291..3ef61f0b509b 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.;
> Hm, this is a conversion to full range BT.601. For limited range, the
> matrix coefficients
>
>     0.2990  0.5870  0.1140
>    -0.1687 -0.3313  0.5000
>     0.5000 -0.4187 -0.0813
>
> should be multiplied with 219/255 (Y) and 224/255 (U,V), respectively:
>
>    Y = R *  .2568 + G *  .5041 + B *  .0979 + 16;
>    U = R * -.1482 + G * -.2910 + B *  .4392 + 128;
>    V = R *  .4392 + G * -.3678 + B * -.0714 + 128;

Looking more closely at these coefficients now, I see you are right, 
they are the BT.601 YUV full-range coefficients (Y range 0 to 1, U and V 
range -0.5 to 0.5). Well, not even that -- the coefficients are not 
being scaled to the limited ranges, but the 0.5 offset (128) _is_ being 
added to U/V, but no offset for Y. So it is even more messed up.

Your corrected coefficients and offsets look correct to me: Y 
coefficients scaled to (235 - 16) / 255 and U/V coefficients scaled to 
(240 - 16)  / 255, and add the offsets for both Y and U/V.

But what about this "SAT_MODE" field in the IC task parameter memory? 
According to the manual the hardware will automatically convert the 
written coefficients to the correct limited ranges. I see there is a 
"sat" field defined in the struct but is not being set in the tables.

So what should we do, define the full range coefficients, and make use 
of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and 
not use SAT_MODE? I'm inclined to do the former.

Steve



>
>>    */
>> -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 },
>> @@ -197,8 +199,11 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr = {
>>   	.scale = 1,
>>   };
>>   
>> -/* transparent RGB->RGB matrix for graphics combining */
>> -static const struct ic_csc_params ic_csc_rgb2rgb = {
>> +/*
>> + * identity matrix, used for transparent RGB->RGB graphics
>> + * combining.
>> + */
>> +static const struct ic_csc_params ic_csc_identity = {
>>   	.coeff = {
>>   		{ 128, 0, 0 },
>>   		{ 0, 128, 0 },
>> @@ -208,11 +213,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);
>>    */
> This looks correct.
>
>> -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,11 +245,11 @@ 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;
>> +		params = &ic_csc_identity;
>>   	else {
>>   		dev_err(priv->ipu->dev, "Unsupported color space conversion\n");
>>   		return -EINVAL;
> regards
> Philipp

Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices

[Philipp Zabel]

Hi Steve,

On Mon, 2019-02-11 at 10:24 -0800, Steve Longerbeam wrote:
[...]
> Looking more closely at these coefficients now, I see you are right, 
> they are the BT.601 YUV full-range coefficients (Y range 0 to 1, U and V 
> range -0.5 to 0.5). Well, not even that -- the coefficients are not 
> being scaled to the limited ranges, but the 0.5 offset (128) _is_ being 
> added to U/V, but no offset for Y. So it is even more messed up.
>
> Your corrected coefficients and offsets look correct to me: Y 
> coefficients scaled to (235 - 16) / 255 and U/V coefficients scaled to 
> (240 - 16)  / 255, and add the offsets for both Y and U/V.
> 
> But what about this "SAT_MODE" field in the IC task parameter memory? 

That just controls the saturation. The result after the matrix
multiplication is either saturated to [0..255] or to [16..235]/[16..240]
when converting from the internal representation to the 8 bit output.

> According to the manual the hardware will automatically convert the 
> written coefficients to the correct limited ranges.

Where did you get that from? "The final calculation result is limited
according to the SAT_MODE parameter and rounded to 8 bits." I see no
mention of coefficients being modified.

> I see there is a "sat" field defined in the struct but is not being
> set in the tables.
> 
> So what should we do, define the full range coefficients, and make use 
> of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and 
> not use SAT_MODE? I'm inclined to do the former.

SAT_MODE should be set for conversions to YUV limited range so that the
coefficients can be rounded to the closest value. Otherwise we'd have to
round towards zero, possibly with a larger error, to make sure the
results are inside the valid ranges.

regards
Philipp

Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices

[Steve Longerbeam]

On 2/12/19 2:17 AM, Philipp Zabel wrote:
> Hi Steve,
>
> On Mon, 2019-02-11 at 10:24 -0800, Steve Longerbeam wrote:
> [...]
>> Looking more closely at these coefficients now, I see you are right,
>> they are the BT.601 YUV full-range coefficients (Y range 0 to 1, U and V
>> range -0.5 to 0.5). Well, not even that -- the coefficients are not
>> being scaled to the limited ranges, but the 0.5 offset (128) _is_ being
>> added to U/V, but no offset for Y. So it is even more messed up.
>>
>> Your corrected coefficients and offsets look correct to me: Y
>> coefficients scaled to (235 - 16) / 255 and U/V coefficients scaled to
>> (240 - 16)  / 255, and add the offsets for both Y and U/V.
>>
>> But what about this "SAT_MODE" field in the IC task parameter memory?
> That just controls the saturation. The result after the matrix
> multiplication is either saturated to [0..255] or to [16..235]/[16..240]
> when converting from the internal representation to the 8 bit output.

By saturation I think you mean clipped to those ranges?

>
>> According to the manual the hardware will automatically convert the
>> written coefficients to the correct limited ranges.
> Where did you get that from? "The final calculation result is limited
> according to the SAT_MODE parameter and rounded to 8 bits." I see no
> mention of coefficients being modified.

Well, as is often the case with this manual, I was interpreting based on 
poorly written information. By "final calculation result is limited 
according to the SAT_MODE parameter" I interpreted that to mean the 
hardware enables scaling from full range to limited range. But I concede 
that it more likely means it clips the output to those ranges.

>
>> I see there is a "sat" field defined in the struct but is not being
>> set in the tables.
>>
>> So what should we do, define the full range coefficients, and make use
>> of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and
>> not use SAT_MODE? I'm inclined to do the former.
> SAT_MODE should be set for conversions to YUV limited range so that the
> coefficients can be rounded to the closest value.

Well, we have already rounded the coefficients to the nearest int in the 
tables. Do you mean the final result (coeff * color component + offset) 
is rounded?

>   Otherwise we'd have to
> round towards zero, possibly with a larger error, to make sure the
> results are inside the valid ranges.

Makes sense, I will turn on that bit for limited range YUV output for 
v5, so that the final color component result is clipped to limited range 
and rounded.


Steve

Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices

[Philipp Zabel]

On Tue, 2019-02-12 at 09:42 -0800, Steve Longerbeam wrote:
[...]
> > > But what about this "SAT_MODE" field in the IC task parameter memory?
> > 
> > That just controls the saturation. The result after the matrix
> > multiplication is either saturated to [0..255] or to [16..235]/[16..240]
> > when converting from the internal representation to the 8 bit output.
> 
> By saturation I think you mean clipped to those ranges?

Yes, thanks. I didn't realize it sounds weird to use saturated this way.
See: https://en.wikipedia.org/wiki/Saturation_arithmetic

> > > According to the manual the hardware will automatically convert the
> > > written coefficients to the correct limited ranges.
> > 
> > Where did you get that from? "The final calculation result is limited
> > according to the SAT_MODE parameter and rounded to 8 bits." I see no
> > mention of coefficients being modified.
> 
> Well, as is often the case with this manual, I was interpreting based on 
> poorly written information. By "final calculation result is limited 
> according to the SAT_MODE parameter" I interpreted that to mean the 
> hardware enables scaling from full range to limited range. But I concede 
> that it more likely means it clips the output to those ranges.

Ok, with this manual I'm never sure that there aren't some conflicting
statements somewhere else that I might have overlooked. Good to see that
we are at least basing our interpretations on the same text.

> > > I see there is a "sat" field defined in the struct but is not being
> > > set in the tables.
> > > 
> > > So what should we do, define the full range coefficients, and make use
> > > of SAT_MODE h/w feature, or scale/offset the coefficients ourselves and
> > > not use SAT_MODE? I'm inclined to do the former.
> > 
> > SAT_MODE should be set for conversions to YUV limited range so that the
> > coefficients can be rounded to the closest value.
> 
> Well, we have already rounded the coefficients to the nearest int in the 
> tables. Do you mean the final result (coeff * color component + offset) 
> is rounded?

The manual says so: "The final calculation result is limited according
to the SAT_MODE parameter and rounded to 8 bits", but that's not what I
meant. Still, I might have been mistaken.

I think due to the fact that the coefficients are multiplied by up to
255 (max pixel value) and then effectively divided by 256 when
converting to 8 bit, the only way to overflow limited range is if two
coefficients are rounded away from zero in the calculation of a single
component. This doesn't seem to happen in practice.

A constructed example, conversion to YUV limited range with carefully
chosen coefficients.

  Y = R * .1817 + G * .6153 + B * .0618 + 16;

Note that .1817 + .6153 + .0618 < 219/255.
With rounded coefficients though:

  Y = (R * 47 + G * 158 + B * 16 + (64 << 6)) / 256 = 236.136

Now 47 + 158 + 16 > 219, and the result is out of range.

regards
Philipp

Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices

[Steve Longerbeam]

On 2/13/19 2:35 AM, Philipp Zabel wrote:
> On Tue, 2019-02-12 at 09:42 -0800, Steve Longerbeam wrote:
> [...]
>>>> But what about this "SAT_MODE" field in the IC task parameter memory?
>>> That just controls the saturation. The result after the matrix
>>> multiplication is either saturated to [0..255] or to [16..235]/[16..240]
>>> when converting from the internal representation to the 8 bit output.
>> By saturation I think you mean clipped to those ranges?
> Yes, thanks. I didn't realize it sounds weird to use saturated this way.
> See:https://en.wikipedia.org/wiki/Saturation_arithmetic

Ok, saturation can mean the same thing as clipping/clamping. Thanks for 
the article.

I tested a RGB->YUV pipeline with the .sat bit set in the BT.601 rgb2yuv 
table, with the following pipeline on the SabreSD:

'ov5640 1-003c':0
         [fmt:RGB565_2X8_LE/1024x768@1/30 field:none colorspace:srgb 
xfer:srgb ycbcr:601 quantization:full-range]

'imx6-mipi-csi2':0
         [fmt:RGB565_2X8_LE/1024x768 field:none colorspace:srgb 
xfer:srgb ycbcr:601 quantization:full-range]

'imx6-mipi-csi2':2
         [fmt:RGB565_2X8_LE/1024x768 field:none colorspace:srgb 
xfer:srgb ycbcr:601 quantization:full-range]

'ipu1_csi1':0
         [fmt:RGB565_2X8_LE/1024x768@1/30 field:none colorspace:srgb 
xfer:srgb ycbcr:601 quantization:full-range
          crop.bounds:(0,0)/1024x768
          crop:(0,0)/1024x768
          compose.bounds:(0,0)/1024x768
          compose:(0,0)/1024x768]

'ipu1_csi1':1
         [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb 
xfer:srgb ycbcr:601 quantization:full-range]

'ipu1_ic_prp':0
         [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb 
xfer:srgb ycbcr:601 quantization:full-range]

'ipu1_ic_prp':1
         [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb 
xfer:srgb ycbcr:601 quantization:full-range]

'ipu1_ic_prpenc':0
         [fmt:ARGB8888_1X32/1024x768@1/30 field:none colorspace:srgb 
xfer:srgb ycbcr:601 quantization:full-range]

'ipu1_ic_prpenc':1
         [fmt:AYUV8_1X32/800x600@1/30 field:none colorspace:srgb 
xfer:srgb ycbcr:601 quantization:lim-range]

/dev/video0:0
Format Video Capture:
     Width/Height      : 800/600
     Pixel Format      : 'YV12'
     Field             : None
     Bytes per Line    : 800
     Size Image        : 720000
     Colorspace        : sRGB
     Transfer Function : sRGB
     YCbCr/HSV Encoding: ITU-R 601
     Quantization      : Limited Range
     Flags             :


The result being that the captured image colors are all off (there's a 
bright pink shade to the images). But I discovered the init_csc() 
function was not setting the saturation bit at the correct bit offset 
within the TPMEM. The saturation bit is bit 42, or bit 10 of the second 
32-bit word. But the code was writing to bit 9 of the second word. After 
correcting this, saturation is working fine. I have added another patch 
that fixes this for v5 series.


>
>>> SAT_MODE should be set for conversions to YUV limited range so that the
>>> coefficients can be rounded to the closest value.
>> Well, we have already rounded the coefficients to the nearest int in the
>> tables. Do you mean the final result (coeff * color component + offset)
>> is rounded?
> The manual says so: "The final calculation result is limited according
> to the SAT_MODE parameter and rounded to 8 bits", but that's not what I
> meant. Still, I might have been mistaken.
>
> I think due to the fact that the coefficients are multiplied by up to
> 255 (max pixel value) and then effectively divided by 256 when
> converting to 8 bit, the only way to overflow limited range is if two
> coefficients are rounded away from zero in the calculation of a single
> component. This doesn't seem to happen in practice.
>
> A constructed example, conversion to YUV limited range with carefully
> chosen coefficients.
>
>    Y = R * .1817 + G * .6153 + B * .0618 + 16;
>
> Note that .1817 + .6153 + .0618 < 219/255.
> With rounded coefficients though:
>
>    Y = (R * 47 + G * 158 + B * 16 + (64 << 6)) / 256 = 236.136

Yes, for a rec.709 conversion and max/worst-case RGB signal = (255,255,255).

But the rec.709 coefficients for Y are actually

Y = (R * 47 + G * 157 + B * 16 + (16 << 8)) / 256


which for RGB = (255,255,255), Y = 235.14, which doesn't overflow 
limited range.

Steve

Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices

[Steve Longerbeam]

On 2/11/19 1:58 AM, Philipp Zabel wrote:
> On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote:
>> 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. The ic_csc_rgb2rgb matrix is just an identity
>> matrix, so rename to ic_csc_identity. No functional changes.
>>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> ---
>> Changes in v2:
>> - rename ic_csc_rgb2rgb matrix to ic_csc_identity.
>> ---
>>   drivers/gpu/ipu-v3/ipu-ic.c | 21 ++++++++++++++-------
>>   1 file changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
>> index 594c3cbc8291..3ef61f0b509b 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.;
> Hm, this is a conversion to full range BT.601. For limited range, the
> matrix coefficients
>
>     0.2990  0.5870  0.1140
>    -0.1687 -0.3313  0.5000
>     0.5000 -0.4187 -0.0813
>
> should be multiplied with 219/255 (Y) and 224/255 (U,V), respectively:
>
>    Y = R *  .2568 + G *  .5041 + B *  .0979 + 16;
>    U = R * -.1482 + G * -.2910 + B *  .4392 + 128;
>    V = R *  .4392 + G * -.3678 + B * -.0714 + 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 },
>> @@ -197,8 +199,11 @@ static const struct ic_csc_params ic_csc_rgb2ycbcr = {
>>   	.scale = 1,
>>   };
>>   
>> -/* transparent RGB->RGB matrix for graphics combining */
>> -static const struct ic_csc_params ic_csc_rgb2rgb = {
>> +/*
>> + * identity matrix, used for transparent RGB->RGB graphics
>> + * combining.
>> + */
>> +static const struct ic_csc_params ic_csc_identity = {
>>   	.coeff = {
>>   		{ 128, 0, 0 },
>>   		{ 0, 128, 0 },
>> @@ -208,11 +213,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);
>>    */
> This looks correct.

Agreed, the coefficients in the comments are correct, but the actual 
table values were a bit off. I will correct them for v5 (Green offset 
should be 272 in the table, not 266).

Steve

>
>> -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,11 +245,11 @@ 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;
>> +		params = &ic_csc_identity;
>>   	else {
>>   		dev_err(priv->ipu->dev, "Unsupported color space conversion\n");
>>   		return -EINVAL;
> regards
> Philipp

[PATCH v4 2/4] gpu: ipu-v3: ipu-ic: Simplify selection of encoding matrix

[Steve Longerbeam]

Simplify the selection of the Y'CbCr encoding matrices in init_csc().
A side-effect of this change is that init_csc() now allows YUV->YUV
using the identity matrix, intead of returning error.

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

diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
index 3ef61f0b509b..e459615a49a1 100644
--- a/drivers/gpu/ipu-v3/ipu-ic.c
+++ b/drivers/gpu/ipu-v3/ipu-ic.c
@@ -244,16 +244,12 @@ 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)
+	if (inf == outf)
+		params = &ic_csc_identity;
+	else if (inf == IPUV3_COLORSPACE_YUV)
 		params = &ic_csc_ycbcr2rgb_bt601;
-	else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_YUV)
+	else
 		params = &ic_csc_rgb2ycbcr_bt601;
-	else if (inf == IPUV3_COLORSPACE_RGB && outf == IPUV3_COLORSPACE_RGB)
-		params = &ic_csc_identity;
-	else {
-		dev_err(priv->ipu->dev, "Unsupported color space conversion\n");
-		return -EINVAL;
-	}
 
 	/* Cast to unsigned */
 	c = (const u16 (*)[3])params->coeff;
-- 
2.17.1

Re: [PATCH v4 2/4] gpu: ipu-v3: ipu-ic: Simplify selection of encoding matrix

[Philipp Zabel]

On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam wrote:
> Simplify the selection of the Y'CbCr encoding matrices in init_csc().
> A side-effect of this change is that init_csc() now allows YUV->YUV
> using the identity matrix, intead of returning error.
> 
> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>

Note that this only works if both YUV encodings have the same range.

regards
Philipp

[PATCH v4 3/4] 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>
---
Changes in v4:
- fix compile error.
Chnges in v3:
- none.
Changes in v2:
- only return "Unsupported YCbCr encoding" error if inf != outf,
  since if inf == outf, the identity matrix can be used. Reported
  by Tim Harvey.
---
 drivers/gpu/ipu-v3/ipu-ic.c                 | 71 +++++++++++++++++++--
 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, 71 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
index e459615a49a1..c5f83d7e357f 100644
--- a/drivers/gpu/ipu-v3/ipu-ic.c
+++ b/drivers/gpu/ipu-v3/ipu-ic.c
@@ -212,6 +212,23 @@ static const struct ic_csc_params ic_csc_identity = {
 	.scale = 2,
 };
 
+/*
+ * 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,
+};
+
 /*
  * Inverse BT.601 encoding from YUV limited range to RGB full range:
  *
@@ -229,12 +246,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];
@@ -244,12 +280,30 @@ 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:
+		if (inf != outf) {
+			dev_err(priv->ipu->dev,
+				"Unsupported YCbCr encoding\n");
+			return -EINVAL;
+		}
+		break;
+	}
+
 	if (inf == outf)
 		params = &ic_csc_identity;
 	else if (inf == IPUV3_COLORSPACE_YUV)
-		params = &ic_csc_ycbcr2rgb_bt601;
+		params = params_yuv2rgb;
 	else
-		params = &ic_csc_rgb2ycbcr_bt601;
+		params = params_rgb2yuv;
 
 	/* Cast to unsigned */
 	c = (const u16 (*)[3])params->coeff;
@@ -390,6 +444,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)
 {
@@ -408,7 +463,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;
 	}
@@ -416,7 +471,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;
 	}
@@ -450,6 +505,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;
@@ -485,7 +541,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;
 	}
@@ -499,10 +555,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 376b504e8a42..60ecf5809cc1 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

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

[Philipp Zabel]

On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam 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>
> ---
> Changes in v4:
> - fix compile error.
> Chnges in v3:
> - none.
> Changes in v2:
> - only return "Unsupported YCbCr encoding" error if inf != outf,
>   since if inf == outf, the identity matrix can be used. Reported
>   by Tim Harvey.
> ---
>  drivers/gpu/ipu-v3/ipu-ic.c                 | 71 +++++++++++++++++++--
>  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, 71 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
> index e459615a49a1..c5f83d7e357f 100644
> --- a/drivers/gpu/ipu-v3/ipu-ic.c
> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
> @@ -212,6 +212,23 @@ static const struct ic_csc_params ic_csc_identity = {
>  	.scale = 2,
>  };
>  
> +/*
> + * 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.;

This is a conversion to YUV full range. Limited range should be:

Y   R *  .1826 + G *  .6142 + B *  .0620 + 16;
U = R * -.1007 + G * -.3385 + B *  .4392 + 128;
V   R *  .4392 + G * -.3990 + B * -.0402 + 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,
> +};
> +
>  /*
>   * Inverse BT.601 encoding from YUV limited range to RGB full range:
>   *
> @@ -229,12 +246,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);

The coefficients look like full range again, conversion from limited
range YUV should look like:

  R = (1.1644 * (Y - 16)) + (1.7927 * (Cr - 128));
  G = (1.1644 * (Y - 16)) - (0.2132 * (Cb - 128)) - (0.5329 * (Cr - 128));
  B = (1.1644 * (Y - 16)) + (2.1124 * (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,

Should we support YUV BT.601 <-> YUV REC.709 conversions? That would
require separate encodings for input and output. Also, this might be a
good time to think about adding quantization range parameters as well.

regards
Philipp

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

[Steve Longerbeam]

On 2/11/19 2:12 AM, Philipp Zabel wrote:
> On Fri, 2019-02-08 at 17:47 -0800, Steve Longerbeam 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>
>> ---
>> Changes in v4:
>> - fix compile error.
>> Chnges in v3:
>> - none.
>> Changes in v2:
>> - only return "Unsupported YCbCr encoding" error if inf != outf,
>>    since if inf == outf, the identity matrix can be used. Reported
>>    by Tim Harvey.
>> ---
>>   drivers/gpu/ipu-v3/ipu-ic.c                 | 71 +++++++++++++++++++--
>>   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, 71 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/ipu-v3/ipu-ic.c b/drivers/gpu/ipu-v3/ipu-ic.c
>> index e459615a49a1..c5f83d7e357f 100644
>> --- a/drivers/gpu/ipu-v3/ipu-ic.c
>> +++ b/drivers/gpu/ipu-v3/ipu-ic.c
>> @@ -212,6 +212,23 @@ static const struct ic_csc_params ic_csc_identity = {
>>   	.scale = 2,
>>   };
>>   
>> +/*
>> + * 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.;
> This is a conversion to YUV full range. Limited range should be:
>
> Y   R *  .1826 + G *  .6142 + B *  .0620 + 16;
> U = R * -.1007 + G * -.3385 + B *  .4392 + 128;
> V   R *  .4392 + G * -.3990 + B * -.0402 + 128;

Yep, I fixed these to encode to limited range YUV, and ...

>> + */
>> +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,
>> +};
>> +
>>   /*
>>    * Inverse BT.601 encoding from YUV limited range to RGB full range:
>>    *
>> @@ -229,12 +246,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);
> The coefficients look like full range again, conversion from limited
> range YUV should look like:
>
>    R = (1.1644 * (Y - 16)) + (1.7927 * (Cr - 128));
>    G = (1.1644 * (Y - 16)) - (0.2132 * (Cb - 128)) - (0.5329 * (Cr - 128));
>    B = (1.1644 * (Y - 16)) + (2.1124 * (Cb - 128);

fixed these to inverse encode from limited range YUV.

>> + */
>> +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,
> Should we support YUV BT.601 <-> YUV REC.709 conversions? That would
> require separate encodings for input and output.

How about if we pass the input and output encodings to the init ic task 
functions, but for now require they be the same? We can support 
transcoding in a later series.

>   Also, this might be a
> good time to think about adding quantization range parameters as well.

Again, I think for now, just include input/output quantization but 
require full range for RGB and limited range for YUV.

But that really balloons the arguments to ipu_ic_task_init_*(). Should 
we create an ipu_ic_task_init structure?

Steve

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

[Philipp Zabel]

Hi Steve,

On Mon, 2019-02-11 at 17:20 -0800, Steve Longerbeam wrote:
[...]
> > Should we support YUV BT.601 <-> YUV REC.709 conversions? That would
> > require separate encodings for input and output.
> 
> How about if we pass the input and output encodings to the init ic task 
> functions, but for now require they be the same? We can support 
> transcoding in a later series.
[...]
> Again, I think for now, just include input/output quantization but 
> require full range for RGB and limited range for YUV.

Yes, that is fine. I'd just like to avoid unnecessary interface changes
between ipu-v3 and imx-media. So if we have to change it right now, why
not plan ahead.

> But that really balloons the arguments to ipu_ic_task_init_*(). Should 
> we create an ipu_ic_task_init structure?

I wonder if we should just expose struct ic_csc_params and provide a
helper to fill it given colorspace and V4L2 encoding/quantization
parameters. Something like:

	struct ipu_ic_csc_params csc;

	imx_media_init_ic_csc_params(&csc,
			in_cs, in_encoding, in_quantization,
			out_cs, out_encoding, out_quantization);

	ipu_ic_task_init(ic,
			in_width, in_height,
			out_width, out_height, &csc);
	// or
	ipu_ic_task_init_rsc(ic, rsc, &csc);

regards
Philipp

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

[Steve Longerbeam]

On 2/12/19 3:34 AM, Philipp Zabel wrote:
> Hi Steve,
>
> On Mon, 2019-02-11 at 17:20 -0800, Steve Longerbeam wrote:
> [...]
>>> Should we support YUV BT.601 <-> YUV REC.709 conversions? That would
>>> require separate encodings for input and output.
>> How about if we pass the input and output encodings to the init ic task
>> functions, but for now require they be the same? We can support
>> transcoding in a later series.
> [...]
>> Again, I think for now, just include input/output quantization but
>> require full range for RGB and limited range for YUV.
> Yes, that is fine. I'd just like to avoid unnecessary interface changes
> between ipu-v3 and imx-media. So if we have to change it right now, why
> not plan ahead.

Agreed!

>
>> But that really balloons the arguments to ipu_ic_task_init_*(). Should
>> we create an ipu_ic_task_init structure?
> I wonder if we should just expose struct ic_csc_params

I had basically the same idea. I wasn't thinking of creating a helper to 
fill in the params but sure, I'll add that.

Steve


>   and provide a
> helper to fill it given colorspace and V4L2 encoding/quantization
> parameters. Something like:
>
> 	struct ipu_ic_csc_params csc;
>
> 	imx_media_init_ic_csc_params(&csc,
> 			in_cs, in_encoding, in_quantization,
> 			out_cs, out_encoding, out_quantization);
>
> 	ipu_ic_task_init(ic,
> 			in_width, in_height,
> 			out_width, out_height, &csc);
> 	// or
> 	ipu_ic_task_init_rsc(ic, rsc, &csc);
>
> regards
> Philipp

[PATCH v4 4/4] 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>
---
Changes in v2:
- move ic_route check above default colorimetry checks, and fill default
  colorimetry for ic_route, otherwise it's not possible to set BT.709
  encoding for ic routes.
---
 drivers/staging/media/imx/imx-media-utils.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 5f110d90a4ef..dde0e47550d7 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -544,6 +544,19 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
 	if (tryfmt->field == V4L2_FIELD_ANY)
 		tryfmt->field = fmt->field;
 
+	if (ic_route) {
+		if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT)
+			tryfmt->colorspace = fmt->colorspace;
+
+		tryfmt->quantization = is_rgb ?
+			V4L2_QUANTIZATION_FULL_RANGE :
+			V4L2_QUANTIZATION_LIM_RANGE;
+
+		if (tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_601 &&
+		    tryfmt->ycbcr_enc != V4L2_YCBCR_ENC_709)
+			tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
+	}
+
 	/* fill colorimetry if necessary */
 	if (tryfmt->colorspace == V4L2_COLORSPACE_DEFAULT) {
 		tryfmt->colorspace = fmt->colorspace;
@@ -566,13 +579,6 @@ void imx_media_fill_default_mbus_fields(struct v4l2_mbus_framefmt *tryfmt,
 					tryfmt->ycbcr_enc);
 		}
 	}
-
-	if (ic_route) {
-		tryfmt->quantization = is_rgb ?
-			V4L2_QUANTIZATION_FULL_RANGE :
-			V4L2_QUANTIZATION_LIM_RANGE;
-		tryfmt->ycbcr_enc = V4L2_YCBCR_ENC_601;
-	}
 }
 EXPORT_SYMBOL_GPL(imx_media_fill_default_mbus_fields);
 
-- 
2.17.1