| Message ID | 1485859714-26619-1-git-send-email-brian.starkey@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote: > Explicitly state the expected CTM equations in the kerneldoc for the CTM > property, and the form of the matrix on struct drm_color_ctm. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > --- > drivers/gpu/drm/drm_color_mgmt.c | 13 +++++++++++++ > include/uapi/drm/drm_mode.h | 8 +++++++- > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > index 789b4c65cd69..7573ca4b6ea6 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c > @@ -62,6 +62,19 @@ > * unit/pass-thru matrix should be used. This is generally the driver > * boot-up state too. > * > + * The output vector is related to the input vector as below: > + * > + * | ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];`` > + * | ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];`` > + * | ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];`` Would that not work better with a preformatted block? Replace the colon in the preceding paragraph with the double colon ::, and indent the block. > + * > + * The component order in the input/output vectors is assumed to be > + * { R, G, B }. > + * > + * The color-space of the input vector must not be confused with the > + * color-space implied by a framebuffer pixel format, which may be the same > + * or different. > + * > * “GAMMA_LUT”: > * Blob property to set the gamma lookup table (LUT) mapping pixel data > * after the transformation matrix to data sent to the connector. The > 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: > + * > + * | matrix[0] matrix[1] matrix[2] | > + * | matrix[3] matrix[4] matrix[5] | > + * | matrix[6] matrix[7] matrix[8] | > + */ Same here. > __s64 matrix[9]; > };
Hi Jani, On Tue, Jan 31, 2017 at 01:30:41PM +0200, Jani Nikula wrote: >On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote: >> Explicitly state the expected CTM equations in the kerneldoc for the CTM >> property, and the form of the matrix on struct drm_color_ctm. >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Brian Starkey <brian.starkey@arm.com> >> --- >> drivers/gpu/drm/drm_color_mgmt.c | 13 +++++++++++++ >> include/uapi/drm/drm_mode.h | 8 +++++++- >> 2 files changed, 20 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c >> index 789b4c65cd69..7573ca4b6ea6 100644 >> --- a/drivers/gpu/drm/drm_color_mgmt.c >> +++ b/drivers/gpu/drm/drm_color_mgmt.c >> @@ -62,6 +62,19 @@ >> * unit/pass-thru matrix should be used. This is generally the driver >> * boot-up state too. >> * >> + * The output vector is related to the input vector as below: >> + * >> + * | ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];`` >> + * | ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];`` >> + * | ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];`` > >Would that not work better with a preformatted block? Replace the colon >in the preceding paragraph with the double colon ::, and indent the >block. > Ah thanks for the tip, I couldn't get it to work but it looks like my syntax was a bit off. I'll resend with that change. >> + * >> + * The component order in the input/output vectors is assumed to be >> + * { R, G, B }. >> + * >> + * The color-space of the input vector must not be confused with the >> + * color-space implied by a framebuffer pixel format, which may be the same >> + * or different. >> + * >> * “GAMMA_LUT”: >> * Blob property to set the gamma lookup table (LUT) mapping pixel data >> * after the transformation matrix to data sent to the connector. The >> 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: >> + * >> + * | matrix[0] matrix[1] matrix[2] | >> + * | matrix[3] matrix[4] matrix[5] | >> + * | matrix[6] matrix[7] matrix[8] | >> + */ > >Same here. This comment isn't actually kerneldoc, so I guess not rst either. I can include the markup if you like, but the |s here were to indicate it's a matrix rather than for rst. Cheers, -Brian > >> __s64 matrix[9]; >> }; > >-- >Jani Nikula, Intel Open Source Technology Center
On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote: > Hi Jani, > > On Tue, Jan 31, 2017 at 01:30:41PM +0200, Jani Nikula wrote: >>On Tue, 31 Jan 2017, Brian Starkey <brian.starkey@arm.com> wrote: >>> Explicitly state the expected CTM equations in the kerneldoc for the CTM >>> property, and the form of the matrix on struct drm_color_ctm. >>> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >>> Signed-off-by: Brian Starkey <brian.starkey@arm.com> >>> --- >>> drivers/gpu/drm/drm_color_mgmt.c | 13 +++++++++++++ >>> include/uapi/drm/drm_mode.h | 8 +++++++- >>> 2 files changed, 20 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c >>> index 789b4c65cd69..7573ca4b6ea6 100644 >>> --- a/drivers/gpu/drm/drm_color_mgmt.c >>> +++ b/drivers/gpu/drm/drm_color_mgmt.c >>> @@ -62,6 +62,19 @@ >>> * unit/pass-thru matrix should be used. This is generally the driver >>> * boot-up state too. >>> * >>> + * The output vector is related to the input vector as below: >>> + * >>> + * | ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];`` >>> + * | ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];`` >>> + * | ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];`` >> >>Would that not work better with a preformatted block? Replace the colon >>in the preceding paragraph with the double colon ::, and indent the >>block. >> > > Ah thanks for the tip, I couldn't get it to work but it looks like my > syntax was a bit off. I'll resend with that change. > >>> + * >>> + * The component order in the input/output vectors is assumed to be >>> + * { R, G, B }. >>> + * >>> + * The color-space of the input vector must not be confused with the >>> + * color-space implied by a framebuffer pixel format, which may be the same >>> + * or different. >>> + * >>> * “GAMMA_LUT”: >>> * Blob property to set the gamma lookup table (LUT) mapping pixel data >>> * after the transformation matrix to data sent to the connector. The >>> 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: >>> + * >>> + * | matrix[0] matrix[1] matrix[2] | >>> + * | matrix[3] matrix[4] matrix[5] | >>> + * | matrix[6] matrix[7] matrix[8] | >>> + */ >> >>Same here. > > This comment isn't actually kerneldoc, so I guess not rst either. > I can include the markup if you like, but the |s here were to indicate > it's a matrix rather than for rst. Oh, right. Up to you. > > Cheers, > -Brian > >> >>> __s64 matrix[9]; >>> }; >> >>-- >>Jani Nikula, Intel Open Source Technology Center
On Tue, Jan 31, 2017 at 10:48:34AM +0000, Brian Starkey wrote: > Explicitly state the expected CTM equations in the kerneldoc for the CTM > property, and the form of the matrix on struct drm_color_ctm. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > Signed-off-by: Brian Starkey <brian.starkey@arm.com> > --- > drivers/gpu/drm/drm_color_mgmt.c | 13 +++++++++++++ > include/uapi/drm/drm_mode.h | 8 +++++++- > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > index 789b4c65cd69..7573ca4b6ea6 100644 > --- a/drivers/gpu/drm/drm_color_mgmt.c > +++ b/drivers/gpu/drm/drm_color_mgmt.c > @@ -62,6 +62,19 @@ > * unit/pass-thru matrix should be used. This is generally the driver > * boot-up state too. > * > + * The output vector is related to the input vector as below: > + * > + * | ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];`` > + * | ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];`` > + * | ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];`` > + * > + * The component order in the input/output vectors is assumed to be > + * { R, G, B }. > + * > + * The color-space of the input vector must not be confused with the > + * color-space implied by a framebuffer pixel format, which may be the same > + * or different. > + * > * “GAMMA_LUT”: > * Blob property to set the gamma lookup table (LUT) mapping pixel data > * after the transformation matrix to data sent to the connector. The > 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). > + * > + * | matrix[0] matrix[1] matrix[2] | > + * | matrix[3] matrix[4] matrix[5] | > + * | matrix[6] matrix[7] matrix[8] | > + */ > __s64 matrix[9]; > }; > > -- > 1.7.9.5
Hi Ville, On Tue, Jan 31, 2017 at 05:18:28PM +0200, Ville Syrjälä wrote: >On Tue, Jan 31, 2017 at 10:48:34AM +0000, Brian Starkey wrote: >> Explicitly state the expected CTM equations in the kerneldoc for the CTM >> property, and the form of the matrix on struct drm_color_ctm. >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> Signed-off-by: Brian Starkey <brian.starkey@arm.com> >> --- >> drivers/gpu/drm/drm_color_mgmt.c | 13 +++++++++++++ >> include/uapi/drm/drm_mode.h | 8 +++++++- >> 2 files changed, 20 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c >> index 789b4c65cd69..7573ca4b6ea6 100644 >> --- a/drivers/gpu/drm/drm_color_mgmt.c >> +++ b/drivers/gpu/drm/drm_color_mgmt.c >> @@ -62,6 +62,19 @@ >> * unit/pass-thru matrix should be used. This is generally the driver >> * boot-up state too. >> * >> + * The output vector is related to the input vector as below: >> + * >> + * | ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];`` >> + * | ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];`` >> + * | ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];`` >> + * >> + * The component order in the input/output vectors is assumed to be >> + * { R, G, B }. >> + * >> + * The color-space of the input vector must not be confused with the >> + * color-space implied by a framebuffer pixel format, which may be the same >> + * or different. >> + * >> * “GAMMA_LUT”: >> * Blob property to set the gamma lookup table (LUT) mapping pixel data >> * after the transformation matrix to data sent to the connector. The >> 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. Cheers, Brian >> + * >> + * | matrix[0] matrix[1] matrix[2] | >> + * | matrix[3] matrix[4] matrix[5] | >> + * | matrix[6] matrix[7] matrix[8] | >> + */ >> __s64 matrix[9]; >> }; >> >> -- >> 1.7.9.5 > >-- >Ville Syrjälä >Intel OTC
On Tue, Jan 31, 2017 at 03:39:29PM +0000, Brian Starkey wrote: > Hi Ville, > > On Tue, Jan 31, 2017 at 05:18:28PM +0200, Ville Syrjälä wrote: > >On Tue, Jan 31, 2017 at 10:48:34AM +0000, Brian Starkey wrote: > >> Explicitly state the expected CTM equations in the kerneldoc for the CTM > >> property, and the form of the matrix on struct drm_color_ctm. > >> > >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> > >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > >> Signed-off-by: Brian Starkey <brian.starkey@arm.com> > >> --- > >> drivers/gpu/drm/drm_color_mgmt.c | 13 +++++++++++++ > >> include/uapi/drm/drm_mode.h | 8 +++++++- > >> 2 files changed, 20 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c > >> index 789b4c65cd69..7573ca4b6ea6 100644 > >> --- a/drivers/gpu/drm/drm_color_mgmt.c > >> +++ b/drivers/gpu/drm/drm_color_mgmt.c > >> @@ -62,6 +62,19 @@ > >> * unit/pass-thru matrix should be used. This is generally the driver > >> * boot-up state too. > >> * > >> + * The output vector is related to the input vector as below: > >> + * > >> + * | ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];`` > >> + * | ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];`` > >> + * | ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];`` > >> + * > >> + * The component order in the input/output vectors is assumed to be > >> + * { R, G, B }. > >> + * > >> + * The color-space of the input vector must not be confused with the > >> + * color-space implied by a framebuffer pixel format, which may be the same > >> + * or different. > >> + * > >> * “GAMMA_LUT”: > >> * Blob property to set the gamma lookup table (LUT) mapping pixel data > >> * after the transformation matrix to data sent to the connector. The > >> 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.
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? -Daniel
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?
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
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
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
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.
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.
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
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.
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
diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c index 789b4c65cd69..7573ca4b6ea6 100644 --- a/drivers/gpu/drm/drm_color_mgmt.c +++ b/drivers/gpu/drm/drm_color_mgmt.c @@ -62,6 +62,19 @@ * unit/pass-thru matrix should be used. This is generally the driver * boot-up state too. * + * The output vector is related to the input vector as below: + * + * | ``out[0] = matrix[0] * in[0] + matrix[1] * in[1] + matrix[2] * in[2];`` + * | ``out[1] = matrix[3] * in[0] + matrix[4] * in[1] + matrix[5] * in[2];`` + * | ``out[2] = matrix[6] * in[0] + matrix[7] * in[1] + matrix[8] * in[2];`` + * + * The component order in the input/output vectors is assumed to be + * { R, G, B }. + * + * The color-space of the input vector must not be confused with the + * color-space implied by a framebuffer pixel format, which may be the same + * or different. + * * “GAMMA_LUT”: * Blob property to set the gamma lookup table (LUT) mapping pixel data * after the transformation matrix to data sent to the connector. The 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: + * + * | matrix[0] matrix[1] matrix[2] | + * | matrix[3] matrix[4] matrix[5] | + * | matrix[6] matrix[7] matrix[8] | + */ __s64 matrix[9]; };
Explicitly state the expected CTM equations in the kerneldoc for the CTM property, and the form of the matrix on struct drm_color_ctm. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Brian Starkey <brian.starkey@arm.com> --- drivers/gpu/drm/drm_color_mgmt.c | 13 +++++++++++++ include/uapi/drm/drm_mode.h | 8 +++++++- 2 files changed, 20 insertions(+), 1 deletion(-)