[prev in list] [next in list] [prev in thread] [next in thread]
List: kwin
Subject: Re: Review Request: Support permanent glSwapBuffer path
From: Thomas_Lübking <thomas.luebking () web ! de>
Date: 2012-11-29 19:11:01
Message-ID: 20121129191101.12894.19540 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> On Nov. 24, 2012, 5:27 p.m., Ralf Jung wrote:
> > I got pointed to a bug where the clutter guys discussed similar issues: \
> > https://bugzilla.gnome.org/show_bug.cgi?id=651312#c37
> > The current patch does not apply cleanly against current master, probably due to \
> > a0ee4035. Other than that, it works as expected. Thomas, is there any noticeable \
> > difference on your NVidia system between the 'c' and the 'p' option? I hope to \
> > test this on my Nvidia box soon.
> > If it were up to me, I'd also remove the glCopyPixels path from the glxbackend, \
> > and add some way for the backend to tell whether it supports partial-screen \
> > updates at all - that would avoid the fake frontbuffer for old drivers, and it \
> > would also help the EGL backend to avoid the costy backbuffer preservation. The \
> > way thinks work now, the EGL backend without PostSubBuffer would still have to \
> > enable backbuffer preservation (breaking v-sync even for buffer swaps), or copy \
> > the glCopyPixels code from the GLX backend.
>
> Thomas Lübking wrote:
> > Thomas, is there any noticeable difference on your NVidia system between the 'c' \
> > and the 'p' option?
> Depends heavily on the paint complexity (the more and layered (esp. transparent) \
> windows and blur and the tinier the actual repaint the better is the "c" variant \
> (but measured in CPU load and QElapsedTimer - the FPS is usually always > 60FPS)
>
> > If it were up to me, I'd also remove the glCopyPixels path from the glxbackend
> Means you'd enforce full repaints for the nvidia blob, because it cannot do \
> copySubBuffer - since the patch introduces a parameter to force full repaints, the \
> egl backend coul depend on that for the purpose.
> *Removing* a complete path after the first beta is imo an absolute no-go.
> Optionally avoid it is ok, opt-into the path during the 2nd beta (for testing) as \
> well, but removing it now sounds faar too regression prone (low-end / old nvidia \
> blob, no idea about fglrx, some mesa driver w/o copySubBuffer support, god knows \
> what)
> Thomas Lübking wrote:
> Backing with a number:
> painting ~500 rects in 100 cycles lasts ~0.15ms (i used clock_gettime to get the ns \
> and added a glFinish()) in the entire pixel copying loop. A 10th of that goes to \
> the client code (ie. it lasts ~0.015ms w/o the glFinish)
> Thomas Lübking wrote:
> s/painting/coyping/
>
> Ralf Jung wrote:
> Do you have any numbers for the other case, i.e. copy a texture to the backbuffer \
> (the 'p' path)? Or, in other words: Is it worth keeping the glCopyPixels paths (the \
> 'c' option, and the code in glxbackend) in the long term for the proprietary NVidia \
> driver, knowing that the open-source DRM drivers should not use it? My answer would \
> be "no" if using the 'p' path instead is comparable in performance, and "only one \
> of the two" otherwise. But of course, that decision is up to the maintainer(s).
> > *Removing* a complete path after the first beta is imo an absolute no-go.
> > Optionally avoid it is ok, opt-into the path during the 2nd beta (for testing) as \
> > well, but removing it now sounds faar too regression prone (low-end / old nvidia \
> > blob, no idea about fglrx, some mesa driver w/o copySubBuffer support, god knows \
> > what)
> Oh, that was not meant as suggestion for KDE 4.10, but for the long-term state \
> (i.e. KDE 4.11). IMHO it's the cleanest solution.
> For KDE 4.11, I'd be happy for something like the p or e solution, at least \
> optionally. But again, I can only ask for someone else to decide - I just hope to \
> see tearing fixed at some point ;-)
Paint time as calculated by stock FPS counter bumps from ~2ms to ~8ms when updating \
~1/16 of the screen (sd video) - without blurring. With blurring, the advance is \
lower (8ms -> 10 ms)
Trying a half screen wido + fps counter actually causes nog significant difference \
(ie. the margins stay)
One of the major and general defects of forcing full repaints, is that the showPaint \
plugin becomes useless for debuggin, so we'll need a way to turn it off.
Notice that the fkae frontbuffer is certainly not necessary for fullscreen & \
redirecting contexts (us) at all and (kinda OT), i was frankly under the impression \
there were dedicated overlay buffer(s), blended into the scanout buffer by the \
overlay color match and the driver could actually clamp the clients frontbuffer \
accesses to the exposed area of the client drawable i don't frankly see why this \
should be so much a problem with the MESA drivers (at least in local contexts), also \
given that the nvidia blob does not care at all. However I assume that the weaker \
IGPs could profit a lot from that - *shrug*
- Thomas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/107198/#review22488
-----------------------------------------------------------
On Nov. 24, 2012, 6:05 p.m., Thomas Lübking wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107198/
> -----------------------------------------------------------
>
> (Updated Nov. 24, 2012, 6:05 p.m.)
>
>
> Review request for kwin, Martin Gräßlin and Ralf Jung.
>
>
> Description
> -------
>
> This refactors the flushBuffer of glxBackend to handle the requirement of swapping \
> and copying independently. A config option allows to swap and then copy damaged \
> areas back to align front and backbuffers, by this use glSwapBuffer all the time \
> (even for minor screen updates)
> The patch has a minor optimization for the fullscreen painting case to shortcut \
> into a plain buffer swap
> Ratio:
> glWaitVideoSync is reported to not be supported by the nvidia blob ever since and \
> is no longer on SNA either, what means effecetively kwin does not provide GL \
> v'syncing for those GPUs / drivers.
> Pending issue:
> the effectframes currently don't perform a clipped repaint (neither does beclock, \
> but that's my problem) so this needs to be changed (for the option being enabled) \
> to fix the various effectframe paints.
>
> This addresses bug 307965.
> http://bugs.kde.org/show_bug.cgi?id=307965
>
>
> Diffs
> -----
>
> kwin/composite.cpp 8eb5b3f
> kwin/eglonxbackend.cpp 01d97c0
> kwin/glxbackend.cpp 627812f
> kwin/options.h f9f623f
> kwin/options.cpp a49ff92
> kwin/scene.h 1b17066
> kwin/scene.cpp 2e4190a
> kwin/scene_opengl.cpp b27fb3a
>
> Diff: http://git.reviewboard.kde.org/r/107198/diff/
>
>
> Testing
> -------
>
> en- and disabled for OpenGL 2 + 1.3
>
>
> Thanks,
>
> Thomas Lübking
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 \
solid;"> <tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/107198/">http://git.reviewboard.kde.org/r/107198/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On November 24th, 2012, 5:27 p.m., <b>Ralf \
Jung</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">I got pointed to a bug where the clutter guys discussed similar issues: \
https://bugzilla.gnome.org/show_bug.cgi?id=651312#c37
The current patch does not apply cleanly against current master, probably due to \
a0ee4035. Other than that, it works as expected. Thomas, is there any noticeable \
difference on your NVidia system between the 'c' and the 'p' option? \
I hope to test this on my Nvidia box soon.
If it were up to me, I'd also remove the glCopyPixels path from the glxbackend, \
and add some way for the backend to tell whether it supports partial-screen updates \
at all - that would avoid the fake frontbuffer for old drivers, and it would also \
help the EGL backend to avoid the costy backbuffer preservation. The way thinks work \
now, the EGL backend without PostSubBuffer would still have to enable backbuffer \
preservation (breaking v-sync even for buffer swaps), or copy the glCopyPixels code \
from the GLX backend.</pre> </blockquote>
<p>On November 24th, 2012, 5:56 p.m., <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> Thomas, is there \
any noticeable difference on your NVidia system between the 'c' and the \
'p' option? Depends heavily on the paint complexity (the more and layered \
(esp. transparent) windows and blur and the tinier the actual repaint the better is \
the "c" variant (but measured in CPU load and QElapsedTimer - the FPS is \
usually always > 60FPS)
> If it were up to me, I'd also remove the glCopyPixels path from the \
glxbackend Means you'd enforce full repaints for the nvidia blob, because it \
cannot do copySubBuffer - since the patch introduces a parameter to force full \
repaints, the egl backend coul depend on that for the purpose.
*Removing* a complete path after the first beta is imo an absolute no-go.
Optionally avoid it is ok, opt-into the path during the 2nd beta (for testing) as \
well, but removing it now sounds faar too regression prone (low-end / old nvidia \
blob, no idea about fglrx, some mesa driver w/o copySubBuffer support, god knows \
what)</pre> </blockquote>
<p>On November 24th, 2012, 6:31 p.m., <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Backing with a number: \
painting ~500 rects in 100 cycles lasts ~0.15ms (i used clock_gettime to get the ns \
and added a glFinish()) in the entire pixel copying loop. A 10th of that goes to the \
client code (ie. it lasts ~0.015ms w/o the glFinish)</pre> </blockquote>
<p>On November 24th, 2012, 6:32 p.m., <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">s/painting/coyping/</pre> </blockquote>
<p>On November 28th, 2012, 6:34 p.m., <b>Ralf Jung</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Do you have any numbers \
for the other case, i.e. copy a texture to the backbuffer (the 'p' path)? Or, \
in other words: Is it worth keeping the glCopyPixels paths (the 'c' option, \
and the code in glxbackend) in the long term for the proprietary NVidia driver, \
knowing that the open-source DRM drivers should not use it? My answer would be \
"no" if using the 'p' path instead is comparable in performance, \
and "only one of the two" otherwise. But of course, that decision is up to \
the maintainer(s).
> *Removing* a complete path after the first beta is imo an absolute no-go.
> Optionally avoid it is ok, opt-into the path during the 2nd beta (for testing) \
as well, but removing it now sounds faar too regression prone (low-end / old nvidia \
blob, no idea about fglrx, some mesa driver w/o > copySubBuffer support, god knows \
what) Oh, that was not meant as suggestion for KDE 4.10, but for the long-term state \
(i.e. KDE 4.11). IMHO it's the cleanest solution.
For KDE 4.11, I'd be happy for something like the p or e solution, at least \
optionally. But again, I can only ask for someone else to decide - I just hope to see \
tearing fixed at some point ;-)</pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Paint time as calculated \
by stock FPS counter bumps from ~2ms to ~8ms when updating ~1/16 of the screen (sd \
video) - without blurring. With blurring, the advance is lower (8ms -> 10 ms)
Trying a half screen wido + fps counter actually causes nog significant difference \
(ie. the margins stay)
One of the major and general defects of forcing full repaints, is that the showPaint \
plugin becomes useless for debuggin, so we'll need a way to turn it off.
Notice that the fkae frontbuffer is certainly not necessary for fullscreen & \
redirecting contexts (us) at all and (kinda OT), i was frankly under the impression \
there were dedicated overlay buffer(s), blended into the scanout buffer by the \
overlay color match and the driver could actually clamp the clients frontbuffer \
accesses to the exposed area of the client drawable i don't frankly see why this \
should be so much a problem with the MESA drivers (at least in local contexts), also \
given that the nvidia blob does not care at all. However I assume that the weaker \
IGPs could profit a lot from that - *shrug*</pre> <br />
<p>- Thomas</p>
<br />
<p>On November 24th, 2012, 6:05 p.m., Thomas Lübking wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" \
style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); \
background-position: left top; background-repeat: repeat-x; border: 1px black \
solid;"> <tr>
<td>
<div>Review request for kwin, Martin Gräßlin and Ralf Jung.</div>
<div>By Thomas Lübking.</div>
<p style="color: grey;"><i>Updated Nov. 24, 2012, 6:05 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">This refactors the flushBuffer of glxBackend to handle the requirement \
of swapping and copying independently. A config option allows to swap and then copy \
damaged areas back to align front and backbuffers, by this use glSwapBuffer all the \
time (even for minor screen updates)
The patch has a minor optimization for the fullscreen painting case to shortcut into \
a plain buffer swap
Ratio:
glWaitVideoSync is reported to not be supported by the nvidia blob ever since and is \
no longer on SNA either, what means effecetively kwin does not provide GL \
v'syncing for those GPUs / drivers.
Pending issue:
the effectframes currently don't perform a clipped repaint (neither does beclock, \
but that's my problem) so this needs to be changed (for the option being enabled) \
to fix the various effectframe paints. </pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;">en- and disabled for OpenGL 2 + 1.3</pre> </td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=307965">307965</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kwin/composite.cpp <span style="color: grey">(8eb5b3f)</span></li>
<li>kwin/eglonxbackend.cpp <span style="color: grey">(01d97c0)</span></li>
<li>kwin/glxbackend.cpp <span style="color: grey">(627812f)</span></li>
<li>kwin/options.h <span style="color: grey">(f9f623f)</span></li>
<li>kwin/options.cpp <span style="color: grey">(a49ff92)</span></li>
<li>kwin/scene.h <span style="color: grey">(1b17066)</span></li>
<li>kwin/scene.cpp <span style="color: grey">(2e4190a)</span></li>
<li>kwin/scene_opengl.cpp <span style="color: grey">(b27fb3a)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/107198/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
kwin mailing list
kwin@kde.org
https://mail.kde.org/mailman/listinfo/kwin
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic