Actually, ignore all my comments. Patches 1-18 are
Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
On Tue, Nov 7, 2017 at 8:47 AM, Jason Ekstrand <ja...@jlekstrand.net> wrote:
> 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
