[Mesa-dev] [PATCH 12/16] anv/image: Separate modifiers from legacy scanout

[Jason Ekstrand]

On Tue, Feb 13, 2018 at 10:48 AM, Daniel Stone <daniel at fooishbar.org> wrote:

> Hi Jason,
>
> On 9 February 2018 at 23:43, Jason Ekstrand <jason at 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.
>

Yeah, image->needs_set_tiling seems reasonable to me.  I'll make the change.


> (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.
>

Yup.


> > +   /** 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.
>

I've written the following:

   /** True if this is needs to be bound to an appropriately tiled BO.
    *
    * When not using modifiers, consumers such as X11, Wayland, and KMS need
    * the tiling passed via I915_GEM_SET_TILING.  When exporting these
buffers
    * we require a dedicated allocation so that we can know to allocate a
    * tiled buffer.
    */
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180213/34d39ee8/attachment-0001.html>