[xserver,1/2] present: cancel flip on reparenting

Submitted by Olivier Fourdan on Sept. 27, 2018, 3:31 p.m.

Details

Message ID 20180927153106.24287-2-ofourdan@redhat.com
State New
Headers show
Series "xwayland: Avoid assert failure in flips_stop()" ( rev: 2 ) in X.org

Not browsing as part of any series.

Commit Message

Olivier Fourdan Sept. 27, 2018, 3:31 p.m.
If a window is reparented and the new parent already has a child
flipping, we need to cancel the flipping on the reparented window.

Install a new ReparentWindow handler in present screen with hooks in
wnmd implementation to check if the new parent has flip pending or
active in which case we cancel flip on the reparented window.

Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
---
 present/present_priv.h   |  3 +++
 present/present_scmd.c   |  1 +
 present/present_screen.c | 16 ++++++++++++++++
 present/present_wnmd.c   | 17 +++++++++++++++++
 4 files changed, 37 insertions(+)

Patch hide | download patch | download mbox

diff --git a/present/present_priv.h b/present/present_priv.h
index f62456755..668322416 100644
--- a/present/present_priv.h
+++ b/present/present_priv.h
@@ -106,6 +106,7 @@  typedef Bool (*present_priv_check_flip_ptr)(RRCrtcPtr crtc,
                                             PresentFlipReason *reason);
 typedef void (*present_priv_check_flip_window_ptr)(WindowPtr window);
 typedef Bool (*present_priv_can_window_flip_ptr)(WindowPtr window);
+typedef void (*present_priv_reparent_window_ptr)(WindowPtr pWin);
 
 typedef int (*present_priv_pixmap_ptr)(WindowPtr window,
                                        PixmapPtr pixmap,
@@ -147,6 +148,7 @@  struct present_screen_priv {
     ConfigNotifyProcPtr         ConfigNotify;
     DestroyWindowProcPtr        DestroyWindow;
     ClipNotifyProcPtr           ClipNotify;
+    ReparentWindowProcPtr       ReparentWindow;
 
     present_vblank_ptr          flip_pending;
     uint64_t                    unflip_event_id;
@@ -171,6 +173,7 @@  struct present_screen_priv {
     present_priv_check_flip_ptr         check_flip;
     present_priv_check_flip_window_ptr  check_flip_window;
     present_priv_can_window_flip_ptr    can_window_flip;
+    present_priv_reparent_window_ptr    reparent_window;
 
     present_priv_pixmap_ptr             present_pixmap;
     present_priv_create_event_id_ptr    create_event_id;
diff --git a/present/present_scmd.c b/present/present_scmd.c
index 0803a0c0b..a3ca16333 100644
--- a/present/present_scmd.c
+++ b/present/present_scmd.c
@@ -828,6 +828,7 @@  present_scmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
 
     screen_priv->abort_vblank       =   &present_scmd_abort_vblank;
     screen_priv->flip_destroy       =   &present_scmd_flip_destroy;
+    screen_priv->reparent_window    =   NULL;
 }
 
 Bool
diff --git a/present/present_screen.c b/present/present_screen.c
index c7e37c5fd..f3f2ddef9 100644
--- a/present/present_screen.c
+++ b/present/present_screen.c
@@ -207,6 +207,21 @@  present_clip_notify(WindowPtr window, int dx, int dy)
     wrap(screen_priv, screen, ClipNotify, present_clip_notify);
 }
 
+static void
+present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
+{
+    ScreenPtr screen = pWin->drawable.pScreen;
+    present_screen_priv_ptr screen_priv = present_screen_priv(screen);
+
+    if (screen_priv->reparent_window)
+        screen_priv->reparent_window(pWin);
+
+    unwrap(screen_priv, screen, ReparentWindow)
+    if (screen->ReparentWindow)
+        screen->ReparentWindow(pWin, pPriorParent);
+    wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
+}
+
 static Bool
 present_screen_register_priv_keys(void)
 {
@@ -232,6 +247,7 @@  present_screen_priv_init(ScreenPtr screen)
     wrap(screen_priv, screen, DestroyWindow, present_destroy_window);
     wrap(screen_priv, screen, ConfigNotify, present_config_notify);
     wrap(screen_priv, screen, ClipNotify, present_clip_notify);
+    wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
 
     dixSetPrivate(&screen->devPrivates, &present_screen_private_key, screen_priv);
 
diff --git a/present/present_wnmd.c b/present/present_wnmd.c
index 8f3836440..0c49a3507 100644
--- a/present/present_wnmd.c
+++ b/present/present_wnmd.c
@@ -682,6 +682,22 @@  present_wnmd_flush(WindowPtr window)
     (*screen_priv->wnmd_info->flush) (window);
 }
 
+static void
+present_wnmd_reparent_window(WindowPtr pWin)
+{
+    present_window_priv_ptr parent_window_priv;
+
+    parent_window_priv = present_window_priv(pWin->parent);
+    if (!parent_window_priv)
+        return;
+
+    /* If the new parent window already has a child flipping, cancel the
+     * flip on the reparented window
+     */
+    if (parent_window_priv->flip_pending || parent_window_priv->flip_active)
+        present_wnmd_cancel_flip(pWin);
+}
+
 void
 present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
 {
@@ -700,4 +716,5 @@  present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
 
     screen_priv->abort_vblank       =   &present_wnmd_abort_vblank;
     screen_priv->flip_destroy       =   &present_wnmd_flip_destroy;
+    screen_priv->reparent_window    =   &present_wnmd_reparent_window;
 }

Comments

I'm a bit unsure on that one. I thought there is no cleanup code
necessary in Present on a reparent because in theory the current
Present code alone allows clients to flip arbitrary many child windows
to a certain parent window as long as they have the same dimension as
the parent. Of course a client trying to do flips on multiple child
windows at the same time all with the same dimension as the parent can
be considered somewhat weird and should not exist in practice. So if
there is a "flipping" window being reparented to one with a child
window doing flips it could be done from Present's side without any
cleanup.

But in the Xwayland case the driver should disallow the reparented
child window doing flips, which then does a cleanup on the Present
side for this particular child window.

Some notes below:

On Thu, Sep 27, 2018 at 5:31 PM Olivier Fourdan <ofourdan@redhat.com> wrote:
> diff --git a/present/present_screen.c b/present/present_screen.c
> index c7e37c5fd..f3f2ddef9 100644
> --- a/present/present_screen.c
> +++ b/present/present_screen.c
> @@ -207,6 +207,21 @@ present_clip_notify(WindowPtr window, int dx, int dy)
>      wrap(screen_priv, screen, ClipNotify, present_clip_notify);
>  }
>
> +static void
> +present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
> +{
> +    ScreenPtr screen = pWin->drawable.pScreen;
> +    present_screen_priv_ptr screen_priv = present_screen_priv(screen);
> +
> +    if (screen_priv->reparent_window)
> +        screen_priv->reparent_window(pWin);
> +
> +    unwrap(screen_priv, screen, ReparentWindow)
> +    if (screen->ReparentWindow)
> +        screen->ReparentWindow(pWin, pPriorParent);
> +    wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
> +}
> +
>  static Bool
>  present_screen_register_priv_keys(void)
>  {
> @@ -232,6 +247,7 @@ present_screen_priv_init(ScreenPtr screen)
>      wrap(screen_priv, screen, DestroyWindow, present_destroy_window);
>      wrap(screen_priv, screen, ConfigNotify, present_config_notify);
>      wrap(screen_priv, screen, ClipNotify, present_clip_notify);
> +    wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
>
>      dixSetPrivate(&screen->devPrivates, &present_screen_private_key, screen_priv);
>
> diff --git a/present/present_wnmd.c b/present/present_wnmd.c
> index 8f3836440..0c49a3507 100644
> --- a/present/present_wnmd.c
> +++ b/present/present_wnmd.c
> @@ -682,6 +682,22 @@ present_wnmd_flush(WindowPtr window)
>      (*screen_priv->wnmd_info->flush) (window);
>  }
>
> +static void
> +present_wnmd_reparent_window(WindowPtr pWin)
> +{
> +    present_window_priv_ptr parent_window_priv;
> +
> +    parent_window_priv = present_window_priv(pWin->parent);

The present priv struct of a parent does not carry the information of
a potentially presenting child. These are saved to the child's priv
struct directly. So currently one would need to iterate all children
and check if one of them is presenting.

> +    if (!parent_window_priv)
> +        return;
> +
> +    /* If the new parent window already has a child flipping, cancel the
> +     * flip on the reparented window
> +     */
> +    if (parent_window_priv->flip_pending || parent_window_priv->flip_active)

present_wnmd_cancel_flip checks these conditions already, so we do not
need to do this here as well.

> +        present_wnmd_cancel_flip(pWin);
> +}
> +
>  void
>  present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
>  {
> @@ -700,4 +716,5 @@ present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
>
>      screen_priv->abort_vblank       =   &present_wnmd_abort_vblank;
>      screen_priv->flip_destroy       =   &present_wnmd_flip_destroy;
> +    screen_priv->reparent_window    =   &present_wnmd_reparent_window;
>  }
> --
> 2.19.0
>
Hi Roman,

On Wed, Oct 3, 2018 at 6:57 PM Roman Gilg <subdiff@gmail.com> wrote:
> I'm a bit unsure on that one. I thought there is no cleanup code
> necessary in Present on a reparent because in theory the current
> Present code alone allows clients to flip arbitrary many child windows
> to a certain parent window as long as they have the same dimension as
> the parent. Of course a client trying to do flips on multiple child
> windows at the same time all with the same dimension as the parent can
> be considered somewhat weird and should not exist in practice. So if
> there is a "flipping" window being reparented to one with a child
> window doing flips it could be done from Present's side without any
> cleanup.
>
> But in the Xwayland case the driver should disallow the reparented
> child window doing flips, which then does a cleanup on the Present
> side for this particular child window.

I'm a bit lost, those two patches were my attempt to translate into
code what you wrote im your reply previously:

https://lists.x.org/archives/xorg-devel/2018-September/057581.html

Quoted below:

> I believe now the root problem is that the window did in fact some flips in the past, but on the reparent operation a new parent
> window with a new xwl_window struct is set, which then has a different present_window value. That means when reparenting
> of a child window with flips the xwl_window->present_window values must be updated on the old parent (to NULL) and on the
> new parent (to the new child window). Or more generic this must be done on any unmap/map operation.

That would have been patch 2/2 "xwayland: update Xwayland present
window on reparent"

> One extra case must be considered: if there is already a different child window doing flips on the new parent window the
> reparented child window must be instructed to stop its flips.

Whereas this would have been this patch 1/2 "present: cancel flip on
reparenting"

So do you mean we don't need this part and should drop this patch instead?

> Some notes below:
>
> On Thu, Sep 27, 2018 at 5:31 PM Olivier Fourdan <ofourdan@redhat.com> wrote:
> > diff --git a/present/present_screen.c b/present/present_screen.c
> > index c7e37c5fd..f3f2ddef9 100644
> > --- a/present/present_screen.c
> > +++ b/present/present_screen.c
> > @@ -207,6 +207,21 @@ present_clip_notify(WindowPtr window, int dx, int dy)
> >      wrap(screen_priv, screen, ClipNotify, present_clip_notify);
> >  }
> >
> > +static void
> > +present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
> > +{
> > +    ScreenPtr screen = pWin->drawable.pScreen;
> > +    present_screen_priv_ptr screen_priv = present_screen_priv(screen);
> > +
> > +    if (screen_priv->reparent_window)
> > +        screen_priv->reparent_window(pWin);
> > +
> > +    unwrap(screen_priv, screen, ReparentWindow)
> > +    if (screen->ReparentWindow)
> > +        screen->ReparentWindow(pWin, pPriorParent);
> > +    wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
> > +}
> > +
> >  static Bool
> >  present_screen_register_priv_keys(void)
> >  {
> > @@ -232,6 +247,7 @@ present_screen_priv_init(ScreenPtr screen)
> >      wrap(screen_priv, screen, DestroyWindow, present_destroy_window);
> >      wrap(screen_priv, screen, ConfigNotify, present_config_notify);
> >      wrap(screen_priv, screen, ClipNotify, present_clip_notify);
> > +    wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
> >
> >      dixSetPrivate(&screen->devPrivates, &present_screen_private_key, screen_priv);
> >
> > diff --git a/present/present_wnmd.c b/present/present_wnmd.c
> > index 8f3836440..0c49a3507 100644
> > --- a/present/present_wnmd.c
> > +++ b/present/present_wnmd.c
> > @@ -682,6 +682,22 @@ present_wnmd_flush(WindowPtr window)
> >      (*screen_priv->wnmd_info->flush) (window);
> >  }
> >
> > +static void
> > +present_wnmd_reparent_window(WindowPtr pWin)
> > +{
> > +    present_window_priv_ptr parent_window_priv;
> > +
> > +    parent_window_priv = present_window_priv(pWin->parent);
>
> The present priv struct of a parent does not carry the information of
> a potentially presenting child. These are saved to the child's priv
> struct directly. So currently one would need to iterate all children
> and check if one of them is presenting.
>
> > +    if (!parent_window_priv)
> > +        return;
> > +
> > +    /* If the new parent window already has a child flipping, cancel the
> > +     * flip on the reparented window
> > +     */
> > +    if (parent_window_priv->flip_pending || parent_window_priv->flip_active)
>
> present_wnmd_cancel_flip checks these conditions already, so we do not
> need to do this here as well.
>
> > +        present_wnmd_cancel_flip(pWin);
> > +}
> > +
> >  void
> >  present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
> >  {
> > @@ -700,4 +716,5 @@ present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
> >
> >      screen_priv->abort_vblank       =   &present_wnmd_abort_vblank;
> >      screen_priv->flip_destroy       =   &present_wnmd_flip_destroy;
> > +    screen_priv->reparent_window    =   &present_wnmd_reparent_window;
> >  }
> > --
> > 2.19.0
> >

Cheers,
Olivier