Bug 29418 - ARB_vp: large offsets when using relative addressing
Summary: ARB_vp: large offsets when using relative addressing
Alias: None
Product: Mesa
Classification: Unclassified
Component: Mesa core (show other bugs)
Version: unspecified
Hardware: All Linux (All)
: low normal
Assignee: mesa-dev
QA Contact:
Depends on:
Reported: 2010-08-06 01:49 UTC by fangxun
Modified: 2019-09-18 20:22 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:

increase INST_INDEX_BITS to 12 (786 bytes, patch)
2010-08-10 08:30 UTC, Brian Paul
Details | Splinter Review
add/use new gl_program_constants::MaxAddressOffset field (46.52 KB, patch)
2010-08-10 08:31 UTC, Brian Paul
Details | Splinter Review

Description fangxun 2010-08-06 01:49:50 UTC
System Environment:
Arch    x86_64
Platform    piketon
Libdrm:      (master)2.4.21-14-g431f7f00db844534dbcf9a63da0d2832a3d91bff
Mesa:        (master)121a625c1651ddc181e374ebdf16bc5c46f6eaa9
Xserver:    (master)xorg-server-
Xf86_video_intel:  (master)2.12.0-65-g6304cb048c745be81dae13f1d936996e04eaa530
Kernel: 2.6.35 (for-linus)9fe6206f400646a2322096b56c59891d530e8d51

Bug detailed description:
piglit case asmparsertest_ARBvp1.0_address-06 fails on piketon and pineview.
It's current mesa causes this problem. Good commit is 9b858186988083918483b00ca8a14a5d36787c82. Bad commit is 121a625c1651ddc181e374ebdf16bc5c46f6eaa9. 

Reproduce steps:
2.run piglit/asmparsertest_ARBvp1.0_address-06
Comment 1 fangxun 2010-08-09 21:59:34 UTC
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.
Comment 2 Brian Paul 2010-08-10 07:27:36 UTC
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.
Comment 3 Brian Paul 2010-08-10 07:37:53 UTC
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.
Comment 4 Marek Olšák 2010-08-10 07:56:13 UTC
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.
Comment 5 Brian Paul 2010-08-10 08:30:27 UTC
Created attachment 37769 [details] [review]
increase INST_INDEX_BITS to 12
Comment 6 Brian Paul 2010-08-10 08:31:24 UTC
Created attachment 37770 [details] [review]
add/use new gl_program_constants::MaxAddressOffset field

Can you try these two patches?
Comment 7 Ian Romanick 2010-08-10 12:10:35 UTC
(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.
Comment 8 Brian Paul 2010-08-10 14:04:42 UTC
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.
Comment 9 Marek Olšák 2011-03-02 07:23:12 UTC

the patches look OK to me. Can we commit them? The first one raising INST_INDEX_BITS seems important.
Comment 10 Brian Paul 2011-03-02 08:34:02 UTC
Committed.  Closing.
Comment 11 fangxun 2011-03-16 23:48:23 UTC
This case still fails on pineview, Ironlake and Sandybridge.
Comment 12 GitLab Migration User 2019-09-18 20:22:13 UTC
-- 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.

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.