[Mesa-dev] [PATCH 1/2] egl: add optional plat_opt to _eglFindDisplay()

[Emil Velikov]

On Thu, 4 Apr 2019 at 18:05, Adam Jackson <ajax at redhat.com> wrote:
>
> On Thu, 2019-04-04 at 16:25 +0100, Emil Velikov wrote:
>
> > I'm not a huge fan of this. Yet again the other platform (x11) that uses
> > these has a number of issues, see below for details. Once those are resolved
> > we could try and make this more uniform.
> >
> >  a) spec does not mention if a new (or the existing) EGLDislay should be
> > given for same native_dpy/screen_no combo.
>
> An EGLDisplay maps to an X11 screen. EGL_KHR_platform_x11 says:
>
> >     On the X11 platform, an EGLDisplay refers to a specific X11 screen rather
> >     than an X11 display connection.
>
> And EGL 1.5 says (with similar text in EGL_EXT_platform_base):
>
> > Multiple calls made to eglGetPlatformDisplay with the same parameters will
> > return the same EGLDisplay handle.
>
> EGL_KHR_platform_x11 is a little confusing on this point in one of the
> issues, because it says a new display connection is created for
> EGL_DEFAULT_DISPLAY; presumably that's only for the first EGLDisplay
> you create though.
>
> >  b) the implementation has number of issues:
> >  - screen 0 is hardcoded when using DEFAULT_DISPLAY
>
> False, but:
>
> >  - screen is _ignored_ when using !DEFAULT_DISPLAY
>
> true, but easily fixed. We definitely store the screen number in
> disp->Options.Platform in _eglParseX11DisplayAttribList, and then:
>
> > 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;
> >    const char *msg;
> >
> >    disp->DriverData = (void *) dri2_dpy;
> >    if (disp->PlatformDisplay == NULL) {
> >       dri2_dpy->conn = xcb_connect(NULL, &screen);
> >       dri2_dpy->own_device = true;
>
> If no display was given, we connect however xcb would, and save what it
> decided was the default screen number.
>
> >    } else {
> >       Display *dpy = disp->PlatformDisplay;
> >
> >       dri2_dpy->conn = XGetXCBConnection(dpy);
> >       screen = DefaultScreen(dpy);
> >    }
>
> If not, we use what the display already had set; this is where we
> ignore the user's screen number and instead use the display's default.
>
> I think the fix is to delete that DefaultScreen line above, and in
> _eglFindDisplay call the appropriate platform's vtable to check for
> identical option lists (this is your _eglSameDeviceDisplay in the next
> patch, just abstracted out a bit).
>
Thanks for having a look Adam.

Can I interest you in writing a fix for the X11 code ;-)
It'll be great if we have inline comments with the spec quotes and
reasoning you mentioned.

-Emil