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 --- 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 {
-----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 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 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 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
dri-devel@lists.freedesktop.org
-
Brian Starkey -
Daniel Vetter -
Harry Wentland -
Shankar, Uma -
Ville Syrjala -
Ville Syrjälä