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

[Chad Versace]

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.

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