[Mesa-dev] [PATCH v2] Support unlimited number of display connections
Kyriazis, George
george.kyriazis at intel.com
Wed Mar 2 17:33:18 UTC 2016
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?
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