[Piglit] [PATCH] shader_image: fix max images if fragment shader has limited outputs.

[Francisco Jerez]

Dave Airlie <airlied at gmail.com> writes:

> On 1 March 2018 at 04:08, Francisco Jerez <currojerez at riseup.net> wrote:
>> Dave Airlie <airlied at gmail.com> writes:
>>
>>> From: Dave Airlie <airlied at redhat.com>
>>>
>>> This drops one from the max images as the fragment shader needs
>>> one output for outputing the results
>>>
>>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>>> ---
>>>  tests/spec/arb_shader_image_load_store/image.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/spec/arb_shader_image_load_store/image.c b/tests/spec/arb_shader_image_load_store/image.c
>>> index e664d3cc4..1bfaebfdb 100644
>>> --- a/tests/spec/arb_shader_image_load_store/image.c
>>> +++ b/tests/spec/arb_shader_image_load_store/image.c
>>> @@ -670,11 +670,14 @@ num_reserved_images(GLbitfield stages)
>>>  unsigned
>>>  image_stage_max_images(const struct image_stage_info *stage)
>>>  {
>>> -        int n = 0;
>>> +        int n = 0, n2 = 0;
>>>
>>>          switch (stage->stage) {
>>>          case GL_FRAGMENT_SHADER:
>>>                  glGetIntegerv(GL_MAX_FRAGMENT_IMAGE_UNIFORMS, &n);
>>> +                glGetIntegerv(GL_MAX_COMBINED_SHADER_OUTPUT_RESOURCES, &n2);
>>> +             if (n == n2)
>>> +                     n--;
>>
>> I don't think this is guaranteed to fix the problem where there is one.
>> GL_MAX_COMBINED_SHADER_OUTPUT_RESOURCES imposes a limit on the number of
>> active image units in the whole pipeline, and you can reach it whether
>> GL_MAX_FRAGMENT_IMAGE_UNIFORMS is equal, lower or greater than
>> GL_MAX_COMBINED_SHADER_OUTPUT_RESOURCES.  You probably need to fix the
>> test case instead to require a lower number of image units.
>
> In practice the number of drivers that set
> GL_MAX_COMBINED_SHADER_OUTPUT_RESOURCES
> to a lower value is evergreen gpus, and they don't support image units
> anywhere else in the pipeline.
>
> I'm not sure we'll see any other gpu where it's very different.
>

This hack might work for you, but assuming that there is a bug in some
of the image load/store tests (I don't think you mentioned what this
even fixes) that causes it to use more than
GL_MAX_COMBINED_SHADER_OUTPUT_RESOURCES, this cannot possibly be
addressing the root of the problem, it's only papering over the symptoms
for evergreen, and the test could possibly blow up again for the next
driver that enables image load/store.  When that happens the next
developer that comes fix this is unlikely to be aware of your hack, so
it will likely be "fixed" twice and the test will no longer behave as
expected on some systems.  Same if somebody comes by and writes a new
image load/store test not expecting image_stage_max_images() to subtract
an arbitrary number from the number of supported uniforms...  but only
for the fragment shader...  and only in an uncertain subset of systems
including evergreen.

Take that as a NAK.  Fixing it properly is likely to involve as much
typing.

> Dave.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/piglit/attachments/20180228/5ba31c29/attachment.sig>