On 4 July 2015 at 04:18, Boyan Ding <boyan.j.d...@gmail.com> wrote: > 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.d...@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. > Iirc for swrast based wayland (platform_wayland.c) we already remove the "it uses dri2" assumption. Then again all this is a simple name change which, as mentioned before, is not strictly related to your work.
>> >>> [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? > Splitting this out might be a bit of an overkill. So don't mind me here. >> >>> [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. Afaics one can either 1) have a sizeable amount of arguments passed to the helpers or 2) consolidate the variables needed in a common struct, although that means that some will be duplicated from EGL/GLX. Wish there was a cleaner way :-\ > Reusing the code needs careful (or even invasive) > design, so I decided to get it working first. Absolutely. > I'll spend some time to > study the code and try to place some reusable code into common > helper functions. > That'll be amazing, thank you. Meanwhile I'll take a look at getting some other helpers started :-) >> >>> [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. > Silly me forgot that the device name passed to wayland was the master one. As Axel (and yourself) noted using drmGetNodeTypeFromFd + drmGetRenderDeviceNameFromFd (which hide some gory details) would help. -Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev