[Mesa-dev] glCompressedTex(Sub)Image(2,3) on mapped PBOs

[Ian Romanick]

On 02/24/2015 08:59 AM, Eduardo Lima Mitev wrote:
> On 02/20/2015 08:12 PM, Ian Romanick wrote:
>> On 02/20/2015 08:13 AM, Eduardo Lima Mitev wrote:
>>>
>>> In the spec GLES 3.1 spec, page 57, there is this: (and similar wording
>>> in GL 4.5 spec, page 76)
>>>
>>> "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."
>>
>> This language is intentionally loose.  It is often perfectly safe to
>> have a portion of a buffer mapped while a GL command operates on a
>> different part of the buffer.  Determining whether a particular
>> operation is safe can be expensive.  Imagine trying to determine whether
>> a glDrawElements command will source data from a mapped region of a
>> vertex buffer object.  Many vendors objected to having to put expensive
>> (or cheap) checks in performance critical paths.
>>
>> I don't think an of the TexSubImage commands are performance critical
>> enough to warrant excluding the test.  I believe some benchmarks use
>> some of the TexSubImage commands, so it's probably worth measuring them
>> on, say, Baytrail with and without the checks.  My expectation is that
>> there's enough CPU wasted in those paths that the checks will be lost in
>> the noise.  We won't know until we measure.
>>
>>> There is no further restriction in gl(Compressed)Tex(Sub)Image(2,3)D
>>> that makes throwing an error mandatory, hence I suspect these tests
>>> might be wrong.
>>
>> I'm also inclined to think they are wrong... or at least overly strict.
>>
>> Do you know of any commands that require this error be generated?
>>
> 
> In GLES 3.1, there is explicit mention to this error for
> glBufferSubData, glCopyBufferSubData at least. Similar for GL 4.5 but
> only if the buffer was not mapped with the MAP_PERSISTENT_BIT set:
> 
>     "An INVALID_OPERATION error is generated if the source buffer
>      object is currently mapped, unless it was mapped with
>      MAP_PERSISTENT_BIT set in the Map*BufferRange access flags."
> 
>>>
>>> Also, shouldn't this restriction be moved to common API entry points, or
>>> are there drivers that do write to PBOs while they are mapped?
>>
>> Looking at that function, I find it odd that either of the errors it
>> generates are generated there.  We should either do these checks in a
>> single, common place, or we should not do them at all.
>>
> 
> From yours and Jason's comments, it looks like we should at least move
> those checks out of the driver's code.
> 
> Regarding the checks for overlapping, it seems doable but I wonder if
> that effort pays off. Even if we manage to implement it with negligible
> or none overhead, how many applications will actually use that? Right
> now these potential applications have to unmap the buffer, so it will
> require changing the code, re-building, etc. I guess it will all depend
> on the performance gain from operating in several parts of a buffer in
> parallel versus doing it synchronously.
> 
> So, what I propose:
> 
> 1) We write a patch to move the checks to API entry points (I have that
> partially done already), and remove current checks in the driver. It
> would be done for both compressed and non-compressed Tex(Sub)Image APIs.
> 
> 2) We implement the checks for buffer overlapping as a 2nd step, if we
> decide is worth.

I don't think #2 is worth anything.  The question is whether not doing
any checks at all is worth anything.  It's not about enabling some
functionality... the spec already tells application developers to not do
this.  It's about not using as many CPU cycles on (somewhat) silly checks.

I would suggest doing two patches.

Patch 1: Remove all the checks from the driver.

Patch 2: Add them back in common code.

We can run those through our benchmark systems to see if there is any
difference.  If not, we'll squash the patches.

> For 1) I can provide a patch this week, perhaps coordinating with Jason
> since he already gave it a thought. For 2) I could use some help.
> 
> cheers,
> Eduardo