<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jan 23, 2017 at 10:21 PM, Ben Widawsky <<a href="mailto:ben@bwidawsk.net" target="_blank">ben@bwidawsk.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This patch begins introducing how we'll actually handle the potentially many modifiers coming in from the API, how we'll store them, and the structure in the code to support it. Prior to this patch, the Y-tiled modifier would be entirely ignored. It shouldn't actually be used until this point because we've not bumped the DRIimage extension version (which is a requirement to use modifiers). With X-tiling: Writes: 6,583.58 MiB Reads: 6,540.93 MiB With Y-tiling: Writes: 5,361.78 MiB Reads 6,052.45 MiB Savings per frame Writes: 2 MiB Reads: .8 MiB Similar functionality was introduced and then reverted here: commit 6a0d036483caf87d43ebe2edd1905873446c9589 Author: Ben Widawsky <<a href="mailto:ben@bwidawsk.net">ben@bwidawsk.net</a>> Date: Thu Apr 21 20:14:58 2016 -0700 i965: Always use Y-tiled buffers on SKL+ Signed-off-by: Ben Widawsky <<a href="mailto:benjamin.widawsky@intel.com">benjamin.widawsky@intel.com</a>> Reviewed-by: Eric Engestrom <<a href="mailto:eric.engestrom@imgtec.com">eric.engestrom@imgtec.com</a>> Acked-by: Daniel Stone <<a href="mailto:daniels@collabora.com">daniels@collabora.com</a>> --- src/mesa/drivers/dri/i965/intel_screen.c | 55 ++++++++++++++++++++++++++++---- 1 file changed, 49 insertions(+), 6 deletions(-) diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c index ebfa74a8ff..6eaf146181 100644 --- a/src/mesa/drivers/dri/i965/intel_screen.c +++ b/src/mesa/drivers/dri/i965/intel_screen.c @@ -23,6 +23,7 @@ * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ +#include <drm_fourcc.h> #include <errno.h> #include <time.h> #include <unistd.h> @@ -559,16 +560,35 @@ select_best_modifier(struct gen_device_info *devinfo, const uint64_t *modifiers, const unsigned count) { - uint64_t modifier = DRM_FORMAT_MOD_INVALID; +#define YTILE (1 << 1) +#define LINEAR (1 << 0) + + const uint64_t prio_modifiers[] = { I915_FORMAT_MOD_Y_TILED, DRM_FORMAT_MOD_LINEAR }; + uint32_t modifier_bitmask = 0; /* API only allows 32 */ </blockquote><div> </div><div>Is 32 an API limitation? Only looking locally, it seems to just be a limitation of the algorithm used here for picking modifiers. Given that the algorithm can be easily extended if needed, I see no reason why we need to make it an API limitation. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> for (int i = 0; i < count; i++) { switch (modifiers[i]) { case DRM_FORMAT_MOD_LINEAR: - return modifiers[i]; + modifier_bitmask |= LINEAR; + break; + case I915_FORMAT_MOD_Y_TILED: + if (devinfo->gen < 9) { + _mesa_warning(NULL, "Invalid Y-tiling parameter\n"); </blockquote><div> </div><div>Why? We can support Y tiling on all our hardware, you just can't scan it out prior to Sky Lake. However, if you were doing mesa <-> mesa for compositing, Y-tiling is perfectly fine all the way back. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + continue; + } + + modifier_bitmask |= YTILE; + break; } } - return modifier; + if (modifier_bitmask) + return prio_modifiers[ffsll(modifier_bitmask)-1]; + + return DRM_FORMAT_MOD_INVALID; + +#undef LINEAR +#undef YTILE } static __DRIimage * @@ -599,6 +619,9 @@ __intel_create_image(__DRIscreen *dri_screen, case DRM_FORMAT_MOD_LINEAR: tiling = I915_TILING_NONE; break; + case I915_FORMAT_MOD_Y_TILED: + tiling = I915_TILING_Y; + break; case DRM_FORMAT_MOD_INVALID: default: break; @@ -650,8 +673,26 @@ intel_create_image_with_modifiers(__DRIscreen *dri_screen, const unsigned count, void *loaderPrivate) { - return __intel_create_image(dri_screen, width, height, format, 0, NULL, 0, - loaderPrivate); + uint64_t local_mods[count]; </blockquote><div> </div><div>Should we really be stack allocating this? It comes in from the user and could be arbitrarily large. It's probably not a problem, but this patch got me thinking about modifier counts. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + int local_count = 0; + + /* This compacts the actual modifiers to the ones we know how to handle */ + for (int i = 0; i < count; i++) { + switch (modifiers[i]) { + case I915_FORMAT_MOD_Y_TILED: + case DRM_FORMAT_MOD_LINEAR: + local_mods[local_count++] = modifiers[i]; + break; + case DRM_FORMAT_MOD_INVALID: + unreachable("Invalid modifiers specified\n"); + default: + /* Modifiers from other vendors would land here. */ + break; + } + } + + return __intel_create_image(dri_screen, width, height, format, 0, + local_mods, local_count, loaderPrivate); } static GLboolean @@ -1912,7 +1953,9 @@ intelAllocateBuffer(__DRIscreen *dri_screen, if (intelBuffer == NULL) return NULL; - /* The front and back buffers are color buffers, which are X tiled. */ + /* The front and back buffers are color buffers, which are X tiled. GEN9+ + * supports Y tiled and compressed buffers, but there is no way to plumb that + * through to here. */ uint32_t tiling = I915_TILING_X; unsigned long pitch; int cpp = format / 8; -- 2.11.0 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div> </div></div>