GNOME Bugzilla – Bug 782923
gl: Add Mesa3D GBM backend
Last modified: 2018-03-02 17:08:16 UTC
On devices with working libdrm support, it is possible to use Mesa3D's GBM library to set up an EGL context directly on top of KMS. This enhancement adds a GBM backend to the GstGL stack.
Created attachment 352297 [details] [review] gstgl mesa3d gbm patch Initial version of GBM backend. Has some remaining TODOs, mostly related to error reporting. Also, some of this code could be shared with kmssink (mostly the DRM setup code).
Unfortunately, this doesn't seem to work on my optimus enabled system :(. It seems to work on intel-only hardware though
And, it works once I add to gst_gl_display_egl_get_from_native(): +#if GST_GL_HAVE_WINDOW_GBM + if (ret == EGL_NO_DISPLAY && (type & GST_GL_DISPLAY_TYPE_GBM) && + (gst_gl_check_extension ("EGL_MESA_platform_gbm", egl_exts) || + gst_gl_check_extension ("EGL_MESA_platform_gbm", egl_exts))) { + ret = _gst_eglGetPlatformDisplay (EGL_PLATFORM_GBM_MESA, (gpointer) display, + NULL); + } +#endif
Review of attachment 352297 [details] [review]: Looks mostly good. gstgl_gbm_private.c should not exist. All the relavant code should be placed in the other files. None of this API is exported so everything is private already. Some of the functions could go into a gstgl_gbm_utils.c/h if you so prefer. ::: configure.ac @@ +1007,3 @@ + if test "x$HAVE_VIV_FB_EGL" = "xno" -o "x$HAVE_GBM_EGL" = "xno"; then + if test "x$HAVE_X11_XCB" = "xno"; then + if test "x$HAVE_WAYLAND_EGL" = "xno"; then Why is this different from the if above? This effectively does if ((!viv || !gbm) && !X11 && !wayland) which doesn't seem correct. @@ +1029,3 @@ + dnl check Mesa GBM *before* X or Wayland since it is + dnl possible that both GBM and X/Wayland are present Any reason this needs to be performed first? As in, why is this comment relevant? ::: gst-libs/gst/gl/gbm/gstgl_gbm_private.c @@ +117,3 @@ + } + + // No match found No C++ comments. We conform to C89. @@ +459,3 @@ + + GST_DEBUG ("Attempting to add GBM BO as scanout framebuffer width/height: %" + PRIu32 "/%" PRIu32 " pixels stride: %" PRIu32 " bytes format: %s " use G_G(U)INT*_FORMAT instead of PRI* macros. @@ +525,3 @@ + const gchar *devnode = g_udev_device_get_device_file (gudevice); + + if (!g_str_has_prefix (devnode, "/dev/dri/card")) NULL check? devnode could be NULL and will critical inside g_str_has_prefix(). @@ +640,3 @@ + " hsync/vsync start %" PRIu16 "/%" PRIu16 " hsync/vsync end %" PRIu16 + "/%" PRIu16 " htotal/vtotal %" PRIu16 "/%" PRIu16 " hskew %" PRIu16 + " vscan %" PRIu16 " vrefresh %" PRIu32 " preferred %d", i, use G_G(U)INT*_FORMAT instead of PRI* macros ::: gst-libs/gst/gl/gbm/gstgl_gbm_private.h @@ +68,3 @@ +int gst_gl_gbm_find_and_open_drm_node (void); + +gboolean gst_gl_display_gbm_setup_drm (GstGLDisplayGBMPrivate *priv); Passing the private structure directly isn't exactly kosher from a GObject standpoint. This should be the actual GstGLDisplayGBM instance instead and the private structure retrieved from the instance instead. ::: gst-libs/gst/gl/gbm/gstgldisplay_gbm.h @@ +61,3 @@ +}; + +//int gst_gl_display_gbm_find_and_open_drm_node (void); Any reason why this is still here? ::: gst-libs/gst/gl/gbm/gstglwindow_gbm_egl.c @@ +90,3 @@ + /* This function is called in here, and not in the open() + * vmethod. The reason for this is explained inside the + * gst_gl_window_gbm_init_surface() function. */ _init_surface() should be called from a _create_window() function that is called from GstGLContextEGL like the 4 other winsys implementations that have this problem. Should make a window vfunc one of these days. @@ +362,3 @@ + + window_egl = g_object_new (GST_TYPE_GL_WINDOW_GBM_EGL, NULL); + window_egl->priv->display_priv = GST_GL_DISPLAY_GBM_PRIVATE (display); Again, not idiomatic GObject. Retrieve the display from the window as needed and then retrieve the private structure.
Created attachment 368846 [details] [review] #1 : Add checks for DRM and libgudev Updated GBM and libgudev checks for gst-plugins-base.
Created attachment 368847 [details] [review] #2 : GstGL Mesa3D GBM patch Updated Mesa3D GBM patch to apply against gst-plugins-base master. Also contains corrections for the aforementioned issues (except for the _init_surface() problem, since it is unclear how to fix that at this point). Also, this patch enhances the previous one by implementing triple buffering, improving performance.
Created attachment 368859 [details] [review] gl: Add meson support for GBM backend I've just added meson support, only build tested for now.
Created attachment 368862 [details] [review] #2 : GstGL Mesa3D GBM patch , v2 Fixed C++ style comments.
Created attachment 368864 [details] [review] gl: Add meson support for GBM backend
I've been doing some testing here on Intel, it seems stable. Though, I see some features don't work quite right, but it looks like Mesa bugs to me (notably dmabuf behaves wrongly (rendering goes wrong). I'd really like to see that merged now, but we need the other to agree considering we are passed the feature freeze for 1.14.
Feature freeze is Tuesday night, so feel free to merge if you think it's ready. Main question is usually how risky/intrusive it is for other existing functionality. Or if this takes over as default.
I compiled everything and also checked on Intel board, there were no bugs during displaying videotestsrc. Thank you so much for this work.
(In reply to Tim-Philipp Müller from comment #11) > Feature freeze is Tuesday night, so feel free to merge if you think it's > ready. Oh, I thought it was passed already. > > Main question is usually how risky/intrusive it is for other existing > functionality. Or if this takes over as default. Should be fine, it's placed after Vivante, so quite low in the stack. The only code that mixes with the rest is the context and window initialization.
Attachment 368846 [details] pushed as 79f9f9e - #1 : Add checks for DRM and libgudev Attachment 368862 [details] pushed as 4df219f - #2 : GstGL Mesa3D GBM patch Attachment 368864 [details] pushed as 1a97338 - gl: Add meson support for GBM backend
(In reply to Vavooon from comment #12) > I compiled everything and also checked on Intel board, there were no bugs > during displaying videotestsrc. Thank you so much for this work. Thanks for testing.
(In reply to Carlos Rafael Giani from comment #6) > Created attachment 368847 [details] [review] [review] > #2 : GstGL Mesa3D GBM patch > > (except for the _init_surface() problem, since it is unclear how to fix > that at this point). Have a look at how *all* the other winsys implementations handle this problem: https://cgit.freedesktop.org/gstreamer/gst-plugins-base/tree/gst-libs/gst/gl/egl/gstglcontext_egl.c#n497 Did you try doing the same thing?
Review of attachment 368862 [details] [review]: One nit. ::: m4/gst-gl.m4 @@ +481,3 @@ + HAVE_WINDOW_GBM=yes + GL_LIBS="$GL_LIBS" + GL_CFLAGS="$GL_CFLAGS" This seems redundant, no?
Review of attachment 368864 [details] [review]: Not quite :) ::: gst-libs/gst/gl/meson.build @@ +603,3 @@ endif +# GDM Checks GBM @@ +708,3 @@ install_headers(gl_x11_headers, subdir : 'gstreamer-1.0/gst/gl/x11') install_headers(gl_wayland_headers, subdir : 'gstreamer-1.0/gst/gl/wayland') + install_headers(gl_gbm_headers, subdir : 'gstreamer-1.0/gst/gl/gbm') This header is not installed in the autotools build.
https://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/?id=ee5953765783fa6969e744c448660c010277853f should also be solved like every other winsys by adding DRM_CFLAGS to GL_CFLAGS in m4/gst-gl.m4.
(In reply to Matthew Waters (ystreet00) from comment #17) > Review of attachment 368862 [details] [review] [review]: > > One nit. > > ::: m4/gst-gl.m4 > @@ +481,3 @@ > + HAVE_WINDOW_GBM=yes > + GL_LIBS="$GL_LIBS" > + GL_CFLAGS="$GL_CFLAGS" > > This seems redundant, no? Oops, I saw it and forgot to remove. I'll fix. (In reply to Matthew Waters (ystreet00) from comment #18) > Review of attachment 368864 [details] [review] [review]: > > Not quite :) > > ::: gst-libs/gst/gl/meson.build > @@ +603,3 @@ > endif > > +# GDM Checks > > GBM > > @@ +708,3 @@ > install_headers(gl_x11_headers, subdir : 'gstreamer-1.0/gst/gl/x11') > install_headers(gl_wayland_headers, subdir : > 'gstreamer-1.0/gst/gl/wayland') > + install_headers(gl_gbm_headers, subdir : 'gstreamer-1.0/gst/gl/gbm') > > This header is not installed in the autotools build. Ok, I'll check which one is right and fix. I've just followed a pattern here without thinking through. (In reply to Matthew Waters (ystreet00) from comment #19) > https://cgit.freedesktop.org/gstreamer/gst-plugins-base/commit/ > ?id=ee5953765783fa6969e744c448660c010277853f should also be solved like > every other winsys by adding DRM_CFLAGS to GL_CFLAGS in m4/gst-gl.m4. Ok.
commit d796188cf168588c24f1117b5dcbc91ba342819b (HEAD -> master) Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Tue Feb 27 13:14:26 2018 -0500 meson: Don't install GL GBM headers commit fbc2375d57ec07db75e2d73b4a6c672c3c83f040 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Tue Feb 27 13:12:59 2018 -0500 gl: Move DRM_CFLAGS into gst-gl.m4 Let me know if you see anything else.
Sorry for chiming in so late here. Firstly, get_{depth,bpp}_from_format are wrong. There is a very very small and fixed depth/bpp -> format mapping inside the kernel, which is pretty much: * 8/8 -> C8 * 15/16 -> XRGB1555 * 16/16 -> RGB565 * 24/24 -> RGB888 * 24/32 -> XRGB8888 * 30/32 -> XRGB2101010 * 32/32 -> ARGB8888 Anything else will silently be assumed to be XRGB8888. So you should delete all but those formats, but _also_ preferentially use drmModeAddFB2, for which you can just pass a format directly. (GBM_FORMAT_* == DRM_FORMAT_*, so no need to add a mapping; you can rely on this and it is both API and ABI guaranteed.) Then you can just use drmModeAddFB as a fallback for the above formats only. You can also delete the GBM_BO_FORMAT_* handling inside those two functions, as gbm_bo_get_format() only ever returns GBM_FORMAT_* enums. Very intuitive, I know. I don't quite follow what the comment in gst_gl_window_gbm_egl_set_window_handle() means (wrt external objects and DRM pageflips), but I'm happy to try to answer if you can elaborate. As a final nitpick, GBM is not Mesa-specific: it's also implemented by the Imagination, ARM, and Vivante proprietary drivers. Those tend to advertise EGL_KHR_platform_gbm rather than EGL_MESA_platform_gbm in the extension string.
Hi Daniel, as this code is merged, can you file a separate bug for these errors, and specify which files are affect ?
Daniel: set_window_handle is part of the GstVideoOverlay functionality. This is how you can embed a GStreamer video sink inside, say, a Gtk interface. set_window_handle() is then called to supply the code with an existing window handle. What this handle is, is platform specific. In X, it is a Window id. Since I do not see why anybody would want to embed GBM buffer objects anywhere, I added that comment. I wrote the GBM support with a single fullscreen video output in mind, just like the Vivante FB support. As for the rest, thanks for the info! I have one remaining question: do you see anything wrong with how the BOs and the page flipping are done in draw_cb() ? I think this is proper triple buffering. Do you?