Re: [PATCH] modesetting: prefer primary crtc when picking over nothing

2016-08-02 Thread Keith Packard
Michel Dänzer  writes:

> I stand corrected. I was thinking of the primary output as reported by
> xrandr.

I had to actually go read the code; I had not remembered what
'RRFirstOutput' did either. Shockingly, there's a *comment* describing
what it does. What was that author thinking!

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] modesetting: prefer primary crtc when picking over nothing

2016-08-02 Thread Michel Dänzer
On 03.08.2016 14:10, Keith Packard wrote:
> Michel Dänzer  writes:
> 
>> This can indeed only avoid the problem by accident, since there's no
>> guarantee that the primary output is even connected.
> 
> Actually, he used the RRFirstOutput function which does find something
> connected to an active CRTC, so at least it should work as he intended?

I stand corrected. I was thinking of the primary output as reported by
xrandr.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer



signature.asc
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] modesetting: prefer primary crtc when picking over nothing

2016-08-02 Thread Keith Packard
Michel Dänzer  writes:

> This can indeed only avoid the problem by accident, since there's no
> guarantee that the primary output is even connected.

Actually, he used the RRFirstOutput function which does find something
connected to an active CRTC, so at least it should work as he intended?

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] modesetting: prefer primary crtc when picking over nothing

2016-08-02 Thread Keith Packard
Dave Airlie  writes:

> So we had a bug report that epiphany was rendering slow on
> modesetting + DRI3, that I tracked down to the fact it was
> rendering offscreen to a single buffer. However due to this
> rendering being offscreen we were getting the fake crtc
> chosen by the X server as the window didn't overlap any
> active CRTCs.

It looks like that's just a bug -- if they don't want to be throttled,
they should fix the swap interval on their crazy redirected and
off-screen window.

> This didn't happen on amdgpu and I ported this patch across,
> but I believe this only fixed this problem by accident,
> it always picks the primary crtc when it needs one.

That means you're never getting the 1fps throttling setting if a screen
is lit up?

> But this fixes it here for me, the question is whether
> we should do better at picking the fake crtc and whether
> we should be throttling things.

I'd say no; the throttling behaviour is designed to reduce power
consumption when an application isn't visible.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 2/2] present: Call set_abort_flip / restore_screen_pixmap in clear_window_flip

2016-08-02 Thread Michel Dänzer
From: Michel Dänzer 

We were asserting that these were called before from other places, but
that isn't always the case, e.g. during server shutdown.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96951
Reported-and-Tested-by: Tod Jackson 
Signed-off-by: Michel Dänzer 
---
 present/present.c| 4 ++--
 present/present_priv.h   | 6 ++
 present/present_screen.c | 6 --
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/present/present.c b/present/present.c
index 8d37395..a7ca06e 100644
--- a/present/present.c
+++ b/present/present.c
@@ -417,7 +417,7 @@ present_set_tree_pixmap(WindowPtr window,
 TraverseTree(window, present_set_tree_pixmap_visit, &visit);
 }
 
-static void
+void
 present_restore_screen_pixmap(ScreenPtr screen)
 {
 present_screen_priv_ptr screen_priv = present_screen_priv(screen);
@@ -451,7 +451,7 @@ present_restore_screen_pixmap(ScreenPtr screen)
 present_set_tree_pixmap(screen->root, NULL, screen_pixmap);
 }
 
-static void
+void
 present_set_abort_flip(ScreenPtr screen)
 {
 present_screen_priv_ptr screen_priv = present_screen_priv(screen);
diff --git a/present/present_priv.h b/present/present_priv.h
index 0d16cfa..dfb4bde 100644
--- a/present/present_priv.h
+++ b/present/present_priv.h
@@ -187,6 +187,12 @@ void
 present_flip_destroy(ScreenPtr screen);
 
 void
+present_restore_screen_pixmap(ScreenPtr screen);
+
+void
+present_set_abort_flip(ScreenPtr screen);
+
+void
 present_check_flip_window(WindowPtr window);
 
 RRCrtcPtr
diff --git a/present/present_screen.c b/present/present_screen.c
index 2f91ac7..9d03c8a 100644
--- a/present/present_screen.c
+++ b/present/present_screen.c
@@ -92,11 +92,13 @@ present_clear_window_flip(WindowPtr window)
 present_vblank_ptr  flip_pending = screen_priv->flip_pending;
 
 if (flip_pending && flip_pending->window == window) {
-assert (flip_pending->abort_flip);
+present_set_abort_flip(screen);
 flip_pending->window = NULL;
 }
-if (screen_priv->flip_window == window)
+if (screen_priv->flip_window == window) {
+present_restore_screen_pixmap(screen);
 screen_priv->flip_window = NULL;
+}
 }
 
 /*
-- 
2.8.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 1/2] present: Make present_restore_screen_pixmap handle screen->root == NULL

2016-08-02 Thread Michel Dänzer
From: Michel Dänzer 

Easier than dealing with it in all paths that can end up here during
server shutdown.

Signed-off-by: Michel Dänzer 
---
 present/present.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/present/present.c b/present/present.c
index 5fde846..8d37395 100644
--- a/present/present.c
+++ b/present/present.c
@@ -439,7 +439,7 @@ present_restore_screen_pixmap(ScreenPtr screen)
  * Only do this the first time for a particular unflip operation, or
  * we'll probably scribble over other windows
  */
-if (screen->GetWindowPixmap(screen->root) == flip_pixmap)
+if (screen->root && screen->GetWindowPixmap(screen->root) == flip_pixmap)
 present_copy_region(&screen_pixmap->drawable, flip_pixmap, NULL, 0, 0);
 
 /* Switch back to using the screen pixmap now to avoid
@@ -447,7 +447,8 @@ present_restore_screen_pixmap(ScreenPtr screen)
  */
 if (flip_window)
 present_set_tree_pixmap(flip_window, flip_pixmap, screen_pixmap);
-present_set_tree_pixmap(screen->root, NULL, screen_pixmap);
+if (screen->root)
+present_set_tree_pixmap(screen->root, NULL, screen_pixmap);
 }
 
 static void
-- 
2.8.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] modesetting: prefer primary crtc when picking over nothing

2016-08-02 Thread Michel Dänzer
On 03.08.2016 09:24, Dave Airlie wrote:
> So we had a bug report that epiphany was rendering slow on
> modesetting + DRI3, that I tracked down to the fact it was
> rendering offscreen to a single buffer. However due to this
> rendering being offscreen we were getting the fake crtc
> chosen by the X server as the window didn't overlap any
> active CRTCs.
> 
> This didn't happen on amdgpu and I ported this patch across,
> but I believe this only fixed this problem by accident,
> it always picks the primary crtc when it needs one.

This can indeed only avoid the problem by accident, since there's no
guarantee that the primary output is even connected.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer



signature.asc
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] Pass ClientPtr to FlushCallback

2016-08-02 Thread Michel Dänzer
On 03.08.2016 09:40, Eric Anholt wrote:
> Keith Packard  writes:
> 
>> Michel Dänzer  writes:
>>
>>> From: Michel Dänzer 
>>>
>>> This change has two effects:
>>>
>>> 1. Only calls FlushCallbacks when we're actually flushing data to a
>>>client. The unnecessary FlushCallback calls could cause significant
>>>performance degradation with compositing, which is significantly
>>>reduced even without any driver changes.
>>
>> Seems like a completely reasonable plan. As you can see, the original
>> goal was to have the callback called only once when flushing output to
>> multiple clients though. That appears to only be used by the record
>> extension, so perhaps we just don't care.

I suspect that the record extension could also take advantage of this
change, but I'm not sure and can't say that I care too much. :)


>>> 2. By passing the ClientPtr to FlushCallbacks, drivers can completely
>>>eliminate unnecessary flushing of GPU commands by keeping track of
>>>whether we're flushing any XDamageNotify events to the client for
>>>which the corresponding rendering commands haven't been flushed to
>>>the GPU yet.
>>
>> Is this something we should be doing in either glamor or DIX itself? It
>> looks like the ATI driver has a number that is incremented every time
>> commands are sent to the GPU and that clients need to be flushed
>> whenever they haven't been flushed since the last time that number was
>> changed?
>>
>> I don't even know how this works in the generic modesetting driver at
>> this point; what makes sure that glFlush is called before data are sent
>> to the client?
> 
> We glFlush in the BlockHandler in glamor.

That's not sufficient for correctness with direct rendering compositors,
because FlushClient can be called before BlockHandler.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer



signature.asc
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] Pass ClientPtr to FlushCallback

2016-08-02 Thread Michel Dänzer
On 03.08.2016 09:40, Eric Anholt wrote:
> Michel Dänzer  writes:
> 
>> From: Michel Dänzer 
>>
>> This change has two effects:
>>
>> 1. Only calls FlushCallbacks when we're actually flushing data to a
>>client. The unnecessary FlushCallback calls could cause significant
>>performance degradation with compositing, which is significantly
>>reduced even without any driver changes.
>>
>> 2. By passing the ClientPtr to FlushCallbacks, drivers can completely
>>eliminate unnecessary flushing of GPU commands by keeping track of
>>whether we're flushing any XDamageNotify events to the client for
>>which the corresponding rendering commands haven't been flushed to
>>the GPU yet.
>>
>> Signed-off-by: Michel Dänzer 
>> ---
>>
>> See https://lists.freedesktop.org/archives/amd-gfx/2016-August/000977.html
>> for an example of how to take advantage of this change to eliminate
>> unnecessary GPU flushes.
> 
> Note: Mesa's DRI2 is (supposed to be) doing XSync() during glXWaitX() to
> ensure that the server has processed the client's X requests and flushed
> its batchbuffers, so that the kernel serializes the batchbuffer from X
> before the next rendering by Mesa.  I think your xf86-video-ati patches
> will break that.

Can you elaborate how? I honestly can't imagine.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer



signature.asc
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] Pass ClientPtr to FlushCallback

2016-08-02 Thread Keith Packard
Eric Anholt  writes:

> We glFlush in the BlockHandler in glamor.

Sure, what ensures that is called before damage events are delivered?

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] Pass ClientPtr to FlushCallback

2016-08-02 Thread Eric Anholt
Keith Packard  writes:

> Michel Dänzer  writes:
>
>> From: Michel Dänzer 
>>
>> This change has two effects:
>>
>> 1. Only calls FlushCallbacks when we're actually flushing data to a
>>client. The unnecessary FlushCallback calls could cause significant
>>performance degradation with compositing, which is significantly
>>reduced even without any driver changes.
>
> Seems like a completely reasonable plan. As you can see, the original
> goal was to have the callback called only once when flushing output to
> multiple clients though. That appears to only be used by the record
> extension, so perhaps we just don't care.
>
>> 2. By passing the ClientPtr to FlushCallbacks, drivers can completely
>>eliminate unnecessary flushing of GPU commands by keeping track of
>>whether we're flushing any XDamageNotify events to the client for
>>which the corresponding rendering commands haven't been flushed to
>>the GPU yet.
>
> Is this something we should be doing in either glamor or DIX itself? It
> looks like the ATI driver has a number that is incremented every time
> commands are sent to the GPU and that clients need to be flushed
> whenever they haven't been flushed since the last time that number was
> changed?
>
> I don't even know how this works in the generic modesetting driver at
> this point; what makes sure that glFlush is called before data are sent
> to the client?

We glFlush in the BlockHandler in glamor.


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] Pass ClientPtr to FlushCallback

2016-08-02 Thread Eric Anholt
Michel Dänzer  writes:

> From: Michel Dänzer 
>
> This change has two effects:
>
> 1. Only calls FlushCallbacks when we're actually flushing data to a
>client. The unnecessary FlushCallback calls could cause significant
>performance degradation with compositing, which is significantly
>reduced even without any driver changes.
>
> 2. By passing the ClientPtr to FlushCallbacks, drivers can completely
>eliminate unnecessary flushing of GPU commands by keeping track of
>whether we're flushing any XDamageNotify events to the client for
>which the corresponding rendering commands haven't been flushed to
>the GPU yet.
>
> Signed-off-by: Michel Dänzer 
> ---
>
> See https://lists.freedesktop.org/archives/amd-gfx/2016-August/000977.html
> for an example of how to take advantage of this change to eliminate
> unnecessary GPU flushes.

Note: Mesa's DRI2 is (supposed to be) doing XSync() during glXWaitX() to
ensure that the server has processed the client's X requests and flushed
its batchbuffers, so that the kernel serializes the batchbuffer from X
before the next rendering by Mesa.  I think your xf86-video-ati patches
will break that.


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] modesetting: prefer primary crtc when picking over nothing

2016-08-02 Thread Dave Airlie
So we had a bug report that epiphany was rendering slow on
modesetting + DRI3, that I tracked down to the fact it was
rendering offscreen to a single buffer. However due to this
rendering being offscreen we were getting the fake crtc
chosen by the X server as the window didn't overlap any
active CRTCs.

This didn't happen on amdgpu and I ported this patch across,
but I believe this only fixed this problem by accident,
it always picks the primary crtc when it needs one.

But this fixes it here for me, the question is whether
we should do better at picking the fake crtc and whether
we should be throttling things.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85064
Signed-off-by: Dave Airlie 
---
 hw/xfree86/drivers/modesetting/vblank.c | 17 ++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/hw/xfree86/drivers/modesetting/vblank.c 
b/hw/xfree86/drivers/modesetting/vblank.c
index 77e0848..0f28f9d 100644
--- a/hw/xfree86/drivers/modesetting/vblank.c
+++ b/hw/xfree86/drivers/modesetting/vblank.c
@@ -102,17 +102,26 @@ ms_covering_crtc(ScrnInfoPtr scrn,
  BoxPtr box, xf86CrtcPtr desired, BoxPtr crtc_box_ret)
 {
 xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
-xf86CrtcPtr crtc, best_crtc;
+xf86CrtcPtr crtc, best_crtc = NULL, primary_crtc = NULL;
 int coverage, best_coverage;
 int c;
 BoxRec crtc_box, cover_box;
+RROutputPtr primary_output = NULL;
 
-best_crtc = NULL;
 best_coverage = 0;
 crtc_box_ret->x1 = 0;
 crtc_box_ret->x2 = 0;
 crtc_box_ret->y1 = 0;
 crtc_box_ret->y2 = 0;
+
+/* Prefer the CRTC of the primary output */
+if (dixPrivateKeyRegistered(rrPrivKey))
+{
+primary_output = RRFirstOutput(scrn->pScreen);
+}
+if (primary_output && primary_output->crtc)
+primary_crtc = primary_output->crtc->devPrivate;
+
 for (c = 0; c < xf86_config->num_crtc; c++) {
 crtc = xf86_config->crtc[c];
 
@@ -127,7 +136,9 @@ ms_covering_crtc(ScrnInfoPtr scrn,
 *crtc_box_ret = crtc_box;
 return crtc;
 }
-if (coverage > best_coverage) {
+if (coverage > best_coverage ||
+(coverage == best_coverage &&
+ crtc == primary_crtc)) {
 *crtc_box_ret = crtc_box;
 best_crtc = crtc;
 best_coverage = coverage;
-- 
2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] xace: Fix XaceCensorImage to actually censor the right part of the image

2016-08-02 Thread Aaron Plattner
The caller passes arguments into XaceCensorImage that are in window-relative
coordinates. However, the pBuf that it uses to construct a temporary pixmap has
its origin at (x, y) relative to the window in question. The code to convert the
censor region into boxes adjusts for the Y coordinate, but leaves the X
coordinate alone. The result is that if x is not zero, it censors the wrong part
of the image.

Fix this by just translating censorRegion into pixmap-relative coordinates and
using the resulting boxes as-is.

Reported-by: Fabien Lelaquais 
Link: https://lists.x.org/archives/xorg/2016-August/058165.html
Signed-off-by: Aaron Plattner 
---
 Xext/xace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Xext/xace.c b/Xext/xace.c
index 91c74d591267..a3a83a20c08a 100644
--- a/Xext/xace.c
+++ b/Xext/xace.c
@@ -245,6 +245,7 @@ XaceCensorImage(ClientPtr client,
 
 /* censorRegion = imageRegion - visibleRegion */
 RegionSubtract(&censorRegion, &imageRegion, pVisibleRegion);
+RegionTranslate(&censorRegion, -x, -y);
 nRects = RegionNumRects(&censorRegion);
 if (nRects > 0) {   /* we have something to censor */
 GCPtr pScratchGC = NULL;
@@ -265,7 +266,7 @@ XaceCensorImage(ClientPtr client,
 }
 for (pBox = RegionRects(&censorRegion), i = 0; i < nRects; i++, 
pBox++) {
 pRects[i].x = pBox->x1;
-pRects[i].y = pBox->y1 - imageBox.y1;
+pRects[i].y = pBox->y1;
 pRects[i].width = pBox->x2 - pBox->x1;
 pRects[i].height = pBox->y2 - pBox->y1;
 }
-- 
2.9.0

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] Pass ClientPtr to FlushCallback

2016-08-02 Thread Keith Packard
Michel Dänzer  writes:

> From: Michel Dänzer 
>
> This change has two effects:
>
> 1. Only calls FlushCallbacks when we're actually flushing data to a
>client. The unnecessary FlushCallback calls could cause significant
>performance degradation with compositing, which is significantly
>reduced even without any driver changes.

Seems like a completely reasonable plan. As you can see, the original
goal was to have the callback called only once when flushing output to
multiple clients though. That appears to only be used by the record
extension, so perhaps we just don't care.

> 2. By passing the ClientPtr to FlushCallbacks, drivers can completely
>eliminate unnecessary flushing of GPU commands by keeping track of
>whether we're flushing any XDamageNotify events to the client for
>which the corresponding rendering commands haven't been flushed to
>the GPU yet.

Is this something we should be doing in either glamor or DIX itself? It
looks like the ATI driver has a number that is incremented every time
commands are sent to the GPU and that clients need to be flushed
whenever they haven't been flushed since the last time that number was
changed?

I don't even know how this works in the generic modesetting driver at
this point; what makes sure that glFlush is called before data are sent
to the client?

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] xwayland: Plug memleak in frame callbacks

2016-08-02 Thread Daniel Stone
On 2 August 2016 at 10:24, Olivier Fourdan  wrote:
> The frame callback set up via wl_surface_frame() needs to be freed with
> wl_callback_destroy() or we'll leak memory.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97065
> Signed-off-by: Olivier Fourdan 

Reviewed-by: Daniel Stone 
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver] xwayland: Plug memleak in frame callbacks

2016-08-02 Thread Olivier Fourdan
The frame callback set up via wl_surface_frame() needs to be freed with
wl_callback_destroy() or we'll leak memory.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=97065
Signed-off-by: Olivier Fourdan 
---
 hw/xwayland/xwayland-cursor.c | 2 ++
 hw/xwayland/xwayland.c| 2 ++
 2 files changed, 4 insertions(+)

diff --git a/hw/xwayland/xwayland-cursor.c b/hw/xwayland/xwayland-cursor.c
index 74dfe4e..7d14a3d 100644
--- a/hw/xwayland/xwayland-cursor.c
+++ b/hw/xwayland/xwayland-cursor.c
@@ -100,6 +100,8 @@ frame_callback(void *data,
uint32_t time)
 {
 struct xwl_seat *xwl_seat = data;
+
+wl_callback_destroy (xwl_seat->cursor_frame_cb);
 xwl_seat->cursor_frame_cb = NULL;
 if (xwl_seat->cursor_needs_update) {
 xwl_seat->cursor_needs_update = FALSE;
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 2d44d07..e9892f7 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -359,6 +359,8 @@ frame_callback(void *data,
uint32_t time)
 {
 struct xwl_window *xwl_window = data;
+
+wl_callback_destroy (xwl_window->frame_callback);
 xwl_window->frame_callback = NULL;
 }
 
-- 
2.7.4

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] FlushAllOutput: Only call FlushCallbacks when actually flushing data

2016-08-02 Thread Michel Dänzer
On 08.07.2016 19:01, Michel Dänzer wrote:
> From: Michel Dänzer 
> 
> The unnecessary FlushCallback calls could cause significant performance
> degradation with compositing.
> 
> As an example, with the radeon driver using glamor, a gtkperf run using
> default parameters (100 iterations of every test) takes:
> (numbers without => with this patch)
> 
> * About 1.9 => 1.9 seconds with xfwm4 without compositing
> * About 2.6 => 2.2 seconds with compton compositing added
> * About 4.1 => 2.2 seconds with kwin compositing
> * About 10.2 => 2.4 seconds with xfwm4 compositing
> 
> Signed-off-by: Michel Dänzer 

I'm retracting this patch in favour of
https://patchwork.freedesktop.org/patch/102509/ .


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver] Pass ClientPtr to FlushCallback

2016-08-02 Thread Michel Dänzer
From: Michel Dänzer 

This change has two effects:

1. Only calls FlushCallbacks when we're actually flushing data to a
   client. The unnecessary FlushCallback calls could cause significant
   performance degradation with compositing, which is significantly
   reduced even without any driver changes.

2. By passing the ClientPtr to FlushCallbacks, drivers can completely
   eliminate unnecessary flushing of GPU commands by keeping track of
   whether we're flushing any XDamageNotify events to the client for
   which the corresponding rendering commands haven't been flushed to
   the GPU yet.

Signed-off-by: Michel Dänzer 
---

See https://lists.freedesktop.org/archives/amd-gfx/2016-August/000977.html
for an example of how to take advantage of this change to eliminate
unnecessary GPU flushes.

 os/connection.c | 2 +-
 os/io.c | 9 +++--
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/os/connection.c b/os/connection.c
index 4294ee7..a901ebf 100644
--- a/os/connection.c
+++ b/os/connection.c
@@ -908,7 +908,7 @@ CloseDownConnection(ClientPtr client)
 OsCommPtr oc = (OsCommPtr) client->osPrivate;
 
 if (FlushCallback)
-CallCallbacks(&FlushCallback, NULL);
+CallCallbacks(&FlushCallback, client);
 
 if (oc->output)
FlushClient(client, oc, (char *) NULL, 0);
diff --git a/os/io.c b/os/io.c
index 88edf12..ff0ec3d 100644
--- a/os/io.c
+++ b/os/io.c
@@ -598,9 +598,6 @@ FlushAllOutput(void)
 register ClientPtr client, tmp;
 Bool newoutput = NewOutputPending;
 
-if (FlushCallback)
-CallCallbacks(&FlushCallback, NULL);
-
 if (!newoutput)
 return;
 
@@ -767,9 +764,6 @@ WriteToClient(ClientPtr who, int count, const void *__buf)
 NewOutputPending = FALSE;
 }
 
-if (FlushCallback)
-CallCallbacks(&FlushCallback, NULL);
-
 return FlushClient(who, oc, buf, count);
 }
 
@@ -815,6 +809,9 @@ FlushClient(ClientPtr who, OsCommPtr oc, const void 
*__extraBuf, int extraCount)
 if (!notWritten)
 return 0;
 
+if (FlushCallback)
+CallCallbacks(&FlushCallback, who);
+
 todo = notWritten;
 while (notWritten) {
 long before = written;  /* amount of whole thing written */
-- 
2.8.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel