[Mesa-dev] [PATCH v2] i965: skip reading clip distances from the URB for the FS if possible

[Iago Toral]

On Thu, 2017-09-28 at 07:24 -0400, Ilia Mirkin wrote:
> On Thu, Sep 28, 2017 at 6:33 AM, Iago Toral Quiroga <itoral at igalia.co
> m> wrote:
> > we can skip these slots when they are not read in the fragment
> > shader
> > and they are positioned right after a VUE header that we are
> > already
> > skipping. We also need to ensure that we are passing at least one
> > other
> > varying, since that is a hardware requirement.
> > 
> > This helps alleviate a problem introduced with 99df02ca26f61 for
> > separate shader objects: without separate shader objects we assign
> > locations sequentially, however, since that commit we have changed
> > the
> > method for SSO so that the VUE slot assigned depends on the number
> > of
> > builtin slots plus the location assigned to the varying. This fixed
> > layout is intended to help SSO programs by avoiding on-the-fly
> > recompiles
> > when swapping out shaders, however, it also means that if a varying
> > uses
> > a large location number close to the maximum allowed by the SF/FS
> > units
> > (31), then the offset introduced by the number of builtin slots can
> > push
> > the location outside the range and trigger an assertion.
> > 
> > This problem is affecting at least the following CTS tests for
> > enhanced layouts:
> > 
> > KHR-GL45.enhanced_layouts.varying_array_components
> > KHR-GL45.enhanced_layouts.varying_array_locations
> > KHR-GL45.enhanced_layouts.varying_components
> > KHR-GL45.enhanced_layouts.varying_locations
> > 
> > which use SSO and the the location layout qualifier to select such
> > location numbers explicitly.
> > 
> > This change helps these tests because for SSO we always have to
> > include
> > VARYING_SLOT_CLIP_DIST{0,1} even if the fragment shader is very
> > unlikely
> > to read them, so by doing this we free builtin slots from the fixed
> > VUE
> > layout and we avoid the tests to crash in this scenario.
> > 
> > Of course, this is not a proper fix, we'd still run into problems
> > if someone
> > tries to use an explicit max location and read gl_ViewportIndex,
> > gl_LayerID or
> > gl_CullDistancein in the FS, but that would be a much less common
> > bug and we
> > can probably wait to see if anyone actually runs into that
> > situation in a real
> > world scenario before making the decision that more aggresive
> > changes are
> > required to support this without reverting 99df02ca26f61.
> > 
> > v2:
> > - Add a debug message when we skip clip distances (Ilia)
> 
> Sorry, I should have been more specific. I meant adding a debug
> message when you detect the buggy condition -- i.e. too many inputs
> being used, or whatever the error condition you're fixing here. You
> mention that this isn't a proper fix and it's still possible to run
> into problems. Would be nice to get a perf_debug() or something when
> you see that happening. Should be possible pretty easily. I suspect
> such a message would have saved you a lot of debugging time had it
> been there, and will likely save the next poor sap a lot of time when
> they run into the issue of using an explicit max location + viewport
> index/etc.

Ah, not really. The driver asserts when this happens because it sees
varying slots that go beyond the maximum limit, so I think we already
have a good starting point for debugging this situations.

Iago