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