DRI3/Present fixes for XServers 1.16 and 1.17rc - (v2)

DRI3/Present fixes for XServers 1.16 and 1.17rc - (v2)

[Mario Kleiner]

Hi,

an updated set of patches to fix the bugs i found in the
xserver dri3/present implementation and one bug in intel-ddx
uxa/dri3/present implementation. Axel Davys comments made me
rethink my original xserver patch and the new solution is
simple and better and afaics how this was actually intended
to work in the server, the server properly using the present_check_flip
ddx driver function.

Patch 1/2 fixes and slightly improves DebugPresent() macros for
the server to avoid crashes at logout, compositor en/disable or
closing windows while flips are pending when the server is compiled
with debug macros on.

Patch 2/2 fixes the use of PresentOptionAsync for page-flipped present,
and makes Present working on nouveau without horrible tearing.

These patches apply to master, 1.17rc and 1.16.2. They were tested
on top of 1.16.2 with the dri3/present backends of nouveau master
(glamor and exa) and intel master (sna and fixed uxa) on single-display
and dual-display, also ran through my hardware timing test equipment.

Patch uxa/present is a required fix for intel-ddx uxa backend, so
intel_present_check_flip no longer lies to the server about its
capabilities.

Can the x-server patches please also be included into the 1.16 series?

thanks,
-mario

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/2] present: Avoid crashes in DebugPresent(), a bit more info.

[Mario Kleiner]

DebugPresent() crashed the server when a dri3 drawable
was closed while a pageflipped present was still pending,
due to vblank->window-> Null-Ptr deref, so debug builds
caused new problems to debug.

E.g.,

glXSwapBuffers(...);
glXDestroyWindow(...);
-> Pageflip for non-existent window completes -> boom.

Also often happens when switching desktop compositor on/off
due to Present unflips, or when logging out of session.

Also add info if a Present is queued for copyswap or pageflip,
if the present is vsynced, and the serial no of the Present
request, to aid debugging of pageflip and vsync issues. The
serial number is useful as Mesa's dri3/present backend encodes
its sendSBC in the serial number, so one can easily correlate
server debug output with Mesa and with the SBC values returned
to actual OpenGL client applications via OML_sync_control and
INTEL_swap_events extension, makes debugging quite a bit more
easy.

Please also cherry-pick this for a 1.16.x stable update.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 present/present.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/present/present.c b/present/present.c
index ac9047e..e5d3fd5 100644
--- a/present/present.c
+++ b/present/present.c
@@ -440,7 +440,7 @@ present_flip_notify(present_vblank_ptr vblank, uint64_t ust, uint64_t crtc_msc)
     DebugPresent(("\tn %lld %p %8lld: %08lx -> %08lx\n",
                   vblank->event_id, vblank, vblank->target_msc,
                   vblank->pixmap ? vblank->pixmap->drawable.id : 0,
-                  vblank->window->drawable.id));
+                  vblank->window ? vblank->window->drawable.id : 0));
 
     assert (vblank == screen_priv->flip_pending);
 
@@ -859,10 +859,10 @@ present_pixmap(WindowPtr window,
     }
 
     if (pixmap)
-        DebugPresent(("q %lld %p %8lld: %08lx -> %08lx (crtc %p)\n",
+        DebugPresent(("q %lld %p %8lld: %08lx -> %08lx (crtc %p) flip %d vsync %d serial %d\n",
                       vblank->event_id, vblank, target_msc,
                       vblank->pixmap->drawable.id, vblank->window->drawable.id,
-                      target_crtc));
+                      target_crtc, vblank->flip, vblank->sync_flip, vblank->serial));
 
     xorg_list_add(&vblank->event_queue, &present_exec_queue);
     vblank->queued = TRUE;
@@ -955,7 +955,7 @@ present_vblank_destroy(present_vblank_ptr vblank)
     DebugPresent(("\td %lld %p %8lld: %08lx -> %08lx\n",
                   vblank->event_id, vblank, vblank->target_msc,
                   vblank->pixmap ? vblank->pixmap->drawable.id : 0,
-                  vblank->window->drawable.id));
+                  vblank->window ? vblank->window->drawable.id : 0));
 
     /* Drop pixmap reference */
     if (vblank->pixmap)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] uxa/present: Handle sync_flip flag in intel_present_check_flip()

[Mario Kleiner]

Make sure we reject async flips if we don't support
async flips.

Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 src/uxa/intel_present.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/uxa/intel_present.c b/src/uxa/intel_present.c
index d20043f..d2aa9ee 100644
--- a/src/uxa/intel_present.c
+++ b/src/uxa/intel_present.c
@@ -58,6 +58,8 @@ struct intel_present_vblank_event {
 	uint64_t        event_id;
 };
 
+static Bool intel_present_has_async_flip(ScreenPtr screen);
+
 static uint32_t pipe_select(int pipe)
 {
 	if (pipe > 1)
@@ -266,6 +268,9 @@ intel_present_check_flip(RRCrtcPtr              crtc,
         if (!bo)
                 return FALSE;
 
+    if (!sync_flip && !intel_present_has_async_flip(screen))
+        return FALSE;
+
 	return TRUE;
 }
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Mario Kleiner]

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@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;
 
     if (!(options & PresentOptionCopy) &&
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2405 bytes --]

Mario Kleiner <mario.kleiner.de@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@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:

Reviewed-by: Eric Anholt <eric@anholt.net>

(So's patch 1/2, regardless).

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@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@anholt.net>
>
> (So's patch 1/2, regardless).

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx