[Mesa-dev] [PATCH 2/5] anv: Drop some VK_IMAGE_TILING_OPTIMAL checks

[Jason Ekstrand]

On Wed, Oct 10, 2018 at 5:17 PM Chad Versace <chadversary at chromium.org>
wrote:

> On Mon 01 Oct 2018, Jason Ekstrand wrote:
> > The DRM format modifiers extension adds a TILING_DRM_FORMAT_MODIFIER
> > which will be used for modifiers so we can no longer use OPTIMAL to
> > indicate tiled inside the driver.
> > ---
> >  src/intel/vulkan/anv_formats.c     | 2 +-
> >  src/intel/vulkan/anv_image.c       | 6 +++---
> >  src/intel/vulkan/genX_cmd_buffer.c | 2 +-
> >  3 files changed, 5 insertions(+), 5 deletions(-)
>
> This patch needs to also fixup some places that test tiling ==
> VK_IMAGE_TILING_LINEAR. I found at least one such place in
> anv_formats.c. The patch also needs to fixup any functions that use
> early returns that are conditioned (perhaps indirectly) on tiling;
> anv_get_format_plane() comes to mind.
>
> I quickly tried, as an experiment, to expand this patch into a correct
> patch. After a few minutes of typing, I concluded that this patch series
> takes the wrong order-of-implementation approach.
>
> For example, what happens to all the calls to anv_get_format_plane() in
> anv_blorp.c? Those need fixing too. Simply fixing the tiling checks is
> not enough, especially because VK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXT
> allows DRM_FORMAT_MOD_LINEAR.
>
> Instead, I think the right way to do this is to incrementally, following
> the callchains bottom-up, teach each function to understand
> VK_EXT_image_drm_format_modifier. Only when that's complete, then move
> WSI to the new structs. Otherwise, there is too much room for
> accidential implementation gaps; gaps that may not hurt WSI, but will
> make it more difficult to discern what-works-and-what-doesn't while
> implementing the full extension.
>
> Just now, I tried writing a patch that starts at the bottom of the
> callchain, anv_get_format_plane(), and teaches it about modifiers. The
> patch is more invasive than expected. Maybe the patch is messy because
> more preliminary cleanups are needed. I'm unsure; I need to take a break
> and revisit it in the morning.
>

I just took a look at anv_get_format_plane and my hopes of this being a
quick extension to implement instantly evaporated. :-(  In the light of
modifiers, piles of assumptions we've been making are invalid.  *sigh*
Time to go rewrite the driver...

--Jason


> >
> > diff --git a/src/intel/vulkan/anv_formats.c
> b/src/intel/vulkan/anv_formats.c
> > index 33faf7cc37f..d44aae1c127 100644
> > --- a/src/intel/vulkan/anv_formats.c
> > +++ b/src/intel/vulkan/anv_formats.c
> > @@ -508,7 +508,7 @@ get_image_format_features(const struct
> gen_device_info *devinfo,
> >        return 0;
> >
> >     struct anv_format_plane base_plane_format = plane_format;
> > -   if (vk_tiling == VK_IMAGE_TILING_OPTIMAL) {
> > +   if (vk_tiling != VK_IMAGE_TILING_LINEAR) {
> >        base_plane_format = anv_get_format_plane(devinfo, vk_format,
> >
>  VK_IMAGE_ASPECT_COLOR_BIT,
> >                                                 VK_IMAGE_TILING_LINEAR);
> > diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> > index b0d8c560adb..70594d6c053 100644
> > --- a/src/intel/vulkan/anv_image.c
> > +++ b/src/intel/vulkan/anv_image.c
> > @@ -334,7 +334,7 @@ make_surface(const struct anv_device *dev,
> >     bool needs_shadow = false;
> >     if (dev->info.gen <= 8 &&
> >         (vk_info->flags &
> VK_IMAGE_CREATE_BLOCK_TEXEL_VIEW_COMPATIBLE_BIT) &&
> > -       vk_info->tiling == VK_IMAGE_TILING_OPTIMAL) {
> > +       vk_info->tiling != VK_IMAGE_TILING_LINEAR) {
> >        assert(isl_format_is_compressed(plane_format.isl_format));
> >        tiling_flags = ISL_TILING_LINEAR_BIT;
> >        needs_shadow = true;
> > @@ -829,7 +829,7 @@ anv_layout_to_aux_usage(const struct gen_device_info
> * const devinfo,
> >        return ISL_AUX_USAGE_NONE;
> >
> >     /* All images that use an auxiliary surface are required to be
> tiled. */
> > -   assert(image->tiling == VK_IMAGE_TILING_OPTIMAL);
> > +   assert(image->planes[plane].surface.isl.tiling != ISL_TILING_LINEAR);
> >
> >     /* Stencil has no aux */
> >     assert(aspect != VK_IMAGE_ASPECT_STENCIL_BIT);
> > @@ -953,7 +953,7 @@ anv_layout_to_fast_clear_type(const struct
> gen_device_info * const devinfo,
> >        return ANV_FAST_CLEAR_NONE;
> >
> >     /* All images that use an auxiliary surface are required to be
> tiled. */
> > -   assert(image->tiling == VK_IMAGE_TILING_OPTIMAL);
> > +   assert(image->planes[plane].surface.isl.tiling != ISL_TILING_LINEAR);
> >
> >     /* Stencil has no aux */
> >     assert(aspect != VK_IMAGE_ASPECT_STENCIL_BIT);
> > diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> > index 099c30f3d66..821506761e2 100644
> > --- a/src/intel/vulkan/genX_cmd_buffer.c
> > +++ b/src/intel/vulkan/genX_cmd_buffer.c
> > @@ -967,7 +967,7 @@ transition_color_buffer(struct anv_cmd_buffer
> *cmd_buffer,
> >     if (base_layer >= anv_image_aux_layers(image, aspect, base_level))
> >        return;
> >
> > -   assert(image->tiling == VK_IMAGE_TILING_OPTIMAL);
> > +   assert(image->planes[plane].surface.isl.tiling != ISL_TILING_LINEAR);
> >
> >     if (initial_layout == VK_IMAGE_LAYOUT_UNDEFINED ||
> >         initial_layout == VK_IMAGE_LAYOUT_PREINITIALIZED) {
> > --
> > 2.17.1
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181011/16fd5541/attachment-0001.html>