On Tue, Nov 7, 2017 at 6:47 AM, Chad Versace <chadvers...@chromium.org>
wrote:
> Replace parameters 'enum isl_format' and 'struct anv_format_plane' with
> new parameter 'const struct anv_format *'.
>
This patch makes me nervous for a few reasons. I made a bunch of comments
below. However, I'd like you to ignore all of them except for the one
about anv_format since I think they all get fixed in later patches.
> The goal is to incrementally fix get_image_format_properties() to return
> a correct result. Currently, it returns incorrect VkFormatFeatureFlags
> which the caller must clean up.
> ---
> src/intel/vulkan/anv_formats.c | 32 +++++++++++++++++++++-----------
> 1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_formats.c b/src/intel/vulkan/anv_
> formats.c
> index 3cc0673cbaf..151c1c9e066 100644
> --- a/src/intel/vulkan/anv_formats.c
> +++ b/src/intel/vulkan/anv_formats.c
> @@ -469,13 +469,12 @@ anv_get_format_plane(const struct gen_device_info
> *devinfo, VkFormat vk_format,
> static VkFormatFeatureFlags
> get_image_format_properties(const struct gen_device_info *devinfo,
> VkFormat vk_format,
> - enum isl_format base_isl_format,
> - struct anv_format_plane plane_format,
> + const struct anv_format *anv_format,
>
At this point, we may as well just take the vk_format and vk_tiling and be
done with it. Are we really gaining anything by also taking the anv_format?
> VkImageTiling vk_tiling)
> {
> VkFormatFeatureFlags flags = 0;
>
> - if (plane_format.isl_format == ISL_FORMAT_UNSUPPORTED)
> + if (anv_format == NULL)
>
Does the caller actually ensure that these are the same? I think it does
but it's subtle.
> return 0;
>
> const VkImageAspectFlags aspects = vk_format_aspects(vk_format);
> @@ -497,6 +496,22 @@ get_image_format_properties(const struct
> gen_device_info *devinfo,
> return flags;
> }
>
> + const struct anv_format_plane plane_format =
> + anv_get_format_plane(devinfo, vk_format, VK_IMAGE_ASPECT_COLOR_BIT,
> + vk_tiling);
>
If we want to move this a bit higher, I think we can just always use plane
0. We handle planar formats specially anyway. I'm not actually convinced
that doing so really helps us but it's a thought. In any case, I think we
want it to be after YUV.
> +
> + if (plane_format.isl_format == ISL_FORMAT_UNSUPPORTED)
> + return 0;
>
If we always use plane 0 (which you do because you're passing
ASPECT_COLOR_BIT), then this is redundant with the check at the top.
> +
> + struct anv_format_plane base_plane_format = plane_format;
> + if (vk_tiling == VK_IMAGE_TILING_OPTIMAL) {
> + base_plane_format = anv_get_format_plane(devinfo, vk_format,
> + VK_IMAGE_ASPECT_COLOR_BIT,
> + VK_IMAGE_TILING_LINEAR);
> + }
> +
> + enum isl_format base_isl_format = base_plane_format.isl_format;
> +
> /* ASTC textures must be in Y-tiled memory */
> if (vk_tiling == VK_IMAGE_TILING_LINEAR &&
> isl_format_get_layout(plane_format.isl_format)->txc ==
> ISL_TXC_ASTC)
> @@ -593,20 +608,15 @@ anv_physical_device_get_format_properties(struct
> anv_physical_device *physical_d
> if (format == NULL) {
> /* Nothing to do here */
> } else {
> - struct anv_format_plane linear_fmt, tiled_fmt;
> + struct anv_format_plane linear_fmt;
> linear_fmt = anv_get_format_plane(&physical_device->info,
> vk_format,
> VK_IMAGE_ASPECT_COLOR_BIT,
> VK_IMAGE_TILING_LINEAR);
> - tiled_fmt = anv_get_format_plane(&physical_device->info, vk_format,
> - VK_IMAGE_ASPECT_COLOR_BIT,
> - VK_IMAGE_TILING_OPTIMAL);
>
> linear = get_image_format_properties(&physical_device->info,
> vk_format,
> - linear_fmt.isl_format,
> linear_fmt,
> - VK_IMAGE_TILING_LINEAR);
> + format,
> VK_IMAGE_TILING_LINEAR);
> tiled = get_image_format_properties(&physical_device->info,
> vk_format,
> - linear_fmt.isl_format,
> tiled_fmt,
> - VK_IMAGE_TILING_OPTIMAL);
> + format,
> VK_IMAGE_TILING_OPTIMAL);
>
This is the part that really makes me nervous. Before, it was clear that
get_image_format_properties was doing different things for tiled vs. linear.
>
> /* XXX: We handle 3-channel formats by switching them out for RGBX
> or
> * RGBA formats behind-the-scenes. This works fine for textures
> --
> 2.13.0
>
> _______________________________________________
> 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
