[Mesa-dev] [PATCH v2] Support unlimited number of display connections
Kyriazis, George
george.kyriazis at intel.com
Thu Mar 3 01:03:21 UTC 2016
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:brianp at vmware.com]
> Sent: Wednesday, March 2, 2016 11:45 AM
> To: Kyriazis, George <george.kyriazis at intel.com>; mesa-
> dev at 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:brianp at vmware.com]
> >> Sent: Tuesday, March 1, 2016 8:45 PM
> >> To: Kyriazis, George <george.kyriazis at intel.com>; mesa-
> >> dev at 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:brianp at vmware.com]
> >>>> Sent: Tuesday, March 1, 2016 6:55 PM
> >>>> To: Kyriazis, George <george.kyriazis at intel.com>; mesa-
> >>>> dev at 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 <brianp at vmware.com>
> >>>
> >
More information about the mesa-dev
mailing list