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

[Brian Paul]

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>
>