GNOME Bugzilla – Bug 726198
egl: Add a way to set the KMS FD
Last modified: 2014-03-15 19:28:45 UTC
See patch. This is needed for the mutter logind integration work.
Created attachment 271642 [details] [review] egl: Add a way to set the KMS FD This is needed for the logind integration work, where logind will send us an already-opened FD to KMS.
Review of attachment 271642 [details] [review]: it looks generally okay to me. do we also need a clutter_egl_get_kms_fd()? ::: clutter/egl/clutter-backend-eglnative.c @@ +82,3 @@ + renderer = cogl_renderer_new (); + cogl_renderer_set_winsys_id (renderer, COGL_WINSYS_ID_EGL_KMS); +{ what happens if _kms_fd is -1? what happens if you want to run this on non-KMS EGL framebuffers? ::: clutter/egl/clutter-egl.h @@ +88,3 @@ EGLDisplay clutter_egl_get_egl_display (void); +void clutter_egl_native_set_kms_fd (int fd); this should probably be called clutter_egl_set_kms_fd(), without the 'native'.
(In reply to comment #2) > do we also need a clutter_egl_get_kms_fd()? Don't think so. It's supposed to be fire-and-forget. And you can get it anyway by digging into Cogl: cogl_kms_renderer_get_kms_fd. > what happens if _kms_fd is -1? Cogl tries to open its own FD by opening /dev/dri/card0. > what happens if you want to run this on non-KMS EGL framebuffers? Run what? An app with the CLUTTER_BACKEND=eglnative envvar set? > this should probably be called clutter_egl_set_kms_fd(), without the 'native'. The backend is called "eglnative", and the backend class is called "CLUTTER_EGL_NATIVE". I'm not really sure about the difference between "eglnative" and "egl" though.
(In reply to comment #3) > (In reply to comment #2) > > do we also need a clutter_egl_get_kms_fd()? > > Don't think so. It's supposed to be fire-and-forget. And you can get it anyway > by digging into Cogl: cogl_kms_renderer_get_kms_fd. okay. > > what happens if _kms_fd is -1? > > Cogl tries to open its own FD by opening /dev/dri/card0. maybe leave a comment, but okay. > > what happens if you want to run this on non-KMS EGL framebuffers? > > Run what? An app with the CLUTTER_BACKEND=eglnative envvar set? if possible, yes. > > this should probably be called clutter_egl_set_kms_fd(), without the 'native'. > > The backend is called "eglnative", and the backend class is called > "CLUTTER_EGL_NATIVE". I'm not really sure about the difference between > "eglnative" and "egl" though. the API is namespaced as ClutterEGL, so that would translate to ClutterEGL.native_set_kms_fd().
(In reply to comment #4) > if possible, yes. I'm not sure what will happen right now, but after this patch if you use CLUTTER_BACKEND=eglnative we force the winsys to be EGL_KMS.
Created attachment 271649 [details] [review] egl: Add a way to set the KMS FD This is needed for the logind integration work, where logind will send us an already-opened FD to KMS.
Review of attachment 271649 [details] [review]: ::: clutter/egl/clutter-backend-eglnative.c @@ +80,3 @@ + CoglRenderer *renderer; + + renderer = cogl_renderer_new (); we could do this conditionally: if _kms_fd is -1 then we chain up; otherwise, we set the winsys id and the KMS fd. this would retain the existing behaviour (untested as it may be) while adding the new KMS functionality.
Created attachment 271781 [details] [review] egl: Add a way to set the KMS FD This is needed for the logind integration work, where logind will send us an already-opened FD to KMS.
Review of attachment 271781 [details] [review]: looks good, thanks.
Attachment 271781 [details] pushed as a96daf8 - egl: Add a way to set the KMS FD
This tripped up the FreeBSD builder (due to cogl_kms_renderer_set_kms_fd). We don't have kms on FreeBSD. Probably needs a check...
Created attachment 271982 [details] [review] egl: Only expose clutter_egl_set_kms_fd if we have KMS support And only call the proper Cogl functions in that case, too. This fixes the build on platforms without KMS, like the BSDs.
Review of attachment 271982 [details] [review]: I can verify that this fixes the build under: jhbuild --conditions=-wayland build clutter
Review of attachment 271982 [details] [review]: I can report that this fixes the build on FreeBSD.
Review of attachment 271982 [details] [review]: let's get this in. ::: clutter/egl/clutter-egl.h @@ +88,3 @@ EGLDisplay clutter_egl_get_egl_display (void); +#ifdef COGL_HAS_EGL_PLATFORM_KMS_SUPPORT this should not be needed, but it's fine: the platform specific headers are currently a mess, I'll have to clean them up anyway.
Attachment 271982 [details] pushed as 79ece18 - egl: Only expose clutter_egl_set_kms_fd if we have KMS support
(In reply to comment #15) > this should not be needed, but it's fine: the platform specific headers are > currently a mess, I'll have to clean them up anyway. I wanted an "undefined function" warning rather than a linking error. It's also more clear from a documentation / introspection standpoint as well.