Re: [PATCH v2] drm/color: Document CTM eqations

[Ville Syrjälä]

On Tue, Jan 31, 2017 at 06:46:39PM +0100, Daniel Vetter wrote:
> On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
> <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> >> >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> >> >> index ce7efe2e8a5e..3401637caf8e 100644
> >> >> --- a/include/uapi/drm/drm_mode.h
> >> >> +++ b/include/uapi/drm/drm_mode.h
> >> >> @@ -525,7 +525,13 @@ struct drm_mode_crtc_lut {
> >> >>  };
> >> >>
> >> >>  struct drm_color_ctm {
> >> >> -  /* Conversion matrix in S31.32 format. */
> >> >> +  /*
> >> >> +   * Conversion matrix in S31.32 format, in row-major form:
> >> >
> >> >s32.32 is how I'd state that (to match the regular s32 and whatnot
> >> >types).
> >> >
> >>
> >> Can you explain a bit more what exactly you mean by s32.32? e.g. what
> >> would be the bitfield representing the most negative number?
> >>
> >> I understand the S31.32 here as a sign + magnitude format (which makes
> >> it rather odd to store it in a signed variable, but never mind). This
> >> also appears to be what igt does in set_ctm() in kms_pipe_color.c:
> >>
> >>       for (i = 0; i < ARRAY_SIZE(ctm.matrix); i++) {
> >>               if (coefficients[i] < 0) {
> >>                       ctm.matrix[i] =
> >>                               (int64_t) (-coefficients[i] * ((int64_t) 1L << 32));
> >>                       ctm.matrix[i] |= 1ULL << 63;
> >>               } else
> >>                       ctm.matrix[i] =
> >>                               (int64_t) (coefficients[i] * ((int64_t) 1L << 32));
> >>       }
> >>
> >> If that's what you meant as well, then I don't think s32.32 is a good
> >> way to describe it, because the integer part has only 31 bits
> >> available.
> >>
> >> If you meant a regular two's-complement fixed-point number, where the
> >> most negative number would be 0x10000000.00000000, then yeah that's
> >> what I thought it meant too originally. Clarifying the docs here
> >> sounds like a great plan.
> >>
> >> I guess the igt implementation means that it's a sign + magnitude
> >> number, and the fact that it's stored in an s64 is a bizarre quirk
> >> that we just live with.
> >
> > Hmm. Two's complement is what I was thinking it is. Which shows that
> > I never managed to read the code in any detail. Definitely needs to
> > be documented properly.
> 
> That sounds supremely backwards. I guess we can't fix this anymore?

I have no idea. Anyone else?

-- 
Ville Syrjälä
Intel OTC