[PATCH] drm: fix lut value extraction function

[PATCH] drm: fix lut value extraction function

[Lionel Landwerlin]

When extracting the value at full precision (16 bits), no need to
round the value.

This was spotted by Jani when running sparse. Unfortunately this fix
doesn't get rid of the warning.

Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Reported-by: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Stone <daniels@collabora.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: dri-devel@lists.freedesktop.org
Fixes: 5488dc16fde7 ("drm: introduce pipe color correction properties")
---
 include/drm/drm_crtc.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index e0170bf..da63b4d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2600,10 +2600,14 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev,
 static inline uint32_t drm_color_lut_extract(uint32_t user_input,
 					     uint32_t bit_precision)
 {
-	uint32_t val = user_input + (1 << (16 - bit_precision - 1));
+	uint32_t val = user_input;
 	uint32_t max = 0xffff >> (16 - bit_precision);
 
-	val >>= 16 - bit_precision;
+	/* Round only if we're not using full precision. */
+	if (bit_precision < 16) {
+		val += 1UL << (16 - bit_precision - 1);
+		val >>= 16 - bit_precision;
+	}
 
 	return clamp_val(val, 0, max);
 }
-- 
2.8.0.rc3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

✗ Fi.CI.BAT: warning for drm: fix lut value extraction function

[Patchwork]

== Series Details ==

Series: drm: fix lut value extraction function
URL   : https://patchwork.freedesktop.org/series/4753/
State : warning

== Summary ==

Series 4753v1 drm: fix lut value extraction function
http://patchwork.freedesktop.org/api/1.0/series/4753/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                dmesg-warn -> PASS       (bsw-nuc-2)
        Subgroup basic-rte:
                pass       -> DMESG-WARN (bsw-nuc-2)

bdw-nuci7        total:192  pass:180  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:192  pass:171  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:192  pass:154  dwarn:1   dfail:0   fail:0   skip:37 
byt-nuc          total:192  pass:156  dwarn:1   dfail:0   fail:0   skip:35 
hsw-brixbox      total:192  pass:170  dwarn:0   dfail:0   fail:0   skip:22 
ivb-t430s        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25 
skl-i5k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
skl-i7k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:192  pass:181  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:192  pass:158  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:192  pass:158  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1677/

83ed25fa1b956275542da63eb98dc8fd2291329d drm-intel-nightly: 2016y-03m-22d-15h-20m-55s UTC integration manifest
6b24ccdc61e6e5e6326582acfe1e58d3212f8b4b drm: fix lut value extraction function

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

Re: [PATCH] drm: fix lut value extraction function

[Lionel Landwerlin]

Ping?

On 22/03/16 14:10, Lionel Landwerlin wrote:
> When extracting the value at full precision (16 bits), no need to
> round the value.
>
> This was spotted by Jani when running sparse. Unfortunately this fix
> doesn't get rid of the warning.
>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Reported-by: Jani Nikula <jani.nikula@intel.com>
> Cc: Daniel Stone <daniels@collabora.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: dri-devel@lists.freedesktop.org
> Fixes: 5488dc16fde7 ("drm: introduce pipe color correction properties")
> ---
>   include/drm/drm_crtc.h | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index e0170bf..da63b4d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2600,10 +2600,14 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev,
>   static inline uint32_t drm_color_lut_extract(uint32_t user_input,
>   					     uint32_t bit_precision)
>   {
> -	uint32_t val = user_input + (1 << (16 - bit_precision - 1));
> +	uint32_t val = user_input;
>   	uint32_t max = 0xffff >> (16 - bit_precision);
>   
> -	val >>= 16 - bit_precision;
> +	/* Round only if we're not using full precision. */
> +	if (bit_precision < 16) {
> +		val += 1UL << (16 - bit_precision - 1);
> +		val >>= 16 - bit_precision;
> +	}
>   
>   	return clamp_val(val, 0, max);
>   }

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

Re: [PATCH] drm: fix lut value extraction function

[Daniel Vetter]

On Mon, Apr 18, 2016 at 12:09:51PM +0100, Lionel Landwerlin wrote:
> Ping?
> 
> On 22/03/16 14:10, Lionel Landwerlin wrote:
> >When extracting the value at full precision (16 bits), no need to
> >round the value.
> >
> >This was spotted by Jani when running sparse. Unfortunately this fix
> >doesn't get rid of the warning.

It sounded like no bug, and the patch itself fails to appease sparse. And
I didn't check what's upsetting sparse itself, so figured "nothing to do
here until a real fix shows up".

Should I do something here?
-Daniel

> >
> >Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> >Reported-by: Jani Nikula <jani.nikula@intel.com>
> >Cc: Daniel Stone <daniels@collabora.com>
> >Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >Cc: Matt Roper <matthew.d.roper@intel.com>
> >Cc: dri-devel@lists.freedesktop.org
> >Fixes: 5488dc16fde7 ("drm: introduce pipe color correction properties")
> >---
> >  include/drm/drm_crtc.h | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> >diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >index e0170bf..da63b4d 100644
> >--- a/include/drm/drm_crtc.h
> >+++ b/include/drm/drm_crtc.h
> >@@ -2600,10 +2600,14 @@ static inline struct drm_property *drm_property_find(struct drm_device *dev,
> >  static inline uint32_t drm_color_lut_extract(uint32_t user_input,
> >  					     uint32_t bit_precision)
> >  {
> >-	uint32_t val = user_input + (1 << (16 - bit_precision - 1));
> >+	uint32_t val = user_input;
> >  	uint32_t max = 0xffff >> (16 - bit_precision);
> >-	val >>= 16 - bit_precision;
> >+	/* Round only if we're not using full precision. */
> >+	if (bit_precision < 16) {
> >+		val += 1UL << (16 - bit_precision - 1);
> >+		val >>= 16 - bit_precision;
> >+	}
> >  	return clamp_val(val, 0, max);
> >  }
> 

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

Re: [PATCH] drm: fix lut value extraction function

[Emil Velikov]

On 18 April 2016 at 13:36, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Apr 18, 2016 at 12:09:51PM +0100, Lionel Landwerlin wrote:
>> Ping?
>>
>> On 22/03/16 14:10, Lionel Landwerlin wrote:
>> >When extracting the value at full precision (16 bits), no need to
>> >round the value.
>> >
>> >This was spotted by Jani when running sparse. Unfortunately this fix
>> >doesn't get rid of the warning.
>
> It sounded like no bug, and the patch itself fails to appease sparse. And
> I didn't check what's upsetting sparse itself, so figured "nothing to do
> here until a real fix shows up".
>
According to the C99 standard a left shift with negative value is
undefined. And we're hitting this case at full precision ;-)

> Should I do something here?
Having the above information in, optionally with the warning message
in place of the current commit message would be recommended imho.

After all the patch is a definite fix, even if I personally I'd write
the inline helper via a switch (makes things dead obvious).

Regardless,
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: fix lut value extraction function

[Daniel Vetter]

On Mon, Apr 18, 2016 at 03:40:11PM +0100, Emil Velikov wrote:
> On 18 April 2016 at 13:36, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Apr 18, 2016 at 12:09:51PM +0100, Lionel Landwerlin wrote:
> >> Ping?
> >>
> >> On 22/03/16 14:10, Lionel Landwerlin wrote:
> >> >When extracting the value at full precision (16 bits), no need to
> >> >round the value.
> >> >
> >> >This was spotted by Jani when running sparse. Unfortunately this fix
> >> >doesn't get rid of the warning.
> >
> > It sounded like no bug, and the patch itself fails to appease sparse. And
> > I didn't check what's upsetting sparse itself, so figured "nothing to do
> > here until a real fix shows up".
> >
> According to the C99 standard a left shift with negative value is
> undefined. And we're hitting this case at full precision ;-)

Well commit message says sparse is still unhappy. So I'm not sure whether
the fix is good enough? And the issue with compiler/static checker noise
is that we really should aim to shut them up completely, because broken
windows and all that (even if it's sometimes a fallacy, I think it applies
here).
-Daniel

> 
> > Should I do something here?
> Having the above information in, optionally with the warning message
> in place of the current commit message would be recommended imho.
> 
> After all the patch is a definite fix, even if I personally I'd write
> the inline helper via a switch (makes things dead obvious).
> 
> Regardless,
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
> 
> -Emil

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

Re: [PATCH] drm: fix lut value extraction function

[Emil Velikov]

On 18 April 2016 at 16:53, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Apr 18, 2016 at 03:40:11PM +0100, Emil Velikov wrote:
>> On 18 April 2016 at 13:36, Daniel Vetter <daniel@ffwll.ch> wrote:
>> > On Mon, Apr 18, 2016 at 12:09:51PM +0100, Lionel Landwerlin wrote:
>> >> Ping?
>> >>
>> >> On 22/03/16 14:10, Lionel Landwerlin wrote:
>> >> >When extracting the value at full precision (16 bits), no need to
>> >> >round the value.
>> >> >
>> >> >This was spotted by Jani when running sparse. Unfortunately this fix
>> >> >doesn't get rid of the warning.
>> >
>> > It sounded like no bug, and the patch itself fails to appease sparse. And
>> > I didn't check what's upsetting sparse itself, so figured "nothing to do
>> > here until a real fix shows up".
>> >
>> According to the C99 standard a left shift with negative value is
>> undefined. And we're hitting this case at full precision ;-)
>
> Well commit message says sparse is still unhappy. So I'm not sure whether
> the fix is good enough? And the issue with compiler/static checker noise
> is that we really should aim to shut them up completely, because broken
> windows and all that (even if it's sometimes a fallacy, I think it applies
> here).
Afaics the fix resolves a real bug and the final solution is bug free
(although one can drop the L form 1UL). If I have to guess I'd say
that sparse does not realise that the precision cannot be greater than
16.

Quick and easy check is to add an early bail out (if bit_precision >
16 return user_input). The compiler will optimise it out anyway (it
does propagate/fold the constants) the end binary will be fine.
Another approach is the earlier suggested, switch which will also get
optimised in the final binary.

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

Re: [PATCH] drm: fix lut value extraction function

[Daniel Vetter]

On Mon, Apr 18, 2016 at 08:39:00PM +0100, Emil Velikov wrote:
> On 18 April 2016 at 16:53, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Apr 18, 2016 at 03:40:11PM +0100, Emil Velikov wrote:
> >> On 18 April 2016 at 13:36, Daniel Vetter <daniel@ffwll.ch> wrote:
> >> > On Mon, Apr 18, 2016 at 12:09:51PM +0100, Lionel Landwerlin wrote:
> >> >> Ping?
> >> >>
> >> >> On 22/03/16 14:10, Lionel Landwerlin wrote:
> >> >> >When extracting the value at full precision (16 bits), no need to
> >> >> >round the value.
> >> >> >
> >> >> >This was spotted by Jani when running sparse. Unfortunately this fix
> >> >> >doesn't get rid of the warning.
> >> >
> >> > It sounded like no bug, and the patch itself fails to appease sparse. And
> >> > I didn't check what's upsetting sparse itself, so figured "nothing to do
> >> > here until a real fix shows up".
> >> >
> >> According to the C99 standard a left shift with negative value is
> >> undefined. And we're hitting this case at full precision ;-)
> >
> > Well commit message says sparse is still unhappy. So I'm not sure whether
> > the fix is good enough? And the issue with compiler/static checker noise
> > is that we really should aim to shut them up completely, because broken
> > windows and all that (even if it's sometimes a fallacy, I think it applies
> > here).
> Afaics the fix resolves a real bug and the final solution is bug free
> (although one can drop the L form 1UL). If I have to guess I'd say
> that sparse does not realise that the precision cannot be greater than
> 16.
> 
> Quick and easy check is to add an early bail out (if bit_precision >
> 16 return user_input). The compiler will optimise it out anyway (it
> does propagate/fold the constants) the end binary will be fine.
> Another approach is the earlier suggested, switch which will also get
> optimised in the final binary.

Ok, count me convinced ;-) Applied to drm-misc.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel