Bug 96410 - [Perf] Pre validate _mesa_sampler_uniforms_pipeline_are_valid like _mesa_sampler_uniforms_are_valid
Summary: [Perf] Pre validate _mesa_sampler_uniforms_pipeline_are_valid like _mesa_samp...
Status: RESOLVED FIXED
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: git
Hardware: Other All
: medium normal
Assignee: mesa-dev
QA Contact: mesa-dev
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-06 17:21 UTC by gregory.hainaut
Modified: 2016-07-07 06:22 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description gregory.hainaut 2016-06-06 17:21:14 UTC
Hello,

Profiling my app (PCSX2), it seems a fair amount of time is spend on the _mesa_sampler_uniforms_pipeline_are_valid. I only uses a single pipeline object so it always trigger revalidation of the pipeline which isn't ideal.

Anyway, it feels that it ought to be possible to cache/prevalidate sampler of program like the _mesa_sampler_uniforms_are_valid function (which is the same function but for monolithic build). 

It would be nice to reduce the overhead of the function. Perf reports around 4% of nouveau_dri.so.

Thanks,
Gregory
Comment 1 gregory.hainaut 2016-06-17 18:06:32 UTC
Currently looking at the code of single program flow namely _mesa_update_shader_textures_used

The current check fails to detect wrongly reused sampler in multiple shader stage. The spec seems to imply that is must be checked in the full program (all stages).

<<
     Errors
  It is not allowed to have variables of different sampler types pointing to
  the same texture image unit within a program object. This situation can only
  be detected at the next rendering command issued which triggers shader invo-
  cations, and an INVALID_OPERATION error will then be generated.
>>

Here a typical example of an invalid program that will wrongly run fine.

***** Vertex Shader
layout(binding = 0) uniform sampler1D sampler_1d;

***** Fragment Shader
layout(binding = 0) uniform sampler2D sampler_2d;
Comment 2 Timothy Arceri 2016-06-18 00:19:51 UTC
(In reply to gregory.hainaut from comment #1)
> Currently looking at the code of single program flow namely
> _mesa_update_shader_textures_used
> 
> The current check fails to detect wrongly reused sampler in multiple shader
> stage. The spec seems to imply that is must be checked in the full program
> (all stages).
> 
> <<
>      Errors
>   It is not allowed to have variables of different sampler types pointing to
>   the same texture image unit within a program object. This situation can
> only
>   be detected at the next rendering command issued which triggers shader
> invo-
>   cations, and an INVALID_OPERATION error will then be generated.
> >>
> 
> Here a typical example of an invalid program that will wrongly run fine.
> 
> ***** Vertex Shader
> layout(binding = 0) uniform sampler1D sampler_1d;
> 
> ***** Fragment Shader
> layout(binding = 0) uniform sampler2D sampler_2d;

See the FIXME I added when I mostly fixed this a while ago:
https://cgit.freedesktop.org/mesa/mesa/tree/src/mesa/main/uniform_query.cpp#n1095

The problem is the spec requires samplers to default to 0. So unless we manage to eliminate everything that is unused (including early array elements) we will likely trigger errors in programs that run fine elsewhere (since last time I checked Nvidia doesn't do this validation at all)
Comment 3 gregory.hainaut 2016-06-18 06:53:47 UTC
I confirm that Nvidia doesn't check anything. And we don't even check it for mono program. I don't know if it was intended or a bug.


Potentially we could do something like the code below (I guess I need to add back the unit 0 case). Code is faster because it only check sampler rather than all uniforms.

An alternative will be to check only SamplersValidated for all stages. It will be equivalent to mono program.

I guess the ideal solution would be to support  GL_KHR_no_error to disable all those crazy validations.

--------------------------8<--------------------------
   for (unsigned idx = 0; idx < ARRAY_SIZE(pipeline->CurrentProgram); idx++) {
      if (!shProg[idx])
         continue;

      shader = shProg[idx]->_LinkedShaders[idx];
      if (!shader || !shader->Program)
         continue;

      mask = shader->Program->SamplersUsed;
      while (mask) {
         const int s = u_bit_scan(&mask);
         GLuint unit = shader->SamplerUnits[s];
         GLuint tgt = shader->SamplerTargets[s];

         if (TexturesUsed[unit] & ~(1 << tgt)) {
            pipeline->InfoLog =
               ralloc_asprintf(pipeline,
                     "Program %d: "
                     "Texture unit %d is accessed with 2 different types",
                     shProg[idx]->Name, unit);
            return false;
         }

         TexturesUsed[unit] |= (1 << tgt);
      }

      active_samplers += shader->num_samplers;
   }
--------------------------->8-------------------------
Comment 4 gregory.hainaut 2016-07-07 06:22:03 UTC
Fixed by commit	6a524c76f502fe15bb3612065a23ece693aed237


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.