[Mesa-dev] [PATCH 2/3] egl/x11: store xcb_screen_t *screen instead of int screen

[Emil Velikov]

On 17 November 2016 at 13:59, Eric Engestrom <eric.engestrom at imgtec.com> wrote:
> On Friday, 2016-11-11 16:31:16 +0000, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov at collabora.com>
>>
>> Just fetch and store it once, rather than doing the
>> xcb_setup_roots_iterator + get_xcb_screen dance five times.
>>
>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>> ---
>> These two patches are an example of the duplication we have within each
>> loader (be that egl, gbm or glx)_and how we can fold it up :-)
>>
>>  src/egl/drivers/dri2/egl_dri2.h          |  2 +-
>>  src/egl/drivers/dri2/platform_x11.c      | 51 ++++++++++----------------------
>>  src/egl/drivers/dri2/platform_x11_dri3.c | 32 ++------------------
>>  3 files changed, 19 insertions(+), 66 deletions(-)
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
>> index 0020a5b..fa94cbe 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.h
>> +++ b/src/egl/drivers/dri2/egl_dri2.h
>> @@ -199,7 +199,7 @@ struct dri2_egl_display
>>
>>  #ifdef HAVE_X11_PLATFORM
>>     xcb_connection_t         *conn;
>> -   int                      screen;
>> +   xcb_screen_t             *screen;
>>     int                      swap_available;
>>  #ifdef HAVE_DRI3
>>     struct loader_dri3_extensions loader_dri3_ext;
>> diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c
>> index e152868..f4bcf4a 100644
>> --- a/src/egl/drivers/dri2/platform_x11.c
>> +++ b/src/egl/drivers/dri2/platform_x11.c
>> @@ -206,10 +206,8 @@ dri2_x11_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
>>     struct dri2_egl_surface *dri2_surf;
>>     xcb_get_geometry_cookie_t cookie;
>>     xcb_get_geometry_reply_t *reply;
>> -   xcb_screen_iterator_t s;
>>     xcb_generic_error_t *error;
>>     xcb_drawable_t drawable;
>> -   xcb_screen_t *screen;
>>     const __DRIconfig *config;
>>
>>     STATIC_ASSERT(sizeof(uintptr_t) == sizeof(native_surface));
>> @@ -228,16 +226,9 @@ dri2_x11_create_surface(_EGLDriver *drv, _EGLDisplay *disp, EGLint type,
>>
>>     dri2_surf->region = XCB_NONE;
>>     if (type == EGL_PBUFFER_BIT) {
>> -      s = xcb_setup_roots_iterator(xcb_get_setup(dri2_dpy->conn));
>> -      screen = get_xcb_screen(s, dri2_dpy->screen);
>> -      if (!screen) {
>> -         _eglError(EGL_BAD_ALLOC, "failed to get xcb screen");
>> -         goto cleanup_surf;
>> -      }
>> -
>>        dri2_surf->drawable = xcb_generate_id(dri2_dpy->conn);
>>        xcb_create_pixmap(dri2_dpy->conn, conf->BufferSize,
>> -                       dri2_surf->drawable, screen->root,
>> +                       dri2_surf->drawable, dri2_dpy->screen->root,
>>                       dri2_surf->base.Width, dri2_surf->base.Height);
>>     } else {
>>        if (!drawable) {
>> @@ -544,20 +535,10 @@ dri2_x11_do_authenticate(struct dri2_egl_display *dri2_dpy, uint32_t id)
>>  {
>>     xcb_dri2_authenticate_reply_t *authenticate;
>>     xcb_dri2_authenticate_cookie_t authenticate_cookie;
>> -   xcb_screen_iterator_t s;
>> -   xcb_screen_t *screen;
>>     int ret = 0;
>>
>> -   s = xcb_setup_roots_iterator(xcb_get_setup(dri2_dpy->conn));
>> -
>> -   screen = get_xcb_screen(s, dri2_dpy->screen);
>> -   if (!screen) {
>> -      _eglLog(_EGL_WARNING, "DRI2: failed to get xcb screen");
>> -      return -1;
>> -   }
>> -
>>     authenticate_cookie =
>> -      xcb_dri2_authenticate_unchecked(dri2_dpy->conn, screen->root, id);
>> +      xcb_dri2_authenticate_unchecked(dri2_dpy->conn, dri2_dpy->screen->root, id);
>>     authenticate =
>>        xcb_dri2_authenticate_reply(dri2_dpy->conn, authenticate_cookie, NULL);
>>
>> @@ -598,8 +579,6 @@ dri2_x11_connect(struct dri2_egl_display *dri2_dpy)
>>     xcb_dri2_connect_reply_t *connect;
>>     xcb_dri2_connect_cookie_t connect_cookie;
>>     xcb_generic_error_t *error;
>> -   xcb_screen_iterator_t s;
>> -   xcb_screen_t *screen;
>>     char *driver_name, *loader_driver_name, *device_name;
>>     const xcb_query_extension_reply_t *extension;
>>
>> @@ -622,13 +601,7 @@ dri2_x11_connect(struct dri2_egl_display *dri2_dpy)
>>                                              XCB_DRI2_MAJOR_VERSION,
>>                                              XCB_DRI2_MINOR_VERSION);
>>
>> -   s = xcb_setup_roots_iterator(xcb_get_setup(dri2_dpy->conn));
>> -   screen = get_xcb_screen(s, dri2_dpy->screen);
>> -   if (!screen) {
>> -      _eglLog(_EGL_WARNING, "DRI2: failed to get xcb screen");
>> -      return EGL_FALSE;
>> -   }
>> -   connect_cookie = xcb_dri2_connect_unchecked(dri2_dpy->conn, screen->root,
>> +   connect_cookie = xcb_dri2_connect_unchecked(dri2_dpy->conn, dri2_dpy->screen->root,
>>                                     XCB_DRI2_DRIVER_TYPE_DRI);
>>
>>     xfixes_query =
>> @@ -720,7 +693,6 @@ static EGLBoolean
>>  dri2_x11_add_configs_for_visuals(struct dri2_egl_display *dri2_dpy,
>>                                   _EGLDisplay *disp, bool supports_preserved)
>>  {
>> -   xcb_screen_iterator_t s;
>>     xcb_depth_iterator_t d;
>>     xcb_visualtype_t *visuals;
>>     int i, j, count;
>> @@ -732,8 +704,7 @@ dri2_x11_add_configs_for_visuals(struct dri2_egl_display *dri2_dpy,
>>          EGL_NONE
>>     };
>>
>> -   s = xcb_setup_roots_iterator(xcb_get_setup(dri2_dpy->conn));
>> -   d = xcb_screen_allowed_depths_iterator(get_xcb_screen(s, dri2_dpy->screen));
>> +   d = xcb_screen_allowed_depths_iterator(dri2_dpy->screen);
>>     count = 0;
>>
>>     surface_type =
>> @@ -1183,20 +1154,30 @@ static EGLBoolean
>>  dri2_get_xcb_connection(_EGLDriver *drv, _EGLDisplay *disp,
>>                          struct dri2_egl_display *dri2_dpy)
>>  {
>> +   xcb_screen_iterator_t s;
>> +   int screen = 0;
>> +
>>     disp->DriverData = (void *) dri2_dpy;
>>     if (disp->PlatformDisplay == NULL) {
>> -      dri2_dpy->conn = xcb_connect(0, &dri2_dpy->screen);
>> +      dri2_dpy->conn = xcb_connect(NULL, &screen);
>>        dri2_dpy->own_device = true;
>>     } else {
>>        Display *dpy = disp->PlatformDisplay;
>>
>>        dri2_dpy->conn = XGetXCBConnection(dpy);
>> -      dri2_dpy->screen = DefaultScreen(dpy);
>> +      screen = DefaultScreen(dpy);
>>     }
>>
>>     if (!dri2_dpy->conn || xcb_connection_has_error(dri2_dpy->conn))
>>        return _eglError(EGL_BAD_ALLOC, "xcb_connect failed");
>>
>> +   s = xcb_setup_roots_iterator(xcb_get_setup(dri2_dpy->conn));
>> +   dri2_dpy->screen = get_xcb_screen(s, screen);
>> +   if (!dri2_dpy->screen) {
>> +      _eglError(EGL_BAD_ALLOC, "failed to get xcb screen");
>> +      return EGL_FALSE;
>
> Shouldn't we disconnect xcb here? We're past the point handling its
> failure, so it should be connected now.
>
Nicely spotted.
I've added xcb_disconnect() even in the xcb_connection_has_error()
case (to plug a mem leak as per the manual) with v2 and sent it out
for posterity.

-Emil