<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Mar 21, 2017 at 8:33 AM, 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-21 08:07:22, Jason Ekstrand wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Tue, Mar 21, 2017 at 8:04 AM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> On Mon, Mar 20, 2017 at 8:35 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). Measuring later in the series with kmscbe: Linear: Read bandwidth: 1048.44 MiB/s Write bandwidth: 1483.17 MiB/s Y-tiled: Read bandwidth: 471.13 MiB/s Write bandwidth: 589.10 MiB/s 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. v3: Use the new priority modifier selection thing. This nullifies the bug fixed by v2 also. 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/gbm/backends/dri/gbm_dri.c | 18 ++++++++++++++++ src/mesa/drivers/dri/i965/intel_screen.c | 36 ++++++++++++++++++++++++++++---- 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/src/gbm/backends/dri/gbm_dri.c b/src/gbm/backends/dri/gbm_dri.c index a7ac149365..a78ea89fca 100644 --- a/src/gbm/backends/dri/gbm_dri.c +++ b/src/gbm/backends/dri/gbm_dri.c @@ -1143,6 +1143,15 @@ gbm_dri_bo_create(struct gbm_device *gbm, goto failed; } + if (modifiers) { + for (int i = 0; i < count; i++) + if (modifiers[i] == DRM_FORMAT_MOD_INVALID) { + fprintf(stderr, "Invalid modifier passed in DRM_FORMAT_MOD_INVALID"); + errno = EINVAL; + goto failed; + } + } + bo->image = dri->image->createImageWithModifiers(dri->screen, width, height, @@ -1240,6 +1249,15 @@ gbm_dri_surface_create(struct gbm_device *gbm, return NULL; } + if (modifiers) { + for (int i = 0; i < count; i++) + if (modifiers[i] == DRM_FORMAT_MOD_INVALID) { + fprintf(stderr, "Invalid modifier passed in DRM_FORMAT_MOD_INVALID"); + errno = EINVAL; + return NULL; + } + } </blockquote> This could be its own patch but I don't care too much. </blockquote> Also, you don't need this anymore now that MOD_INVALID just falls through and doesn't assert below. Feel free to drop it, pull it into its own patch, or leave it here. (I have a mild preference for dropping it and just ignoring MOD_INVALID but don't care too much). With that, series is Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> </blockquote> </div></div> intel_create_image_with_modifiers() (which will be called by this) will assert if INVALID modifier slips through - which is what I wanted. I don't want any dri implementation to have to deal with INVALID modifiers. So I think it needs to be here. Did I misunderstand you?</blockquote><div> </div><div>That's a problem because the user could provide a list of modifiers none of which we support. If this happens, we'll return INVALID from select_best_modifier anyway. There's no way that a higher layer can possibly know whether or not a list of modifiers contains something that we can handle unless we require LINEAR to be in the list (which seems a bit harsh). I think we should just fail to create the image in that case and set errno to -EINVAL or something like that. </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"> + <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> surf = calloc(1, sizeof *surf); if (surf == NULL) { errno = ENOMEM; diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c index 14e60ef1a1..e4f858ed33 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> @@ -518,11 +519,13 @@ intel_destroy_image(__DRIimage *image) enum modifier_priority { MODIFIER_PRIORITY_INVALID = 0, MODIFIER_PRIORITY_LINEAR, + MODIFIER_PRIORITY_Y, }; const uint64_t priority_to_modifier[] = { [MODIFIER_PRIORITY_INVALID] = DRM_FORMAT_MOD_INVALID, [MODIFIER_PRIORITY_LINEAR] = DRM_FORMAT_MOD_LINEAR, + [MODIFIER_PRIORITY_Y] = I915_FORMAT_MOD_Y_TILED, }; static uint64_t @@ -530,11 +533,13 @@ select_best_modifier(struct gen_device_info *devinfo, const uint64_t *modifiers, const unsigned count) { - enum modifier_priority prio = MODIFIER_PRIORITY_INVALID; for (int i = 0; i < count; i++) { switch (modifiers[i]) { + case I915_FORMAT_MOD_Y_TILED: + prio = MAX2(prio, MODIFIER_PRIORITY_Y); + break; case DRM_FORMAT_MOD_LINEAR: prio = MAX2(prio, MODIFIER_PRIORITY_LINEAR); break; @@ -575,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; @@ -626,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"); + 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 @@ -1997,7 +2023,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 </blockquote> </blockquote></blockquote> </div></div></blockquote></div> </div></div>