<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Mar 20, 2017 at 3:25 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"><div class="HOEnZb"><div class="h5">On 17-03-20 12:00:44, Jason Ekstrand wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Fri, Mar 17, 2017 at 5:34 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" target="_blank">ben@bwidawsk.net</a>> Date: Thu Apr 21 20:14:58 2016 -0700 i965: Always use Y-tiled buffers on SKL+ v2: Use last set bit instead of first set bit in modifiers to address bug found by Daniel Stone. Signed-off-by: Ben Widawsky <<a href="mailto:benjamin.widawsky@intel.com" target="_blank">benjamin.widawsky@intel.com</a>> Reviewed-by: Eric Engestrom <<a href="mailto:eric.engestrom@imgtec.com" target="_blank">eric.engestrom@imgtec.com</a>> Acked-by: Daniel Stone <<a href="mailto:daniels@collabora.com" target="_blank">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 22ab3a30b6..1954757d1e 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> @@ -520,16 +521,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> The bitfield thing is still confusing to me. Here's an idea on how we could maybe make it better. enum modifier_priority { MODIFIER_PRIORITY_LINEAR, MODIFIER_PRIORITY_X, MODIFIER_PRIORITY_Y, MODIFIER_PRIORITY_Y_CCS, }; uint32_t priority_to_modifier[] = { [MODIFIER_PRIORITY_LINEAR] = DRM_FORMAT_MOD_LINEAR, [MODIFIER_PRIORITY_X] = I915_FORMAT_MOD_X_TILED, [MODIFIER_PRIORITY_Y] = I915_FORMAT_MOD_Y_TILED, [MODIFIER_PRIORITY_Y_CCS] = I915_FORMAT_MOD_Y_TILED_CCS, } enum modier_priority prio = 0; for (int i = 0; i < count; i++) { switch (modifiers[i]) { case DRM_FORMAT_MOD_LINEAR: prio = MAX2(prio, MODIFIER_PRIORITY_LINEAR); break; case DRM_FORMAT_MOD_X_TILED: prio = MAX2(prio, MODIFIER_PRIORITY_X); break; case DRM_FORMAT_MOD_Y_TILED: prio = MAX2(prio, MODIFIER_PRIORITY_Y); break; case DRM_FORMAT_MOD_Y_TILIED_CCS: prio = MAX2(prio, MODIFIER_PRIORITY_Y_CCS); break; } return priority_to_modifier[prio]; 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. </blockquote> </div></div> I don't really prefer. This looks pretty good. Seems no less complex to me, but I wrote the first one, so perhaps I'm partial. Originally, I had some code in the equivalent function (before select_best was separate) which would try fallbacks, ie. if Y tiled allocation failed, it'd go down to the next modifier just walking down the bits, but logic is now gone, so there isn't really a point in the bitmask. Will respin with this and the fixes meant below. <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <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"); + continue; </blockquote> 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. </blockquote> As a general function to support modifiers, you are correct, however since this is only called for image creation, I believe the existing warning is correct. </blockquote><div> </div><div>But what if I want to create an image to share between two userspace processes with no scanout involved? While the GBM portion of the API is mostly intended for scanout, the EGL extension will be something that can and will be used for GL <-> GL. I guess we can always flip it on when we add support for the EGL extension but I see no reason why it shouldn't work through GBM. </div><div>Part of the problem may be that I really don't understand why GBM exists. It's like a linux-specific half-of-EGL thing. :-/ </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + } + + modifier_bitmask |= YTILE; + break; } } - return modifier; + if (modifier_bitmask) + return prio_modifiers[util_last_bit64(modifier_bitmask)-1]; + + return DRM_FORMAT_MOD_INVALID; + +#undef LINEAR +#undef YTILE } static __DRIimage * @@ -560,6 +580,9 @@ intel_create_image_common(__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; @@ -611,8 +634,26 @@ intel_create_image_with_modifiers(__DRIscreen *dri_screen, const unsigned count, void *loaderPrivate) { - return intel_create_image_common(dri_screen, width, height, format, 0, NULL, - 0, loaderPrivate); + uint64_t local_mods[count]; + 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"); </blockquote> 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. </blockquote> </div></div> Nothing. It was meant to be handled in the entry points, but I missed it apparently.</blockquote><div> </div><div>If you want to add code to the entrypoints to handle it, and keep the unreachable, that's fine with me. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="HOEnZb"><div class="h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> + default: + /* Modifiers from other vendors would land here. */ + break; + } + } + + return intel_create_image_common(dri_screen, width, height, format, 0, + local_mods, local_count, loaderPrivate); } static GLboolean @@ -1867,7 +1908,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.12.0 _______________________________________________ mesa-dev mailing list <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">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></blockquote> </div></div></blockquote></div> </div></div>