[Mesa-dev] [PATCH 13/16] anv/image: Add support for modifiers for WSI

[Daniel Stone]

Hi,

On 13 February 2018 at 22:29, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Tue, Feb 13, 2018 at 10:55 AM, Daniel Stone <daniel at fooishbar.org> wrote:
>> > +   uint64_t modifiers[] = {
>> > +      DRM_FORMAT_MOD_LINEAR,
>> > +      I915_FORMAT_MOD_X_TILED,
>> > +      I915_FORMAT_MOD_Y_TILED,
>> > +      I915_FORMAT_MOD_Y_TILED_CCS,
>> > +   };
>>
>> Can this please be inverted to preferred-first? Most of the other APIs
>> do this, so you can pop the head off the list.
>
> 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.

Yeah, that makes sense.

>> > +static uint32_t
>> > +score_drm_format_mod(uint64_t modifier)
>> > +{
>> > +   switch (modifier) {
>> > +   case DRM_FORMAT_MOD_LINEAR: return 1;
>> > +   case I915_FORMAT_MOD_X_TILED: return 2;
>> > +   case I915_FORMAT_MOD_Y_TILED: return 3;
>> > +   case I915_FORMAT_MOD_Y_TILED_CCS: return 4;
>> > +   default: unreachable("bad DRM format modifier");
>> > +   }
>> > +}
>>
>> If the array previously could be shared, you could just score based on
>> the modifier's index in the array, rather than explicit scoring as
>> here.
>
> 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?

Yeah, I don't actually really feel at all strongly about it tbh.

>> > @@ -367,13 +367,9 @@ wsi_create_native_image(const struct wsi_swapchain
>> > *chain,
>> >     if (result != VK_SUCCESS)
>> >        goto fail;
>> >
>> > -   if (wsi->supports_modifiers)
>> > +   if (image_modifier_count > 0) {
>> >        image->drm_modifier = wsi->image_get_modifier(image->image);
>> > -   else
>> > -      image->drm_modifier = DRM_FORMAT_MOD_INVALID;
>>
>> Belongs in a previous patch, and also going along the right lines but
>> not quite correct. ;) The other hunks also need to be squashed into
>> some other patch.
>
> 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"

Suggested fixup: https://hastebin.com/zaheyoriwa

This makes sure we only try to allocate with modifiers when _both_
winsys and driver support it.

Cheers,
Daniel