* [PATCH v2] drm/color: Document CTM eqations @ 2017-01-31 10:48 ` Brian Starkey 0 siblings, 0 replies; 32+ messages in thread From: Brian Starkey @ 2017-01-31 10:48 UTC (permalink / raw) To: dri-devel Cc: linux-kernel, Jani Nikula, Sean Paul, Ville Syrjälä, Lionel Landwerlin, Daniel Vetter 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: + * + * | 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 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [PATCH v2] drm/color: Document CTM eqations @ 2017-01-31 10:48 ` Brian Starkey 0 siblings, 0 replies; 32+ messages in thread From: Brian Starkey @ 2017-01-31 10:48 UTC (permalink / raw) To: dri-devel; +Cc: Daniel Vetter, linux-kernel 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: + * + * | 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-01-31 10:48 ` Brian Starkey @ 2017-01-31 11:30 ` Jani Nikula -1 siblings, 0 replies; 32+ messages in thread From: Jani Nikula @ 2017-01-31 11:30 UTC (permalink / raw) To: Brian Starkey, dri-devel Cc: linux-kernel, Sean Paul, Ville Syrjälä, Lionel Landwerlin, Daniel Vetter 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]; > }; -- Jani Nikula, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-01-31 11:30 ` Jani Nikula 0 siblings, 0 replies; 32+ messages in thread From: Jani Nikula @ 2017-01-31 11:30 UTC (permalink / raw) To: Brian Starkey, dri-devel; +Cc: linux-kernel, Daniel Vetter 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]; > }; -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-01-31 11:30 ` Jani Nikula @ 2017-01-31 12:02 ` Brian Starkey -1 siblings, 0 replies; 32+ messages in thread From: Brian Starkey @ 2017-01-31 12:02 UTC (permalink / raw) To: Jani Nikula Cc: dri-devel, linux-kernel, Sean Paul, Ville Syrjälä, Lionel Landwerlin, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-01-31 12:02 ` Brian Starkey 0 siblings, 0 replies; 32+ messages in thread From: Brian Starkey @ 2017-01-31 12:02 UTC (permalink / raw) To: Jani Nikula; +Cc: Daniel Vetter, linux-kernel, dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-01-31 12:02 ` Brian Starkey @ 2017-01-31 12:37 ` Jani Nikula -1 siblings, 0 replies; 32+ messages in thread From: Jani Nikula @ 2017-01-31 12:37 UTC (permalink / raw) To: Brian Starkey Cc: dri-devel, linux-kernel, Sean Paul, Ville Syrjälä, Lionel Landwerlin, Daniel Vetter 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 -- Jani Nikula, Intel Open Source Technology Center ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-01-31 12:37 ` Jani Nikula 0 siblings, 0 replies; 32+ messages in thread From: Jani Nikula @ 2017-01-31 12:37 UTC (permalink / raw) To: Brian Starkey; +Cc: Daniel Vetter, linux-kernel, dri-devel 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 -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-01-31 10:48 ` Brian Starkey @ 2017-01-31 15:18 ` Ville Syrjälä -1 siblings, 0 replies; 32+ messages in thread From: Ville Syrjälä @ 2017-01-31 15:18 UTC (permalink / raw) To: Brian Starkey Cc: dri-devel, linux-kernel, Jani Nikula, Sean Paul, Lionel Landwerlin, Daniel Vetter 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 -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-01-31 15:18 ` Ville Syrjälä 0 siblings, 0 replies; 32+ messages in thread From: Ville Syrjälä @ 2017-01-31 15:18 UTC (permalink / raw) To: Brian Starkey; +Cc: Daniel Vetter, dri-devel, linux-kernel 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 -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-01-31 15:18 ` Ville Syrjälä @ 2017-01-31 15:39 ` Brian Starkey -1 siblings, 0 replies; 32+ messages in thread From: Brian Starkey @ 2017-01-31 15:39 UTC (permalink / raw) To: Ville Syrjälä Cc: dri-devel, linux-kernel, Jani Nikula, Sean Paul, Lionel Landwerlin, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-01-31 15:39 ` Brian Starkey 0 siblings, 0 replies; 32+ messages in thread From: Brian Starkey @ 2017-01-31 15:39 UTC (permalink / raw) To: Ville Syrjälä; +Cc: Daniel Vetter, dri-devel, linux-kernel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-01-31 15:39 ` Brian Starkey @ 2017-01-31 17:22 ` Ville Syrjälä -1 siblings, 0 replies; 32+ messages in thread From: Ville Syrjälä @ 2017-01-31 17:22 UTC (permalink / raw) To: Brian Starkey Cc: dri-devel, linux-kernel, Jani Nikula, Sean Paul, Lionel Landwerlin, Daniel Vetter 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. -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-01-31 17:22 ` Ville Syrjälä 0 siblings, 0 replies; 32+ messages in thread From: Ville Syrjälä @ 2017-01-31 17:22 UTC (permalink / raw) To: Brian Starkey; +Cc: Daniel Vetter, dri-devel, linux-kernel 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. -- Ville Syrjälä Intel OTC _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-01-31 17:22 ` Ville Syrjälä @ 2017-01-31 17:46 ` Daniel Vetter -1 siblings, 0 replies; 32+ messages in thread From: Daniel Vetter @ 2017-01-31 17:46 UTC (permalink / raw) To: Ville Syrjälä Cc: Brian Starkey, dri-devel, Linux Kernel Mailing List, Jani Nikula, Sean Paul, Lionel Landwerlin 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-01-31 17:46 ` Daniel Vetter 0 siblings, 0 replies; 32+ messages in thread From: Daniel Vetter @ 2017-01-31 17:46 UTC (permalink / raw) To: Ville Syrjälä; +Cc: dri-devel, Linux Kernel Mailing List 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-01-31 17:46 ` Daniel Vetter @ 2017-02-15 11:39 ` Ville Syrjälä -1 siblings, 0 replies; 32+ messages in thread From: Ville Syrjälä @ 2017-02-15 11:39 UTC (permalink / raw) To: Daniel Vetter Cc: Brian Starkey, dri-devel, Linux Kernel Mailing List, Jani Nikula, Sean Paul, Lionel Landwerlin 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-02-15 11:39 ` Ville Syrjälä 0 siblings, 0 replies; 32+ messages in thread From: Ville Syrjälä @ 2017-02-15 11:39 UTC (permalink / raw) To: Daniel Vetter; +Cc: dri-devel, Linux Kernel Mailing List 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-02-15 11:39 ` Ville Syrjälä @ 2017-02-15 11:56 ` Daniel Stone -1 siblings, 0 replies; 32+ messages in thread From: Daniel Stone @ 2017-02-15 11:56 UTC (permalink / raw) To: Ville Syrjälä Cc: Daniel Vetter, Brian Starkey, dri-devel, Linux Kernel Mailing List, Jani Nikula, Sean Paul, Lionel Landwerlin 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-02-15 11:56 ` Daniel Stone 0 siblings, 0 replies; 32+ messages in thread From: Daniel Stone @ 2017-02-15 11:56 UTC (permalink / raw) To: Ville Syrjälä Cc: Daniel Vetter, Linux Kernel Mailing List, dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-02-15 11:56 ` Daniel Stone @ 2017-02-17 13:54 ` Brian Starkey -1 siblings, 0 replies; 32+ messages in thread From: Brian Starkey @ 2017-02-17 13:54 UTC (permalink / raw) To: Daniel Stone Cc: Ville Syrjälä, Daniel Vetter, dri-devel, Linux Kernel Mailing List, Jani Nikula, Sean Paul, Lionel Landwerlin 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-02-17 13:54 ` Brian Starkey 0 siblings, 0 replies; 32+ messages in thread From: Brian Starkey @ 2017-02-17 13:54 UTC (permalink / raw) To: Daniel Stone; +Cc: Daniel Vetter, Linux Kernel Mailing List, dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-02-17 13:54 ` Brian Starkey @ 2017-02-17 14:42 ` Lionel Landwerlin -1 siblings, 0 replies; 32+ messages in thread From: Lionel Landwerlin @ 2017-02-17 14:42 UTC (permalink / raw) To: Brian Starkey, Daniel Stone Cc: Ville Syrjälä, Daniel Vetter, dri-devel, Linux Kernel Mailing List, Jani Nikula, Sean Paul 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-02-17 14:42 ` Lionel Landwerlin 0 siblings, 0 replies; 32+ messages in thread From: Lionel Landwerlin @ 2017-02-17 14:42 UTC (permalink / raw) To: Brian Starkey, Daniel Stone Cc: Daniel Vetter, Linux Kernel Mailing List, dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-02-17 14:42 ` Lionel Landwerlin (?) @ 2017-02-17 14:56 ` Ville Syrjälä 2017-02-17 15:05 ` Lionel Landwerlin 2017-02-17 15:15 ` Daniel Stone -1 siblings, 2 replies; 32+ messages in thread From: Ville Syrjälä @ 2017-02-17 14:56 UTC (permalink / raw) To: Lionel Landwerlin Cc: Brian Starkey, Daniel Stone, Daniel Vetter, dri-devel, Linux Kernel Mailing List, Jani Nikula, Sean Paul 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-02-17 14:56 ` Ville Syrjälä @ 2017-02-17 15:05 ` Lionel Landwerlin 2017-02-17 15:15 ` Daniel Stone 1 sibling, 0 replies; 32+ messages in thread From: Lionel Landwerlin @ 2017-02-17 15:05 UTC (permalink / raw) To: Ville Syrjälä Cc: Brian Starkey, Daniel Stone, Daniel Vetter, dri-devel, Linux Kernel Mailing List, Jani Nikula, Sean Paul 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. ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-02-17 15:05 ` Lionel Landwerlin 0 siblings, 0 replies; 32+ messages in thread From: Lionel Landwerlin @ 2017-02-17 15:05 UTC (permalink / raw) To: Ville Syrjälä Cc: Daniel Vetter, Linux Kernel Mailing List, dri-devel 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. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-02-17 15:05 ` Lionel Landwerlin @ 2017-02-17 15:16 ` Ville Syrjälä -1 siblings, 0 replies; 32+ messages in thread From: Ville Syrjälä @ 2017-02-17 15:16 UTC (permalink / raw) To: Lionel Landwerlin Cc: Brian Starkey, Daniel Stone, Daniel Vetter, dri-devel, Linux Kernel Mailing List, Jani Nikula, Sean Paul 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-02-17 15:16 ` Ville Syrjälä 0 siblings, 0 replies; 32+ messages in thread From: Ville Syrjälä @ 2017-02-17 15:16 UTC (permalink / raw) To: Lionel Landwerlin; +Cc: Daniel Vetter, Linux Kernel Mailing List, dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-02-17 14:56 ` Ville Syrjälä @ 2017-02-17 15:15 ` Daniel Stone 2017-02-17 15:15 ` Daniel Stone 1 sibling, 0 replies; 32+ messages in thread From: Daniel Stone @ 2017-02-17 15:15 UTC (permalink / raw) To: Ville Syrjälä Cc: Lionel Landwerlin, Brian Starkey, Daniel Vetter, dri-devel, Linux Kernel Mailing List, Jani Nikula, Sean Paul 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations @ 2017-02-17 15:15 ` Daniel Stone 0 siblings, 0 replies; 32+ messages in thread From: Daniel Stone @ 2017-02-17 15:15 UTC (permalink / raw) To: Ville Syrjälä Cc: Daniel Vetter, Linux Kernel Mailing List, dri-devel 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 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH v2] drm/color: Document CTM eqations 2017-02-17 13:54 ` Brian Starkey (?) (?) @ 2017-02-26 19:45 ` Daniel Vetter -1 siblings, 0 replies; 32+ messages in thread From: Daniel Vetter @ 2017-02-26 19:45 UTC (permalink / raw) To: Brian Starkey Cc: Daniel Stone, Ville Syrjälä, Daniel Vetter, dri-devel, Linux Kernel Mailing List, Jani Nikula, Sean Paul, Lionel Landwerlin 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 ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2017-02-26 19:53 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-31 10:48 [PATCH v2] drm/color: Document CTM eqations Brian Starkey 2017-01-31 10:48 ` Brian Starkey 2017-01-31 11:30 ` Jani Nikula 2017-01-31 11:30 ` Jani Nikula 2017-01-31 12:02 ` Brian Starkey 2017-01-31 12:02 ` Brian Starkey 2017-01-31 12:37 ` Jani Nikula 2017-01-31 12:37 ` Jani Nikula 2017-01-31 15:18 ` Ville Syrjälä 2017-01-31 15:18 ` Ville Syrjälä 2017-01-31 15:39 ` Brian Starkey 2017-01-31 15:39 ` Brian Starkey 2017-01-31 17:22 ` Ville Syrjälä 2017-01-31 17:22 ` Ville Syrjälä 2017-01-31 17:46 ` Daniel Vetter 2017-01-31 17:46 ` Daniel Vetter 2017-02-15 11:39 ` Ville Syrjälä 2017-02-15 11:39 ` Ville Syrjälä 2017-02-15 11:56 ` Daniel Stone 2017-02-15 11:56 ` Daniel Stone 2017-02-17 13:54 ` Brian Starkey 2017-02-17 13:54 ` Brian Starkey 2017-02-17 14:42 ` Lionel Landwerlin 2017-02-17 14:42 ` Lionel Landwerlin 2017-02-17 14:56 ` Ville Syrjälä 2017-02-17 15:05 ` Lionel Landwerlin 2017-02-17 15:05 ` Lionel Landwerlin 2017-02-17 15:16 ` Ville Syrjälä 2017-02-17 15:16 ` Ville Syrjälä 2017-02-17 15:15 ` Daniel Stone 2017-02-17 15:15 ` Daniel Stone 2017-02-26 19:45 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.