On 11/01/2019 12:40, Iago Toral wrote:
On Fri, 2019-01-11 at 12:31 +0000, Lionel Landwerlin wrote:
On 11/01/2019 12:12, 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+. It also sets MAX_IMAGES to 64 so we can
actually use up to 64 images in shaders, and then adds an assert
specific to gen8 to check that we never exceed 8.

v2:
   - <= instead of < in the assert (Eric, Lionel)
   - Change the way the assertion is written (Eric)
---
   src/intel/vulkan/anv_device.c                    | 7 +++++--
   src/intel/vulkan/anv_nir_apply_pipeline_layout.c | 3 ++-
   src/intel/vulkan/anv_private.h                   | 4 ++--
   3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/intel/vulkan/anv_device.c
b/src/intel/vulkan/anv_device.c
index 523f1483e2..f85458b672 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 b3daf702bc..53de347b3c 100644
--- a/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
+++ b/src/intel/vulkan/anv_nir_apply_pipeline_layout.c
@@ -529,7 +529,8 @@ anv_nir_apply_pipeline_layout(const struct
anv_physical_device *pdevice,
      }
if (map->image_count > 0) {
-      assert(map->image_count <= MAX_IMAGES);
+      assert((pdevice->compiler->devinfo->gen <= 8 && map-
image_count <= MAX_GEN8_IMAGES) ||
+              map->image_count <= MAX_IMAGES);

I'm not sure why it wasn't < instead of <= previously, sounds like
it
should be <.
I think <= is fine, map->image_count is not an index but the total
count so its value should be fine up to the maximum allowed, right?


Oh dear, sorry, you're right.



Also I don't think this version works, your previous version was okay
:


image_count < MAX_GEN8_IMAGE || (gen > 8 && image_count < MAX_IMAGES)
Right, I didn't read Eric's proposal properly, and it would not work
for gen8. I'll revert the form of the assertion to the previous
version.

         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 770254e93e..9ddd41b1fa 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
@@ -1882,7 +1883,6 @@ struct anv_push_constants {
      /* Used for vkCmdDispatchBase */
      uint32_t base_work_group_id[3];
- /* Image data for image_load_store on pre-SKL */
      struct brw_image_param images[MAX_IMAGES];
   };




_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to