One way to solve this is to avoid deleting the screen at the end of xmesa_close_display(). This ends up being not worse than the original code, however we get the benefit of unlimited connections.
Thanks, George > -----Original Message----- > From: mesa-dev [mailto:mesa-dev-boun...@lists.freedesktop.org] On > Behalf Of Kyriazis, George > Sent: Wednesday, March 2, 2016 7:03 PM > To: Brian Paul <bri...@vmware.com>; mesa-dev@lists.freedesktop.org > Subject: Re: [Mesa-dev] [PATCH v2] Support unlimited number of display > connections > > I am re-sending an updated patch that fixes this stomping, however, there is > an additional issue that is exposed in programs that use multiple windows > (for example manywin). The issue is that resources are shared between > contexts, so when we destroy context 2, it tries to free resources that > already > freed by destroying screen 1. > > I think what needs to happen is that these resources should have a refcount. > > Thanks, > > George > > > > -----Original Message----- > > From: Brian Paul [mailto:bri...@vmware.com] > > Sent: Wednesday, March 2, 2016 11:45 AM > > To: Kyriazis, George <george.kyria...@intel.com>; mesa- > > d...@lists.freedesktop.org > > Subject: Re: [Mesa-dev] [PATCH v2] Support unlimited number of display > > connections > > > > On 03/02/2016 10:33 AM, Kyriazis, George wrote: > > > Brian, > > > > > > I looked for a trivial/tri test in the mesa repo and I only found > > > the > > gallium/trivial/tri test. This one I found is loading the device > > pipes directly and is not using glx so it's not affected by my change. > > It shows some valgrind memory leaks upon exit regardless of my change. > > > > > > Or maybe are you talking about a different test? > > > > Yeah, check out the mesa demos tree at > > git.freedesktop.org/git/mesa/demos > > > > > > -Brian > > > > > > > Thank you, > > > > > > George > > > > > >> -----Original Message----- > > >> From: Brian Paul [mailto:bri...@vmware.com] > > >> Sent: Tuesday, March 1, 2016 8:45 PM > > >> To: Kyriazis, George <george.kyria...@intel.com>; mesa- > > >> d...@lists.freedesktop.org > > >> Subject: Re: [Mesa-dev] [PATCH v2] Support unlimited number of > > >> display connections > > >> > > >> Hmm, I applied your patch locally and ran the trivial/tri demo and > > >> saw a few valgrind errors upon exit (invalid memory reads). Can > > >> you look > > into that? > > >> > > >> -Brian > > >> > > >> On 03/01/2016 07:20 PM, Kyriazis, George wrote: > > >>> Thanks Brian, > > >>> > > >>> Since I don't have git write privileges, can somebody check this in for > me? > > >>> > > >>> I am working with Tim Rowley @ Intel for the openswr driver. > > >>> > > >>> George > > >>> > > >>>> -----Original Message----- > > >>>> From: Brian Paul [mailto:bri...@vmware.com] > > >>>> Sent: Tuesday, March 1, 2016 6:55 PM > > >>>> To: Kyriazis, George <george.kyria...@intel.com>; mesa- > > >>>> d...@lists.freedesktop.org > > >>>> Subject: Re: [Mesa-dev] [PATCH v2] Support unlimited number of > > >>>> display connections > > >>>> > > >>>> On 03/01/2016 05:44 PM, George Kyriazis wrote: > > >>>>> There is a limit of 10 display connections, which was a problem > > >>>>> for apps/tests that were continuously opening/closing display > > >>>>> connections. > > >>>>> > > >>>>> This fix uses XAddExtension() and XESetCloseDisplay() to keep > > >>>>> track of the status of the display connections from the X > > >>>>> server, freeing mesa-related data as X displays get destroyed by the > X server. > > >>>>> > > >>>>> Poster child is the VTK "TimingTests" > > >>>>> > > >>>>> v2: Added missing initializer in struct > > >>>>> > > >>>>> --- > > >>>>> src/gallium/state_trackers/glx/xlib/xm_api.c | 122 > > >>>> ++++++++++++++++++++++----- > > >>>>> 1 file changed, 103 insertions(+), 19 deletions(-) > > >>>>> > > >>>>> diff --git a/src/gallium/state_trackers/glx/xlib/xm_api.c > > >>>> b/src/gallium/state_trackers/glx/xlib/xm_api.c > > >>>>> index 2f5e1f5..2f1bfae 100644 > > >>>>> --- a/src/gallium/state_trackers/glx/xlib/xm_api.c > > >>>>> +++ b/src/gallium/state_trackers/glx/xlib/xm_api.c > > >>>>> @@ -110,14 +110,6 @@ void xmesa_set_driver( const struct > > xm_driver > > >>>> *templ ) > > >>>>> } > > >>>>> > > >>>>> > > >>>>> -/* > > >>>>> - * XXX replace this with a linked list, or better yet, try to > > >>>>> attach the > > >>>>> - * gallium/mesa extra bits to the X Display object with > > XAddExtension(). > > >>>>> - */ > > >>>>> -#define MAX_DISPLAYS 10 > > >>>>> -static struct xmesa_display Displays[MAX_DISPLAYS]; -static int > > >>>>> NumDisplays = 0; > > >>>>> - > > >>>>> static int > > >>>>> xmesa_get_param(struct st_manager *smapi, > > >>>>> enum st_manager_param param) @@ -130,34 > > >>>>> +122,125 @@ xmesa_get_param(struct st_manager *smapi, > > >>>>> } > > >>>>> } > > >>>>> > > >>>>> +/* linked list of XMesaDisplay hooks per display */ typedef > > >>>>> +struct _XMesaExtDisplayInfo { > > >>>>> + struct _XMesaExtDisplayInfo *next; > > >>>>> + Display *display; > > >>>>> + XExtCodes *codes; > > >>>>> + struct xmesa_display mesaDisplay; } XMesaExtDisplayInfo; > > >>>>> + > > >>>>> +typedef struct _XMesaExtInfo { > > >>>>> + XMesaExtDisplayInfo *head; > > >>>>> + int ndisplays; > > >>>>> +} XMesaExtInfo; > > >>>>> + > > >>>>> +static XMesaExtInfo MesaExtInfo; > > >>>>> + > > >>>>> +/* hook to delete XMesaDisplay on XDestroyDisplay */ static int > > >>>>> +xmesa_close_display(Display *display, XExtCodes *codes) { > > >>>>> + XMesaExtDisplayInfo *info, *prev; > > >>>>> + > > >>>>> + assert(MesaExtInfo.ndisplays > 0); > > >>>>> + assert(MesaExtInfo.head); > > >>>>> + > > >>>>> + _XLockMutex(_Xglobal_lock); > > >>>>> + /* first find display */ > > >>>>> + prev = NULL; > > >>>>> + for (info = MesaExtInfo.head; info; info = info->next) { > > >>>>> + if (info->display == display) { > > >>>>> + prev = info; > > >>>>> + break; > > >>>>> + } > > >>>>> + } > > >>>>> + > > >>>>> + if (info == NULL) { > > >>>>> + /* no display found */ > > >>>>> + _XUnlockMutex(_Xglobal_lock); > > >>>>> + return 0; > > >>>>> + } > > >>>>> + > > >>>>> + /* remove display entry from list */ > > >>>>> + if (prev != MesaExtInfo.head) { > > >>>>> + prev->next = info->next; > > >>>>> + } else { > > >>>>> + MesaExtInfo.head = info->next; > > >>>>> + } > > >>>>> + MesaExtInfo.ndisplays--; > > >>>>> + > > >>>>> + _XUnlockMutex(_Xglobal_lock); > > >>>>> + > > >>>>> + /* don't forget to clean up mesaDisplay */ > > >>>>> + XMesaDisplay xmdpy = &info->mesaDisplay; > > >>>>> + > > >>>>> + if (xmdpy->screen) { > > >>>>> + xmdpy->screen->destroy(xmdpy->screen); > > >>>>> + } > > >>>>> + free(xmdpy->smapi); > > >>>>> + > > >>>>> + XFree((char *) info); > > >>>>> + return 1; > > >>>>> +} > > >>>>> + > > >>>>> static XMesaDisplay > > >>>>> xmesa_init_display( Display *display ) > > >>>>> { > > >>>>> pipe_static_mutex(init_mutex); > > >>>>> XMesaDisplay xmdpy; > > >>>>> - int i; > > >>>>> + XMesaExtDisplayInfo *info; > > >>>>> + > > >>>>> + if (display == NULL) { > > >>>>> + return NULL; > > >>>>> + } > > >>>>> > > >>>>> pipe_mutex_lock(init_mutex); > > >>>>> > > >>>>> - /* Look for XMesaDisplay which corresponds to 'display' */ > > >>>>> - for (i = 0; i < NumDisplays; i++) { > > >>>>> - if (Displays[i].display == display) { > > >>>>> + /* Look for XMesaDisplay which corresponds to this display */ > > >>>>> + info = MesaExtInfo.head; > > >>>>> + while(info) { > > >>>>> + if (info->display == display) { > > >>>>> /* Found it */ > > >>>>> pipe_mutex_unlock(init_mutex); > > >>>>> - return &Displays[i]; > > >>>>> + return &info->mesaDisplay; > > >>>>> } > > >>>>> + info = info->next; > > >>>>> } > > >>>>> > > >>>>> - /* Create new XMesaDisplay */ > > >>>>> + /* Not found. Create new XMesaDisplay */ > > >>>>> + /* first allocate X-related resources and hook destroy > > >>>>> + callback */ > > >>>>> > > >>>>> - assert(NumDisplays < MAX_DISPLAYS); > > >>>>> - xmdpy = &Displays[NumDisplays]; > > >>>>> - NumDisplays++; > > >>>>> - > > >>>>> - if (!xmdpy->display && display) { > > >>>>> + /* allocate mesa display info */ > > >>>>> + info = (XMesaExtDisplayInfo *) > > Xmalloc(sizeof(XMesaExtDisplayInfo)); > > >>>>> + if (info == NULL) { > > >>>>> + pipe_mutex_unlock(init_mutex); > > >>>>> + return NULL; > > >>>>> + } > > >>>>> + info->display = display; > > >>>>> + info->codes = XAddExtension(display); > > >>>>> + if (info->codes == NULL) { > > >>>>> + /* could not allocate extension. Fail */ > > >>>>> + Xfree(info); > > >>>>> + pipe_mutex_unlock(init_mutex); > > >>>>> + return NULL; > > >>>>> + } > > >>>>> + XESetCloseDisplay(display, info->codes->extension, > > >>>> xmesa_close_display); > > >>>>> + xmdpy = &info->mesaDisplay; /* to be filled out below */ > > >>>>> + > > >>>>> + /* chain to the list of displays */ > > >>>>> + _XLockMutex(_Xglobal_lock); > > >>>>> + info->next = MesaExtInfo.head; > > >>>>> + MesaExtInfo.head = info; > > >>>>> + MesaExtInfo.ndisplays++; > > >>>>> + _XUnlockMutex(_Xglobal_lock); > > >>>>> + > > >>>>> + /* now create the new XMesaDisplay info */ > > >>>>> + if (display) { > > >>>>> xmdpy->display = display; > > >>>>> xmdpy->screen = driver.create_pipe_screen(display); > > >>>>> xmdpy->smapi = CALLOC_STRUCT(st_manager); > > >>>>> + xmdpy->pipe = NULL; > > >>>>> if (xmdpy->smapi) { > > >>>>> xmdpy->smapi->screen = xmdpy->screen; > > >>>>> xmdpy->smapi->get_param = xmesa_get_param; @@ > > >>>>> -185,6 > > >>>>> +268,7 @@ xmesa_init_display( Display *display ) > > >>>>> return xmdpy; > > >>>>> } > > >>>>> > > >>>>> + > > >>>>> > > >>>> > > >> > > > /*************************************************************** > > >>>> *******/ > > >>>>> /***** X Utility Functions > > >>>>> *****/ > > >>>>> > > >>>> > > >> > > > /*************************************************************** > > >>>> *******/ > > >>>>> > > >>>> > > >>>> LGTM. Reviewed-by: Brian Paul <bri...@vmware.com> > > >>> > > > > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
