Re: [Intel-gfx] [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

2018-03-06 Thread Harry Wentland 
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ä 
>>>
>>> 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-de...@lists.freedesktop.org
>>> Cc: Mihail Atanassov 
>>> Cc: Liviu Dudau 
>>> Cc: Brian Starkey 
>>> Cc: Mali DP Maintainers 
>>> Cc: Johnson Lin 
>>> Cc: Uma Shankar 
>>> Cc: Shashank Sharma 
>>> Signed-off-by: Ville Syrjälä 
>>
>> 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 
>>
>> (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-de...@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

2018-03-05 Thread Daniel Vetter 
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ä 
> > 
> > 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-de...@lists.freedesktop.org
> > Cc: Mihail Atanassov 
> > Cc: Liviu Dudau 
> > Cc: Brian Starkey 
> > Cc: Mali DP Maintainers 
> > Cc: Johnson Lin 
> > Cc: Uma Shankar 
> > Cc: Shashank Sharma 
> > Signed-off-by: Ville Syrjälä 
> 
> 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 
> 
> (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-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

2018-02-28 Thread Ville Syrjälä 
On Fri, Feb 23, 2018 at 04:04:10PM +0200, Ville Syrjälä wrote:
> On Fri, Feb 23, 2018 at 01:52:22PM +, Brian Starkey wrote:
> > Hi Ville,
> > 
> > On Thu, Feb 22, 2018 at 11:42:29PM +0200, Ville Syrjala wrote:
> > >From: Ville Syrjälä 
> > >
> > >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-de...@lists.freedesktop.org
> > >Cc: Mihail Atanassov 
> > >Cc: Liviu Dudau 
> > >Cc: Brian Starkey 
> > >Cc: Mali DP Maintainers 
> > >Cc: Johnson Lin 
> > >Cc: Uma Shankar 
> > >Cc: Shashank Sharma 
> > >Signed-off-by: Ville Syrjälä 
> > >---
> > > 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...

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

2018-02-23 Thread Ville Syrjälä 
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ä 
> > 
> > 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-de...@lists.freedesktop.org
> > Cc: Mihail Atanassov 
> > Cc: Liviu Dudau 
> > Cc: Brian Starkey 
> > Cc: Mali DP Maintainers 
> > Cc: Johnson Lin 
> > Cc: Uma Shankar 
> > Cc: Shashank Sharma 
> > Signed-off-by: Ville Syrjälä 
> 
> 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 
> 
> (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 {
> > 

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

2018-02-23 Thread Harry Wentland 
On 2018-02-22 04:42 PM, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> 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-de...@lists.freedesktop.org
> Cc: Mihail Atanassov 
> Cc: Liviu Dudau 
> Cc: Brian Starkey 
> Cc: Mali DP Maintainers 
> Cc: Johnson Lin 
> Cc: Uma Shankar 
> Cc: Shashank Sharma 
> Signed-off-by: Ville Syrjälä 

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 

(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 {
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

2018-02-23 Thread Ville Syrjälä 
On Fri, Feb 23, 2018 at 01:52:22PM +, Brian Starkey wrote:
> Hi Ville,
> 
> On Thu, Feb 22, 2018 at 11:42:29PM +0200, Ville Syrjala wrote:
> >From: Ville Syrjälä 
> >
> >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-de...@lists.freedesktop.org
> >Cc: Mihail Atanassov 
> >Cc: Liviu Dudau 
> >Cc: Brian Starkey 
> >Cc: Mali DP Maintainers 
> >Cc: Johnson Lin 
> >Cc: Uma Shankar 
> >Cc: Shashank Sharma 
> >Signed-off-by: Ville Syrjälä 
> >---
> > 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.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

2018-02-23 Thread Brian Starkey 

Hi Ville,

On Thu, Feb 22, 2018 at 11:42:29PM +0200, Ville Syrjala wrote:

From: Ville Syrjälä 

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-de...@lists.freedesktop.org
Cc: Mihail Atanassov 
Cc: Liviu Dudau 
Cc: Brian Starkey 
Cc: Mali DP Maintainers 
Cc: Johnson Lin 
Cc: Uma Shankar 
Cc: Shashank Sharma 
Signed-off-by: Ville Syrjälä 
---
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


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

2018-02-23 Thread Shankar, Uma 


>-Original Message-
>From: Ville Syrjala [mailto:ville.syrj...@linux.intel.com]
>Sent: Friday, February 23, 2018 3:12 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: dri-de...@lists.freedesktop.org; Mihail Atanassov
>; Liviu Dudau ; Brian
>Starkey ; Mali DP Maintainers
>; Lin, Johnson ; Shankar, Uma
>; Sharma, Shashank 
>Subject: [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude
>representation
>
>From: Ville Syrjälä 
>
>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 

>Cc: dri-de...@lists.freedesktop.org
>Cc: Mihail Atanassov 
>Cc: Liviu Dudau 
>Cc: Brian Starkey 
>Cc: Mali DP Maintainers 
>Cc: Johnson Lin 
>Cc: Uma Shankar 
>Cc: Shashank Sharma 
>Signed-off-by: Ville Syrjälä 
>---
> 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

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/4] drm/uapi: The ctm matrix uses sign-magnitude representation

2018-02-22 Thread Ville Syrjala 
From: Ville Syrjälä 

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-de...@lists.freedesktop.org
Cc: Mihail Atanassov 
Cc: Liviu Dudau 
Cc: Brian Starkey 
Cc: Mali DP Maintainers 
Cc: Johnson Lin 
Cc: Uma Shankar 
Cc: Shashank Sharma 
Signed-off-by: Ville Syrjälä 
---
 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

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx