<div dir="ltr"><div><div>This looks fine to me.  We should probably also do this for snorm formats.  I don't care if that's part of this or in a separate patch.<br></div>--Jason<br><br></div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason.ekstrand@intel.com">jason.ekstrand@intel.com</a>><br><div><div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 14, 2015 at 11:46 AM, Neil Roberts <span dir="ltr"><<a href="mailto:neil@linux.intel.com" target="_blank">neil@linux.intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">When converting to a format that has fewer bits the previous code was just<br>
shifting off the bits. This doesn't provide very accurate results. For example<br>
when converting from 8 bits to 5 bits it is equivalent to doing this:<br>
<br>
x * 32 / 256<br>
<br>
This works as if it's taking a value from a range where 256 represents 1.0 and<br>
scaling it down to a range where 32 represents 1.0. However this is not<br>
correct because it is actually 255 and 31 that represent 1.0.<br>
<br>
We can do better with a formula like this:<br>
<br>
(x * 31 + 127) / 255<br>
<br>
The +127 is to make it round correctly.<br>
<br>
</span>The new code has a special case to use uint64_t when the result of the<br>
multiplication would overflow an unsigned int. This function is inline and<br>
only ever called with constant values so hopefully the if statements will be<br>
folded.<br>
<span class=""><br>
The main incentive to do this is to make the CPU conversion path pick the same<br>
values as the hardware would if it did the conversion. This fixes failures<br>
with the ‘texsubimage pbo’ test when using the patches from here:<br>
<br>
<a href="http://lists.freedesktop.org/archives/mesa-dev/2015-January/074312.html" target="_blank">http://lists.freedesktop.org/archives/mesa-dev/2015-January/074312.html</a><br>
<br>
</span>v2: Use 64-bit arithmetic when src_bits+dst_bits > 32<br>
---<br>
<br>
Ok, this time I've actually run it through Piglit before sending it.<br>
It doesn't cause any regressions at least on IvyBridge.<br>
<br>
 src/mesa/main/format_utils.h | 15 ++++++++++++---<br>
 1 file changed, 12 insertions(+), 3 deletions(-)<br>
<br>
diff --git a/src/mesa/main/format_utils.h b/src/mesa/main/format_utils.h<br>
index b588695..b49415c 100644<br>
--- a/src/mesa/main/format_utils.h<br>
+++ b/src/mesa/main/format_utils.h<br>
@@ -96,10 +96,19 @@ _mesa_half_to_unorm(uint16_t x, unsigned dst_bits)<br>
<span class=""> static inline unsigned<br>
 _mesa_unorm_to_unorm(unsigned x, unsigned src_bits, unsigned dst_bits)<br>
 {<br>
-   if (src_bits < dst_bits)<br>
+   if (src_bits < dst_bits) {<br>
       return EXTEND_NORMALIZED_INT(x, src_bits, dst_bits);<br>
-   else<br>
-      return x >> (src_bits - dst_bits);<br>
+   } else {<br>
+      unsigned src_half = (1 << (src_bits - 1)) - 1;<br>
+<br>
</span>+      if (src_bits + dst_bits > sizeof(x) * 8) {<br>
+         assert(src_bits + dst_bits <= sizeof(uint64_t) * 8);<br>
+         return (((uint64_t) x * MAX_UINT(dst_bits) + src_half) /<br>
+                 MAX_UINT(src_bits));<br>
+      } else {<br>
<div class="HOEnZb"><div class="h5">+         return (x * MAX_UINT(dst_bits) + src_half) / MAX_UINT(src_bits);<br>
+      }<br>
+   }<br>
 }<br>
<br>
 static inline unsigned<br>
--<br>
1.9.3<br>
<br>
</div></div></blockquote></div><br></div></div></div></div></div></div>