egl: Fix EGL_PLATFORM_X11_SCREEN_KHR handling

Submitted by Adam Jackson on April 17, 2019, 6:57 p.m.

Details

Message ID 20190417185708.27858-1-ajax@redhat.com
State New
Headers show
Series "Series without cover letter" ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Adam Jackson April 17, 2019, 6:57 p.m.
If this was specified and a non-NULL display was passed to
eglGetPlatformDisplay, we would ignore the attribute and instead use
whatever xcb thought the default screen would be.

To fix this, store a copy of the attribute list in the _EGLDisplay, and
use that to look up any non-default screen number if we need it.
---
 src/egl/drivers/dri2/platform_x11.c | 28 ++++++++--
 src/egl/main/eglapi.c               |  2 +-
 src/egl/main/egldisplay.c           | 85 +++++++++++++++++++++++------
 src/egl/main/egldisplay.h           |  4 +-
 4 files changed, 96 insertions(+), 23 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/egl/drivers/dri2/platform_x11.c b/src/egl/drivers/dri2/platform_x11.c
index c8c676d2f00..7b95634e77c 100644
--- a/src/egl/drivers/dri2/platform_x11.c
+++ b/src/egl/drivers/dri2/platform_x11.c
@@ -1276,24 +1276,44 @@  static const __DRIextension *swrast_loader_extensions[] = {
    NULL,
 };
 
+static int
+dri2_find_screen_for_display(Display *dpy, _EGLDisplay *disp)
+{
+   int screen = -1;
+   const EGLAttrib *attr;
+
+   if (dpy)
+      screen = DefaultScreen(dpy);
+
+   for (attr = disp->Options.Attribs; attr; attr += 2) {
+      if (attr[0] == EGL_PLATFORM_X11_SCREEN_EXT) {
+         screen = attr[1];
+         break;
+      }
+      if (attr[0] == EGL_NONE)
+         break;
+   }
+
+   return screen;
+}
+
 static EGLBoolean
 dri2_get_xcb_connection(_EGLDriver *drv, _EGLDisplay *disp,
                         struct dri2_egl_display *dri2_dpy)
 {
    xcb_screen_iterator_t s;
-   int screen = (uintptr_t)disp->Options.Platform;
+   int screen;
    const char *msg;
 
    disp->DriverData = (void *) dri2_dpy;
    if (disp->PlatformDisplay == NULL) {
-      dri2_dpy->conn = xcb_connect(NULL, &screen);
+      dri2_dpy->conn = xcb_connect(NULL, NULL);
       dri2_dpy->own_device = true;
    } else {
       Display *dpy = disp->PlatformDisplay;
-
       dri2_dpy->conn = XGetXCBConnection(dpy);
-      screen = DefaultScreen(dpy);
    }
+   screen = dri2_find_screen_for_display(disp->PlatformDisplay, disp);
 
    if (!dri2_dpy->conn || xcb_connection_has_error(dri2_dpy->conn)) {
       msg = "xcb_connect failed";
diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index 588c6a5f1eb..4696eca8f82 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -379,7 +379,7 @@  eglGetDisplay(EGLNativeDisplayType nativeDisplay)
    native_display_ptr = (void*) nativeDisplay;
 
    plat = _eglGetNativePlatform(native_display_ptr);
-   disp = _eglFindDisplay(plat, native_display_ptr);
+   disp = _eglFindDisplay(plat, native_display_ptr, NULL);
    return _eglGetDisplayHandle(disp);
 }
 
diff --git a/src/egl/main/egldisplay.c b/src/egl/main/egldisplay.c
index b26a9575087..8aae6900f09 100644
--- a/src/egl/main/egldisplay.c
+++ b/src/egl/main/egldisplay.c
@@ -202,20 +202,64 @@  _eglFiniDisplay(void)
          }
       }
 
+      free(disp->Options.Attribs);
       free(disp);
    }
    _eglGlobal.DisplayList = NULL;
 }
 
+static int
+_eglNumAttribs(const EGLAttrib *attrib_list)
+{
+   int ret = 0;
+   const EGLAttrib *attr;
+
+   for (attr = attrib_list; attr; attr++) {
+      ret++;
+      if ((ret % 2 == 1) && (*attr == EGL_NONE))
+         break;
+   }
+
+   return ret;
+}
+
+static int
+_eglSameAttribs(const EGLAttrib *a, const EGLAttrib *b)
+{
+   int na = _eglNumAttribs(a);
+   int nb = _eglNumAttribs(b);
+
+   /* different numbers of attributes must be different */
+   if (na != nb)
+      return 0;
+
+   /* both lists NULL are the same */
+   if (!a && !b)
+      return 1;
+
+   /* otherwise, compare the lists */
+   return memcmp(a, b, na) == 0;
+}
 
 /**
  * Find the display corresponding to the specified native display, or create a
- * new one.
+ * new one. EGL 1.5 says:
+ *
+ *     Multiple calls made to eglGetPlatformDisplay with the same parameters
+ *     will return the same EGLDisplay handle.
+ *
+ * We read this extremely strictly, and treat a call with NULL attribs as
+ * different from a call with attribs only equal to { EGL_NONE }. Similarly
+ * we do not sort the attribute list, so even if all attribute _values_ are
+ * identical, different attribute orders will be considered different
+ * parameters.
  */
 _EGLDisplay *
-_eglFindDisplay(_EGLPlatformType plat, void *plat_dpy)
+_eglFindDisplay(_EGLPlatformType plat, void *plat_dpy,
+                const EGLAttrib *attrib_list)
 {
    _EGLDisplay *disp;
+   int num_attribs = _eglNumAttribs(attrib_list);
 
    if (plat == _EGL_INVALID_PLATFORM)
       return NULL;
@@ -225,7 +269,8 @@  _eglFindDisplay(_EGLPlatformType plat, void *plat_dpy)
    /* search the display list first */
    disp = _eglGlobal.DisplayList;
    while (disp) {
-      if (disp->Platform == plat && disp->PlatformDisplay == plat_dpy)
+      if (disp->Platform == plat && disp->PlatformDisplay == plat_dpy &&
+          _eglSameAttribs(disp->Options.Attribs, attrib_list))
          break;
       disp = disp->Next;
    }
@@ -237,6 +282,16 @@  _eglFindDisplay(_EGLPlatformType plat, void *plat_dpy)
          mtx_init(&disp->Mutex, mtx_plain);
          disp->Platform = plat;
          disp->PlatformDisplay = plat_dpy;
+         if (num_attribs) {
+            disp->Options.Attribs = calloc(num_attribs, sizeof(EGLAttrib));
+            if (disp->Options.Attribs) {
+               memcpy(disp->Options.Attribs, attrib_list,
+                      num_attribs * sizeof(EGLAttrib));
+            } else {
+               free(disp);
+               goto out;
+            }
+         }
 
          /* add to the display list */ 
          disp->Next = _eglGlobal.DisplayList;
@@ -244,6 +299,7 @@  _eglFindDisplay(_EGLPlatformType plat, void *plat_dpy)
       }
    }
 
+out:
    mtx_unlock(_eglGlobal.Mutex);
 
    return disp;
@@ -456,17 +512,12 @@  _eglParseX11DisplayAttribList(_EGLDisplay *display,
       return EGL_TRUE;
    }
 
+   /* EGL_EXT_platform_x11 recognizes exactly one attribute,
+    * EGL_PLATFORM_X11_SCREEN_EXT, which is optional.
+    */
    for (i = 0; attrib_list[i] != EGL_NONE; i += 2) {
-      EGLAttrib attrib = attrib_list[i];
-      EGLAttrib value = attrib_list[i + 1];
-
-      /* EGL_EXT_platform_x11 recognizes exactly one attribute,
-       * EGL_PLATFORM_X11_SCREEN_EXT, which is optional.
-       */
-      if (attrib != EGL_PLATFORM_X11_SCREEN_EXT)
+      if (attrib_list[i] != EGL_PLATFORM_X11_SCREEN_EXT)
          return _eglError(EGL_BAD_ATTRIBUTE, "eglGetPlatformDisplay");
-
-      display->Options.Platform = (void *)(uintptr_t)value;
    }
 
    return EGL_TRUE;
@@ -477,7 +528,8 @@  _eglGetX11Display(Display *native_display,
                   const EGLAttrib *attrib_list)
 {
    _EGLDisplay *display = _eglFindDisplay(_EGL_PLATFORM_X11,
-                                          native_display);
+                                          native_display,
+                                          attrib_list);
 
    if (!display) {
       _eglError(EGL_BAD_ALLOC, "eglGetPlatformDisplay");
@@ -503,7 +555,7 @@  _eglGetGbmDisplay(struct gbm_device *native_display,
       return NULL;
    }
 
-   return _eglFindDisplay(_EGL_PLATFORM_DRM, native_display);
+   return _eglFindDisplay(_EGL_PLATFORM_DRM, native_display, attrib_list);
 }
 #endif /* HAVE_DRM_PLATFORM */
 
@@ -518,7 +570,7 @@  _eglGetWaylandDisplay(struct wl_display *native_display,
       return NULL;
    }
 
-   return _eglFindDisplay(_EGL_PLATFORM_WAYLAND, native_display);
+   return _eglFindDisplay(_EGL_PLATFORM_WAYLAND, native_display, attrib_list);
 }
 #endif /* HAVE_WAYLAND_PLATFORM */
 
@@ -539,6 +591,7 @@  _eglGetSurfacelessDisplay(void *native_display,
       return NULL;
    }
 
-   return _eglFindDisplay(_EGL_PLATFORM_SURFACELESS, native_display);
+   return _eglFindDisplay(_EGL_PLATFORM_SURFACELESS, native_display,
+                          attrib_list);
 }
 #endif /* HAVE_SURFACELESS_PLATFORM */
diff --git a/src/egl/main/egldisplay.h b/src/egl/main/egldisplay.h
index cfd0ff66d64..9bd6bf76556 100644
--- a/src/egl/main/egldisplay.h
+++ b/src/egl/main/egldisplay.h
@@ -167,7 +167,7 @@  struct _egl_display
    /* options that affect how the driver initializes the display */
    struct {
       EGLBoolean ForceSoftware; /**< Use software path only */
-      void *Platform;         /**< Platform-specific options */
+      EGLAttrib *Attribs;     /**< Platform-specific options */
    } Options;
 
    /* these fields are set by the driver during init */
@@ -202,7 +202,7 @@  _eglFiniDisplay(void);
 
 
 extern _EGLDisplay *
-_eglFindDisplay(_EGLPlatformType plat, void *plat_dpy);
+_eglFindDisplay(_EGLPlatformType plat, void *plat_dpy, const EGLAttrib *attr);
 
 
 extern void

Comments

Hi Adam,

On Wed, 17 Apr 2019 at 19:57, Adam Jackson <ajax@redhat.com> wrote:
>
> If this was specified and a non-NULL display was passed to
> eglGetPlatformDisplay, we would ignore the attribute and instead use
> whatever xcb thought the default screen would be.
>
> To fix this, store a copy of the attribute list in the _EGLDisplay, and
> use that to look up any non-default screen number if we need it.
> ---
>  src/egl/drivers/dri2/platform_x11.c | 28 ++++++++--
>  src/egl/main/eglapi.c               |  2 +-
>  src/egl/main/egldisplay.c           | 85 +++++++++++++++++++++++------
>  src/egl/main/egldisplay.h           |  4 +-
>  4 files changed, 96 insertions(+), 23 deletions(-)
>
We have multiple different things happening here. Can you split things up a bit?

1. Flesh out a simpler version of _eglNumAttribs based off
_eglConvertIntsToAttribs. For example:

len = 0;
if (attribs) {
   while (attribs[len] != EGL_NONE)
      len += 2;
   len++;
}
return len;

2. Optional: update the codebase to use ^^ helper
3. Introduce Options::Attribs, populate and compare
Note: add extra wording why compare is safe - as-is it will cause
functional changes in some corner-cases
4. update platform_x11.c to honour the attribs
5. Kill off Options::Platform - _eglParseX11DisplayAttribList and elsewhere.
6. Optional: move _eglParseX11DisplayAttribList validation before the
_eglFindDisplay() ... just like all !X11 platforms do.

Nit: Make _eglSameAttribs return a bool - an c99 or EGL one.

Thanks
Emil