Re: [Mesa-dev] [PATCH 0/5] dri3, gallium: Correctness and performance fixes

[Sinclair Yeh]

This series:  Reviewed-by: Sinclair Yeh 

On Thu, Jun 22, 2017 at 12:42:32PM +0200, Thomas Hellstrom wrote:
> A patch series that deals with dri3 correctness- and performance fixes.
> 
> The corectness fixes attempts to deal with the fact that we need to wait for
> all pending swapbuffers before we touch the front buffer. Otherwise a
> front buffer change may be overwritten by a pending swapbuffer when it
> was actually intended to be drawn *after* the swapbuffer. Also a post
> swapbuffer front read could actually occur *before* the swapbuffer.
> 
> Patch 1 deals with the dri3 internal synchronization. All frontbuffer
> accesses introduce a "swapbuffer barrier" to order with respect to
> pending swapbuffers.
> The exception is _WaitX because if we call _WaitX we're ordering with
> respect to X rendering and if there are pending swapbuffers, an application
> would already have called _WaitGL to be able to do the X rendering correctly,
> and _WaitGL is ordering with respect to pending swapbuffers. This patch
> fixes the piglit copysubbuffer test.
> 
> Patch 2 to 4 deals with having glFinish() order with respect to pending
> swapbuffers. The behaviour is actually not correct in that it doesn't wait
> for the pending swapbuffers to complete, but a user shouldn't be able to
> tell the difference. This patch series is motivated by the fact that the
> glXWaitGL man page states that glFinish() can be used instead of glXWaitGL,
> and without this series it can't. The functionality is only implemented for
> gallium. Other drivers need to provide their own implementation. Ideally
> we should have accomplished this without the dri interface changes by
> calling an unconditional flush_frontbuffer, but at least the gallium
> flush_frontbuffer implementation is relying on us having a fake front which
> is not always the case.
> 
> Patch 5 replaces the back-to-fake-front full buffer copies with a swap
> during swapbuffers. Should be saving a lot of work when we actually have a
> fake front. There were some conserns raised when this was posted as an RFC
> that the separate-server-gpu case would be broken as well as the buffer
> age functionality. I've audited the code and I think that's not the case.
> The piglit buffer age test still reports a pass after this change. The
> separate-server-gpu case I guess needs additional testing.
> 
> Tested wih piglit -quick without regressions.
> 
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/5] dri3, gallium: Correctness and performance fixes

[Thomas Hellstrom]

On 06/30/2017 03:35 PM, Brian Paul wrote:

On 06/28/2017 11:07 AM, Thomas Hellstrom wrote:

On 06/22/2017 12:42 PM, Thomas Hellstrom wrote:

A patch series that deals with dri3 correctness- and performance fixes.

The corectness fixes attempts to deal with the fact that we need to
wait for
all pending swapbuffers before we touch the front buffer. Otherwise a
front buffer change may be overwritten by a pending swapbuffer when it
was actually intended to be drawn *after* the swapbuffer. Also a post
swapbuffer front read could actually occur *before* the swapbuffer.

Patch 1 deals with the dri3 internal synchronization. All frontbuffer
accesses introduce a "swapbuffer barrier" to order with respect to
pending swapbuffers.
The exception is _WaitX because if we call _WaitX we're ordering with
respect to X rendering and if there are pending swapbuffers, an
application
would already have called _WaitGL to be able to do the X rendering
correctly,
and _WaitGL is ordering with respect to pending swapbuffers. This patch
fixes the piglit copysubbuffer test.

Patch 2 to 4 deals with having glFinish() order with respect to pending
swapbuffers. The behaviour is actually not correct in that it doesn't
wait
for the pending swapbuffers to complete, but a user shouldn't be 
able to
tell the difference. This patch series is motivated by the fact that 
the

glXWaitGL man page states that glFinish() can be used instead of
glXWaitGL,
and without this series it can't. The functionality is only
implemented for
gallium. Other drivers need to provide their own implementation. 
Ideally

we should have accomplished this without the dri interface changes by
calling an unconditional flush_frontbuffer, but at least the gallium
flush_frontbuffer implementation is relying on us having a fake front
which
is not always the case.

Patch 5 replaces the back-to-fake-front full buffer copies with a swap
during swapbuffers. Should be saving a lot of work when we actually
have a
fake front. There were some conserns raised when this was posted as an
RFC
that the separate-server-gpu case would be broken as well as the buffer
age functionality. I've audited the code and I think that's not the 
case.

The piglit buffer age test still reports a pass after this change. The
separate-server-gpu case I guess needs additional testing.

Tested wih piglit -quick without regressions.


Hi!

Patches 1,2 and 5 are still unreviewed. It would be good to have at
least a pair of extra eyes on them.


Looks like Emil had an EGL concern with patch 5.  Otherwise, I'm not a 
DRI3 expert but the code looks OK to me.


Reviewed-by: Brian Paul 




Thanks, Brian.

I won't push patch 5 until after the vacation when I can be there to 
rapidly back it out if it breaks something. Also I need to fix up the 
EGL breakage.


/Thomas


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/5] dri3, gallium: Correctness and performance fixes

[Brian Paul]

On 06/28/2017 11:07 AM, Thomas Hellstrom wrote:

On 06/22/2017 12:42 PM, Thomas Hellstrom wrote:

A patch series that deals with dri3 correctness- and performance fixes.

The corectness fixes attempts to deal with the fact that we need to
wait for
all pending swapbuffers before we touch the front buffer. Otherwise a
front buffer change may be overwritten by a pending swapbuffer when it
was actually intended to be drawn *after* the swapbuffer. Also a post
swapbuffer front read could actually occur *before* the swapbuffer.

Patch 1 deals with the dri3 internal synchronization. All frontbuffer
accesses introduce a "swapbuffer barrier" to order with respect to
pending swapbuffers.
The exception is _WaitX because if we call _WaitX we're ordering with
respect to X rendering and if there are pending swapbuffers, an
application
would already have called _WaitGL to be able to do the X rendering
correctly,
and _WaitGL is ordering with respect to pending swapbuffers. This patch
fixes the piglit copysubbuffer test.

Patch 2 to 4 deals with having glFinish() order with respect to pending
swapbuffers. The behaviour is actually not correct in that it doesn't
wait
for the pending swapbuffers to complete, but a user shouldn't be able to
tell the difference. This patch series is motivated by the fact that the
glXWaitGL man page states that glFinish() can be used instead of
glXWaitGL,
and without this series it can't. The functionality is only
implemented for
gallium. Other drivers need to provide their own implementation. Ideally
we should have accomplished this without the dri interface changes by
calling an unconditional flush_frontbuffer, but at least the gallium
flush_frontbuffer implementation is relying on us having a fake front
which
is not always the case.

Patch 5 replaces the back-to-fake-front full buffer copies with a swap
during swapbuffers. Should be saving a lot of work when we actually
have a
fake front. There were some conserns raised when this was posted as an
RFC
that the separate-server-gpu case would be broken as well as the buffer
age functionality. I've audited the code and I think that's not the case.
The piglit buffer age test still reports a pass after this change. The
separate-server-gpu case I guess needs additional testing.

Tested wih piglit -quick without regressions.


Hi!

Patches 1,2 and 5 are still unreviewed. It would be good to have at
least a pair of extra eyes on them.


Looks like Emil had an EGL concern with patch 5.  Otherwise, I'm not a 
DRI3 expert but the code looks OK to me.


Reviewed-by: Brian Paul 



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 0/5] dri3, gallium: Correctness and performance fixes

[Thomas Hellstrom]

On 06/22/2017 12:42 PM, Thomas Hellstrom wrote:

A patch series that deals with dri3 correctness- and performance fixes.

The corectness fixes attempts to deal with the fact that we need to wait for
all pending swapbuffers before we touch the front buffer. Otherwise a
front buffer change may be overwritten by a pending swapbuffer when it
was actually intended to be drawn *after* the swapbuffer. Also a post
swapbuffer front read could actually occur *before* the swapbuffer.

Patch 1 deals with the dri3 internal synchronization. All frontbuffer
accesses introduce a "swapbuffer barrier" to order with respect to
pending swapbuffers.
The exception is _WaitX because if we call _WaitX we're ordering with
respect to X rendering and if there are pending swapbuffers, an application
would already have called _WaitGL to be able to do the X rendering correctly,
and _WaitGL is ordering with respect to pending swapbuffers. This patch
fixes the piglit copysubbuffer test.

Patch 2 to 4 deals with having glFinish() order with respect to pending
swapbuffers. The behaviour is actually not correct in that it doesn't wait
for the pending swapbuffers to complete, but a user shouldn't be able to
tell the difference. This patch series is motivated by the fact that the
glXWaitGL man page states that glFinish() can be used instead of glXWaitGL,
and without this series it can't. The functionality is only implemented for
gallium. Other drivers need to provide their own implementation. Ideally
we should have accomplished this without the dri interface changes by
calling an unconditional flush_frontbuffer, but at least the gallium
flush_frontbuffer implementation is relying on us having a fake front which
is not always the case.

Patch 5 replaces the back-to-fake-front full buffer copies with a swap
during swapbuffers. Should be saving a lot of work when we actually have a
fake front. There were some conserns raised when this was posted as an RFC
that the separate-server-gpu case would be broken as well as the buffer
age functionality. I've audited the code and I think that's not the case.
The piglit buffer age test still reports a pass after this change. The
separate-server-gpu case I guess needs additional testing.

Tested wih piglit -quick without regressions.


Hi!

Patches 1,2 and 5 are still unreviewed. It would be good to have at 
least a pair of extra eyes on them.


Thanks,

Thomas

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev