| Message ID | 1458655833-19547-1-git-send-email-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > }
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); > > } >
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
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
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
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
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); }
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(-)