[Mesa-dev] [PATCH v2] Support unlimited number of display connections

Kyriazis, George george.kyriazis at intel.com
Thu Mar 3 16:23:33 UTC 2016


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-bounces at lists.freedesktop.org] On
> Behalf Of Kyriazis, George
> Sent: Wednesday, March 2, 2016 7:03 PM
> To: Brian Paul <brianp at vmware.com>; mesa-dev at 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: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>
> > >>>
> > >
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


More information about the mesa-dev mailing list