On 11/10/2018 17:21, Jason Ekstrand wrote:
On Wed, Oct 10, 2018 at 5:17 PM Chad Versace <chadvers...@chromium.org
<mailto:chadvers...@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
Switching over to isl_tiling internally?
Having all entrypoints immediately convert VkImageTiling into that.
-
Lionel
>
> 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@lists.freedesktop.org
<mailto:mesa-dev@lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
