diff mbox series

[v2,3/7] drm/i915: Fix CHV CGM CSC coefficient sign handling

Message ID 20230413164916.4221-4-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: CTM stuff mostly | expand

Commit Message

Ville Syrjälä April 13, 2023, 4:49 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The CHV CGM CSC coefficients are in s4.12 two's complement
format. Fix the CTM->CGM conversion to handle that correctly
instead of pretending that the hw coefficients are also
in some sign-magnitude format.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_color.c | 46 ++++++++++++++--------
 1 file changed, 29 insertions(+), 17 deletions(-)

Comments

Shankar, Uma May 25, 2023, 8:55 p.m. UTC | #1 
> -----Original Message-----
> From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> Sent: Thursday, April 13, 2023 10:19 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Subject: [PATCH v2 3/7] drm/i915: Fix CHV CGM CSC coefficient sign handling
> 
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The CHV CGM CSC coefficients are in s4.12 two's complement format. Fix the CTM-
> >CGM conversion to handle that correctly instead of pretending that the hw
> coefficients are also in some sign-magnitude format.

Spec is slightly confusing when it says:
"CGM CSC :  Input pixels to the CGM CSC are 14 bits. (u.14 format). Coefficients are 16 bits (s3.12)."
Also here:
"Programmable parameters : 
c0[15 :0], c1[15 :0], c2[15 :0], c3[15 :0], c4[15 :0], c5[15 :0], c6[15 :0], c7[15 :0], c8[15 :0] ; // signed matrix coefficients  (s3.12)"

But the coefficients are 16bits, can you help understand how were you able to crack this
Shankar, Uma May 25, 2023, 9:27 p.m. UTC | #2 
> -----Original Message-----
> From: Shankar, Uma
> Sent: Friday, May 26, 2023 2:25 AM
> To: Ville Syrjala <ville.syrjala@linux.intel.com>; intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Subject: RE: [PATCH v2 3/7] drm/i915: Fix CHV CGM CSC coefficient sign handling
> 
> 
> 
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of
> > Ville Syrjala
> > Sent: Thursday, April 13, 2023 10:19 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org
> > Subject: [PATCH v2 3/7] drm/i915: Fix CHV CGM CSC coefficient sign
> > handling
> >
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > The CHV CGM CSC coefficients are in s4.12 two's complement format. Fix
> > the CTM-
> > >CGM conversion to handle that correctly instead of pretending that
> > >the hw
> > coefficients are also in some sign-magnitude format.
> 
> Spec is slightly confusing when it says:
> "CGM CSC :  Input pixels to the CGM CSC are 14 bits. (u.14 format). Coefficients are
> 16 bits (s3.12)."
> Also here:
> "Programmable parameters :
> c0[15 :0], c1[15 :0], c2[15 :0], c3[15 :0], c4[15 :0], c5[15 :0], c6[15 :0], c7[15 :0],
> c8[15 :0] ; // signed matrix coefficients  (s3.12)"
> 
> But the coefficients are 16bits, can you help understand how were you able to crack
> this
Ville Syrjälä May 26, 2023, 1:48 p.m. UTC | #3 
On Thu, May 25, 2023 at 08:55:08PM +0000, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Ville Syrjala
> > Sent: Thursday, April 13, 2023 10:19 PM
> > To: intel-gfx@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org
> > Subject: [PATCH v2 3/7] drm/i915: Fix CHV CGM CSC coefficient sign handling
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The CHV CGM CSC coefficients are in s4.12 two's complement format. Fix the CTM-
> > >CGM conversion to handle that correctly instead of pretending that the hw
> > coefficients are also in some sign-magnitude format.
> 
> Spec is slightly confusing when it says:
> "CGM CSC :  Input pixels to the CGM CSC are 14 bits. (u.14 format). Coefficients are 16 bits (s3.12)."
> Also here:
> "Programmable parameters : 
> c0[15 :0], c1[15 :0], c2[15 :0], c3[15 :0], c4[15 :0], c5[15 :0], c6[15 :0], c7[15 :0], c8[15 :0] ; // signed matrix coefficients  (s3.12)"

Yeah, the spec just uses a weird notation where it doesn't count the msb
in the bits.

> 
> But the coefficients are 16bits, can you help understand how were you able to crack this
Shankar, Uma May 29, 2023, 5:13 a.m. UTC | #4 
> -----Original Message-----
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Sent: Friday, May 26, 2023 7:18 PM
> To: Shankar, Uma <uma.shankar@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH v2 3/7] drm/i915: Fix CHV CGM CSC coefficient sign handling
> 
> On Thu, May 25, 2023 at 08:55:08PM +0000, Shankar, Uma wrote:
> >
> >
> > > -----Original Message-----
> > > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf
> > > Of Ville Syrjala
> > > Sent: Thursday, April 13, 2023 10:19 PM
> > > To: intel-gfx@lists.freedesktop.org
> > > Cc: dri-devel@lists.freedesktop.org
> > > Subject: [PATCH v2 3/7] drm/i915: Fix CHV CGM CSC coefficient sign
> > > handling
> > >
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > The CHV CGM CSC coefficients are in s4.12 two's complement format.
> > > Fix the CTM-
> > > >CGM conversion to handle that correctly instead of pretending that
> > > >the hw
> > > coefficients are also in some sign-magnitude format.
> >
> > Spec is slightly confusing when it says:
> > "CGM CSC :  Input pixels to the CGM CSC are 14 bits. (u.14 format). Coefficients are
> 16 bits (s3.12)."
> > Also here:
> > "Programmable parameters :
> > c0[15 :0], c1[15 :0], c2[15 :0], c3[15 :0], c4[15 :0], c5[15 :0], c6[15 :0], c7[15 :0],
> c8[15 :0] ; // signed matrix coefficients  (s3.12)"
> 
> Yeah, the spec just uses a weird notation where it doesn't count the msb in the bits.
> 
> >
> > But the coefficients are 16bits, can you help understand how were you
> > able to crack this
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_color.c b/drivers/gpu/drm/i915/display/intel_color.c
index 4fc16cac052d..63141f4ed372 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -568,29 +568,41 @@  static void icl_load_csc_matrix(const struct intel_crtc_state *crtc_state)
 		icl_update_output_csc(crtc, &crtc_state->output_csc);
 }
 
+static u16 ctm_to_twos_complement(u64 coeff, int int_bits, int frac_bits)
+{
+	s64 c = CTM_COEFF_ABS(coeff);
+
+	/* leave an extra bit for rounding */
+	c >>= 32 - frac_bits - 1;
+
+	/* round and drop the extra bit */
+	c = (c + 1) >> 1;
+
+	if (CTM_COEFF_NEGATIVE(coeff))
+		c = -c;
+
+	c = clamp(c, -(s64)BIT(int_bits + frac_bits - 1),
+		  (s64)(BIT(int_bits + frac_bits - 1) - 1));
+
+	return c & (BIT(int_bits + frac_bits) - 1);
+}
+
+/*
+ * CHV Color Gamut Mapping (CGM) CSC
+ * |r|   | c0 c1 c2 |   |r|
+ * |g| = | c3 c4 c5 | x |g|
+ * |b|   | c6 c7 c8 |   |b|
+ *
+ * Coefficients are two's complement s4.12.
+ */
 static void chv_cgm_csc_convert_ctm(const struct intel_crtc_state *crtc_state,
 				    struct intel_csc_matrix *csc)
 {
 	const struct drm_color_ctm *ctm = crtc_state->hw.ctm->data;
 	int i;
 
-	for (i = 0; i < 9; i++) {
-		u64 abs_coeff = ((1ULL << 63) - 1) & ctm->matrix[i];
-
-		/* Round coefficient. */
-		abs_coeff += 1 << (32 - 13);
-		/* Clamp to hardware limits. */
-		abs_coeff = clamp_val(abs_coeff, 0, CTM_COEFF_8_0 - 1);
-
-		csc->coeff[i] = 0;
-
-		/* Write coefficients in S3.12 format. */
-		if (ctm->matrix[i] & (1ULL << 63))
-			csc->coeff[i] |= 1 << 15;
-
-		csc->coeff[i] |= ((abs_coeff >> 32) & 7) << 12;
-		csc->coeff[i] |= (abs_coeff >> 20) & 0xfff;
-	}
+	for (i = 0; i < 9; i++)
+		csc->coeff[i] = ctm_to_twos_complement(ctm->matrix[i], 4, 12);
 }
 
 static void chv_load_cgm_csc(struct intel_crtc *crtc,