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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKMK7uHWvKBOyJ5grOxpsq8tiV_O9AbHyJthJCDVf821hYiTrQ@mail.gmail.com>
Date:   Tue, 31 Jan 2017 18:46:39 +0100
From:   Daniel Vetter <daniel.vetter@...ll.ch>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:     Brian Starkey <brian.starkey@....com>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Sean Paul <seanpaul@...omium.org>,
        Lionel Landwerlin <lionel.g.landwerlin@...el.com>
Subject: Re: [PATCH v2] drm/color: Document CTM eqations

On Tue, Jan 31, 2017 at 6:22 PM, Ville Syrjälä
<ville.syrjala@...ux.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