<div dir="ltr"><div>Actually, ignore all my comments.  Patches 1-18 are<br><br></div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 7, 2017 at 8:47 AM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Tue, Nov 7, 2017 at 6:47 AM, Chad Versace <span dir="ltr"><<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Replace parameters 'enum isl_format' and 'struct anv_format_plane' with<br>
new parameter 'const struct anv_format *'.<br></blockquote><div><br></div></span><div>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.<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The goal is to incrementally fix get_image_format_properties() to return<br>
a correct result.  Currently, it returns incorrect VkFormatFeatureFlags<br>
which the caller must clean up.<br>
---<br>
 src/intel/vulkan/anv_formats.<wbr>c | 32 +++++++++++++++++++++---------<wbr>--<br>
 1 file changed, 21 insertions(+), 11 deletions(-)<br>
<br>
diff --git a/src/intel/vulkan/anv_formats<wbr>.c b/src/intel/vulkan/anv_formats<wbr>.c<br>
index 3cc0673cbaf..151c1c9e066 100644<br>
--- a/src/intel/vulkan/anv_formats<wbr>.c<br>
+++ b/src/intel/vulkan/anv_formats<wbr>.c<br>
@@ -469,13 +469,12 @@ anv_get_format_plane(const struct gen_device_info *devinfo, VkFormat vk_format,<br>
 static VkFormatFeatureFlags<br>
 get_image_format_properties(c<wbr>onst struct gen_device_info *devinfo,<br>
                             VkFormat vk_format,<br>
-                            enum isl_format base_isl_format,<br>
-                            struct anv_format_plane plane_format,<br>
+                            const struct anv_format *anv_format,<br></blockquote><div><br></div></span><div>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?<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                             VkImageTiling vk_tiling)<br>
 {<br>
    VkFormatFeatureFlags flags = 0;<br>
<br>
-   if (plane_format.isl_format == ISL_FORMAT_UNSUPPORTED)<br>
+   if (anv_format == NULL)<br></blockquote><div><br></div></span><div>Does the caller actually ensure that these are the same?  I think it does but it's subtle.<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
       return 0;<br>
<br>
    const VkImageAspectFlags aspects = vk_format_aspects(vk_format);<br>
@@ -497,6 +496,22 @@ get_image_format_properties(co<wbr>nst struct gen_device_info *devinfo,<br>
       return flags;<br>
    }<br>
<br>
+   const struct anv_format_plane plane_format =<br>
+      anv_get_format_plane(devinfo, vk_format, VK_IMAGE_ASPECT_COLOR_BIT,<br>
+                           vk_tiling);<br></blockquote><div><br></div></span><div>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.<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+   if (plane_format.isl_format == ISL_FORMAT_UNSUPPORTED)<br>
+      return 0;<br></blockquote><div><br></div></span><div>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.<br></div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+   struct anv_format_plane base_plane_format = plane_format;<br>
+   if (vk_tiling == VK_IMAGE_TILING_OPTIMAL) {<br>
+      base_plane_format = anv_get_format_plane(devinfo, vk_format,<br>
+                                               VK_IMAGE_ASPECT_COLOR_BIT,<br>
+                                               VK_IMAGE_TILING_LINEAR);<br>
+   }<br>
+<br>
+   enum isl_format base_isl_format = base_plane_format.isl_format;<br>
+<br>
    /* ASTC textures must be in Y-tiled memory */<br>
    if (vk_tiling == VK_IMAGE_TILING_LINEAR &&<br>
        isl_format_get_layout(plane_fo<wbr>rmat.isl_format)->txc == ISL_TXC_ASTC)<br>
@@ -593,20 +608,15 @@ anv_physical_device_get_format<wbr>_properties(struct anv_physical_device *physical_d<br>
    if (format == NULL) {<br>
       /* Nothing to do here */<br>
    } else {<br>
-      struct anv_format_plane linear_fmt, tiled_fmt;<br>
+      struct anv_format_plane linear_fmt;<br>
       linear_fmt = anv_get_format_plane(&physical<wbr>_device->info, vk_format,<br>
                                         VK_IMAGE_ASPECT_COLOR_BIT,<br>
                                         VK_IMAGE_TILING_LINEAR);<br>
-      tiled_fmt = anv_get_format_plane(&physical<wbr>_device->info, vk_format,<br>
-                                       VK_IMAGE_ASPECT_COLOR_BIT,<br>
-                                       VK_IMAGE_TILING_OPTIMAL);<br>
<br>
       linear = get_image_format_properties(&p<wbr>hysical_device->info, vk_format,<br>
-                                           linear_fmt.isl_format, linear_fmt,<br>
-                                           VK_IMAGE_TILING_LINEAR);<br>
+                                           format, VK_IMAGE_TILING_LINEAR);<br>
       tiled = get_image_format_properties(&p<wbr>hysical_device->info, vk_format,<br>
-                                          linear_fmt.isl_format, tiled_fmt,<br>
-                                          VK_IMAGE_TILING_OPTIMAL);<br>
+                                          format, VK_IMAGE_TILING_OPTIMAL);<br></blockquote><div><br></div></div></div><div>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.<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
       /* XXX: We handle 3-channel formats by switching them out for RGBX or<br>
        * RGBA formats behind-the-scenes.  This works fine for textures<br>
<span class="m_914192482832774171HOEnZb"><font color="#888888">--<br>
2.13.0<br>
<br>
______________________________<wbr>_________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/<wbr>mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></span></div><br></div></div>
</blockquote></div><br></div>