[Mesa-dev] [PATCH RFC] egl/dri2: Add dri3 support to x11 platform
Boyan Ding
boyan.j.ding at gmail.com
Fri Jul 3 20:18:18 PDT 2015
Hi Emil,
On 07/03/2015 10:36 PM, Emil Velikov wrote:
> Hi Boyan,
>
> Thank you for doing this ! A few suggestions which you might be interesting:
>
> Considering that the backend has handled more than dri2 perhaps we can
> do a s/dri2/dri/ :-) That obviously is independent of your work.
>
> On 01/07/15 16:31, Boyan Ding wrote:
>> Signed-off-by: Boyan Ding <boyan.j.ding at gmail.com>
>> ---
>> configure.ac | 3 +
>> src/egl/drivers/dri2/Makefile.am | 5 +
>> src/egl/drivers/dri2/egl_dri2.c | 65 +-
>> src/egl/drivers/dri2/egl_dri2.h | 12 +-
>> src/egl/drivers/dri2/platform_x11.c | 127 ++-
>> src/egl/drivers/dri2/platform_x11_dri3.c | 1591 ++++++++++++++++++++++++++++++
>> src/egl/drivers/dri2/platform_x11_dri3.h | 140 +++
>> 7 files changed, 1926 insertions(+), 17 deletions(-)
>> create mode 100644 src/egl/drivers/dri2/platform_x11_dri3.c
>> create mode 100644 src/egl/drivers/dri2/platform_x11_dri3.h
>>
>> diff --git a/configure.ac b/configure.ac
>> index af61aa2..090e6c9 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -1545,6 +1545,9 @@ if test "x$enable_egl" = xyes; then
>> if test "x$enable_shared_glapi" = xno; then
>> AC_MSG_ERROR([egl_dri2 requires --enable-shared-glapi])
>> fi
>> + if test "x$enable_dri3" = xyes; then
>> + EGL_LIB_DEPS="$EGL_LIB_DEPS -lxcb-dri3 -lxcb-present -lXxf86vm -lxshmfence -lxcb-sync"
> Neither one of these should be a direct -lfoo expression. We should
> check/use PKG_CHECK_MODULES and foo_{CFLAGS,LIBS}.
I'll correct that.
>
>> + fi
>> else
>> # Avoid building an "empty" libEGL. Drop/update this
>> # when other backends (haiku?) come along.
>> diff --git a/src/egl/drivers/dri2/Makefile.am b/src/egl/drivers/dri2/Makefile.am
>> index 55be4a7..d5b511c 100644
>> --- a/src/egl/drivers/dri2/Makefile.am
>> +++ b/src/egl/drivers/dri2/Makefile.am
>> @@ -52,6 +52,11 @@ if HAVE_EGL_PLATFORM_X11
>> libegl_dri2_la_SOURCES += platform_x11.c
>> AM_CFLAGS += -DHAVE_X11_PLATFORM
>> AM_CFLAGS += $(XCB_DRI2_CFLAGS)
>> +if HAVE_DRI3
>> +libegl_dri2_la_SOURCES += \
>> + platform_x11_dri3.c \
>> + platform_x11_dri3.h
>> +endif
>> endif
>>
>> if HAVE_EGL_PLATFORM_WAYLAND
>> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
>> index b1b65f7..052c579 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.c
>> +++ b/src/egl/drivers/dri2/egl_dri2.c
>> @@ -322,6 +322,12 @@ struct dri2_extension_match {
>> int offset;
>> };
>>
>> +static struct dri2_extension_match dri3_driver_extensions[] = {
>> + { __DRI_CORE, 1, offsetof(struct dri2_egl_display, core) },
>> + { __DRI_IMAGE_DRIVER, 1, offsetof(struct dri2_egl_display, image_driver) },
>> + { NULL, 0, 0 }
>> +};
>> +
>> static struct dri2_extension_match dri2_driver_extensions[] = {
>> { __DRI_CORE, 1, offsetof(struct dri2_egl_display, core) },
>> { __DRI_DRI2, 2, offsetof(struct dri2_egl_display, dri2) },
>> @@ -464,6 +470,24 @@ dri2_open_driver(_EGLDisplay *disp)
>> }
>>
>> EGLBoolean
>> +dri2_load_driver_dri3(_EGLDisplay *disp)
> dri3_load_driver perhaps ?
I prefixed all my functions in public files with "dri2" instead of
"dri3" because the EGL driver is currently called "DRI2" (although it
seems funny to see dri3 in a driver called "dri2").
The current name of the driver is "DRI2" because it uses dri2 to talk
with the X server and uses similar technologies with direct rendering
on other platforms. With my patch, the first reason becomes invalid.
But should the name of the driver or namespace of all functions (or
just the functions involved with dri3) change? I'm open to others'
suggestions. The same applies to comments on function names below.
>
>> [snip]
>>
>> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
>> index f0cc6da..d258753 100644
>> --- a/src/egl/drivers/dri2/egl_dri2.h
>> +++ b/src/egl/drivers/dri2/egl_dri2.h
>> @@ -153,11 +153,16 @@ struct dri2_egl_display
>>
>> int dri2_major;
>> int dri2_minor;
>> + int dri3_major;
>> + int dri3_minor;
>> + int present_major;
>> + int present_minor;
> Many of these are unused. Same goes for the glx code which was the
> inspiration for this work :-)
I think it's safe to remove them then.
>
>> __DRIscreen *dri_screen;
>> int own_dri_screen;
>> const __DRIconfig **driver_configs;
>> void *driver;
>> const __DRIcoreExtension *core;
>> + const __DRIimageDriverExtension *image_driver;
>> const __DRIdri2Extension *dri2;
>> const __DRIswrastExtension *swrast;
>> const __DRI2flushExtension *flush;
>> @@ -189,6 +194,7 @@ struct dri2_egl_display
>> #ifdef HAVE_X11_PLATFORM
>> xcb_connection_t *conn;
>> int screen;
>> + Display *dpy;
> If we only loved XF86VIDMODE and/or xcb a bit more we wouldn't need this
> :'-(
Yeah, that is the "ugly" part of my code. The egl code for x11 all
uses xcb. But the SystemTimeExtension used in glx (my code copied that
implementation) uses XF86VIDMODE, which is only available in Xlib. I
wonder if that code can be replaced using xrandr. If so, this hack and
the hunks below that you find unnecessary will disappear.
>
>> #endif
>>
>> #ifdef HAVE_WAYLAND_PLATFORM
>> @@ -202,8 +208,9 @@ struct dri2_egl_display
>> int formats;
>> uint32_t capabilities;
>> int is_render_node;
>> - int is_different_gpu;
>> #endif
>> +
>> + int is_different_gpu;
> This could be a separate patch.
This single hunk or all the code that is associated with gpu
offloading?
>
>> [snip]
>>
>> diff --git a/src/egl/drivers/dri2/platform_x11_dri3.c b/src/egl/drivers/dri2/platform_x11_dri3.c
>> new file mode 100644
>> index 0000000..dcd7128
>> --- /dev/null
>> +++ b/src/egl/drivers/dri2/platform_x11_dri3.c
> Considering that this is heavily based on the glx code I'm wondering if
> we can get some of that into some common helpers ? Something like
> src/loader/dri3_helper.[ch] perhaps.
I also hope to have more code-reuse. But I met some problems in the
process of writing this. The most significant one of which is the
difference in details of the data structures. For example glx/dri3
stores the width and height of a "drawable" in dri3_drawable struct.
But in EGL, their counterpart is stored in _EGLSurface, the parent
struct of dri3_egl_surface, which is the egl counterpart of
dri3_drawable. Reusing the code needs careful (or even invasive)
design, so I decided to get it working first. I'll spend some time to
study the code and try to place some reusable code into common
helper functions.
>
>> [snip]
>>
>> +static void
>> +dri3_copy_area (xcb_connection_t *c /**< */,
> Please, no space between function name and opening bracket.
>
>> + xcb_drawable_t src_drawable /**< */,
>> + xcb_drawable_t dst_drawable /**< */,
>> + xcb_gcontext_t gc /**< */,
>> + int16_t src_x /**< */,
>> + int16_t src_y /**< */,
>> + int16_t dst_x /**< */,
>> + int16_t dst_y /**< */,
>> + uint16_t width /**< */,
>> + uint16_t height /**< */)
> Add some comments or just drop the "/**< */"s ?
Okay.
>
>> +{
>> + xcb_void_cookie_t cookie;
>> +
>> + cookie = xcb_copy_area_checked(c,
>> + src_drawable,
>> + dst_drawable,
>> + gc,
>> + src_x,
>> + src_y,
>> + dst_x,
>> + dst_y,
>> + width,
>> + height);
>> + xcb_discard_reply(c, cookie.sequence);
>> +}
>> +
>> +/**
>> + * Called by the driver when it needs to update the real front buffer with the
>> + * contents of its fake front buffer.
>> + */
>> +static void
>> +dri3_flush_front_buffer(__DRIdrawable *driDrawable, void *loaderPrivate)
>> +{
>> +#if 0
>> + struct glx_context *gc;
>> + struct dri3_drawable *pdraw = loaderPrivate;
>> + struct dri3_screen *psc;
>> +
>> + if (!pdraw)
>> + return;
>> +
>> + if (!pdraw->base.psc)
>> + return;
>> +
>> + psc = (struct dri3_screen *) pdraw->base.psc;
>> +
>> + (void) __glXInitialize(psc->base.dpy);
>> +
>> + gc = __glXGetCurrentContext();
>> +
>> + dri3_flush(psc, pdraw, __DRI2_FLUSH_DRAWABLE, __DRI2_THROTTLE_FLUSHFRONT);
>> +
>> + dri3_wait_gl(gc);
>> +#endif
>> + /* FIXME */
>> + (void) driDrawable;
>> + (void) loaderPrivate;
> If we don't get to fixing this now, providing some feedback
> (printf(foo)) would be appreciated. Same goes for other stubbed functions.
Okay.
>
>> [snip]
>>
>> +/* FIXME: Is this right? Seems problematic for WL_bind_wayland_display */
> What seems to be the problem ?
>
> Afaik xcb_dri3_open_reply_fds should return an FD which is ok (be that a
> render_node device, or a master one with explicit auth).
The problem is that WL_bind_wayland_display don't work under dri3 on
x11. I only found it yesterday that to get it work, we'll need to add a
mechanism to pass fd instead of name of dri device in wl_drm protocol.
Previously, if a wayland client wants to use hardware accelerated EGL,
it (with the help of libEGL in mesa) will bind to wl_drm object, and
wl_drm will immediately send the name of dri device to the wayland
client (actually also libEGL in mesa). After wayland platform code
opens the device, it has to send the fd to the X server or drm to
get authentication. Things are different with dri3, where a fd is
directly sent to the client without the need to authenticate.
I propose the following addition in wl_drm protocol:
There are two kinds of wl_drm implementation. One is the current form.
The other one, called "dri3-capable" (or whatever name), include wl_drm
object built on dri3 directly or indirectly through wayland platform.
If a client binds to a "dri3-capable" wl_drm object, it will send a "device"
event to the client with NULL or empty string (so a client who knows
nothing about it can safely fail). If the client knows about dri3-capable
wl_drm object, it will send a request named get_fd and wl_drm will
respond it with an fd acquired with dri3. If the wl_drm object is not
"dri3-capable" it will raise an error if it receives a get_fd request,
so will a "dri3-capable" wl_drm object if it receives authenticate
request.
So the following dri3_authenticate function is not needed. Let's not
expose WL_bind_wayland_display for now, and its enablement
should be separate patches.
>> +static int
>> +dri3_authenticate(_EGLDisplay *disp, uint32_t id)
>> +{
>> + return 0;
>> +}
>> +
>> +struct dri2_egl_display_vtbl dri3_x11_display_vtbl = {
> static ?
It is currently used in x11_platform.c.
>
>> + .authenticate = dri3_authenticate,
>> + .create_window_surface = dri3_create_window_surface,
>> + .create_pixmap_surface = dri3_create_pixmap_surface,
>> + .create_pbuffer_surface = dri3_create_pbuffer_surface,
>> + .destroy_surface = dri3_destroy_surface,
>> + .create_image = dri2_create_image_khr,
>> + .swap_interval = dri3_set_swap_interval,
>> + .swap_buffers = dri3_swap_buffers,
>> + .swap_buffers_with_damage = dri2_fallback_swap_buffers_with_damage,
>> + .swap_buffers_region = dri2_fallback_swap_buffers_region,
>> + .post_sub_buffer = dri2_fallback_post_sub_buffer,
>> + .copy_buffers = dri2_fallback_copy_buffers,
>> + .query_buffer_age = dri3_query_buffer_age,
>> + .create_wayland_buffer_from_image = dri2_fallback_create_wayland_buffer_from_image,
>> + .get_sync_values = dri3_get_sync_values,
>> +};
>> +
>> +/** dri3_open
>> + *
>> + * Wrapper around xcb_dri3_open
>> + */
>> +static int
>> +dri3_open(xcb_connection_t *conn,
>> + xcb_window_t root,
>> + uint32_t provider)
>> +{
>> + xcb_dri3_open_cookie_t cookie;
>> + xcb_dri3_open_reply_t *reply;
>> + int fd;
>> +
>> + cookie = xcb_dri3_open(conn,
>> + root,
>> + provider);
>> +
>> + reply = xcb_dri3_open_reply(conn, cookie, NULL);
>> + if (!reply)
>> + return -1;
>> +
>> + if (reply->nfd != 1) {
>> + free(reply);
>> + return -1;
>> + }
>> +
>> + fd = xcb_dri3_open_reply_fds(conn, reply)[0];
>> + fcntl(fd, F_SETFD, FD_CLOEXEC);
> I do recall looking at this (in src/glx) and wondering... aren't we
> missing fcntl(F_GETFD) ?
>
>> +
>> + return fd;
>> +}
>> +
>> +EGLBoolean
>> +dri3_x11_connect(struct dri2_egl_display *dri2_dpy)
>> +{
>> + xcb_dri3_query_version_reply_t *dri3_query;
>> + xcb_dri3_query_version_cookie_t dri3_query_cookie;
>> + xcb_present_query_version_reply_t *present_query;
>> + xcb_present_query_version_cookie_t present_query_cookie;
>> + xcb_generic_error_t *error;
>> + xcb_screen_iterator_t s;
>> + xcb_screen_t *screen;
>> + const xcb_query_extension_reply_t *extension;
>> +
>> + xcb_prefetch_extension_data (dri2_dpy->conn, &xcb_dri3_id);
>> + xcb_prefetch_extension_data (dri2_dpy->conn, &xcb_present_id);
>> +
>> + extension = xcb_get_extension_data(dri2_dpy->conn, &xcb_dri3_id);
>> + if (!(extension && extension->present))
>> + return EGL_FALSE;
>> +
>> + extension = xcb_get_extension_data(dri2_dpy->conn, &xcb_present_id);
>> + if (!(extension && extension->present))
>> + return EGL_FALSE;
>> +
>> + dri3_query_cookie = xcb_dri3_query_version(dri2_dpy->conn,
>> + XCB_DRI3_MAJOR_VERSION,
>> + XCB_DRI3_MINOR_VERSION);
>> +
>> + present_query_cookie = xcb_present_query_version(dri2_dpy->conn,
>> + XCB_PRESENT_MAJOR_VERSION,
>> + XCB_PRESENT_MINOR_VERSION);
>> +
>> + /* FIXME: a little bit memory leak here */
>> + dri3_query =
>> + xcb_dri3_query_version_reply(dri2_dpy->conn, dri3_query_cookie, &error);
>> + if (dri3_query == NULL || error != NULL) {
>> + _eglLog(_EGL_WARNING, "DRI2: failed to query dri3 version");
>> + free(error);
>> + return EGL_FALSE;
>> + }
>> + dri2_dpy->dri3_major = dri3_query->major_version;
>> + dri2_dpy->dri3_minor = dri3_query->minor_version;
>> + free(dri3_query);
>> +
>> + present_query =
>> + xcb_present_query_version_reply(dri2_dpy->conn,
>> + present_query_cookie, &error);
>> + if (present_query == NULL || error != NULL) {
>> + _eglLog(_EGL_WARNING, "DRI2: failed to query Present version");
>> + free(error);
>> + return EGL_FALSE;
>> + }
>> + dri2_dpy->present_major = present_query->major_version;
>> + dri2_dpy->present_minor = present_query->minor_version;
>> + free(present_query);
>> +
>> + s = xcb_setup_roots_iterator(xcb_get_setup(dri2_dpy->conn));
>> + screen = get_xcb_screen(s, dri2_dpy->screen);
>> + if (!screen) {
>> + _eglError(EGL_BAD_NATIVE_WINDOW, "dri3_x11_connect");
>> + return EGL_FALSE;
>> + }
>> +
>> + dri2_dpy->fd = dri3_open(dri2_dpy->conn, screen->root, 0);
>> + if (dri2_dpy->fd < 0) {
>> + int conn_error = xcb_connection_has_error(dri2_dpy->conn);
>> + _eglLog(_EGL_WARNING, "DRI2: Screen seem not DRI3 capable");
>> +
>> + if (conn_error)
>> + _eglLog(_EGL_WARNING, "DRI2: Failed to initialize DRI3");
>> +
>> + return EGL_FALSE;
>> + }
>> +
>> + dri2_dpy->fd = loader_get_user_preferred_fd(dri2_dpy->fd, &dri2_dpy->is_different_gpu);
>> +
>> + dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd, 0);
>> + if (!dri2_dpy->driver_name) {
>> + _eglLog(_EGL_WARNING, "DRI2: No driver found");
>> + close(dri2_dpy->fd);
>> + return EGL_FALSE;
>> + }
>> +
>> + dri2_dpy->device_name = loader_get_device_name_for_fd(dri2_dpy->fd);
> Seems like wayland_drm_init is the only user of device_name in the whole
> of EGL. Wondering if it'll be worth keeping it wayland specific and
> ripping it out of platform_{drm,x11}.
Not really. It is used to support WL_bind_wayland_display, to send name
of dri device to wayland client as mentioned above. drm and x11 (dri2)
platforms need this. Maybe some comment can be added to explain it.
>
> Cheers,
> Emil
>
Thanks for the review!
Regards,
Boyan Ding
More information about the mesa-dev
mailing list