Hi Jason, On 9 February 2018 at 23:43, Jason Ekstrand <ja...@jlekstrand.net> wrote: > - /* For images using modifiers, we require a dedicated allocation > - * and we set the BO tiling to match the tiling of the underlying > - * modifier. This is a bit unfortunate as this is completely > - * pointless for Vulkan. However, GL needs to be able to map things > - * so it needs the tiling to be set. The only way to do this in a > - * non-racy way is to set the tiling in the creator of the BO so > that > - * makes it our job. > - * > - * One of these days, once the GL driver learns to not map things > - * through the GTT in random places, we can drop this and start > - * allowing multiple modified images in the same BO. > + /* For legacy scanout images, no tiling information is passed along > + * directly. Instead, consumers pull the tiling from BO. > */ > - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) { > - assert(isl_drm_modifier_get_info(image->drm_format_mod)->tiling > == > - image->planes[0].surface.isl.tiling); > + if (image->legacy_scanout) {
Hm. There's a couple of things here. There's no-modifier protocols like DRI2, current DRI3 (see patches to improve this), and wl_drm which don't carry modifier information. For those, we set the tiling so the importer can know what we've done. This is either linear or X-tiled, and is more legacy-winsys than legacy-scanout. For actual scanout, the kernel very specifically demands that if the BO is X-tiled, then set_tiling be called with TILING_X. This applies even if we explicitly allocate with MOD_X_TILED and pass that in via drmModeAddFB2WithModifiers: there is an explicit check for !!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED). I think this would be better called image->needs_set_tiling or similar, which enforced dedicated allocation and a set_tiling call, and was called if we have no modifier information, _or_ if the buffer is X-tiled. (Is there a port of vkcube to the new modifier bits somewhere?) > @@ -2217,7 +2206,7 @@ void anv_GetImageMemoryRequirements2KHR( > switch (ext->sType) { > case VK_STRUCTURE_TYPE_MEMORY_DEDICATED_REQUIREMENTS_KHR: { > VkMemoryDedicatedRequirementsKHR *requirements = (void *)ext; > - if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) { > + if (image->legacy_scanout) { > /* Require a dedicated allocation for images with modifiers. Also this comment is stale. > + /** True if this is a legacy scanout image */ > + bool legacy_scanout; The comment in the header could also perhaps go a brief way to explaining the bear trap outlined above. Rest looks good though. Cheers, Daniel _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev