[Intel-gfx] [PATCH 2/2] present: Fix use of vsynced pageflips and honor PresentOptionAsync. (v3)

[Mario Kleiner]

On 12/05/2014 12:56 AM, Eric Anholt wrote:
> Mario Kleiner <mario.kleiner.de at gmail.com> writes:
>
>> Pageflips for Pixmap presents were not synchronized to vblank on
>> drivers with support for PresentCapabilityAsync, due to some
>> missing init for vblank->sync_flips. The PresentOptionAsync
>> flag was completely ignored for pageflipped presents.
>>
>> Vsynced flips only worked by accident on the intel-ddx, as that
>> driver doesn't have PresentCapabilityAsync support.
>>
>> On nouveau-ddx, which supports PresentCapabilityAsync, this
>> always caused non-vsynced pageflips with pretty ugly tearing.
>>
>> This patch fixes the problem, as tested on top of XOrg 1.16.2
>> on nouveau and intel.
>>
>> Please also apply to XOrg 1.17 and XOrg 1.16.2 stable.
>>
>> Applying on top of XOrg 1.16.2 may require cherry-picking
>> commit 2051514652481a83bd7cf22e57cb0fcd40333f33
>> which trivially fixes lack of support for protocol option
>> PresentOptionCopy - get two bug fixes for the price of one!
>>
>> Signed-off-by: Mario Kleiner <mario.kleiner.de at gmail.com>
>> ---
>>   present/present.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/present/present.c b/present/present.c
>> index e5d3fd5..be1c9f1 100644
>> --- a/present/present.c
>> +++ b/present/present.c
>> @@ -834,7 +834,7 @@ present_pixmap(WindowPtr window,
>>       vblank->notifies = notifies;
>>       vblank->num_notifies = num_notifies;
>>   
>> -    if (!screen_priv->info || !(screen_priv->info->capabilities & PresentCapabilityAsync))
>> +    if (!(options & PresentOptionAsync))
>>           vblank->sync_flip = TRUE;
> I think I'd like to see a hunk like this in with this patch, so that
> each driver doesn't need to have the cap check:
>
> diff --git a/present/present.c b/present/present.c
> index a9f2214..ed0d734 100644
> --- a/present/present.c
> +++ b/present/present.c
> @@ -838,6 +838,9 @@ present_pixmap(WindowPtr window,
>           vblank->sync_flip = TRUE;
>   
>       if (!(options & PresentOptionCopy) &&
> +        !((options & PresentOptionAsync) &&
> +          (!screen_priv->info ||
> +           !(screen_priv->info->capabilities & PresentCapabilityAsync))) &&
>           pixmap != NULL &&
>           present_check_flip (target_crtc, window, pixmap, vblank->sync_flip, valid, x_off, y_off))
>       {
>
> Seem reasonable?  If you wanted to squash this in, then this is:

I'm not sure if drivers will really avoid the cap check, as i assume the 
definition of the check_flip() function requires them to implement it 
anyway? Does some spec somewhere require them to do it? Do driver 
writers check all server implementations to see if they can get away 
with less?

But then having this hunk in doesn't hurt either, and it would keep the 
current intel-ddx uxa backends working, so i'll integrate it - after 
some urgently needed sleep.

Thanks for the review. These server patches are actually the critical 
ones for me. Without them in XOrg 1.16+, all the mesa fixes would be 
utterly useless for my kind of applications.

-mario

> Reviewed-by: Eric Anholt <eric at anholt.net>
>
> (So's patch 1/2, regardless).