Hi Jason,
On 9 February 2018 at 23:43, Jason Ekstrand <ja...@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 <dani...@collabora.com>
Cheers,
Daniel
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
