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

login
register
mail settings
Submitter Steve Longerbeam
Date Feb. 9, 2019, 1:47 a.m.
Message ID <20190209014748.10427-2-slongerbeam@gmail.com>
Download mbox | patch
Permalink /patch/54416/
State New
Delegated to: Hans Verkuil
Headers show

Comments

Steve Longerbeam - Feb. 9, 2019, 1:47 a.m.
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(-)
Philipp Zabel - Feb. 11, 2019, 9:58 a.m.
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
Steve Longerbeam - Feb. 11, 2019, 6:24 p.m.
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
Steve Longerbeam - Feb. 11, 2019, 11:33 p.m.
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
Philipp Zabel - Feb. 12, 2019, 10:17 a.m.
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
Steve Longerbeam - Feb. 12, 2019, 5:42 p.m.
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
Philipp Zabel - Feb. 13, 2019, 10:35 a.m.
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
Steve Longerbeam - Feb. 14, 2019, 6:54 p.m.
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

Patch

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;

Privacy Policy