<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Mar 17, 2017 at 5:34 PM, Ben Widawsky <span dir="ltr"><<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This patch begins introducing how we'll actually handle the potentially<br>
many modifiers coming in from the API, how we'll store them, and the<br>
structure in the code to support it.<br>
<br>
Prior to this patch, the Y-tiled modifier would be entirely ignored. It<br>
shouldn't actually be used until this point because we've not bumped the<br>
DRIimage extension version (which is a requirement to use modifiers).<br>
<br>
With X-tiling:<br>
Writes: 6,583.58 MiB<br>
Reads: 6,540.93 MiB<br>
<br>
With Y-tiling:<br>
Writes: 5,361.78 MiB<br>
Reads 6,052.45 MiB<br>
<br>
Savings per frame<br>
Writes: 2 MiB<br>
Reads: .8 MiB<br>
<br>
Similar functionality was introduced and then reverted here:<br>
<br>
commit 6a0d036483caf87d43ebe2edd19058<wbr>73446c9589<br>
Author: Ben Widawsky <<a href="mailto:ben@bwidawsk.net">ben@bwidawsk.net</a>><br>
Date: Thu Apr 21 20:14:58 2016 -0700<br>
<br>
i965: Always use Y-tiled buffers on SKL+<br>
<br>
v2: Use last set bit instead of first set bit in modifiers to address<br>
bug found by Daniel Stone.<br>
<br>
Signed-off-by: Ben Widawsky <<a href="mailto:benjamin.widawsky@intel.com">benjamin.widawsky@intel.com</a>><br>
Reviewed-by: Eric Engestrom <<a href="mailto:eric.engestrom@imgtec.com">eric.engestrom@imgtec.com</a>><br>
Acked-by: Daniel Stone <<a href="mailto:daniels@collabora.com">daniels@collabora.com</a>><br>
---<br>
src/mesa/drivers/dri/i965/<wbr>intel_screen.c | 55 ++++++++++++++++++++++++++++--<wbr>--<br>
1 file changed, 49 insertions(+), 6 deletions(-)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/<wbr>intel_screen.c b/src/mesa/drivers/dri/i965/<wbr>intel_screen.c<br>
index 22ab3a30b6..1954757d1e 100644<br>
--- a/src/mesa/drivers/dri/i965/<wbr>intel_screen.c<br>
+++ b/src/mesa/drivers/dri/i965/<wbr>intel_screen.c<br>
@@ -23,6 +23,7 @@<br>
* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.<br>
*/<br>
<br>
+#include <drm_fourcc.h><br>
#include <errno.h><br>
#include <time.h><br>
#include <unistd.h><br>
@@ -520,16 +521,35 @@ select_best_modifier(struct gen_device_info *devinfo,<br>
const uint64_t *modifiers,<br>
const unsigned count)<br>
{<br>
- uint64_t modifier = DRM_FORMAT_MOD_INVALID;<br>
+#define YTILE (1 << 1)<br>
+#define LINEAR (1 << 0)<br>
+<br>
+ const uint64_t prio_modifiers[] = { I915_FORMAT_MOD_Y_TILED, DRM_FORMAT_MOD_LINEAR };<br>
+ uint32_t modifier_bitmask = 0; /* API only allows 32 */<br></blockquote><div><br></div><div>The bitfield thing is still confusing to me. Here's an idea on how we could maybe make it better.<br><br></div><div>enum modifier_priority {<br></div><div> MODIFIER_PRIORITY_LINEAR,<br> MODIFIER_PRIORITY_X,<br> MODIFIER_PRIORITY_Y,<br> MODIFIER_PRIORITY_Y_CCS,</div><div>};<br><br></div><div>uint32_t priority_to_modifier[] = {<br></div><div> [MODIFIER_PRIORITY_LINEAR] = DRM_FORMAT_MOD_LINEAR,<br></div><div> [MODIFIER_PRIORITY_X] = I915_FORMAT_MOD_X_TILED,<br><div> [MODIFIER_PRIORITY_Y] = I915_FORMAT_MOD_Y_TILED,<br><div> [MODIFIER_PRIORITY_Y_CCS] = I915_FORMAT_MOD_Y_TILED_CCS,<span class="gmail-"></span><br></div></div></div><div>}<br><br></div><div>enum modier_priority prio = 0;<br></div><div>for (int i = 0; i < count; i++) {<br></div><div> switch (modifiers[i]) {<br></div><div> case DRM_FORMAT_MOD_LINEAR:<br></div><div> prio = MAX2(prio, MODIFIER_PRIORITY_LINEAR);<br></div><div> break;<br><br><div> case DRM_FORMAT_MOD_X_TILED:<br></div><div> prio = MAX2(prio, MODIFIER_PRIORITY_X);<br></div> break;<span class=""><br><br></span><div> case DRM_FORMAT_MOD_Y_TILED:<br></div><div> prio = MAX2(prio, MODIFIER_PRIORITY_Y);<br></div> break;<span class=""><br><br></span><div> case DRM_FORMAT_MOD_Y_TILIED_CCS:<br></div><div> prio = MAX2(prio, MODIFIER_PRIORITY_Y_CCS);<br></div> break;</div><div>}<br></div><div><br></div>return priority_to_modifier[prio];<br><br></div><div class="gmail_quote">How does this strike your fancy? I'm ok with the bit set approach if you really prefer it but I find it hard to reason about.<br></div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
for (int i = 0; i < count; i++) {<br>
switch (modifiers[i]) {<br>
case DRM_FORMAT_MOD_LINEAR:<br>
- return modifiers[i];<br>
+ modifier_bitmask |= LINEAR;<br>
+ break;<br>
+ case I915_FORMAT_MOD_Y_TILED:<br>
+ if (devinfo->gen < 9) {<br>
+ _mesa_warning(NULL, "Invalid Y-tiling parameter\n");<br>
+ continue;<br></blockquote><div><br></div><div>This isn't invalid. It's just invalid for scanout. If you wanted to create an image to share between two GL implementations, Y-tiling works fine on everything.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ }<br>
+<br>
+ modifier_bitmask |= YTILE;<br>
+ break;<br>
}<br>
}<br>
<br>
- return modifier;<br>
+ if (modifier_bitmask)<br>
+ return prio_modifiers[util_last_<wbr>bit64(modifier_bitmask)-1];<br>
+<br>
+ return DRM_FORMAT_MOD_INVALID;<br>
+<br>
+#undef LINEAR<br>
+#undef YTILE<br>
}<br>
<br>
static __DRIimage *<br>
@@ -560,6 +580,9 @@ intel_create_image_common(__<wbr>DRIscreen *dri_screen,<br>
case DRM_FORMAT_MOD_LINEAR:<br>
tiling = I915_TILING_NONE;<br>
break;<br>
+ case I915_FORMAT_MOD_Y_TILED:<br>
+ tiling = I915_TILING_Y;<br>
+ break;<br>
case DRM_FORMAT_MOD_INVALID:<br>
default:<br>
break;<br>
@@ -611,8 +634,26 @@ intel_create_image_with_<wbr>modifiers(__DRIscreen *dri_screen,<br>
const unsigned count,<br>
void *loaderPrivate)<br>
{<br>
- return intel_create_image_common(dri_<wbr>screen, width, height, format, 0, NULL,<br>
- 0, loaderPrivate);<br>
+ uint64_t local_mods[count];<br>
+ int local_count = 0;<br>
+<br>
+ /* This compacts the actual modifiers to the ones we know how to handle */<br>
+ for (int i = 0; i < count; i++) {<br>
+ switch (modifiers[i]) {<br>
+ case I915_FORMAT_MOD_Y_TILED:<br>
+ case DRM_FORMAT_MOD_LINEAR:<br>
+ local_mods[local_count++] = modifiers[i];<br>
+ break;<br>
+ case DRM_FORMAT_MOD_INVALID:<br>
+ unreachable("Invalid modifiers specified\n");<br></blockquote><div><br></div><div>I'm not sure this is truely unreachable. What prevents someone from calling create_image_with_modifiers with INVALID? Do we have a check somewhere higher up that that prevents it? If this is something that's actually triggerable from client code, then I think we probably want to just let it fall through.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ default:<br>
+ /* Modifiers from other vendors would land here. */<br>
+ break;<br>
+ }<br>
+ }<br>
+<br>
+ return intel_create_image_common(dri_<wbr>screen, width, height, format, 0,<br>
+ local_mods, local_count, loaderPrivate);<br>
}<br>
<br>
static GLboolean<br>
@@ -1867,7 +1908,9 @@ intelAllocateBuffer(__<wbr>DRIscreen *dri_screen,<br>
if (intelBuffer == NULL)<br>
return NULL;<br>
<br>
- /* The front and back buffers are color buffers, which are X tiled. */<br>
+ /* The front and back buffers are color buffers, which are X tiled. GEN9+<br>
+ * supports Y tiled and compressed buffers, but there is no way to plumb that<br>
+ * through to here. */<br>
uint32_t tiling = I915_TILING_X;<br>
unsigned long pitch;<br>
int cpp = format / 8;<br>
<span class="gmail-HOEnZb"><font color="#888888">--<br>
2.12.0<br>
<br>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div></div>