Re: [PATCH xserver] damage: Add screen func called before damage event delivery

[Michel Dänzer]

On 17/09/16 12:51 AM, Keith Packard wrote:
> Michel Dänzer  writes:
> 
>> Does FlushClient get called after every DamageExtNotify call? Otherwise,
>> some of the GPU flushes performed by DamageFlushDrawable will be wasted,
>> hurting performance.
> 
> The driver gets told which drawable is being affected, so it could limit
> flushes if there were no rendering operations affecting that pixmap.

When would damage be posted to a drawable without any corresponding
rendering operations? ;)

What I might want to do in our drivers is to use the new hook instead of
an EventCallback to determine when we're sending damage events to a
client. But for that purpose, a) the hook is missing a ClientPtr
parameter and b) its name is a bit misleading.


> I didn't add that code as it wasn't necessary to fix the bug.

FWIW, as is it might result in the modesetting driver getting lower
scores in some 2D benchmarks than our drivers.


>> I don't like glamor hooking into this automatically, because it means
>> the Xorg driver can't know whether or not glamor needs to be flushed.
>> The Xorg driver needs to flush glamor for other reasons as well, e.g.
>> for DRI2/3.
> 
> The lower level driver could wrap the function if it likes; glamor
> certainly needs to flush at this point, so if the lower level driver
> also has work to do at that point, it could.

Unless the hook gets at least a ClientPtr parameter, I'm afraid it's
useless for our drivers, and they'll just have to set the hook to NULL.
Seems a bit pointless. Let the modesetting driver and any other drivers
which want to use glamor_damage_flush_drawable hook it up?


>> FWIW, this will do nothing for GPU screens. I'm not sure whether or not
>> GPU screens need to be flushed for damage events, what are others'
>> thoughts on that?
> 
> The immediate need here is for applications which use DRI to access
> window pixmaps. As GPU screen pixmaps can also be directly accessed
> through DRI, I suspect they would also need these hooks, but I guess I
> don't understand why that wouldn't already be the case here?

In the sub-thread with Hans, we came to the conclusion that GPU screens
don't need to be flushed for damage events, because there's no way for
clients to directly use GPU screen pixmaps. In which case, there's
actually the reverse problem that this patch would introduce superfluous
flushes on GPU screens.


-- 
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

Re: [PATCH xserver] damage: Add screen func called before damage event delivery

[Keith Packard]

Michel Dänzer  writes:

> I'm afraid I'm not sure this is going in a good direction.

Yeah, with the WriteToClient changes from a few months ago, I think we
need to do something though. We used to be able to rely on no events
being delivered before the BlockHandler was invoked. That's no longer true.

> At the very least, the damage and glamor parts should be split into
> separate patches.

That seems reasonable enough.

> Does FlushClient get called after every DamageExtNotify call? Otherwise,
> some of the GPU flushes performed by DamageFlushDrawable will be wasted,
> hurting performance.

The driver gets told which drawable is being affected, so it could limit
flushes if there were no rendering operations affecting that pixmap. I
didn't add that code as it wasn't necessary to fix the bug.

> I don't like glamor hooking into this automatically, because it means
> the Xorg driver can't know whether or not glamor needs to be flushed.
> The Xorg driver needs to flush glamor for other reasons as well, e.g.
> for DRI2/3.

The lower level driver could wrap the function if it likes; glamor
certainly needs to flush at this point, so if the lower level driver
also has work to do at that point, it could.

> FWIW, this will do nothing for GPU screens. I'm not sure whether or not
> GPU screens need to be flushed for damage events, what are others'
> thoughts on that?

The immediate need here is for applications which use DRI to access
window pixmaps. As GPU screen pixmaps can also be directly accessed
through DRI, I suspect they would also need these hooks, but I guess I
don't understand why that wouldn't already be the case here?

-- 
-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] damage: Add screen func called before damage event delivery

[Michel Dänzer]

On 16/09/16 06:14 PM, Hans de Goede wrote:
> On 16-09-16 10:59, Michel Dänzer wrote:
>> On 16/09/16 05:37 PM, Hans de Goede wrote:
>>> On 16-09-16 08:26, Michel Dänzer wrote:
 On 16/09/16 01:37 PM, Keith Packard wrote:
> @@ -1943,3 +1948,13 @@ DamageReportDamage(DamagePtr pDamage,
> RegionPtr pDamageRegion)
>  break;
>  }
>  }
> +
> +void
> +DamageFlushDrawable(DrawablePtr pDrawable)
> +{
> +ScreenPtr pScreen = pDrawable->pScreen;
> +damageScrPriv(pScreen);
> +
> +if (pScrPriv->funcs.Flush)
> +(*pScrPriv->funcs.Flush)(pDrawable);
> +}

 FWIW, this will do nothing for GPU screens. I'm not sure whether or not
 GPU screens need to be flushed for damage events, what are others'
 thoughts on that?
>>>
>>> With render offloading I do think we want to flush,
>>
>> Do you have a specific scenario in mind where a GPU screen needs to be
>> flushed before damage events are sent to a client?
> 
> If a client is somehow tracking a pixmap which gets rendered
> by a secondary GPU, but I did not think about the copy that
> involves, so thinking about this again in that scenario we
> will have:
> 
> 1) Rendering on secondary GPU (aka a GPU screen)
> 2) We need a flush here, but that should already be taken care of
> 3) Copy to buffer/pixmap on primary GPU
> 4) The copy will do damage to the pixmap, but then we're already
> on the primary GPU
> 
>> I've actually almost convinced myself that it's not necessary, because
>> damage events can't refer to any pixmaps directly rendered to by GPU
>> screens?
> 
> See above I think you're right. Following the same logic though
> I do not think the code this patch adds will ever get called on
> a GPU screen, so no need to check for that ?

Right, I merely pointed out that the code in this patch won't do
anything for GPU screens, because I wasn't sure yet if GPU screens need
to be flushed for damage events or not. Sorry if that was confusing.


-- 
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

Re: [PATCH xserver] damage: Add screen func called before damage event delivery

[Hans de Goede]

Hi,

On 16-09-16 10:59, Michel Dänzer wrote:

On 16/09/16 05:37 PM, Hans de Goede wrote:

On 16-09-16 08:26, Michel Dänzer wrote:

On 16/09/16 01:37 PM, Keith Packard wrote:

This lets the video driver flush rendering to the kernel before the
client receives a damage event to a pixmap which the client has direct
rendering access to.


I'm afraid I'm not sure this is going in a good direction.

At the very least, the damage and glamor parts should be split into
separate patches.



diff --git a/damageext/damageext.c b/damageext/damageext.c
index 86b54ee..547f048 100644
--- a/damageext/damageext.c
+++ b/damageext/damageext.c
@@ -98,6 +98,7 @@ DamageExtNotify(DamageExtPtr pDamageExt, BoxPtr
pBoxes, int nBoxes)
 damageGetGeometry(pDrawable, , , , );

 UpdateCurrentTimeIf();
+DamageFlushDrawable(pDrawable);
 ev = (xDamageNotifyEvent) {
 .type = DamageEventBase + XDamageNotify,
 .level = pDamageExt->level,


Does FlushClient get called after every DamageExtNotify call? Otherwise,
some of the GPU flushes performed by DamageFlushDrawable will be wasted,
hurting performance.


Assuming GPU screens don't need to be flushed for damage events, our
drivers could use the damage Flush hook instead of an EventCallback
scanning for damage events though.



@@ -1943,3 +1948,13 @@ DamageReportDamage(DamagePtr pDamage,
RegionPtr pDamageRegion)
 break;
 }
 }
+
+void
+DamageFlushDrawable(DrawablePtr pDrawable)
+{
+ScreenPtr pScreen = pDrawable->pScreen;
+damageScrPriv(pScreen);
+
+if (pScrPriv->funcs.Flush)
+(*pScrPriv->funcs.Flush)(pDrawable);
+}


FWIW, this will do nothing for GPU screens. I'm not sure whether or not
GPU screens need to be flushed for damage events, what are others'
thoughts on that?


With render offloading I do think we want to flush,


Do you have a specific scenario in mind where a GPU screen needs to be
flushed before damage events are sent to a client?


If a client is somehow tracking a pixmap which gets rendered
by a secondary GPU, but I did not think about the copy that
involves, so thinking about this again in that scenario we
will have:

1) Rendering on secondary GPU (aka a GPU screen)
2) We need a flush here, but that should already be taken care of
3) Copy to buffer/pixmap on primary GPU
4) The copy will do damage to the pixmap, but then we're already
on the primary GPU


I've actually almost convinced myself that it's not necessary, because
damage events can't refer to any pixmaps directly rendered to by GPU
screens?


See above I think you're right. Following the same logic though
I do not think the code this patch adds will ever get called on
a GPU screen, so no need to check for that ?


for the slave output case we should never get any>> Damage marked on the slave 
GPU pixmaps to begin with
(and we do want the flush on master GPU pixmaps before
 they get passed to the slave).


This patch only affects client damage records, not PRIME slave output.


Ah rights, it sits in damageext not in mi/damageext.


I assume that there is a check somewhere in the call chain
to not do the flush if there is no damage on the pixmap ?


I don't know of such a thing FWIW.


If it is not there yet, it really should be, I do not think
we want to do flushes to make sure an undamaged pixmap
is completely rendered, as it already should be completely
rendered.

Regards,

Hans
___
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] damage: Add screen func called before damage event delivery

[Michel Dänzer]

On 16/09/16 05:37 PM, Hans de Goede wrote:
> On 16-09-16 08:26, Michel Dänzer wrote:
>> On 16/09/16 01:37 PM, Keith Packard wrote:
>>> This lets the video driver flush rendering to the kernel before the
>>> client receives a damage event to a pixmap which the client has direct
>>> rendering access to.
>>
>> I'm afraid I'm not sure this is going in a good direction.
>>
>> At the very least, the damage and glamor parts should be split into
>> separate patches.
>>
>>
>>> diff --git a/damageext/damageext.c b/damageext/damageext.c
>>> index 86b54ee..547f048 100644
>>> --- a/damageext/damageext.c
>>> +++ b/damageext/damageext.c
>>> @@ -98,6 +98,7 @@ DamageExtNotify(DamageExtPtr pDamageExt, BoxPtr
>>> pBoxes, int nBoxes)
>>>  damageGetGeometry(pDrawable, , , , );
>>>
>>>  UpdateCurrentTimeIf();
>>> +DamageFlushDrawable(pDrawable);
>>>  ev = (xDamageNotifyEvent) {
>>>  .type = DamageEventBase + XDamageNotify,
>>>  .level = pDamageExt->level,
>>
>> Does FlushClient get called after every DamageExtNotify call? Otherwise,
>> some of the GPU flushes performed by DamageFlushDrawable will be wasted,
>> hurting performance.

Assuming GPU screens don't need to be flushed for damage events, our
drivers could use the damage Flush hook instead of an EventCallback
scanning for damage events though.


>>> @@ -1943,3 +1948,13 @@ DamageReportDamage(DamagePtr pDamage,
>>> RegionPtr pDamageRegion)
>>>  break;
>>>  }
>>>  }
>>> +
>>> +void
>>> +DamageFlushDrawable(DrawablePtr pDrawable)
>>> +{
>>> +ScreenPtr pScreen = pDrawable->pScreen;
>>> +damageScrPriv(pScreen);
>>> +
>>> +if (pScrPriv->funcs.Flush)
>>> +(*pScrPriv->funcs.Flush)(pDrawable);
>>> +}
>>
>> FWIW, this will do nothing for GPU screens. I'm not sure whether or not
>> GPU screens need to be flushed for damage events, what are others'
>> thoughts on that?
> 
> With render offloading I do think we want to flush,

Do you have a specific scenario in mind where a GPU screen needs to be
flushed before damage events are sent to a client?

I've actually almost convinced myself that it's not necessary, because
damage events can't refer to any pixmaps directly rendered to by GPU
screens?


> for the slave output case we should never get any
> Damage marked on the slave GPU pixmaps to begin with
> (and we do want the flush on master GPU pixmaps before
>  they get passed to the slave).

This patch only affects client damage records, not PRIME slave output.


> I assume that there is a check somewhere in the call chain
> to not do the flush if there is no damage on the pixmap ?

I don't know of such a thing FWIW.


-- 
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

Re: [PATCH xserver] damage: Add screen func called before damage event delivery

[Hans de Goede]

Hi,

On 16-09-16 08:26, Michel Dänzer wrote:

On 16/09/16 01:37 PM, Keith Packard wrote:

This lets the video driver flush rendering to the kernel before the
client receives a damage event to a pixmap which the client has direct
rendering access to.


I'm afraid I'm not sure this is going in a good direction.

At the very least, the damage and glamor parts should be split into
separate patches.



diff --git a/damageext/damageext.c b/damageext/damageext.c
index 86b54ee..547f048 100644
--- a/damageext/damageext.c
+++ b/damageext/damageext.c
@@ -98,6 +98,7 @@ DamageExtNotify(DamageExtPtr pDamageExt, BoxPtr pBoxes, int 
nBoxes)
 damageGetGeometry(pDrawable, , , , );

 UpdateCurrentTimeIf();
+DamageFlushDrawable(pDrawable);
 ev = (xDamageNotifyEvent) {
 .type = DamageEventBase + XDamageNotify,
 .level = pDamageExt->level,


Does FlushClient get called after every DamageExtNotify call? Otherwise,
some of the GPU flushes performed by DamageFlushDrawable will be wasted,
hurting performance.



@@ -362,6 +379,9 @@ glamor_create_screen_resources(ScreenPtr screen)
 ret = screen->CreateScreenResources(screen);
 screen->CreateScreenResources = glamor_create_screen_resources;

+damage_funcs = DamageGetScreenFuncs(screen);
+if (damage_funcs)
+damage_funcs->Flush = glamor_damage_flush_drawable;
 return ret;
 }


I don't like glamor hooking into this automatically, because it means
the Xorg driver can't know whether or not glamor needs to be flushed.
The Xorg driver needs to flush glamor for other reasons as well, e.g.
for DRI2/3.



@@ -1943,3 +1948,13 @@ DamageReportDamage(DamagePtr pDamage, RegionPtr 
pDamageRegion)
 break;
 }
 }
+
+void
+DamageFlushDrawable(DrawablePtr pDrawable)
+{
+ScreenPtr pScreen = pDrawable->pScreen;
+damageScrPriv(pScreen);
+
+if (pScrPriv->funcs.Flush)
+(*pScrPriv->funcs.Flush)(pDrawable);
+}


FWIW, this will do nothing for GPU screens. I'm not sure whether or not
GPU screens need to be flushed for damage events, what are others'
thoughts on that?


With render offloading I do think we want to flush, for
the slave output case we should never get any
Damage marked on the slave GPU pixmaps to begin with
(and we do want the flush on master GPU pixmaps before
 they get passed to the slave).

I assume that there is a check somewhere in the call chain
to not do the flush if there is no damage on the pixmap ?

Regards,

Hans
___
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] damage: Add screen func called before damage event delivery

[Michel Dänzer]

On 16/09/16 01:37 PM, Keith Packard wrote:
> This lets the video driver flush rendering to the kernel before the
> client receives a damage event to a pixmap which the client has direct
> rendering access to.

I'm afraid I'm not sure this is going in a good direction.

At the very least, the damage and glamor parts should be split into
separate patches.


> diff --git a/damageext/damageext.c b/damageext/damageext.c
> index 86b54ee..547f048 100644
> --- a/damageext/damageext.c
> +++ b/damageext/damageext.c
> @@ -98,6 +98,7 @@ DamageExtNotify(DamageExtPtr pDamageExt, BoxPtr pBoxes, int 
> nBoxes)
>  damageGetGeometry(pDrawable, , , , );
>  
>  UpdateCurrentTimeIf();
> +DamageFlushDrawable(pDrawable);
>  ev = (xDamageNotifyEvent) {
>  .type = DamageEventBase + XDamageNotify,
>  .level = pDamageExt->level,

Does FlushClient get called after every DamageExtNotify call? Otherwise,
some of the GPU flushes performed by DamageFlushDrawable will be wasted,
hurting performance.


> @@ -362,6 +379,9 @@ glamor_create_screen_resources(ScreenPtr screen)
>  ret = screen->CreateScreenResources(screen);
>  screen->CreateScreenResources = glamor_create_screen_resources;
>  
> +damage_funcs = DamageGetScreenFuncs(screen);
> +if (damage_funcs)
> +damage_funcs->Flush = glamor_damage_flush_drawable;
>  return ret;
>  }

I don't like glamor hooking into this automatically, because it means
the Xorg driver can't know whether or not glamor needs to be flushed.
The Xorg driver needs to flush glamor for other reasons as well, e.g.
for DRI2/3.


> @@ -1943,3 +1948,13 @@ DamageReportDamage(DamagePtr pDamage, RegionPtr 
> pDamageRegion)
>  break;
>  }
>  }
> +
> +void
> +DamageFlushDrawable(DrawablePtr pDrawable)
> +{
> +ScreenPtr pScreen = pDrawable->pScreen;
> +damageScrPriv(pScreen);
> +
> +if (pScrPriv->funcs.Flush)
> +(*pScrPriv->funcs.Flush)(pDrawable);
> +}

FWIW, this will do nothing for GPU screens. I'm not sure whether or not
GPU screens need to be flushed for damage events, what are others'
thoughts on that?


-- 
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] damage: Add screen func called before damage event delivery

[Keith Packard]

This lets the video driver flush rendering to the kernel before the
client receives a damage event to a pixmap which the client has direct
rendering access to.

Signed-off-by: Keith Packard 
---
 damageext/damageext.c |  1 +
 glamor/glamor.c   | 20 
 miext/damage/damage.c | 19 +--
 miext/damage/damage.h |  5 +
 4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/damageext/damageext.c b/damageext/damageext.c
index 86b54ee..547f048 100644
--- a/damageext/damageext.c
+++ b/damageext/damageext.c
@@ -98,6 +98,7 @@ DamageExtNotify(DamageExtPtr pDamageExt, BoxPtr pBoxes, int 
nBoxes)
 damageGetGeometry(pDrawable, , , , );
 
 UpdateCurrentTimeIf();
+DamageFlushDrawable(pDrawable);
 ev = (xDamageNotifyEvent) {
 .type = DamageEventBase + XDamageNotify,
 .level = pDamageExt->level,
diff --git a/glamor/glamor.c b/glamor/glamor.c
index 903f6bd..7030e69 100644
--- a/glamor/glamor.c
+++ b/glamor/glamor.c
@@ -35,6 +35,7 @@
 
 #include "glamor_priv.h"
 #include "mipict.h"
+#include "damage.h"
 
 DevPrivateKeyRec glamor_screen_private_key;
 DevPrivateKeyRec glamor_pixmap_private_key;
@@ -346,6 +347,21 @@ fallback:
 }
 
 
+static void
+glamor_flush_screen(ScreenPtr screen)
+{
+glamor_screen_private *glamor_priv = glamor_get_screen_private(screen);
+
+glamor_make_current(glamor_priv);
+glFlush();
+}
+
+static void
+glamor_damage_flush_drawable(DrawablePtr drawable)
+{
+glamor_flush_screen(drawable->pScreen);
+}
+
 /**
  * Creates any pixmaps used internally by glamor, since those can't be
  * allocated at ScreenInit time.
@@ -355,6 +371,7 @@ glamor_create_screen_resources(ScreenPtr screen)
 {
 glamor_screen_private *glamor_priv = glamor_get_screen_private(screen);
 Bool ret = TRUE;
+DamageScreenFuncsPtr damage_funcs;
 
 screen->CreateScreenResources =
 glamor_priv->saved_procs.create_screen_resources;
@@ -362,6 +379,9 @@ glamor_create_screen_resources(ScreenPtr screen)
 ret = screen->CreateScreenResources(screen);
 screen->CreateScreenResources = glamor_create_screen_resources;
 
+damage_funcs = DamageGetScreenFuncs(screen);
+if (damage_funcs)
+damage_funcs->Flush = glamor_damage_flush_drawable;
 return ret;
 }
 
diff --git a/miext/damage/damage.c b/miext/damage/damage.c
index 17c2abf..bb79020 100644
--- a/miext/damage/damage.c
+++ b/miext/damage/damage.c
@@ -1897,8 +1897,13 @@ DamageSetReportAfterOp(DamagePtr pDamage, Bool 
reportAfter)
 DamageScreenFuncsPtr
 DamageGetScreenFuncs(ScreenPtr pScreen)
 {
-damageScrPriv(pScreen);
-return >funcs;
+if (dixPrivateKeyRegistered(damageScrPrivateKey)) {
+damageScrPriv(pScreen);
+
+if (pScrPriv)
+return >funcs;
+}
+return NULL;
 }
 
 void
@@ -1943,3 +1948,13 @@ DamageReportDamage(DamagePtr pDamage, RegionPtr 
pDamageRegion)
 break;
 }
 }
+
+void
+DamageFlushDrawable(DrawablePtr pDrawable)
+{
+ScreenPtr pScreen = pDrawable->pScreen;
+damageScrPriv(pScreen);
+
+if (pScrPriv->funcs.Flush)
+(*pScrPriv->funcs.Flush)(pDrawable);
+}
diff --git a/miext/damage/damage.h b/miext/damage/damage.h
index 525b2db..47a3a00 100644
--- a/miext/damage/damage.h
+++ b/miext/damage/damage.h
@@ -45,12 +45,14 @@ typedef void (*DamageScreenCreateFunc) (DamagePtr);
 typedef void (*DamageScreenRegisterFunc) (DrawablePtr, DamagePtr);
 typedef void (*DamageScreenUnregisterFunc) (DrawablePtr, DamagePtr);
 typedef void (*DamageScreenDestroyFunc) (DamagePtr);
+typedef void (*DamageScreenFlushFunc) (DrawablePtr);
 
 typedef struct _damageScreenFuncs {
 DamageScreenCreateFunc Create;
 DamageScreenRegisterFunc Register;
 DamageScreenUnregisterFunc Unregister;
 DamageScreenDestroyFunc Destroy;
+DamageScreenFlushFunc Flush;
 } DamageScreenFuncsRec, *DamageScreenFuncsPtr;
 
 extern _X_EXPORT void miDamageCreate(DamagePtr);
@@ -112,4 +114,7 @@ extern _X_EXPORT void
 
 extern _X_EXPORT DamageScreenFuncsPtr DamageGetScreenFuncs(ScreenPtr);
 
+extern _X_EXPORT void
+DamageFlushDrawable(DrawablePtr pDrawable);
+
 #endif  /* _DAMAGE_H_ */
-- 
2.9.3

___
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