[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, &x, &y, &w, &h);
>>>>
>>>>      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