Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1581247 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2] drm/color: Document CTM eqations Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-02-15 12:50 +0100
    Re: [PATCH v2] drm/color: Document CTM eqations Daniel Stone <daniel@fooishbar.org> - 2017-02-15 13:00 +0100
      Re: [PATCH v2] drm/color: Document CTM eqations Brian Starkey <brian.starkey@arm.com> - 2017-02-17 15:00 +0100
        Re: [PATCH v2] drm/color: Document CTM eqations Lionel Landwerlin <lionel.g.landwerlin@intel.com> - 2017-02-17 15:50 +0100
          Re: [PATCH v2] drm/color: Document CTM eqations Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-02-17 16:00 +0100
            Re: [PATCH v2] drm/color: Document CTM eqations Lionel Landwerlin <lionel.g.landwerlin@intel.com> - 2017-02-17 16:10 +0100
              Re: [PATCH v2] drm/color: Document CTM eqations Ville Syrjälä <ville.syrjala@linux.intel.com> - 2017-02-17 16:20 +0100
            Re: [PATCH v2] drm/color: Document CTM eqations Daniel Stone <daniel@fooishbar.org> - 2017-02-17 16:20 +0100
        Re: [PATCH v2] drm/color: Document CTM eqations Daniel Vetter <daniel@ffwll.ch> - 2017-02-26 21:00 +0100

#1581247 — 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@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?

I have no idea. Anyone else?

-- 
Ville Syrjälä
Intel OTC

[toc] | [next] | [standalone]

#1581254

Hi,

On 15 February 2017 at 11:39, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> 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@linux.intel.com> wrote:
>> > 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?

I don't know of any implementation using this; maybe closed Intel
Android stuff? Certainly GitHub showed no-one using it, and neither X
nor Weston/Mutter are using it yet.

Cheers,
Daniel

[toc] | [prev] | [next] | [standalone]

#1583421

What's the verdict? We've got [1] which is about to become another
(driver) implementation - better to change before that merges than
after I guess.

-Brian

[1] https://lkml.org/lkml/2017/2/13/304

On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
>Hi,
>
>On 15 February 2017 at 11:39, Ville Syrjälä
><ville.syrjala@linux.intel.com> wrote:
>> 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@linux.intel.com> wrote:
>>> > 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?
>
>I don't know of any implementation using this; maybe closed Intel
>Android stuff? Certainly GitHub showed no-one using it, and neither X
>nor Weston/Mutter are using it yet.
>
>Cheers,
>Daniel

[toc] | [prev] | [next] | [standalone]

#1583489

On 17/02/17 13:54, Brian Starkey wrote:
> What's the verdict? We've got [1] which is about to become another
> (driver) implementation - better to change before that merges than
> after I guess.
>
> -Brian
>
> [1] https://lkml.org/lkml/2017/2/13/304
>
> On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
>> Hi,
>>
>> On 15 February 2017 at 11:39, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>>> 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@linux.intel.com> wrote:
>>>> > 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?
>>
>> I don't know of any implementation using this; maybe closed Intel
>> Android stuff? Certainly GitHub showed no-one using it, and neither X
>> nor Weston/Mutter are using it yet.
>>
>> Cheers,
>> Daniel
>
If we're talking fixed point reprsentation, ChromeOS is using this :

https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209

-
Lionel

[toc] | [prev] | [next] | [standalone]

#1583501

On Fri, Feb 17, 2017 at 02:42:26PM +0000, Lionel Landwerlin wrote:
> On 17/02/17 13:54, Brian Starkey wrote:
> > What's the verdict? We've got [1] which is about to become another
> > (driver) implementation - better to change before that merges than
> > after I guess.
> >
> > -Brian
> >
> > [1] https://lkml.org/lkml/2017/2/13/304
> >
> > On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
> >> Hi,
> >>
> >> On 15 February 2017 at 11:39, Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >>> 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@linux.intel.com> wrote:
> >>>> > 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?
> >>
> >> I don't know of any implementation using this; maybe closed Intel
> >> Android stuff? Certainly GitHub showed no-one using it, and neither X
> >> nor Weston/Mutter are using it yet.
> >>
> >> Cheers,
> >> Daniel
> >
> If we're talking fixed point reprsentation, ChromeOS is using this :
> 
> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209

So it's already using the sign+magnitude stuff. Which presumably
means we can't change it to two's complement anymore :( Maybe we add a
CTM2 property ;)

Using sign+magnitude definitely looks rather inefficient since there's
a branch inside the loop. With two's complement you wouldn't need that
thing slowing you down.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1583507

On 17/02/17 14:56, Ville Syrjälä wrote:
> On Fri, Feb 17, 2017 at 02:42:26PM +0000, Lionel Landwerlin wrote:
>> On 17/02/17 13:54, Brian Starkey wrote:
>>> What's the verdict? We've got [1] which is about to become another
>>> (driver) implementation - better to change before that merges than
>>> after I guess.
>>>
>>> -Brian
>>>
>>> [1] https://lkml.org/lkml/2017/2/13/304
>>>
>>> On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
>>>> Hi,
>>>>
>>>> On 15 February 2017 at 11:39, Ville Syrjälä
>>>> <ville.syrjala@linux.intel.com> wrote:
>>>>> 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@linux.intel.com> wrote:
>>>>>>> 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?
>>>> I don't know of any implementation using this; maybe closed Intel
>>>> Android stuff? Certainly GitHub showed no-one using it, and neither X
>>>> nor Weston/Mutter are using it yet.
>>>>
>>>> Cheers,
>>>> Daniel
>> If we're talking fixed point reprsentation, ChromeOS is using this :
>>
>> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209
> So it's already using the sign+magnitude stuff. Which presumably
> means we can't change it to two's complement anymore :( Maybe we add a
> CTM2 property ;)
>
> Using sign+magnitude definitely looks rather inefficient since there's
> a branch inside the loop. With two's complement you wouldn't need that
> thing slowing you down.
>
If you're seriously considering that, you might also want to bump struct 
drm_color_lut to use 32bits fields.
It seems some people have concerned about HDR.

[toc] | [prev] | [next] | [standalone]

#1583521

On Fri, Feb 17, 2017 at 03:05:28PM +0000, Lionel Landwerlin wrote:
> On 17/02/17 14:56, Ville Syrjälä wrote:
> > On Fri, Feb 17, 2017 at 02:42:26PM +0000, Lionel Landwerlin wrote:
> >> On 17/02/17 13:54, Brian Starkey wrote:
> >>> What's the verdict? We've got [1] which is about to become another
> >>> (driver) implementation - better to change before that merges than
> >>> after I guess.
> >>>
> >>> -Brian
> >>>
> >>> [1] https://lkml.org/lkml/2017/2/13/304
> >>>
> >>> On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
> >>>> Hi,
> >>>>
> >>>> On 15 February 2017 at 11:39, Ville Syrjälä
> >>>> <ville.syrjala@linux.intel.com> wrote:
> >>>>> 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@linux.intel.com> wrote:
> >>>>>>> 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?
> >>>> I don't know of any implementation using this; maybe closed Intel
> >>>> Android stuff? Certainly GitHub showed no-one using it, and neither X
> >>>> nor Weston/Mutter are using it yet.
> >>>>
> >>>> Cheers,
> >>>> Daniel
> >> If we're talking fixed point reprsentation, ChromeOS is using this :
> >>
> >> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209
> > So it's already using the sign+magnitude stuff. Which presumably
> > means we can't change it to two's complement anymore :( Maybe we add a
> > CTM2 property ;)
> >
> > Using sign+magnitude definitely looks rather inefficient since there's
> > a branch inside the loop. With two's complement you wouldn't need that
> > thing slowing you down.
> >
> If you're seriously considering that, you might also want to bump struct 
> drm_color_lut to use 32bits fields.

Which is what I thought we had already agreed to do when we started
planning this color management stuff. But I guess that plan got
somehow scrapped after I was no longer part of the discussions.

> It seems some people have concerned about HDR.

HDR is definitely something I'd like to have a look at.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1583519

Hi,

On 17 February 2017 at 14:56, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 17, 2017 at 02:42:26PM +0000, Lionel Landwerlin wrote:
>> If we're talking fixed point reprsentation, ChromeOS is using this :
>>
>> https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/gpu/drm_device.cc?q=DrmDevice&l=209
>
> So it's already using the sign+magnitude stuff. Which presumably
> means we can't change it to two's complement anymore :( Maybe we add a
> CTM2 property ;)

I wouldn't be so sure; AFAIK it only ships on platforms where the
kernel is also built from the same tree, and generally where the
support is backported anyway. So it would be possible to atomically
land a change to CrOS such that the kernels and Chrome move together
to a changed representation.

Cheers,
Daniel

[toc] | [prev] | [next] | [standalone]

#1588387

On Fri, Feb 17, 2017 at 01:54:52PM +0000, Brian Starkey wrote:
> What's the verdict? We've got [1] which is about to become another
> (driver) implementation - better to change before that merges than
> after I guess.

We could also just switch the internal representation to something more
reasonable, and add glue code in the atomic core to remap between whatever
the properties are and the internal representation. Same thing we
essentially already do with the display mode.

That would then also extend to CTM2 or whatever.

For sure not really a good reason to stall a driver imo.
-Daniel

> 
> -Brian
> 
> [1] https://lkml.org/lkml/2017/2/13/304
> 
> On Wed, Feb 15, 2017 at 11:56:55AM +0000, Daniel Stone wrote:
> > Hi,
> > 
> > On 15 February 2017 at 11:39, Ville Syrjälä
> > <ville.syrjala@linux.intel.com> wrote:
> > > 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@linux.intel.com> wrote:
> > > > > 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?
> > 
> > I don't know of any implementation using this; maybe closed Intel
> > Android stuff? Certainly GitHub showed no-one using it, and neither X
> > nor Weston/Mutter are using it yet.
> > 
> > Cheers,
> > Daniel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web