<html dir="ltr"><head></head><body style="text-align:left; direction:ltr;"><div>On Thu, 2019-01-17 at 12:57 -0600, Jason Ekstrand wrote:</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div dir="ltr"><div>Yup, that'll do it. Gotta watch out for ++... Assuming it fixes the problem, that patch is</div><div><br></div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Jan 17, 2019 at 12:35 PM Lionel Landwerlin <<a href="mailto:lionel.g.landwerlin@intel.com">lionel.g.landwerlin@intel.com</a>> wrote:<br></div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<div bgcolor="#FFFFFF">
<div class="gmail-m_6603867202764786441moz-cite-prefix">Looking at the change the binding table
emission, I think the image++ has been moved such that it doesn't
produce the same tables anymore.</div>
<div class="gmail-m_6603867202764786441moz-cite-prefix">Trying this change on CI :
<a class="gmail-m_6603867202764786441moz-txt-link-freetext" href="https://github.com/djdeath/mesa/commit/a6b8eaf1325389d94d1d8a5b3bb952a362125eb2" target="_blank">https://github.com/djdeath/mesa/commit/a6b8eaf1325389d94d1d8a5b3bb952a362125eb2</a><br>
</div>
</div></blockquote></div></blockquote><div><br></div><div><br></div><div>Yes, this is what the patch that I sent for review did, I just pushed a previous version, sorry for the mess :-(</div><div><br></div><div>Iago<br>
</div><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div class="gmail_quote"><blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex"><div bgcolor="#FFFFFF">
<div class="gmail-m_6603867202764786441moz-cite-prefix">On 17/01/2019 18:19, Clayton Craft
wrote:<br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>Quoting Mark Janes (2019-01-17 10:13:37)</pre><pre><br></pre>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>Hi Iago,</pre><pre><br></pre><pre>It looks like you tested this patch in CI and got the same failures that</pre><pre>we are seeing on master:</pre><pre><br></pre><pre><a class="gmail-m_6603867202764786441moz-txt-link-freetext" href="http://mesa-ci-results.jf.intel.com/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845" target="_blank">http://mesa-ci-results.jf.intel.com/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845</a></pre><pre><br></pre>
</blockquote>
<pre>The correct link is:</pre><pre><a class="gmail-m_6603867202764786441moz-txt-link-freetext" href="https://mesa-ci.01.org/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845" target="_blank">https://mesa-ci.01.org/itoral/builds/263/group/63a9f0ea7bb98050796b649e85481845</a></pre><pre><br></pre><pre><br></pre>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>Why was this patch pushed?</pre><pre><br></pre><pre>-Mark</pre><pre><br></pre><pre>Mark Janes <a class="gmail-m_6603867202764786441moz-txt-link-rfc2396E" href="mailto:mark.a.janes@intel.com" target="_blank"><mark.a.janes@intel.com></a> writes:</pre><pre><br></pre><pre><br></pre>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>This patch regresses thousands of tests on BDW and HSW:</pre><pre><br></pre><pre><a class="gmail-m_6603867202764786441moz-txt-link-freetext" href="http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails" target="_blank">http://mesa-ci-results.jf.intel.com/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails</a></pre><pre><br></pre>
</blockquote>
</blockquote>
<pre><a class="gmail-m_6603867202764786441moz-txt-link-freetext" href="https://mesa-ci.01.org/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails" target="_blank">https://mesa-ci.01.org/vulkancts/builds/10035/group/63a9f0ea7bb98050796b649e85481845#fails</a></pre><pre><br></pre><pre><br></pre>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>I'll revert it as soon as my testing completes.</pre><pre><br></pre><pre>Iago Toral Quiroga <a class="gmail-m_6603867202764786441moz-txt-link-rfc2396E" href="mailto:itoral@igalia.com" target="_blank"><itoral@igalia.com></a> writes:</pre><pre><br></pre><pre><br></pre>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<pre>We had defined MAX_IMAGES as 8, which we used to size the array for</pre><pre>image push constant data. The comment there stated that this was for</pre><pre>gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes</pre><pre>that array, asserting that we don't exceed that number of images,</pre><pre>which imposes a limit of MAX_IMAGES on all gens.</pre><pre><br></pre><pre>Furthermore, despite this, we are exposing up to 64 images per shader</pre><pre>stage on all gens, gen8 included.</pre><pre><br></pre><pre>This patch lowers the number of images we expose in gen8 to 8 and</pre><pre>keeps 64 images for gen9+ while making sure that only pre-SKL gens</pre><pre>use push constant space to handle images.</pre><pre><br></pre><pre>v2:</pre><pre> - <= instead of < in the assert (Eric, Lionel)</pre><pre> - Change the way the assertion is written (Eric)</pre><pre><br></pre><pre>v3:</pre><pre> - Revert the way the assertion is written to the form it had in v1,</pre><pre> the version in v2 was not equivalent and was incorrect. (Lionel)</pre><pre><br></pre><pre>v4:</pre><pre> - gen9+ doesn't need push constants for images at all (Jason)</pre><pre>---</pre><pre> src/intel/vulkan/anv_device.c | 7 ++++--</pre><pre> .../vulkan/anv_nir_apply_pipeline_layout.c | 4 +--</pre><pre> src/intel/vulkan/anv_private.h | 5 ++--</pre><pre> src/intel/vulkan/genX_cmd_buffer.c | 25 +++++++++++++------</pre><pre> 4 files changed, 28 insertions(+), 13 deletions(-)</pre><pre><br></pre><pre>diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c</pre><pre>index 523f1483e29..f85458b672e 100644</pre><pre>--- a/src/intel/vulkan/anv_device.c</pre><pre>+++ b/src/intel/vulkan/anv_device.c</pre><pre>@@ -987,9 +987,12 @@ void anv_GetPhysicalDeviceProperties(</pre><pre> const uint32_t max_samplers = (devinfo->gen >= 8 || devinfo->is_haswell) ?</pre><pre> 128 : 16;</pre><pre> </pre><pre>+ const uint32_t max_images = devinfo->gen < 9 ? MAX_GEN8_IMAGES : MAX_IMAGES;</pre><pre>+</pre><pre> VkSampleCountFlags sample_counts =</pre><pre> isl_device_get_sample_counts(&pdevice->isl_dev);</pre><pre> </pre><pre>+</pre><pre> VkPhysicalDeviceLimits limits = {</pre><pre> .maxImageDimension1D = (1 << 14),</pre><pre> .maxImageDimension2D = (1 << 14),</pre><pre>@@ -1009,7 +1012,7 @@ void anv_GetPhysicalDeviceProperties(</pre><pre> .maxPerStageDescriptorUniformBuffers = 64,</pre><pre> .maxPerStageDescriptorStorageBuffers = 64,</pre><pre> .maxPerStageDescriptorSampledImages = max_samplers,</pre><pre>- .maxPerStageDescriptorStorageImages = 64,</pre><pre>+ .maxPerStageDescriptorStorageImages = max_images,</pre><pre> .maxPerStageDescriptorInputAttachments = 64,</pre><pre> .maxPerStageResources = 250,</pre><pre> .maxDescriptorSetSamplers = 6 * max_samplers, /* number of stages * maxPerStageDescriptorSamplers */</pre><pre>@@ -1018,7 +1021,7 @@ void anv_GetPhysicalDeviceProperties(</pre><pre> .maxDescriptorSetStorageBuffers = 6 * 64, /* number of stages * maxPerStageDescriptorStorageBuffers */</pre><pre> .maxDescriptorSetStorageBuffersDynamic = MAX_DYNAMIC_BUFFERS / 2,</pre><pre> .maxDescriptorSetSampledImages = 6 * max_samplers, /* number of stages * maxPerStageDescriptorSampledImages */</pre><pre>- .maxDescriptorSetStorageImages = 6 * 64, /* number of stages * maxPerStageDescriptorStorageImages */</pre><pre>+ .maxDescriptorSetStorageImages = 6 * max_images, /* number of stages * maxPerStageDescriptorStorageImages */</pre><pre> .maxDescriptorSetInputAttachments = 256,</pre><pre> .maxVertexInputAttributes = MAX_VBS,</pre><pre> .maxVertexInputBindings = MAX_VBS,</pre><pre>diff --git a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c</pre><pre>index b3daf702bc0..623984b0f8c 100644</pre><pre>--- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c</pre><pre>+++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c</pre><pre>@@ -528,8 +528,8 @@ anv_nir_apply_pipeline_layout(const struct anv_physical_device *pdevice,</pre><pre> }</pre><pre> }</pre><pre> </pre><pre>- if (map->image_count > 0) {</pre><pre>- assert(map->image_count <= MAX_IMAGES);</pre><pre>+ if (map->image_count > 0 && pdevice->compiler->devinfo->gen < 9) {</pre><pre>+ assert(map->image_count <= MAX_GEN8_IMAGES);</pre><pre> assert(shader->num_uniforms == prog_data->nr_params * 4);</pre><pre> state.first_image_uniform = shader->num_uniforms;</pre><pre> uint32_t *param = brw_stage_prog_data_add_params(prog_data,</pre><pre>diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h</pre><pre>index 770254e93ea..47878adb066 100644</pre><pre>--- a/src/intel/vulkan/anv_private.h</pre><pre>+++ b/src/intel/vulkan/anv_private.h</pre><pre>@@ -157,7 +157,8 @@ struct gen_l3_config;</pre><pre> #define MAX_SCISSORS 16</pre><pre> #define MAX_PUSH_CONSTANTS_SIZE 128</pre><pre> #define MAX_DYNAMIC_BUFFERS 16</pre><pre>-#define MAX_IMAGES 8</pre><pre>+#define MAX_IMAGES 64</pre><pre>+#define MAX_GEN8_IMAGES 8</pre><pre> #define MAX_PUSH_DESCRIPTORS 32 /* Minimum requirement */</pre><pre> </pre><pre> /* The kernel relocation API has a limitation of a 32-bit delta value</pre><pre>@@ -1883,7 +1884,7 @@ struct anv_push_constants {</pre><pre> uint32_t base_work_group_id[3];</pre><pre> </pre><pre> /* Image data for image_load_store on pre-SKL */</pre><pre>- struct brw_image_param images[MAX_IMAGES];</pre><pre>+ struct brw_image_param images[MAX_GEN8_IMAGES];</pre><pre> };</pre><pre> </pre><pre> struct anv_dynamic_state {</pre><pre>diff --git a/src/intel/vulkan/genX_cmd_buffer.c b/src/intel/vulkan/genX_cmd_buffer.c</pre><pre>index 35a70f7fe37..e23f8cfda45 100644</pre><pre>--- a/src/intel/vulkan/genX_cmd_buffer.c</pre><pre>+++ b/src/intel/vulkan/genX_cmd_buffer.c</pre><pre>@@ -2006,6 +2006,7 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer,</pre><pre> gl_shader_stage stage,</pre><pre> struct anv_state *bt_state)</pre><pre> {</pre><pre>+ const struct gen_device_info *devinfo = &cmd_buffer->device->info;</pre><pre> struct anv_subpass *subpass = cmd_buffer->state.subpass;</pre><pre> struct anv_cmd_pipeline_state *pipe_state;</pre><pre> struct anv_pipeline *pipeline;</pre><pre>@@ -2063,7 +2064,8 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer,</pre><pre> if (map->surface_count == 0)</pre><pre> goto out;</pre><pre> </pre><pre>- if (map->image_count > 0) {</pre><pre>+ /* We only use push constant space for images before gen9 */</pre><pre>+ if (map->image_count > 0 && devinfo->gen < 9) {</pre><pre> VkResult result =</pre><pre> anv_cmd_buffer_ensure_push_constant_field(cmd_buffer, stage, images);</pre><pre> if (result != VK_SUCCESS)</pre><pre>@@ -2177,10 +2179,15 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer,</pre><pre> assert(surface_state.alloc_size);</pre><pre> add_surface_state_relocs(cmd_buffer, sstate);</pre><pre> </pre><pre>- struct brw_image_param *image_param =</pre><pre>- &cmd_buffer->state.push_constants[stage]->images[image++];</pre><pre>+ if (devinfo->gen < 9) {</pre><pre>+ assert(image < MAX_GEN8_IMAGES);</pre><pre>+ struct brw_image_param *image_param =</pre><pre>+ &cmd_buffer->state.push_constants[stage]->images[image];</pre><pre> </pre><pre>- *image_param = desc->image_view->planes[binding->plane].storage_image_param;</pre><pre>+ *image_param =</pre><pre>+ desc->image_view->planes[binding->plane].storage_image_param;</pre><pre>+ }</pre><pre>+ image++;</pre><pre> break;</pre><pre> }</pre><pre> </pre><pre>@@ -2226,10 +2233,14 @@ emit_binding_table(struct anv_cmd_buffer *cmd_buffer,</pre><pre> add_surface_reloc(cmd_buffer, surface_state,</pre><pre> desc->buffer_view->address);</pre><pre> </pre><pre>- struct brw_image_param *image_param =</pre><pre>- &cmd_buffer->state.push_constants[stage]->images[image++];</pre><pre>+ if (devinfo->gen < 9) {</pre><pre>+ assert(image < MAX_GEN8_IMAGES);</pre><pre>+ struct brw_image_param *image_param =</pre><pre>+ &cmd_buffer->state.push_constants[stage]->images[image];</pre><pre> </pre><pre>- *image_param = desc->buffer_view->storage_image_param;</pre><pre>+ *image_param = desc->buffer_view->storage_image_param;</pre><pre>+ }</pre><pre>+ image++;</pre><pre> break;</pre><pre> </pre><pre> default:</pre></blockquote></blockquote></blockquote></blockquote></div></blockquote></div></blockquote></body></html>