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

[Jason Ekstrand]

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

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

For that, we have a supports_modifiers field in the wsi_display struct.
The winsys is supposed to check that flag and not call into here with
modifiers if the device doesn't support them.


> > +      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).
>

In order to work with Chad's spec, we actually have to do a bit more work.
Not only do we have to check if the modifier is in the modifiers list, we
also have to call vkGetPhysicalDeviceImageFormatProperties with the
modifier to verify that the image creation parameters are ok.  It may bet
that we can skip the first step (checking if the modifier is in the list)
and just trust vkGetPhysicalDeviceImageFormatProperties to return zero
features if it isn't.  I'm not 100% clear on that.


> 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'?
>

"Try it and see if it fails" isn't really a very Vulkan approach.  It's
convenient, but not very Vulkan.  I'm not quite sure how I feel.


> > +   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;
>

See comment above.


> > +      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?)
>

it's not always offset of 0 which is bad since it is a dedicated
allocation.  I think using a dedicated allocation is a bit wrong for
modifiers so that'll have to change.

--Jason


> The rest looks good to me though. Thanks very much for taking this on!
> Reviewed-by: Daniel Stone <daniels at collabora.com>
>
> Cheers,
> Daniel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180213/d717859c/attachment-0001.html>