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 for Android: free password hash cracker in your pocket

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

Message-ID: <20170215113959.GT31595@intel.com>
Date:   Wed, 15 Feb 2017 13:39:59 +0200
From:   Ville Syrjälä <ville.syrjala@...ux.intel.com>
To:     Daniel Vetter <daniel.vetter@...ll.ch>
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 06:46:39PM +0100, Daniel Vetter wrote:
> 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?

I have no idea. Anyone else?

-- 
Ville Syrjälä
Intel OTC