[Mesa-dev] [PATCH RFC] egl/dri2: Add dri3 support to x11 platform
Emil Velikov
emil.l.velikov at gmail.com
Mon Jul 6 03:55:56 PDT 2015
On 4 July 2015 at 04:18, Boyan Ding <boyan.j.ding at 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.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.
>
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
More information about the mesa-dev
mailing list