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

[Daniel Stone]

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.

(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