lkml.org 
[lkml]   [2019]   [Feb]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices
From
Date
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

\
 
 \ /
  Last update: 2019-02-11 19:25    [W:0.062 / U:0.412 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site