Yup. Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
On Wed, Jan 16, 2019 at 6:08 AM Iago Toral <ito...@igalia.com> wrote: > 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: > >
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev