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@xxxxxxxxx>
---
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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
