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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux