| Message ID | 20180222214232.6064-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>-----Original Message----- >From: Ville Syrjala [mailto:ville.syrjala@linux.intel.com] >Sent: Friday, February 23, 2018 3:12 AM >To: intel-gfx@lists.freedesktop.org >Cc: dri-devel@lists.freedesktop.org; Mihail Atanassov ><mihail.atanassov@arm.com>; Liviu Dudau <liviu.dudau@arm.com>; Brian >Starkey <brian.starkey@arm.com>; Mali DP Maintainers ><malidp@foss.arm.com>; Lin, Johnson <johnson.lin@intel.com>; Shankar, Uma ><uma.shankar@intel.com>; Sharma, Shashank <shashank.sharma@intel.com> >Subject: [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude >representation > >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >The documentation for the ctm matrix suggests a two's complement format, but >at least the i915 implementation is using sign-magnitude instead. And looks like >malidp is doing the same. Change the docs to match the current implementation, >and change the type from __s64 to __u64 to drive the point home. > Looks ok to me. Reviewed-by: Uma Shankar <uma.shankar@intel.com> >Cc: dri-devel@lists.freedesktop.org >Cc: Mihail Atanassov <mihail.atanassov@arm.com> >Cc: Liviu Dudau <liviu.dudau@arm.com> >Cc: Brian Starkey <brian.starkey@arm.com> >Cc: Mali DP Maintainers <malidp@foss.arm.com> >Cc: Johnson Lin <johnson.lin@intel.com> >Cc: Uma Shankar <uma.shankar@intel.com> >Cc: Shashank Sharma <shashank.sharma@intel.com> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >--- > include/uapi/drm/drm_mode.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > >diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >index 2c575794fb52..b5d7d9e0eff5 100644 >--- a/include/uapi/drm/drm_mode.h >+++ b/include/uapi/drm/drm_mode.h >@@ -598,8 +598,11 @@ struct drm_mode_crtc_lut { }; > > struct drm_color_ctm { >- /* Conversion matrix in S31.32 format. */ >- __s64 matrix[9]; >+ /* >+ * Conversion matrix in S31.32 sign-magnitude >+ * (not two's complement!) format. >+ */ >+ __u64 matrix[9]; > }; > > struct drm_color_lut { >-- >2.16.1
Hi Ville, On Thu, Feb 22, 2018 at 11:42:29PM +0200, Ville Syrjala wrote: >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >The documentation for the ctm matrix suggests a two's complement >format, but at least the i915 implementation is using sign-magnitude >instead. And looks like malidp is doing the same. Change the docs >to match the current implementation, and change the type from __s64 >to __u64 to drive the point home. I totally agree that this is a good idea, but... > >Cc: dri-devel@lists.freedesktop.org >Cc: Mihail Atanassov <mihail.atanassov@arm.com> >Cc: Liviu Dudau <liviu.dudau@arm.com> >Cc: Brian Starkey <brian.starkey@arm.com> >Cc: Mali DP Maintainers <malidp@foss.arm.com> >Cc: Johnson Lin <johnson.lin@intel.com> >Cc: Uma Shankar <uma.shankar@intel.com> >Cc: Shashank Sharma <shashank.sharma@intel.com> >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >--- > include/uapi/drm/drm_mode.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > >diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >index 2c575794fb52..b5d7d9e0eff5 100644 >--- a/include/uapi/drm/drm_mode.h >+++ b/include/uapi/drm/drm_mode.h >@@ -598,8 +598,11 @@ struct drm_mode_crtc_lut { > }; > > struct drm_color_ctm { >- /* Conversion matrix in S31.32 format. */ >- __s64 matrix[9]; >+ /* >+ * Conversion matrix in S31.32 sign-magnitude >+ * (not two's complement!) format. >+ */ >+ __u64 matrix[9]; Isn't changing the type liable to break something for someone? Thanks, -Brian > }; > > struct drm_color_lut { >-- >2.16.1 >
On Fri, Feb 23, 2018 at 01:52:22PM +0000, Brian Starkey wrote: > Hi Ville, > > On Thu, Feb 22, 2018 at 11:42:29PM +0200, Ville Syrjala wrote: > >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > >The documentation for the ctm matrix suggests a two's complement > >format, but at least the i915 implementation is using sign-magnitude > >instead. And looks like malidp is doing the same. Change the docs > >to match the current implementation, and change the type from __s64 > >to __u64 to drive the point home. > > I totally agree that this is a good idea, but... > > > > >Cc: dri-devel@lists.freedesktop.org > >Cc: Mihail Atanassov <mihail.atanassov@arm.com> > >Cc: Liviu Dudau <liviu.dudau@arm.com> > >Cc: Brian Starkey <brian.starkey@arm.com> > >Cc: Mali DP Maintainers <malidp@foss.arm.com> > >Cc: Johnson Lin <johnson.lin@intel.com> > >Cc: Uma Shankar <uma.shankar@intel.com> > >Cc: Shashank Sharma <shashank.sharma@intel.com> > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >--- > > include/uapi/drm/drm_mode.h | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > >diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > >index 2c575794fb52..b5d7d9e0eff5 100644 > >--- a/include/uapi/drm/drm_mode.h > >+++ b/include/uapi/drm/drm_mode.h > >@@ -598,8 +598,11 @@ struct drm_mode_crtc_lut { > > }; > > > > struct drm_color_ctm { > >- /* Conversion matrix in S31.32 format. */ > >- __s64 matrix[9]; > >+ /* > >+ * Conversion matrix in S31.32 sign-magnitude > >+ * (not two's complement!) format. > >+ */ > >+ __u64 matrix[9]; > > Isn't changing the type liable to break something for someone? I hope not. Renaming the member would be a no no, but just changing the type should be mostly safe I think. I suppose if someone is building something with very strict compiler -W flags and -Werror it might cause a build failure, so I guess one might label it as a minor api break but not an abi break. If people think that's a serious concern I guess we can keep the __s64, but I'd rather not give people that much rope to hang themselves by interpreting it as 2's complement.
On 2018-02-22 04:42 PM, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The documentation for the ctm matrix suggests a two's complement > format, but at least the i915 implementation is using sign-magnitude > instead. And looks like malidp is doing the same. Change the docs > to match the current implementation, and change the type from __s64 > to __u64 to drive the point home. > > Cc: dri-devel@lists.freedesktop.org > Cc: Mihail Atanassov <mihail.atanassov@arm.com> > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Brian Starkey <brian.starkey@arm.com> > Cc: Mali DP Maintainers <malidp@foss.arm.com> > Cc: Johnson Lin <johnson.lin@intel.com> > Cc: Uma Shankar <uma.shankar@intel.com> > Cc: Shashank Sharma <shashank.sharma@intel.com> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Good clarification. Our new CTM implementation (1) actually assumed two's complement but nobody's using it yet, so we'll patch it to convert. Reviewed-by: Harry Wentland <harry.wentland@amd.com> (1) https://patchwork.freedesktop.org/patch/204005/ Harry > --- > include/uapi/drm/drm_mode.h | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 2c575794fb52..b5d7d9e0eff5 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -598,8 +598,11 @@ struct drm_mode_crtc_lut { > }; > > struct drm_color_ctm { > - /* Conversion matrix in S31.32 format. */ > - __s64 matrix[9]; > + /* > + * Conversion matrix in S31.32 sign-magnitude > + * (not two's complement!) format. > + */ > + __u64 matrix[9]; > }; > > struct drm_color_lut { >
On Fri, Feb 23, 2018 at 11:26:41AM -0500, Harry Wentland wrote: > On 2018-02-22 04:42 PM, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The documentation for the ctm matrix suggests a two's complement > > format, but at least the i915 implementation is using sign-magnitude > > instead. And looks like malidp is doing the same. Change the docs > > to match the current implementation, and change the type from __s64 > > to __u64 to drive the point home. > > > > Cc: dri-devel@lists.freedesktop.org > > Cc: Mihail Atanassov <mihail.atanassov@arm.com> > > Cc: Liviu Dudau <liviu.dudau@arm.com> > > Cc: Brian Starkey <brian.starkey@arm.com> > > Cc: Mali DP Maintainers <malidp@foss.arm.com> > > Cc: Johnson Lin <johnson.lin@intel.com> > > Cc: Uma Shankar <uma.shankar@intel.com> > > Cc: Shashank Sharma <shashank.sharma@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Good clarification. Our new CTM implementation (1) actually assumed two's complement but nobody's using it yet, so we'll patch it to convert. Nice. Looks like we caught this just in time. > > Reviewed-by: Harry Wentland <harry.wentland@amd.com> > > (1) https://patchwork.freedesktop.org/patch/204005/ > > Harry > > > --- > > include/uapi/drm/drm_mode.h | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > > index 2c575794fb52..b5d7d9e0eff5 100644 > > --- a/include/uapi/drm/drm_mode.h > > +++ b/include/uapi/drm/drm_mode.h > > @@ -598,8 +598,11 @@ struct drm_mode_crtc_lut { > > }; > > > > struct drm_color_ctm { > > - /* Conversion matrix in S31.32 format. */ > > - __s64 matrix[9]; > > + /* > > + * Conversion matrix in S31.32 sign-magnitude > > + * (not two's complement!) format. > > + */ > > + __u64 matrix[9]; > > }; > > > > struct drm_color_lut { > >
On Fri, Feb 23, 2018 at 04:04:10PM +0200, Ville Syrjälä wrote: > On Fri, Feb 23, 2018 at 01:52:22PM +0000, Brian Starkey wrote: > > Hi Ville, > > > > On Thu, Feb 22, 2018 at 11:42:29PM +0200, Ville Syrjala wrote: > > >From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > >The documentation for the ctm matrix suggests a two's complement > > >format, but at least the i915 implementation is using sign-magnitude > > >instead. And looks like malidp is doing the same. Change the docs > > >to match the current implementation, and change the type from __s64 > > >to __u64 to drive the point home. > > > > I totally agree that this is a good idea, but... > > > > > > > >Cc: dri-devel@lists.freedesktop.org > > >Cc: Mihail Atanassov <mihail.atanassov@arm.com> > > >Cc: Liviu Dudau <liviu.dudau@arm.com> > > >Cc: Brian Starkey <brian.starkey@arm.com> > > >Cc: Mali DP Maintainers <malidp@foss.arm.com> > > >Cc: Johnson Lin <johnson.lin@intel.com> > > >Cc: Uma Shankar <uma.shankar@intel.com> > > >Cc: Shashank Sharma <shashank.sharma@intel.com> > > >Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > >--- > > > include/uapi/drm/drm_mode.h | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > >diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > > >index 2c575794fb52..b5d7d9e0eff5 100644 > > >--- a/include/uapi/drm/drm_mode.h > > >+++ b/include/uapi/drm/drm_mode.h > > >@@ -598,8 +598,11 @@ struct drm_mode_crtc_lut { > > > }; > > > > > > struct drm_color_ctm { > > >- /* Conversion matrix in S31.32 format. */ > > >- __s64 matrix[9]; > > >+ /* > > >+ * Conversion matrix in S31.32 sign-magnitude > > >+ * (not two's complement!) format. > > >+ */ > > >+ __u64 matrix[9]; > > > > Isn't changing the type liable to break something for someone? > > I hope not. Renaming the member would be a no no, but just changing the > type should be mostly safe I think. I suppose if someone is building > something with very strict compiler -W flags and -Werror it might cause > a build failure, so I guess one might label it as a minor api break but > not an abi break. > > If people think that's a serious concern I guess we can keep the > __s64, but I'd rather not give people that much rope to hang > themselves by interpreting it as 2's complement. OK, no one complained loudly so I've gone and pushed this to drm-misc-next. Now we wait and see whether I can dodge the egg...
On Fri, Feb 23, 2018 at 11:26:41AM -0500, Harry Wentland wrote: > On 2018-02-22 04:42 PM, Ville Syrjala wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > The documentation for the ctm matrix suggests a two's complement > > format, but at least the i915 implementation is using sign-magnitude > > instead. And looks like malidp is doing the same. Change the docs > > to match the current implementation, and change the type from __s64 > > to __u64 to drive the point home. > > > > Cc: dri-devel@lists.freedesktop.org > > Cc: Mihail Atanassov <mihail.atanassov@arm.com> > > Cc: Liviu Dudau <liviu.dudau@arm.com> > > Cc: Brian Starkey <brian.starkey@arm.com> > > Cc: Mali DP Maintainers <malidp@foss.arm.com> > > Cc: Johnson Lin <johnson.lin@intel.com> > > Cc: Uma Shankar <uma.shankar@intel.com> > > Cc: Shashank Sharma <shashank.sharma@intel.com> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Good clarification. Our new CTM implementation (1) actually assumed > two's complement but nobody's using it yet, so we'll patch it to > convert. Another reason to start looking into igt and the tests there? That would have caught this too ... -Daniel > > Reviewed-by: Harry Wentland <harry.wentland@amd.com> > > (1) https://patchwork.freedesktop.org/patch/204005/ > > Harry > > > --- > > include/uapi/drm/drm_mode.h | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > > index 2c575794fb52..b5d7d9e0eff5 100644 > > --- a/include/uapi/drm/drm_mode.h > > +++ b/include/uapi/drm/drm_mode.h > > @@ -598,8 +598,11 @@ struct drm_mode_crtc_lut { > > }; > > > > struct drm_color_ctm { > > - /* Conversion matrix in S31.32 format. */ > > - __s64 matrix[9]; > > + /* > > + * Conversion matrix in S31.32 sign-magnitude > > + * (not two's complement!) format. > > + */ > > + __u64 matrix[9]; > > }; > > > > struct drm_color_lut { > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 2018-03-06 02:51 AM, Daniel Vetter wrote: > On Fri, Feb 23, 2018 at 11:26:41AM -0500, Harry Wentland wrote: >> On 2018-02-22 04:42 PM, Ville Syrjala wrote: >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> >>> The documentation for the ctm matrix suggests a two's complement >>> format, but at least the i915 implementation is using sign-magnitude >>> instead. And looks like malidp is doing the same. Change the docs >>> to match the current implementation, and change the type from __s64 >>> to __u64 to drive the point home. >>> >>> Cc: dri-devel@lists.freedesktop.org >>> Cc: Mihail Atanassov <mihail.atanassov@arm.com> >>> Cc: Liviu Dudau <liviu.dudau@arm.com> >>> Cc: Brian Starkey <brian.starkey@arm.com> >>> Cc: Mali DP Maintainers <malidp@foss.arm.com> >>> Cc: Johnson Lin <johnson.lin@intel.com> >>> Cc: Uma Shankar <uma.shankar@intel.com> >>> Cc: Shashank Sharma <shashank.sharma@intel.com> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >> Good clarification. Our new CTM implementation (1) actually assumed >> two's complement but nobody's using it yet, so we'll patch it to >> convert. > > Another reason to start looking into igt and the tests there? That would > have caught this too ... There need to be new IGT tests that can actually test for signed-magnitude vs two's compliment differences, which would have to be tests of the CTM in some non-RGB colorspace where negative numbers are actually meaningful. The existing tests will test a negative matrix but in RGB colorspace any negative number will simply map to zero/black, no matter the magnitude. I'll put such a test on our team's backlog but not sure when we'll actually get to it. We'd have to check if we could have a meaningful test for this with the current capabilities of DC. Harry > -Daniel > >> >> Reviewed-by: Harry Wentland <harry.wentland@amd.com> >> >> (1) https://patchwork.freedesktop.org/patch/204005/ >> >> Harry >> >>> --- >>> include/uapi/drm/drm_mode.h | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >>> index 2c575794fb52..b5d7d9e0eff5 100644 >>> --- a/include/uapi/drm/drm_mode.h >>> +++ b/include/uapi/drm/drm_mode.h >>> @@ -598,8 +598,11 @@ struct drm_mode_crtc_lut { >>> }; >>> >>> struct drm_color_ctm { >>> - /* Conversion matrix in S31.32 format. */ >>> - __s64 matrix[9]; >>> + /* >>> + * Conversion matrix in S31.32 sign-magnitude >>> + * (not two's complement!) format. >>> + */ >>> + __u64 matrix[9]; >>> }; >>> >>> struct drm_color_lut { >>> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h index 2c575794fb52..b5d7d9e0eff5 100644 --- a/include/uapi/drm/drm_mode.h +++ b/include/uapi/drm/drm_mode.h @@ -598,8 +598,11 @@ struct drm_mode_crtc_lut { }; struct drm_color_ctm { - /* Conversion matrix in S31.32 format. */ - __s64 matrix[9]; + /* + * Conversion matrix in S31.32 sign-magnitude + * (not two's complement!) format. + */ + __u64 matrix[9]; }; struct drm_color_lut {