[PATCH v2] drm/color: Document CTM eqations

[Daniel Vetter]

On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
<ville.syrjala at linux.intel.com> 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?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch