<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 13, 2018 at 10:27 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Jason,<br>
<span class=""><br>
On 9 February 2018 at 23:43, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote:<br>
> + uint32_t image_modifier_count = 0, modifier_prop_count = 0;<br>
> + struct wsi_format_modifier_properties *modifier_props = NULL;<br>
> + uint64_t *image_modifiers = NULL;<br>
> + if (num_modifier_lists == 0) {<br>
> + /* If we don't have modifiers, fall back to the legacy "scanout" flag */<br>
> + image_wsi_info.scanout = true;<br>
<br>
</span>For below, I mean here.<br>
<span class=""><br>
> + } else {<br>
> + struct wsi_format_modifier_<wbr>properties_list modifier_props_list = {<br>
> + .sType = VK_STRUCTURE_TYPE_WSI_FORMAT_<wbr>MODIFIER_PROPERTIES_LIST_MESA,<br>
> + .pNext = NULL,<br>
> + };<br>
> + VkFormatProperties2KHR format_props = {<br>
> + .sType = VK_STRUCTURE_TYPE_FORMAT_<wbr>PROPERTIES_2_KHR,<br>
> + .pNext = &modifier_props_list,<br>
> + };<br>
> + wsi-><wbr>GetPhysicalDeviceFormatPropert<wbr>ies2KHR(wsi->pdevice,<br>
> + pCreateInfo->imageFormat,<br>
> + &format_props);<br>
> + assert(modifier_props_list.<wbr>modifier_count > 0);<br>
<br>
</span>We need to leap into the above no-modifiers branch if modifier_count<br>
is 0. This can happen if our client driver doesn't support modifiers,<br>
but the underlying winsys does.<br><div><div class="h5"></div></div></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> + uint32_t max_modifier_count = 0;<br>
> + for (uint32_t l = 0; l < num_modifier_lists; l++)<br>
> + max_modifier_count = MAX2(max_modifier_count, num_modifiers[l]);<br>
> +<br>
> + image_modifiers = vk_alloc(&chain->alloc,<br>
> + sizeof(*image_modifiers) *<br>
> + max_modifier_count,<br>
> + 8,<br>
> + VK_SYSTEM_ALLOCATION_SCOPE_<wbr>COMMAND);<br>
> + if (!image_modifiers) {<br>
> + result = VK_ERROR_OUT_OF_HOST_MEMORY;<br>
> + goto fail;<br>
> + }<br>
> +<br>
> + image_modifier_count = 0;<br>
> + for (uint32_t l = 0; l < num_modifier_lists; l++) {<br>
> + /* Walk the modifier lists and construct a list of supported<br>
> + * modifiers.<br>
> + */<br>
> + for (uint32_t i = 0; i < num_modifiers[l]; i++) {<br>
> + for (uint32_t j = 0; j < modifier_prop_count; j++) {<br>
> + if (modifier_props[j].modifier == modifiers[l][i])<br>
> + image_modifiers[image_<wbr>modifier_count++] = modifiers[l][i];<br>
> + }<br>
> + }<br>
> +<br>
> + /* We only want to take the modifiers from the first list */<br>
> + if (image_modifier_count > 0)<br>
> + break;<br>
> + }<br>
<br>
</div></div>I do quite like this approach, but think it could be even more simple:<br>
just take the entire winsys modifier list, if the intersection is<br>
non-empty. This avoids an extra allocation, and makes it slightly<br>
easier to loop through multiple lists, if we find a valid modifier<br>
from a prior set, but allocation fails and we can only progress from a<br>
subsequent set (for whatever implementation-dependent reason).<br></blockquote><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
This does explicitly contradict Chad's spec as it stands, but s/Each<br>
_modifier_/At least one _modifier_/ in<br>
VkImageDrmFormatModifierListCr<wbr>eateInfoEXT VU would fix it. Chad, is<br>
that reasonable, or would you prefer to keep it as 'every modifier<br>
must be filtered'?<span class=""><br></span></blockquote><div><br></div><div>"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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> + if (wsi->supports_modifiers)<br>
> + image->drm_modifier = wsi->image_get_modifier(image-<wbr>>image);<br>
> + else<br>
> + image->drm_modifier = DRM_FORMAT_MOD_INVALID;<br>
> +<br>
> + VkSubresourceLayout image_layout;<br>
> + if (num_modifier_lists > 0) {<br>
<br>
</span>Similarly to the first one, we need to check for both winsys & driver<br>
capability. Maybe bool use_modifiers = wsi->supports_modifiers &&<br>
num_modifier_lists > 0;<span class=""><br></span></blockquote><div><br></div><div>See comment above.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
> + const VkImageSubresource image_subresource = {<br>
> + .aspectMask = VK_IMAGE_ASPECT_COLOR_BIT,<br>
> + .mipLevel = 0,<br>
> + .arrayLayer = 0,<br>
> + };<br>
> + wsi-><wbr>GetImageSubresourceLayout(<wbr>chain->device, image->image,<br>
> + &image_subresource, &image_layout);<br>
> +<br>
> + image->num_planes = 1;<br>
> + image->sizes[0] = reqs.size;<br>
> + image->row_pitches[0] = image_layout.rowPitch;<br>
> + image->offsets[0] = 0;<br>
<br>
</span>assert(image_layout.offset == 0)<br>
<br>
Unlikely since it's a dedicated-alloc image, but ... (or is this<br>
already guaranteed and I can't read?)<br></blockquote><div><br></div><div>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.<br><br></div><div>--Jason<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The rest looks good to me though. Thanks very much for taking this on!<br>
Reviewed-by: Daniel Stone <<a href="mailto:daniels@collabora.com">daniels@collabora.com</a>><br>
<br>
Cheers,<br>
Daniel<br>
</blockquote></div><br></div></div>