[Mesa-dev] [PATCH 13/16] anv/image: Add support for modifiers for WSI

[Daniel Stone]

Hi Jason,

On 9 February 2018 at 23:43, Jason Ekstrand <jason at jlekstrand.net> wrote:
> +static void
> +get_wsi_format_modifier_properties_list(const struct anv_physical_device *physical_device,
> +                                        VkFormat vk_format,
> +                                        struct wsi_format_modifier_properties_list *list)
> +{
> +   const struct anv_format *anv_format = anv_get_format(vk_format);
> +
> +   VK_OUTARRAY_MAKE(out, list->modifier_properties, &list->modifier_count);
> +
> +   /* This is a simplified list where all the modifiers are available */
> +   assert(vk_format == VK_FORMAT_B8G8R8_SRGB ||
> +          vk_format == VK_FORMAT_B8G8R8_UNORM ||
> +          vk_format == VK_FORMAT_B8G8R8A8_SRGB ||
> +          vk_format == VK_FORMAT_B8G8R8A8_UNORM);
> +
> +   uint64_t modifiers[] = {
> +      DRM_FORMAT_MOD_LINEAR,
> +      I915_FORMAT_MOD_X_TILED,
> +      I915_FORMAT_MOD_Y_TILED,
> +      I915_FORMAT_MOD_Y_TILED_CCS,
> +   };

Can this please be inverted to preferred-first? Most of the other APIs
do this, so you can pop the head off the list.

> +static uint32_t
> +score_drm_format_mod(uint64_t modifier)
> +{
> +   switch (modifier) {
> +   case DRM_FORMAT_MOD_LINEAR: return 1;
> +   case I915_FORMAT_MOD_X_TILED: return 2;
> +   case I915_FORMAT_MOD_Y_TILED: return 3;
> +   case I915_FORMAT_MOD_Y_TILED_CCS: return 4;
> +   default: unreachable("bad DRM format modifier");
> +   }
> +}

If the array previously could be shared, you could just score based on
the modifier's index in the array, rather than explicit scoring as
here.

> @@ -706,8 +747,13 @@ void anv_GetImageSubresourceLayout(
>      VkSubresourceLayout*                        layout)
>  {
>     ANV_FROM_HANDLE(anv_image, image, _image);
> -   const struct anv_surface *surface =
> -      get_surface(image, subresource->aspectMask);
> +
> +   const struct anv_surface *surface;
> +   if (subresource->aspectMask == VK_IMAGE_ASPECT_PLANE_1_BIT_KHR &&
> +       isl_drm_modifier_has_aux(image->drm_format_mod))
> +      surface = &image->planes[0].aux_surface;
> +   else
> +      surface = get_surface(image, subresource->aspectMask);

It would probably be easier to split this up: just core modifier (and
Y-tiling) support in this patch, and CCS/aux support in a follow-up
one. Ditto the next two hunks.

> @@ -367,13 +367,9 @@ wsi_create_native_image(const struct wsi_swapchain *chain,
>     if (result != VK_SUCCESS)
>        goto fail;
>
> -   if (wsi->supports_modifiers)
> +   if (image_modifier_count > 0) {
>        image->drm_modifier = wsi->image_get_modifier(image->image);
> -   else
> -      image->drm_modifier = DRM_FORMAT_MOD_INVALID;

Belongs in a previous patch, and also going along the right lines but
not quite correct. ;) The other hunks also need to be squashed into
some other patch.

I'm happy with the rest though; looks good from what I can see, and
the new WSI bits have made life much easier!

Cheers,
Daniel