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

[Roland Scheidegger]

Am 27.02.2018 um 19:07 schrieb Roland Scheidegger:
> Am 27.02.2018 um 17:39 schrieb Brian Paul:
>> On 02/26/2018 07:45 PM, Roland Scheidegger wrote:
>>> Am 27.02.2018 um 03:38 schrieb sroland at vmware.com:
>>>> 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
>>>>
>>>
>>>
>>> FWIW if drivers are doing the right thing (so, don't blindly announce
>>> support for PIPE_MAX_SHADER_SAMPLER_VIEWS assuming it's 32 if you can't
>>> handle more etc.), this should just work. It will, however, make some
>>> allocations bigger (cso context comes to mind, but there's probably
>>> more), so I think it warrants some discussion. Apart from that I don't
>>> think there should really be any performance degradation, since code
>>> should not blindly iterate over max views typically (rather, only the
>>> views up to the max number which has changed and so on).
>>
>> Looks like there's such a loop at sp_state_sampler.c:184 that should be
>> fixed.
> I once fixed all such loops in llvmpipe (because even with 32, they
> actually showed up in profiles). I probably missed the one in draw
> (since that one doesn't do ref counting just set things to zero it
> probably was much lower overhead) and wasn't that worried about
> softpipe... But I'll fix this one.
> 
>>
>> Other instances found with git grep "for.*PIPE_MAX_SHADER_SAMPLER_VIEWS"
>> src/gallium/ seem to be in context init/destroy functions.
> The one in cso_context destroy should be fixable easily.
> The problem with the ones in draw aaline/pstipple is that they don't
> actually store the number of active views (albeit they pretend they do).
> Since the interface there with start/num just says which ones are
> replaced, but the ones above start+num remain. This is not handled
> correctly by these pipeline stages, but at least at stage destruction we
> definitely free them. Probably time to fix that...
> Those were the only 3 I've found with a quick grep.
> 

Ok I've got fixes for the one in cso_context and softpipe.

The draw pstipple/aaline ones though are difficult to fix. The code
there is really broken in a dozen ways, and non-trivial to fix up.
I hate these stages for a reason:
- they have to intercept all fs / sampler / view calls, ref count the
views (and great, we're doing it twice due to two stages needing it),
even though it's likely the stage may never get used at all.
- they will only use TEX opcodes and will decide which sampler (and
therefore view) slot to use based solely on the used samplers. The view
slot could already be in use (with dx10 style shaders).
- they don't use the syntax of setting samplers and views (with start
and num) correctly. So the num_samplers/views used isn't actually what
they think it is, albeit it should still work with some luck (but it's
due to this that the loop can't be avoided easily on context destruction).
I gave up on it, and just thinking if context destruction is a
performance critical path in your app, you're doing it wrong.

Roland