Bug 38452

Created attachment 48255 [details] [review]
[PATCH] mesa: fix range check in _mesa_validate_DrawElements

It's a bug in ETQW. With this patch mesa will ignore invalid calls instead of rendering garbage, but it won't fix the real bug in the game.

The bug is related to index buffers and setting "r_useIndexBuffers" to 1 works for me. If it doesn't help, then it's another bug or you probably need to restart the game after changing that value.

On my system, the patch causes the display to simply stop updating when trying to reproduce the bug. Look away from the area in the game causing the bug and the screen starts updating again.

Are you sure you have diagnosed this correctly and that the bug is in the game? Because none of the other Mesa drivers I have tried shows this behaviour (i965, r300g).

(In reply to comment #2)
> On my system, the patch causes the display to simply stop updating when trying
> to reproduce the bug. Look away from the area in the game causing the bug and
> the screen starts updating again.

It's exactly the expected behaviour.

> Are you sure you have diagnosed this correctly and that the bug is in the game?
> Because none of the other Mesa drivers I have tried shows this behaviour (i965,
> r300g).

Probably the game settings were adjusted by some auto-detect features in the game while you have been testing with different drivers. Have you used the same game installation with the same settings for testing? Also, have you tried replaying your own trace file with different drivers?

Of course, it's possible that I've missed something. You could check it if you are familiar with opengl. Open your trace with qapitrace and set filter: Trace->Options->Only show the following events-> type "glDrawEle|glBindBuf" - this will show only important events. Then select Edit->Go to call->674429. You will see the following call: "glBindBufferARB(GL_ELEMENT_ARRAY_BUFFER, 1024)". So the game still uses index buffers for something and this call binds buffer 1024 as the index buffer. And then you could see that after binding the index buffer there are calls to glDrawElements - they should do the rendering. When the index buffer is bound, last parameter of glDrawElements is interpreted by gl as the offset in the buffer. And when the index buffer is not bound, last parameter should be the pointer to data in the memory. In the first DrawElements call after binding the index buffer you could see the value "0x9b46d720". Obviously, it is not valid value for the offset in the buffer (buffer 1024 is ~2mb in size at this moment). It looks like a pointer, so it seems the game engine is passing pointers to glDrawElements as if no index buffer is bound. So the game should either unbind the index buffer or never bind it at all before using DrawElements calls with pointers. And this also explains why setting "r_useIndexBuffers" to 1 helps - if the game will always use index buffers for rendering, then such situation will be impossible.

The bug is reproducible here with r600g (garbage or freeze), swrastg (crash), and catalyst 11.5 (garbage or freeze). I'm using demo and you are using full game, so some differences are possible, but afaics its not the case.

I think r_useIndexBuffers=0 was intended only for ancient low-end hw and was tested with lowest settings only. If it is default on some configurations for modern hw, then I think it is just autodetection fail in the game.

Indeed, I must have fouled up the settings for the game when I was trying different drivers, as you said. Anyway, thanks for taking the time to explain things!

Thanks for your work, Vadim, it's really appreciated.

As far as I'm concerned, this bug can be closed, unless it should be kept open to track the inclusion of the patch Vadim wrote?

Yes I guess.

(In reply to comment #3)

> The bug is reproducible here with r600g (garbage or freeze), swrastg (crash),
> and catalyst 11.5 (garbage or freeze). I'm using demo and you are using full
> game, so some differences are possible, but afaics its not the case.
> 
> I think r_useIndexBuffers=0 was intended only for ancient low-end hw and was
> tested with lowest settings only. If it is default on some configurations for
> modern hw, then I think it is just autodetection fail in the game.

I already reported this for r300g about a year ago, and the resolution was to always make sure that r_useIndexBuffers=1 in your config. Otherwise, on r300g it crashes with any detail setting. The game sets it to 0 whenever the gl renderer string changes (AFAICT). See comment 85 on RadeonProgram.

Can this be closed?

Yeah, let's consider this fixed.