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

[Ilia Mirkin]

On Thu, Sep 28, 2017 at 6:33 AM, Iago Toral Quiroga <itoral at igalia.com> 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.

  -ilia