Hi Roman, On Wed, Oct 3, 2018 at 6:57 PM Roman Gilg <subd...@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 <ofour...@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 _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel