[Mesa-dev] [PATCH 0/3] verify max vertex attrib stride

[Roland Scheidegger]

Am 09.07.2018 um 09:54 schrieb Erik Faye-Lund:
> On 06. juli 2018 18:43, Roland Scheidegger wrote:
>> Am 06.07.2018 um 12:03 schrieb Erik Faye-Lund:
>>> OpenGL 4.4 and OpenGL ES 3.1 both require the maximum
>>> vertex attrib stride to be at least 2048. If this isn't
>>> the case, we shouldn't expose these API versions.
>>>
>>> Unfortunately, the r600 driver only supports 2047. To
>>> avoid regressions in the supported GL version, the
>>> first patch modifies the returned value.
>>>
>>> I'm not sure if that last one is a good idea or not, as
>>> it's strictly speaking non-conformant. But applications
>>> won't expect GL errors generated when using strides of
>>> 2048 either, which is what currently happens.
>>>
>>> The initial motivation for this patch-series is to avoid
>>> exposing a too high spec version in virgl and then get
>>> dEQP failures when running on old hardware. The virgl
>>> specific bits are being sent separately, because they
>>> depend on some other not-yet-upstream things ATM.
>>>
>>> Thoughts?
>>>
>>> Erik Faye-Lund (3):
>>>    r600: report incorrect max-vertex-attrib for GL 4.4
>>>    mesa: verify MaxVertexAttribStride for GL 4.4
>>>    mesa: verify MaxVertexAttribStride for GLES 3.1
>>>
>>>   src/gallium/drivers/r600/r600_pipe.c | 3 ++-
>>>   src/mesa/main/version.c              | 2 ++
>>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>>
>> Personally I think it's _much_ better to lie about the supported GL
>> version rather than the maximum vertex attrib stride (I don't know if
>> dEQP would actually have a test which tests for the max stride also
>> working than just being advertized, which would be way more relevant and
>> fail in any case).
> 
> I don't think I agree in this case; OpenGL 4.4 applications that use
> *exactly* strides of 2048probably won't even check the max-stride, as it's
> guaranteed to work by the standard anyway. Applications that use *more*
> might check for this value, but as the query was introduced in 4.4 it's
> likely that applications will just get a gl-error down an unexpected
> code-path, which can lead to the wrong attribute bound (possibly pointing
> to client-memory, which can lead to segfaults). Lying about the max
> attribute stride will "just" lead to rendering glitches.
> 
> But even *if* it was better to lie about the GL version, I don't think we
> have a good way to contain something like this to just the driver in
> question, and I *certainly* don't think it's better to have hacks for a
> single driver in the generic code that affects *all* drivers. But if we
> were to lie about the GL version rather than the max-stride, we'd IMO need
> to introduce something like a driver-switch to control the GL version,
> which just seems like a bad idea to me, as it's easy to get stuck behind
> on an old version for artificial reasons, or to miss something else
> important and get an artificially high version (and risk breaking apps
> on working systems when fixing it again).
> 
> There's of course a third option here, which is to go by the book and
> leave r600 on OpenGL 4.3 / OpenGL ES 3.0. That feels like the "correct
> thing to do" to me, but I'm sure someone will be annoyed if they can't
> play the games they've bought already before. But perhaps it's better for
> the users to work around this on applications where it's known to work,
> than having *any* sort of deceptive code in here?
> 
>> (Because if you're going to use stride 2048, it is hugely unlikely you'd
>> just rely on GL 4.4 for that, hence it wouldn't work with older versions
>> of GL anyway neither, where ALL strides are just supposed to work.)
> 
> I think what ARB realized for GL 4.4 is that not all implementations
> support all strides. In that light, I think it's reasonable to say
> that for GL 4.3 and before, an *undefined* stride was supposed to
> work.
> 
>> Roland
>>
>> FWIW, I think it should be possible to work around that limitation on
>> r600. Since vertex fetch just uses a fetch shader, you could have a
>> shader key bit indicating strides of 2048, program a stride of 1024
>> instead and multiply the index to fetch by 2 in the shader. Of course
>> that could cause shader recompiles (never in practice...) and there's
>> some overhead associated with it, so might not be worth it...
>>
> 
> I don't think it would be a very high priority thing to fix, but it would
> certainly be interesting. Sadly, I don't have an r600 card myself, so I
> can't really work on that.
> 
> Another option would be to do some CPU-side reshuffling of vertex-
> attributes that are overly distant. Dunno.
I don't think that's practical, with UBOs and all. If you really wanted
to fix it, IMHO fixing up the fetch shader (and differently programming
the stride) is definitely the way to go.

> 
> If we decided to go down the "let's stick to the spec"-approach here, one
> of these approaches might be appropriate to regain the 4.4 feature...

But alright, it looks like I'm the only one thinking it's better to lie
about the GL version rather than about the supported stride. So, I can
live with that...

Roland