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

[Olivier Fourdan]

Hi Roman,

On Wed, Oct 3, 2018 at 6:57 PM Roman Gilg <subdiff at 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 at 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