[Mesa-dev] [PATCH] mesa/api: validate buffers are unmapped

[Ilia Mirkin]

On Tue, Sep 1, 2015 at 7:50 PM, Dave Airlie <airlied at gmail.com> wrote:
> On 2 September 2015 at 09:42, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>> On Tue, Sep 1, 2015 at 7:39 PM, Dave Airlie <airlied at gmail.com> wrote:
>>> From: Dave Airlie <airlied at redhat.com>
>>>
>>> GL 3.2 introduced new language in this area, and CTS enforces it,
>>> this patches checks all the vertex buffers and the index buffer
>>> are mapped in the core profile case. I'm not sure what GLES
>>> expects here.
>>>
>>> This fixes
>>> GL33-CTS.draw_elements_base_vertex_tests.invalid_mapped_bos
>>>
>>> Signed-off-by: Dave Airlie <airlied at redhat.com>
>>> ---
>>>  src/mesa/main/api_validate.c | 33 +++++++++++++++++++++++++++++++++
>>>  1 file changed, 33 insertions(+)
>>>
>>> diff --git a/src/mesa/main/api_validate.c b/src/mesa/main/api_validate.c
>>> index 53c8fb8..8b79d5c 100644
>>> --- a/src/mesa/main/api_validate.c
>>> +++ b/src/mesa/main/api_validate.c
>>> @@ -34,7 +34,21 @@
>>>  #include "transformfeedback.h"
>>>  #include <stdbool.h>
>>>
>>> +/* GL3.2 introduces checks for buffer mappings */
>>> +static bool
>>> +check_buffers_are_unmapped(struct gl_vertex_buffer_binding *inputs)
>>> +{
>>> +   GLuint i;
>>>
>>> +   for (i = 0; i < VERT_ATTRIB_MAX; i++) {
>>> +      if (inputs[i].BufferObj) {
>>> +         struct gl_buffer_object *obj = inputs[i].BufferObj;
>>> +         if (_mesa_check_disallowed_mapping(obj))
>>> +            return false;
>>> +      }
>>> +   }
>>> +   return true;
>>> +}
>>>  /**
>>>   * Check if OK to draw arrays/elements.
>>>   */
>>> @@ -58,6 +72,25 @@ check_valid_to_render(struct gl_context *ctx, const char *function)
>>>        break;
>>>
>>>     case API_OPENGL_CORE:
>>> +
>>> +      /* Section 2.9.3 (Mapping and Unmapping Buffer Data" of the OpenGL 4.2
>>> +       * Core Profile says:
>>> +       *    "Most, but not all GL commands will detect attempts to read data
>>> +       *     from a mapped buffer object. When such an attempt is detected, an
>>> +       *     INVALID_OPERATION error will be generated.
>>> +       */
>>> +      if (!check_buffers_are_unmapped(ctx->Array.VAO->VertexBinding)) {
>>> +         _mesa_error(ctx, GL_INVALID_OPERATION, "%s(buffers mapped)", function);
>>> +         return false;
>>> +      }
>>> +
>>> +      if (ctx->Array.VAO->IndexBufferObj) {
>>> +         if (_mesa_check_disallowed_mapping(ctx->Array.VAO->IndexBufferObj)) {
>>> +            _mesa_error(ctx, GL_INVALID_OPERATION, "%s(buffers mapped)", function);
>>
>> May I suggest improving the messages to indicate that a vertex or
>> index buffer was being mapped? This should reduce confusion,
>> hopefully.
>
> you mean someone is going to see this error message in the wild? I
> admire your optimism :-)

I mean I'm going to scratch my head less when I attempt to get a
piglit test working and the draws start failing randomly. (But don't
worry, there will still be a fair bit of head scratching in such an
event, so not all is lost.)

>From what I can tell, the spec is unclear whether commands are
supposed to do the checking or not... hopefully someone with more spec
experience can comment.

>
> But yes I spotted that myself, so I can do so.
>
> Dave.