<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 13, 2018 at 10:55 AM, Daniel Stone <span dir="ltr"><<a href="mailto:daniel@fooishbar.org" target="_blank">daniel@fooishbar.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Jason,<br>
<span class="gmail-"><br>
On 9 February 2018 at 23:43, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br>
> +static void<br>
> +get_wsi_format_modifier_<wbr>properties_list(const struct anv_physical_device *physical_device,<br>
> + VkFormat vk_format,<br>
> + struct wsi_format_modifier_<wbr>properties_list *list)<br>
> +{<br>
> + const struct anv_format *anv_format = anv_get_format(vk_format);<br>
> +<br>
> + VK_OUTARRAY_MAKE(out, list->modifier_properties, &list->modifier_count);<br>
> +<br>
> + /* This is a simplified list where all the modifiers are available */<br>
> + assert(vk_format == VK_FORMAT_B8G8R8_SRGB ||<br>
> + vk_format == VK_FORMAT_B8G8R8_UNORM ||<br>
> + vk_format == VK_FORMAT_B8G8R8A8_SRGB ||<br>
> + vk_format == VK_FORMAT_B8G8R8A8_UNORM);<br>
> +<br>
> + uint64_t modifiers[] = {<br>
> + DRM_FORMAT_MOD_LINEAR,<br>
> + I915_FORMAT_MOD_X_TILED,<br>
> + I915_FORMAT_MOD_Y_TILED,<br>
> + I915_FORMAT_MOD_Y_TILED_CCS,<br>
> + };<br>
<br>
</span>Can this please be inverted to preferred-first? Most of the other APIs<br>
do this, so you can pop the head off the list.<span class="gmail-"><br></span></blockquote><div><br></div><div>If someone is popping the head of the list, they're using modifiers wrong. When we get Chad's extension in full, they're going to have to also call vkGetPhysicalDeviceImageFormatProperties to verify that your parameters actually work with the modifier. There may be other restrictions. For instance, we're likely to disallow CCS for storage images.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
> +static uint32_t<br>
> +score_drm_format_mod(uint64_t modifier)<br>
> +{<br>
> + switch (modifier) {<br>
> + case DRM_FORMAT_MOD_LINEAR: return 1;<br>
> + case I915_FORMAT_MOD_X_TILED: return 2;<br>
> + case I915_FORMAT_MOD_Y_TILED: return 3;<br>
> + case I915_FORMAT_MOD_Y_TILED_CCS: return 4;<br>
> + default: unreachable("bad DRM format modifier");<br>
> + }<br>
> +}<br>
<br>
</span>If the array previously could be shared, you could just score based on<br>
the modifier's index in the array, rather than explicit scoring as<br>
here.<span class="gmail-"><br></span></blockquote><div><br></div><div>True. It's really too bad that they're in different files. I'm not entirely happy that anv_image and anv_format are split up. I've also thought about trying to move some of it into ISL but that's a bit sticky because the list of supported modifiers needs to be per-driver since things like Y_TILED_CCS takes a little extra work in each driver. All in all, it's not a lot of code duplication so meh?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
> @@ -706,8 +747,13 @@ void anv_GetImageSubresourceLayout(<br>
> VkSubresourceLayout* layout)<br>
> {<br>
> ANV_FROM_HANDLE(anv_image, image, _image);<br>
> - const struct anv_surface *surface =<br>
> - get_surface(image, subresource->aspectMask);<br>
> +<br>
> + const struct anv_surface *surface;<br>
> + if (subresource->aspectMask == VK_IMAGE_ASPECT_PLANE_1_BIT_<wbr>KHR &&<br>
> + isl_drm_modifier_has_aux(<wbr>image->drm_format_mod))<br>
> + surface = &image->planes[0].aux_surface;<br>
> + else<br>
> + surface = get_surface(image, subresource->aspectMask);<br>
<br>
</span>It would probably be easier to split this up: just core modifier (and<br>
Y-tiling) support in this patch, and CCS/aux support in a follow-up<br>
one. Ditto the next two hunks.<br>
<span class="gmail-"><br>
> @@ -367,13 +367,9 @@ wsi_create_native_image(const struct wsi_swapchain *chain,<br>
> if (result != VK_SUCCESS)<br>
> goto fail;<br>
><br>
> - if (wsi->supports_modifiers)<br>
> + if (image_modifier_count > 0) {<br>
> image->drm_modifier = wsi->image_get_modifier(image-<wbr>>image);<br>
> - else<br>
> - image->drm_modifier = DRM_FORMAT_MOD_INVALID;<br>
<br>
</span>Belongs in a previous patch, and also going along the right lines but<br>
not quite correct. ;) The other hunks also need to be squashed into<br>
some other patch.<br></blockquote><div><br></div><div>I'm not sure what you mean by "not quite correct". I've moved it into "vulkan/wsi: Add modifiers support to wsi_create_native_image"<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I'm happy with the rest though; looks good from what I can see, and<br>
the new WSI bits have made life much easier!<br>
<br>
Cheers,<br>
Daniel<br>
</blockquote></div><br></div></div>