[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, &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.
>
>
>> @@ -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