On Thu, Oct 11, 2018 at 11:21 AM Jason Ekstrand <ja...@jlekstrand.net>
wrote:
> On Wed, Oct 10, 2018 at 5:17 PM Chad Versace <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...
>
Hope is not lost! I was digging around and I remembered why I left
anv_get_format_plane alone. It very specifically checks for
VK_IMAGE_TILING_OPTIMAL for the RGBA/RGBX workaround. This is because
VK_IMAGE_TILING_OPTIMAL lets us hide details about the actual image data
under the hood so we can just fake things. Both VK_IMAGE_TILING_LINEAR and
VK_IMAGE_TILING_DRM_FORMAT_MODIFIER_EXT make the image data transparent so
no faking is allowed. Also, there are no RGB fourcc formats; only RGBA and
RGBX. I'm happy to add an assert to that effect in anv_get_format_plane.
--Jason
> --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@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
