[Mesa-dev] [PATCH v2] anv: set right datatypes in anv_pipeline_binding

[Tapani Pälli]

On 08/29/2017 12:16 PM, Juan A. Suarez Romero wrote:
> On Tue, 2017-08-29 at 09:48 +0300, Tapani Pälli wrote:
>> Hi;
>>
>> On 08/25/2017 06:32 PM, Juan A. Suarez Romero wrote:
>>> This structure contains two fields, binding and index, that store the
>>> binding in the descriptor set and the index inside the binding.
>>>
>>> These structures are defined as uint8_t, but the types in Vulkan
>>> specification are uint32_t, so big values are clamp.
>>>
>>> This fixes dEQP-VK.binding_model.shader_access.*.multiple_arbitrary_descriptors.*
>>>
>>> v2: use UINT32_MAX for index when having no render targets (Tapani)
>>
>> Sorry, I think I did not catch everything here. There's UINT8_MAX in use
>> also when checking index in has_color_buffer_write_enabled().
>>
> 
> Yes. The v2 patch already covers this case (using UINT32_MAX instead).
> It's the last part of the patch

Indeed it is :)


>> Then .. in many places where anv_pipeline_bind_map is used, there is
>> magic number 256 in use with surface to descriptor and sampler to
>> descriptor mappings, are these related or where does 256 come from?
>>
>>
> 
> AFAIK, that number is to define arrays of 256 anv_pipeline_bind_map. It
> is not related with the fact of the datatype for the internal field of
> the map.

Right, makes sense. Would be nice to have it defined somewhere but 
that's not related to this change;

Reviewed-by: Tapani Pälli <tapani.palli at intel.com>

> 
> 	J.A.
> 
>>
>>> ---
>>>    src/intel/vulkan/anv_pipeline.c  | 2 +-
>>>    src/intel/vulkan/anv_private.h   | 4 ++--
>>>    src/intel/vulkan/genX_pipeline.c | 2 +-
>>>    3 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/src/intel/vulkan/anv_pipeline.c b/src/intel/vulkan/anv_pipeline.c
>>> index 279d76561a..13ee5f47e2 100644
>>> --- a/src/intel/vulkan/anv_pipeline.c
>>> +++ b/src/intel/vulkan/anv_pipeline.c
>>> @@ -907,7 +907,7 @@ anv_pipeline_compile_fs(struct anv_pipeline *pipeline,
>>>             rt_bindings[0] = (struct anv_pipeline_binding) {
>>>                .set = ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS,
>>>                .binding = 0,
>>> -            .index = UINT8_MAX,
>>> +            .index = UINT32_MAX,
>>>             };
>>>             num_rts = 1;
>>>          }
>>> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
>>> index 6b2414429f..d5657c5e09 100644
>>> --- a/src/intel/vulkan/anv_private.h
>>> +++ b/src/intel/vulkan/anv_private.h
>>> @@ -1236,10 +1236,10 @@ struct anv_pipeline_binding {
>>>       uint8_t set;
>>>    
>>>       /* Binding in the descriptor set */
>>> -   uint8_t binding;
>>> +   uint32_t binding;
>>>    
>>>       /* Index in the binding */
>>> -   uint8_t index;
>>> +   uint32_t index;
>>>    
>>>       /* Input attachment index (relative to the subpass) */
>>>       uint8_t input_attachment_index;
>>> diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
>>> index 55db5339d6..acf3ee37d3 100644
>>> --- a/src/intel/vulkan/genX_pipeline.c
>>> +++ b/src/intel/vulkan/genX_pipeline.c
>>> @@ -1346,7 +1346,7 @@ has_color_buffer_write_enabled(const struct anv_pipeline *pipeline)
>>>          if (bind_map->surface_to_descriptor[i].set !=
>>>              ANV_DESCRIPTOR_SET_COLOR_ATTACHMENTS)
>>>             continue;
>>> -      if (bind_map->surface_to_descriptor[i].index != UINT8_MAX)
>>> +      if (bind_map->surface_to_descriptor[i].index != UINT32_MAX)
>>>             return true;
>>>       }
>>>    
>>>
>>
>>