Skip to content

WIP:RFC: defensive code for reservedMustBeZero field

Alejandro Piñeiro requested to merge apinheiro/mesa:rfc/defensive-code-for-reservedMustBeZero into master

From OpenGL ES 3.1 spec (also 3.2, and equivalent for Desktop < 4.2):

void DrawArraysIndirect(enum mode, const void *indirect );

typedef struct { uint count; uint instanceCount; uint first; uint reservedMustBeZero; } DrawArraysIndirectCommand;

Results are undefined if reservedMustBeZero is non-zero, but may not result in program termination.

The final sentence also implies that no GL error should be triggered. So, the following CTS test checks that with a non-zero value for reservedMustBeZero, no GL errors are triggered, and no crash happens: KHR-GLES31.core.draw_indirect.misc-reservedMustBeZero-arrays

The same for elements. That field is defined on desktop GL >= 4.2 as baseInstance.

So, there is one CTS that on purpose, uses a wrong value for the reserved field when calling that method. On v3d, this causes a problem. After executing that test, the following tests start to fail: KHR-GLES31.core.draw_indirect.basic-drawArrays-instancing KHR-GLES31.core.draw_indirect.basic-drawArrays-vertexIds KHR-GLES31.core.draw_indirect.basic-drawElements-instancing KHR-GLES31.core.draw_indirect.basic-drawElements-vertexId

So the execution of the reservedMustBeZero test let the driver/hw is a bad state. FWIW, v3d hw supports baseInstance, so that call is in the end using a wrong baseInstance value for the current data.

I see four options here:

  1. Trying to fix the issue at v3d when emitting the job (v3dx_draw.c:866): but as this is in an indirect command, at that point the wrong value is on a external buffer, and we just have the address, so it doesn't look simple.
  2. Trying to somehow restore the v3d driver/hw to a working status: even if that were possible (I still don't know if that is possible or not), that would mean the need to add some sanity code for each call of indirect calls, just to cover a wrong usage of the OpenGL APIs.
  3. Add some defensive code on DrawArrays/DrawElements and bufferData: that is implemented on the patch included on the MR. The advantage of this approach is that other drivers would benefit if they plan to run that CTS tests (although FWIW, testing with my laptop, i965 with Coffelake doesn't need this). The disadvantage is that the code is somewhat hacky/ugly as both Elements and Arrays uses GL_DRAW_INDIRECT_BUFFER, but with a different struct, so you need to rely on the size to know which one to cast in order to set that field. Also, this doesn't cover the case where the buffer is modified by other means (although that looks like a specific corner case).
  4. Conclude that the test is bogus: from "may not result in program termination." the test not only assumes that the driver will not crash, but also that it should keep working normally after the method call, even if the "Results are undefined". So although I think that it would be nice to prevent that state, or to fix it if happens, perhaps it makes more sense to just conclude that the test is wrong, and open a CTS issue.

So as the implemented solution, 3., applies any driver, this RFC is general. But I'm more interested on @anholt opinion, as it is affecting directly v3d.

Merge request reports