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

[Jason Ekstrand]

On Wed, Feb 14, 2018 at 4:13 AM, Daniel Stone <daniel at fooishbar.org> wrote:

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

Ok, we clearly have different philosophies here so we should get that
sorted.  My philosophy is that the winsys code will look at the
wsi_device::supports_modifiers flag and not ask for modifiers if it's
false.  You seem to think that the winsys code should just go ahead and ask
for modifiers all the time and we will try to deal with it in
wsi_create_native.  Thoughts?  Arguments?  Strong opinions?

If we keep my philosophy, we should add asserts to better document and
enforce it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180214/ca3ffc76/attachment.html>