Bug 29418

0614006d090902324149387ec150231b647928fd is first bad commit.
commit 0614006d090902324149387ec150231b647928fd
Author: Marek Olšák <maraeo@gmail.com>
Date:   Sat Jul 31 20:12:21 2010 +0200

    mesa: increase the relative address offset limit to 4096 in ARB_vp/fp

    Even though the spec says that the limits should be -64/+63, proprietary
    drivers support much larger relative offsets and some applications do
    depend on this non-standard behavior.

    Also program_parse.tab.c has been regenerated.

    This fixes the parser error:

      ARB_vp: error: relative address offset too large

    See also: https://bugs.freedesktop.org/show_bug.cgi?id=28628

    4096 * sizeof(vec4) is the maximum size of the constant buffer on NV50.
    It is not supposed to be a definite hardware limit, it is for the parser
    not to get in the way and let the underlying driver decide whether it can
    run the shader or not.

The range of relative addressing offsets was increased to allow Wine apps to run.  Other GL drivers allow offsets outside the [-64,63] range too.  We should probably remove that piglit test, or have it generate a warning rather than a failure.

There's another issue with commit 0614006d090902324149387ec150231b647928fd, however.

The offset limits are now [-4096,4095] but that's greater than what will fit in the prog_src_register::Index field which is currently 10 bits wide (see INST_INDEX_BITS).

Marek, do we really need [-4096,4095] or would [-1024,1023] be enough?  If we need the former range we'll need to increase INST_INDEX_BITS to 12.

I think it would be reasonable to increase INST_INDEX_BITS as some GLSL programs might overflow this easily in theory.

Sorry for not spotting this regression earlier.

Created attachment 37769 [details] [review]
increase INST_INDEX_BITS to 12

Created attachment 37770 [details] [review]
add/use new gl_program_constants::MaxAddressOffset field

Can you try these two patches?

(In reply to comment #2)
> The range of relative addressing offsets was increased to allow Wine apps to
> run.  Other GL drivers allow offsets outside the [-64,63] range too.  We should
> probably remove that piglit test, or have it generate a warning rather than a
> failure.

I have two major problems with this.

1. There is no way to query this limit.  The spec sets the limit at [-64,63], so that is the only portable thing an application can use.  Relying on unspecified API behavior is a slippery slope.

2. By accepting larger limits, we accept shaders that other spec compliant implementations (e.g., Apple) reject on the same piece of hardware.  So much for OpenGL being a portable abstraction layer.

Quite frankly, if an application wants more than what ARB_vertex_program can provide, it should either use GLSL or use one of the Nvidia extensions that explicitly provide larger limits.

I would normally object to going outside the spec with something like this but I think that the benefit (allowing various apps to run that wouldn't otherwise) outweighs the downside.

Brian,

the patches look OK to me. Can we commit them? The first one raising INST_INDEX_BITS seems important.

Committed.  Closing.

This case still fails on pineview, Ironlake and Sandybridge.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/mesa/mesa/issues/962.