<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone <span dir="ltr"><<a href="mailto:daniel@fooishbar.org" target="_blank">daniel@fooishbar.org</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">Hi Jason,<br>
<span class="gmail-"><br>
On 9 February 2018 at 23:43, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br>
> -         /* For images using modifiers, we require a dedicated allocation<br>
> -          * and we set the BO tiling to match the tiling of the underlying<br>
> -          * modifier.  This is a bit unfortunate as this is completely<br>
> -          * pointless for Vulkan.  However, GL needs to be able to map things<br>
> -          * so it needs the tiling to be set.  The only way to do this in a<br>
> -          * non-racy way is to set the tiling in the creator of the BO so that<br>
> -          * makes it our job.<br>
> -          *<br>
> -          * One of these days, once the GL driver learns to not map things<br>
> -          * through the GTT in random places, we can drop this and start<br>
> -          * allowing multiple modified images in the same BO.<br>
> +         /* For legacy scanout images, no tiling information is passed along<br>
> +          * directly.  Instead, consumers pull the tiling from BO.<br>
>            */<br>
> -         if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) {<br>
> -            assert(isl_drm_modifier_get_<wbr>info(image->drm_format_mod)-><wbr>tiling ==<br>
> -                   image->planes[0].surface.isl.<wbr>tiling);<br>
> +         if (image->legacy_scanout) {<br>
<br>
</span>Hm. There's a couple of things here. There's no-modifier protocols<br>
like DRI2, current DRI3 (see patches to improve this), and wl_drm<br>
which don't carry modifier information. For those, we set the tiling<br>
so the importer can know what we've done. This is either linear or<br>
X-tiled, and is more legacy-winsys than legacy-scanout.<br>
<br>
For actual scanout, the kernel very specifically demands that if the<br>
BO is X-tiled, then set_tiling be called with TILING_X. This applies<br>
even if we explicitly allocate with MOD_X_TILED and pass that in via<br>
drmModeAddFB2WithModifiers: there is an explicit check for<br>
!!(bo_tiling == TILING_X) == !!(modifier == MOD_X_TILED).<br>
<br>
I think this would be better called image->needs_set_tiling or<br>
similar, which enforced dedicated allocation and a set_tiling call,<br>
and was called if we have no modifier information, _or_ if the buffer<br>
is X-tiled.<br>
</blockquote><div><br></div><div>Yeah, image->needs_set_tiling seems reasonable to me.  I'll make the change.<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">
(Is there a port of vkcube to the new modifier bits somewhere?)<br>
<span class="gmail-"><br>
> @@ -2217,7 +2206,7 @@ void anv_<wbr>GetImageMemoryRequirements2KHR<wbr>(<br>
>        switch (ext->sType) {<br>
>        case VK_STRUCTURE_TYPE_MEMORY_<wbr>DEDICATED_REQUIREMENTS_KHR: {<br>
>           VkMemoryDedicatedRequirementsK<wbr>HR *requirements = (void *)ext;<br>
> -         if (image->drm_format_mod != DRM_FORMAT_MOD_INVALID) {<br>
> +         if (image->legacy_scanout) {<br>
>              /* Require a dedicated allocation for images with modifiers.<br>
<br>
</span>Also this comment is stale.<span class="gmail-"><br></span></blockquote><div><br></div><div>Yup.<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"><span class="gmail-">
> +   /** True if this is a legacy scanout image */<br>
> +   bool legacy_scanout;<br>
<br>
</span>The comment in the header could also perhaps go a brief way to<br>
explaining the bear trap outlined above.<br></blockquote><div><br></div><div>I've written the following:<br><br>   /** True if this is needs to be bound to an appropriately tiled BO.<br>    *<br>    * When not using modifiers, consumers such as X11, Wayland, and KMS need<br>    * the tiling passed via I915_GEM_SET_TILING.  When exporting these buffers<br>    * we require a dedicated allocation so that we can know to allocate a<br>    * tiled buffer.<br>    */<br><br></div></div></div></div>