[Mesa-dev] [PATCH 11/16] vulkan/wsi: Add modifiers support to wsi_create_native_image

[Daniel Stone]

Hi Jason,

On 9 February 2018 at 23:43, Jason Ekstrand <jason at jlekstrand.net> wrote:
> +   uint32_t image_modifier_count = 0, modifier_prop_count = 0;
> +   struct wsi_format_modifier_properties *modifier_props = NULL;
> +   uint64_t *image_modifiers = NULL;
> +   if (num_modifier_lists == 0) {
> +      /* If we don't have modifiers, fall back to the legacy "scanout" flag */
> +      image_wsi_info.scanout = true;

For below, I mean here.

> +   } else {
> +      struct wsi_format_modifier_properties_list modifier_props_list = {
> +         .sType = VK_STRUCTURE_TYPE_WSI_FORMAT_MODIFIER_PROPERTIES_LIST_MESA,
> +         .pNext = NULL,
> +      };
> +      VkFormatProperties2KHR format_props = {
> +         .sType = VK_STRUCTURE_TYPE_FORMAT_PROPERTIES_2_KHR,
> +         .pNext = &modifier_props_list,
> +      };
> +      wsi->GetPhysicalDeviceFormatProperties2KHR(wsi->pdevice,
> +                                                 pCreateInfo->imageFormat,
> +                                                 &format_props);
> +      assert(modifier_props_list.modifier_count > 0);

We need to leap into the above no-modifiers branch if modifier_count
is 0. This can happen if our client driver doesn't support modifiers,
but the underlying winsys does.

> +      uint32_t max_modifier_count = 0;
> +      for (uint32_t l = 0; l < num_modifier_lists; l++)
> +         max_modifier_count = MAX2(max_modifier_count, num_modifiers[l]);
> +
> +      image_modifiers = vk_alloc(&chain->alloc,
> +                                 sizeof(*image_modifiers) *
> +                                 max_modifier_count,
> +                                 8,
> +                                 VK_SYSTEM_ALLOCATION_SCOPE_COMMAND);
> +      if (!image_modifiers) {
> +         result = VK_ERROR_OUT_OF_HOST_MEMORY;
> +         goto fail;
> +      }
> +
> +      image_modifier_count = 0;
> +      for (uint32_t l = 0; l < num_modifier_lists; l++) {
> +         /* Walk the modifier lists and construct a list of supported
> +          * modifiers.
> +          */
> +         for (uint32_t i = 0; i < num_modifiers[l]; i++) {
> +            for (uint32_t j = 0; j < modifier_prop_count; j++) {
> +               if (modifier_props[j].modifier == modifiers[l][i])
> +                  image_modifiers[image_modifier_count++] = modifiers[l][i];
> +            }
> +         }
> +
> +         /* We only want to take the modifiers from the first list */
> +         if (image_modifier_count > 0)
> +            break;
> +      }

I do quite like this approach, but think it could be even more simple:
just take the entire winsys modifier list, if the intersection is
non-empty. This avoids an extra allocation, and makes it slightly
easier to loop through multiple lists, if we find a valid modifier
from a prior set, but allocation fails and we can only progress from a
subsequent set (for whatever implementation-dependent reason).

This does explicitly contradict Chad's spec as it stands, but s/Each
_modifier_/At least one _modifier_/ in
VkImageDrmFormatModifierListCreateInfoEXT VU would fix it. Chad, is
that reasonable, or would you prefer to keep it as 'every modifier
must be filtered'?

> +   if (wsi->supports_modifiers)
> +      image->drm_modifier = wsi->image_get_modifier(image->image);
> +   else
> +      image->drm_modifier = DRM_FORMAT_MOD_INVALID;
> +
> +   VkSubresourceLayout image_layout;
> +   if (num_modifier_lists > 0) {

Similarly to the first one, we need to check for both winsys & driver
capability. Maybe bool use_modifiers = wsi->supports_modifiers &&
num_modifier_lists > 0;

> +      const VkImageSubresource image_subresource = {
> +         .aspectMask = VK_IMAGE_ASPECT_COLOR_BIT,
> +         .mipLevel = 0,
> +         .arrayLayer = 0,
> +      };
> +      wsi->GetImageSubresourceLayout(chain->device, image->image,
> +                                     &image_subresource, &image_layout);
> +
> +      image->num_planes = 1;
> +      image->sizes[0] = reqs.size;
> +      image->row_pitches[0] = image_layout.rowPitch;
> +      image->offsets[0] = 0;

assert(image_layout.offset == 0)

Unlikely since it's a dedicated-alloc image, but ... (or is this
already guaranteed and I can't read?)

The rest looks good to me though. Thanks very much for taking this on!
Reviewed-by: Daniel Stone <daniels at collabora.com>

Cheers,
Daniel