[Mesa-dev] [PATCH] vbo: Return INVALID_OPERATION during draw with a mapped buffer

[Marek Olšák]

On Wed, Apr 27, 2016 at 6:03 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Wednesday, April 27, 2016 11:22:48 AM PDT Marek Olšák wrote:
>> On Wed, Apr 27, 2016 at 5:05 AM, Kenneth Graunke <kenneth at whitecape.org>
> wrote:
>> > On Monday, April 25, 2016 5:54:28 PM PDT Jordan Justen wrote:
>> >> Fixes the OpenGLES 3.1 CTS:
>> >>  * ESEXT-CTS.draw_elements_base_vertex_tests.invalid_mapped_bos
>> >>
>> >> Because this is triggering the error message after the normal API
>> >> validation phase, we don't have the API function name available, and
>> >> therefore we generate an error message that is a little confusing:
>> >>
>> >> Mesa: User error: GL_INVALID_OPERATION in Vertex buffers are mapped
> during
>> > draw call!
>> >>
>> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95142
>> >> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
>> >> Cc: Kenneth Graunke <kenneth at whitecape.org>
>> >> ---
>> >>
>> >>  Perhaps it would be better to push the vbo_bind_arrays before API
>> >>  validation, and then make this check be part of API validation...
>> >
>> > This looks fine to me, though it'd be nice to get a review from someone
>> > else who's familiar with this code.  Cc'ing Marek and Fredrik in case
>> > they have an opinion.
>>
>> The patch says it's a GLES fix, but it modifies desktop GL behavior
>> too. Is that intentional?
>>
>> Marek
>
> I wasn't able to find any specific spec text requiring us to raise an
> error for draw calls when VBOs are mapped, which is rather concerning.
> However, given that there's a conformance test, it seems likely that
> other vendors implement this behavior.
>
> It's certainly legal to make the change in both APIs:
>
> ES 3.2 says:
>
> "6.3.2 Effects of Mapping Buffers on Other GL Commands
>
>  Any GL command which attempts to read from, write to, or change the
>  state of a buffer object may generate an INVALID_OPERATION error if all
>  or part of the buffer object is mapped. However, only commands which
>  explicitly describe this error are required to do so. If an error is not
>  generated, using such commands to perform invalid reads, writes, or
>  state changes will have undefined results and may
>  result in GL interruption or termination.
>
> GL 4.5 has similar text, but mentions MAP_PERSISTENT_BIT:
>
> "6.3.2 Effects of Mapping Buffers on Other GL Commands
>
>  Any GL command which attempts to read from, write to, or change the
>  state of a buffer object may generate an INVALID_OPERATION error if all
>  or part of the buffer object is mapped, unless it was allocated by a
>  call to *BufferStorage with the MAP_PERSISTENT_BIT included in flags.
>  However, only commands which explicitly describe this error are
>  required to do so. If an error is not generated, such commands will
>  have undefined results and may result in GL interruption or
>  termination."
>
> It also seems pretty reasonable to expect them to use a PERSISTENT
> mapping if they want to leave it mapped on the CPU while drawing
> (reading) from it on the GPU.
>
> So I figure...we may as well be consistent and implement the same
> behavior in both APIs.

Sounds good.

Reviewed-by: Marek Olšák <marek.olsak at amd.com>

Marek