[Mesa-dev] [PATCH] RFC: gallium: increase PIPE_MAX_SHADER_SAMPLER_VIEWS to 128

[Roland Scheidegger]

Am 28.02.2018 um 11:54 schrieb Jose Fonseca:
> On 27/02/18 02:38, sroland at vmware.com wrote:
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> Some state trackers require 128.
>> (There are no plans to increase PIPE_MAX_SAMPLERS too, since with gl
>> state tracker it's unlikely more than 32 will be needed, if you need
>> more use bindless.)
>> ---
>>   src/gallium/include/pipe/p_state.h | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/include/pipe/p_state.h
>> b/src/gallium/include/pipe/p_state.h
>> index 2b56d60..cddb3b4 100644
>> --- a/src/gallium/include/pipe/p_state.h
>> +++ b/src/gallium/include/pipe/p_state.h
>> @@ -64,7 +64,7 @@ extern "C" {
>>   #define PIPE_MAX_SAMPLERS         32
>>   #define PIPE_MAX_SHADER_INPUTS    80 /* 32 GENERIC + 32 PATCH + 16
>> others */
>>   #define PIPE_MAX_SHADER_OUTPUTS   80 /* 32 GENERIC + 32 PATCH + 16
>> others */
>> -#define PIPE_MAX_SHADER_SAMPLER_VIEWS 32
>> +#define PIPE_MAX_SHADER_SAMPLER_VIEWS 128
>>   #define PIPE_MAX_SHADER_BUFFERS   32
>>   #define PIPE_MAX_SHADER_IMAGES    32
>>   #define PIPE_MAX_TEXTURE_LEVELS   16
>>
> 
> Bumping PIPE_MAX_SHADER_SAMPLER_VIEWS defintely sounds good to me.
> 
> 
> But besides changing the #defnie, I suppose there are quite a few places
> where bitmasks will need to increase from 1 32bit words to something else.
> 
> Such as src/gallium/drivers/llvmpipe/lp_state_fs.c's make_variant_key()
> 
> Which also means that tgsi_shader_info::file_mask should be a 128 bit
> word.  But I don't think 128 bit words are portable (GCC/Clang support,
> but MSVC does not)
Ah yes, I missed this one, probably because it really originates in
code outside llvmpipe (not sure why it still worked - must have gotten
lucky...)

> So perhaps we need re-spec file_mask to mean the _first_ 64bit, and have
> a second file_mask (eg file_mask_hi) with the second 64 bits.  Another
> alternative is to leave tgsi_shader_info::file_mask alone, and have a
> special uint64 srv_mask[2] field dedicated for sampler views.
Yes, that's going to make things complicated.

> 
> I suspect there are more places that do 1 << which will need to be updated.
I think this one is the only one.

> 
> BTW, GCC/Clang have options to detect bad shifts: -fsanitize=undefined
> or -fsanitize=shift-exponent.
> 
> It's not the first time we get in troubles due to this, so we should
> consider just set it on all debug builds.  And we should also find a
> simple test that exercise all 128 SRVs to weed these places out.
> 
> Jose

Roland