<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Oct 10, 2018 at 5:17 PM Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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. </blockquote><div> </div><div>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...</div><div> </div><div>--Jason </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> > > 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 > <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div></div>