[Mesa-dev] [RFC PATCH v1 09/30] anv: Refactor get_image_format_properties() - Reduce params

[Jason Ekstrand]

Actually, ignore all my comments.  Patches 1-18 are

Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>

On Tue, Nov 7, 2017 at 8:47 AM, Jason Ekstrand <jason at jlekstrand.net> wrote:

> On Tue, Nov 7, 2017 at 6:47 AM, Chad Versace <chadversary at 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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20171107/dc860408/attachment.html>