Messages in this thread | | | Subject | Re: [PATCH v4 1/4] gpu: ipu-v3: ipu-ic: Rename yuv2rgb encoding matrices | From | Steve Longerbeam <> | Date | Mon, 11 Feb 2019 10:24:28 -0800 |
| |
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
| |