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); }
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Emil Velikov -
Lionel Landwerlin