[Mesa-dev] [PATCH v4] anv/device: fix maximum number of images supported
Lionel Landwerlin
lionel.g.landwerlin at intel.com
Thu Jan 17 18:35:28 UTC 2019
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
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 that
>> we 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.janes at 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 <itoral at igalia.com> writes:
>>>
>>>> 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:
>>>> --
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190117/0398523c/attachment-0001.html>
More information about the mesa-dev
mailing list