<div dir="ltr"><div>Yup.</div><div> </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div></div> <div class="gmail_quote"><div dir="ltr">On Wed, Jan 16, 2019 at 6:08 AM Iago Toral <<a href="mailto:itoral@igalia.com">itoral@igalia.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Jason, does this version look good to you? On Mon, 2019-01-14 at 12:42 +0100, Iago Toral Quiroga wrote: > We had defined MAX_IMAGES as 8, which we used to size the array for > image push constant data. The comment there stated that this was for > gen8, but anv_nir_apply_pipeline_layout runs for all gens and writes > that 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 shader > stage on all gens, gen8 included. > > This patch lowers the number of images we expose in gen8 to 8 and > keeps 64 images for gen9+ while making sure that only pre-SKL gens > use 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.c > index 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( > .maxPerStageDescriptorUniformBuffers = 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( > .maxDescriptorSetStorageBuffers = 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.c > index 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.h > index 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.c > index 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_buffer, > 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: </blockquote></div>