RE: [PATCH v2 3/7] drm/i915: Fix CHV CGM CSC coefficient sign handling
> -Original Message-
> From: Ville Syrjälä
> Sent: Friday, May 26, 2023 7:18 PM
> To: Shankar, Uma
> Cc: intel-...@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 +, Shankar, Uma wrote:
> >
> >
> > > -Original Message-
> > > From: dri-devel On Behalf
> > > Of Ville Syrjala
> > > Sent: Thursday, April 13, 2023 10:19 PM
> > > To: intel-...@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ä
> > >
> > > 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
>
> The 16bit coefficient already made me suspect they screwed up the notation.
> Testing specific values on real hardware confirmed that.
Got it, thanks Ville for the clarification.
> >
> > > Signed-off-by: Ville Syrjälä
> > > ---
> > > drivers/gpu/drm/i915/display/intel_color.c | 46
> > > ++
> > > 1 file changed, 29 insertions(+), 17 deletions(-)
> > >
> > > 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, _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,
> > > --
> > > 2.39.2
> >
>
> --
> Ville Syrjälä
> Intel
Re: [PATCH v2 3/7] drm/i915: Fix CHV CGM CSC coefficient sign handling
On Thu, May 25, 2023 at 08:55:08PM +, Shankar, Uma wrote:
>
>
> > -Original Message-
> > From: dri-devel On Behalf Of
> > Ville Syrjala
> > Sent: Thursday, April 13, 2023 10:19 PM
> > To: intel-...@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ä
> >
> > 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
The 16bit coefficient already made me suspect they screwed up
the notation. Testing specific values on real hardware
confirmed that.
>
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/display/intel_color.c | 46 ++
> > 1 file changed, 29 insertions(+), 17 deletions(-)
> >
> > 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, _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,
> > --
> > 2.39.2
>
--
Ville Syrjälä
Intel
RE: [PATCH v2 3/7] drm/i915: Fix CHV CGM CSC coefficient sign handling
> -Original Message-
> From: Shankar, Uma
> Sent: Friday, May 26, 2023 2:25 AM
> To: Ville Syrjala ;
> intel-...@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 On Behalf Of
> > Ville Syrjala
> > Sent: Thursday, April 13, 2023 10:19 PM
> > To: intel-...@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ä
> >
> > 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
I think I got it. Looks good to me.
Reviewed-by: Uma Shankar
> > Signed-off-by: Ville Syrjälä
> > ---
> > drivers/gpu/drm/i915/display/intel_color.c | 46
> > ++
> > 1 file changed, 29 insertions(+), 17 deletions(-)
> >
> > 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, _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,
> > --
> > 2.39.2
RE: [PATCH v2 3/7] drm/i915: Fix CHV CGM CSC coefficient sign handling
> -Original Message-
> From: dri-devel On Behalf Of Ville
> Syrjala
> Sent: Thursday, April 13, 2023 10:19 PM
> To: intel-...@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ä
>
> 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
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/display/intel_color.c | 46 ++
> 1 file changed, 29 insertions(+), 17 deletions(-)
>
> 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, _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,
> --
> 2.39.2
