Re: [PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Alexandre Julliard]

Miklós Máté <mtmkls(a)gmail.com> writes:
> @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
>  /***********************************************************************
>   *              create_gl_drawable
>   */
> -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
>  {
>      gl->drawable = 0;
>
> if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())  /* top-level window */
>      {
> -        struct x11drv_win_data *data = get_win_data( hwnd );
> -
>          if (data)
>          {
>              gl->type = DC_GL_WINDOW;
>              gl->window = create_client_window( data, gl->visual );
>              if (gl->window)
> +            {
>                  gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> -            release_win_data( data );
> +                XSync( gdi_display, False );

Do you really need all the extra XSync calls?  We try to avoid server
round-trips as much as possible.

-- 
Alexandre Julliard
julliard(a)winehq.org

[PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Miklós Máté]

On 04/04/17 12:11, Alexandre Julliard wrote:
> Miklós Máté <mtmkls(a)gmail.com> writes:
> > @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
> >   /***********************************************************************
> >    *              create_gl_drawable
> >    */
> > -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> > +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
> >   {
> >       gl->drawable = 0;
> >
> > if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())  /* top-level window */
> >       {
> > -        struct x11drv_win_data *data = get_win_data( hwnd );
> > -
> >           if (data)
> >           {
> >               gl->type = DC_GL_WINDOW;
> >               gl->window = create_client_window( data, gl->visual );
> >               if (gl->window)
> > +            {
> >                   gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> > -            release_win_data( data );
> > +                XSync( gdi_display, False );
> > Do you really need all the extra XSync calls?  We try to avoid server
> > round-trips as much as possible.

Yes. We must ensure that glXCreateWindow() and glXDestroyWindow() refer 
to a valid X window, so the command queue of data->display and 
gdi_display have to be in sync around those calls.

MM

[PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Alexandre Julliard]

Miklós Máté <mtmkls(a)gmail.com> writes:
> On 04/04/17 12:11, Alexandre Julliard wrote:
> > Miklós Máté <mtmkls(a)gmail.com> writes:
> > > @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
> > >   /***********************************************************************
> > >    *              create_gl_drawable
> > >    */
> > > -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> > > +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
> > >   {
> > >       gl->drawable = 0;
> > >         if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())
> > > /* top-level window */
> > >       {
> > > -        struct x11drv_win_data *data = get_win_data( hwnd );
> > > -
> > >           if (data)
> > >           {
> > >               gl->type = DC_GL_WINDOW;
> > >               gl->window = create_client_window( data, gl->visual );
> > >               if (gl->window)
> > > +            {
> > >                   gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> > > -            release_win_data( data );
> > > +                XSync( gdi_display, False );
> > > Do you really need all the extra XSync calls?  We try to avoid server
> > > round-trips as much as possible.
>
> Yes. We must ensure that glXCreateWindow() and glXDestroyWindow()
> refer to a valid X window, so the command queue of data->display and
> gdi_display have to be in sync around those calls.

If the goal is that the X window is valid, I'd have expected an XSync on
the thread display. Why is the XSync on gdi_display needed here? What is
the scenario that fails?

I'm not questioning that some XSyncs are necessary (and we already have
a few), but I want to make sure we don't add more than needed.

-- 
Alexandre Julliard
julliard(a)winehq.org

[PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Miklós Máté]

On 04/04/17 16:06, Alexandre Julliard wrote:
> Miklós Máté <mtmkls(a)gmail.com> writes:
> > On 04/04/17 12:11, Alexandre Julliard wrote:
> > > Miklós Máté <mtmkls(a)gmail.com> writes:
> > > > @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
> > > >    /***********************************************************************
> > > >     *              create_gl_drawable
> > > >     */
> > > > -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> > > > +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
> > > >    {
> > > >        gl->drawable = 0;
> > > >          if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())
> > > > /* top-level window */
> > > >        {
> > > > -        struct x11drv_win_data *data = get_win_data( hwnd );
> > > > -
> > > >            if (data)
> > > >            {
> > > >                gl->type = DC_GL_WINDOW;
> > > >                gl->window = create_client_window( data, gl->visual );
> > > >                if (gl->window)
> > > > +            {
> > > >                    gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> > > > -            release_win_data( data );
> > > > +                XSync( gdi_display, False );
> > > > Do you really need all the extra XSync calls?  We try to avoid server
> > > > round-trips as much as possible.
> >
> > Yes. We must ensure that glXCreateWindow() and glXDestroyWindow()
> > refer to a valid X window, so the command queue of data->display and
> > gdi_display have to be in sync around those calls.
> > If the goal is that the X window is valid, I'd have expected an XSync on
> > the thread display. Why is the XSync on gdi_display needed here? What is
> > the scenario that fails?
>
> I'm not questioning that some XSyncs are necessary (and we already have
> a few), but I want to make sure we don't add more than needed.

This one prevents crash in the following scenario:

1. create_gl_drawable()

2. X11DRV_ThreadDetach() calls XCloseDisplay( data->display );

3. the Xserver processes the glXCreateWindow in the command queue of 
gdi_display -> X window is not valid

This happens sometimes during the ddraw and d3d9 tests.

MM

[PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Alexandre Julliard]

Miklós Máté <mtmkls(a)gmail.com> writes:
> On 04/04/17 16:06, Alexandre Julliard wrote:
> > Miklós Máté <mtmkls(a)gmail.com> writes:
> > > On 04/04/17 12:11, Alexandre Julliard wrote:
> > > > Miklós Máté <mtmkls(a)gmail.com> writes:
> > > > > @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
> > > > >    /***********************************************************************
> > > > >     *              create_gl_drawable
> > > > >     */
> > > > > -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> > > > > +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
> > > > >    {
> > > > >        gl->drawable = 0;
> > > > >          if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())
> > > > > /* top-level window */
> > > > >        {
> > > > > -        struct x11drv_win_data *data = get_win_data( hwnd );
> > > > > -
> > > > >            if (data)
> > > > >            {
> > > > >                gl->type = DC_GL_WINDOW;
> > > > >                gl->window = create_client_window( data, gl->visual );
> > > > >                if (gl->window)
> > > > > +            {
> > > > >                    gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> > > > > -            release_win_data( data );
> > > > > +                XSync( gdi_display, False );
> > > > > Do you really need all the extra XSync calls?  We try to avoid server
> > > > > round-trips as much as possible.
> > >
> > > Yes. We must ensure that glXCreateWindow() and glXDestroyWindow()
> > > refer to a valid X window, so the command queue of data->display and
> > > gdi_display have to be in sync around those calls.
> > > If the goal is that the X window is valid, I'd have expected an XSync on
> > > the thread display. Why is the XSync on gdi_display needed here? What is
> > > the scenario that fails?
> >
> > I'm not questioning that some XSyncs are necessary (and we already have
> > a few), but I want to make sure we don't add more than needed.
>
> This one prevents crash in the following scenario:
>
> 1. create_gl_drawable()
>
> 2. X11DRV_ThreadDetach() calls XCloseDisplay( data->display );
>
> 3. the Xserver processes the glXCreateWindow in the command queue of
> gdi_display -> X window is not valid
>
> This happens sometimes during the ddraw and d3d9 tests.

Shouldn't this be happening even without your other changes then?

-- 
Alexandre Julliard
julliard(a)winehq.org

[PATCH 2/4] winex11: move XDestroyWindow() after free_gl_drawable() in set_win_format()

[Miklós Máté]

On 05/04/17 19:52, Alexandre Julliard wrote:
> Miklós Máté<mtmkls(a)gmail.com>  writes:
> > On 04/04/17 16:06, Alexandre Julliard wrote:
> > > Miklós Máté<mtmkls(a)gmail.com>  writes:
> > > > On 04/04/17 12:11, Alexandre Julliard wrote:
> > > > > Miklós Máté<mtmkls(a)gmail.com>  writes:
> > > > > > @@ -1334,21 +1334,21 @@ static void free_gl_drawable( struct gl_drawable *gl )
> > > > > >     /***********************************************************************
> > > > > >      *              create_gl_drawable
> > > > > >      */
> > > > > > -static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl )
> > > > > > +static BOOL create_gl_drawable( HWND hwnd, struct gl_drawable *gl, struct x11drv_win_data *data )
> > > > > >     {
> > > > > >         gl->drawable = 0;
> > > > > >           if (GetAncestor( hwnd, GA_PARENT ) == GetDesktopWindow())
> > > > > > /* top-level window */
> > > > > >         {
> > > > > > -        struct x11drv_win_data *data = get_win_data( hwnd );
> > > > > > -
> > > > > >             if (data)
> > > > > >             {
> > > > > >                 gl->type = DC_GL_WINDOW;
> > > > > >                 gl->window = create_client_window( data, gl->visual );
> > > > > >                 if (gl->window)
> > > > > > +            {
> > > > > >                     gl->drawable = pglXCreateWindow( gdi_display, gl->format->fbconfig, gl->window, NULL );
> > > > > > -            release_win_data( data );
> > > > > > +                XSync( gdi_display, False );
> > > > > > Do you really need all the extra XSync calls?  We try to avoid server
> > > > > > round-trips as much as possible.
> > > >
> > > > Yes. We must ensure that glXCreateWindow() and glXDestroyWindow()
> > > > refer to a valid X window, so the command queue of data->display and
> > > > gdi_display have to be in sync around those calls.
> > > > If the goal is that the X window is valid, I'd have expected an XSync on
> > > > the thread display. Why is the XSync on gdi_display needed here? What is
> > > > the scenario that fails?
> > >
> > > I'm not questioning that some XSyncs are necessary (and we already have
> > > a few), but I want to make sure we don't add more than needed.
> >
> > This one prevents crash in the following scenario:
> >
> > 1. create_gl_drawable()
> >
> > 2. X11DRV_ThreadDetach() calls XCloseDisplay( data->display );
> >
> > 3. the Xserver processes the glXCreateWindow in the command queue of
> > gdi_display -> X window is not valid
> >
> > This happens sometimes during the ddraw and d3d9 tests.
> > Shouldn't this be happening even without your other changes then?

Well, according to my notes from half a year ago the ddraw test of 
wine-csmt definitely crashes without this, but now I can't reproduce it. 
I don't know how likely the above scenario is in the real world.

MM