On Fri, Apr 3, 2015 at 1:35 PM, Chad Versace <chad.vers...@intel.com> wrote: > Time to revive this patch! > > Why? > - I don't like large patchsets living in Chrome OS for too long. > - Frank submitted Waffle patches to support this, and I hesitate to > add Waffle support unless the platform is upstream. > > Of course, the patch no longer applies to master. So I rebased and > pushed it to a personal branch: > > git fetch git://github.com/chadversary/mesa > refs/heads/cooking/hshi/egl-platform-null && git checkout FETCH_HEAD > https://github.com/chadversary/mesa/tree/cooking/hshi/egl-platform-null > > On Tue 17 Feb 2015, Haixia Shi wrote: >> >> The NULL platform is for off-screen rendering only. Render node support is >> required. > > > Naming it the "NULL" platform seems very odd. It actually *does* stuff. > Usually things named "null" do nothing. > > Also, this platform is not unique in its NULL requirement. That is, > there do exist other EGL platforms in which eglGetDisplay accepts only > NULL, such as Android. I strongly suspect this is also the case for > other lesser known operating systems. > > But there isn't an obviously better name. Me and Jordan chatted about > this for a long time and came to no conclusion. > > Usually, an EGL platform is named after (1) the operating system, (2) > the underlying window system, or (3) the real type of > EGLNativeDisplayType. None of those precedents help here. However, this > platform does have a single, unique property that distinguishes it from > all other EGL platforms: ___this is the only platform that has no > support for EGLSurface___. Perhaps we should name the platform after > that property? > > Perhaps EGL_MESA_platform_surfaceless and platform_surfaceless.c?
That's a very good name. As it happens, it also matches Chrome's naming. > >> Only consider the render nodes. Do not use normal nodes as they require >> auth hooks. > > > I agree with that decision. The platform should not fallback to > /dev/dri/card*, at least not in this initial patch. That adds too much > complication. > > I have comments below on how the rendernode gets selected. > >> Signed-off-by: Haixia Shi <h...@chromium.org> >> --- >> src/egl/drivers/dri2/Makefile.am | 5 ++ >> src/egl/drivers/dri2/egl_dri2.c | 13 ++- >> src/egl/drivers/dri2/egl_dri2.h | 3 + >> src/egl/drivers/dri2/platform_null.c | 169 >> +++++++++++++++++++++++++++++++++++ >> 4 files changed, 187 insertions(+), 3 deletions(-) >> create mode 100644 src/egl/drivers/dri2/platform_null.c >> >> diff --git a/src/egl/drivers/dri2/egl_dri2.c >> b/src/egl/drivers/dri2/egl_dri2.c >> index 86e5f24..6ed137e 100644 >> --- a/src/egl/drivers/dri2/egl_dri2.c >> +++ b/src/egl/drivers/dri2/egl_dri2.c > > >> @@ -632,6 +632,13 @@ dri2_initialize(_EGLDriver *drv, _EGLDisplay *disp) >> return EGL_FALSE; >> >> switch (disp->Platform) { >> +#ifdef HAVE_NULL_PLATFORM >> + case _EGL_PLATFORM_NULL: >> + if (disp->Options.TestOnly) >> + return EGL_TRUE; >> + return dri2_initialize_null(drv, disp); >> +#endif > > > The platform has a major deficiency in this hunk, but I don't think it > needs fixing in this initial patch. Due to the way > eglGetDisplay(EGLNativeDisplay dpy) > auto-detects the real type of dpy, it's impossible to build Mesa > with platform_null and platform_x11 enabled and actually have both > usable. eglGetDisplay will work for only one, and the one that works > will be the first that occurs in the platform list given to > --with-egl-platforms=... . In other words, > > --with-egl-platforms=x11,null => eglGetDisplay(NULL) opens the default > X11 display > --with-egl-platforms=null,x11 => eglGetDisplay(NULL) opens a render node > > Again, I don't think you need to fix this in the initial patch. The > proper solution is to implement a platform extension, like > EGL_MESA_platform_gbm [1], which can be done in a follow-up patch. > Without a platform extension, distro packagers and most Mesa developers > will not be able to ever test this platform. > > [1] > https://www.khronos.org/registry/egl/extensions/MESA/EGL_MESA_platform_gbm.txt > >> diff --git a/src/egl/drivers/dri2/platform_null.c >> b/src/egl/drivers/dri2/platform_null.c >> new file mode 100644 >> index 0000000..55ceab6 >> --- /dev/null >> +++ b/src/egl/drivers/dri2/platform_null.c >> @@ -0,0 +1,169 @@ > > > >> +static __DRIbuffer * >> +null_get_buffers_with_format(__DRIdrawable * driDrawable, >> + int *width, int *height, >> + unsigned int *attachments, int count, >> + int *out_count, void *loaderPrivate) >> +{ >> + struct dri2_egl_surface *dri2_surf = loaderPrivate; >> + struct dri2_egl_display *dri2_dpy = >> + dri2_egl_display(dri2_surf->base.Resource.Display); > > > dri2_dpy is unused. > > >> + >> + dri2_surf->buffer_count = 1; >> + if (width) >> + *width = dri2_surf->base.Width; >> + if (height) >> + *height = dri2_surf->base.Height; >> + *out_count = dri2_surf->buffer_count;; >> + return dri2_surf->buffers; >> +} >> + >> +#define DRM_RENDER_DEV_NAME "%s/renderD%d" >> + >> +EGLBoolean >> +dri2_initialize_null(_EGLDriver *drv, _EGLDisplay *disp) >> +{ >> + struct dri2_egl_display *dri2_dpy; >> + const char* err; >> + int i, render_node; > > > render_node is unused. > > >> + int driver_loaded = 0; >> + >> + loader_set_logger(_eglLog); >> + >> + dri2_dpy = calloc(1, sizeof *dri2_dpy); >> + if (!dri2_dpy) >> + return _eglError(EGL_BAD_ALLOC, "eglInitialize"); >> + >> + disp->DriverData = (void *) dri2_dpy; >> + >> + const int limit = 64; >> + const int base = 128; >> + for (i = 0; i < limit; ++i) { >> + char *card_path; >> + if (asprintf(&card_path, DRM_RENDER_DEV_NAME, DRM_DIR_NAME, base + >> i) < 0) >> + continue; > > > Manually looping over render node names feels slightly wrong. You could > use udev instead, but that might be overkill. Anyways... I'm unable to > suggest a conclusively better method, so this hunk is ok with me. Yeah, we went over this internally as well, maybe we should hide it in a helper function and change that function once we figure out something better. Stéphane > > >> + for (i = 0; dri2_dpy->driver_configs[i]; i++) { >> + EGLint attr_list[1]; >> + attr_list[0] = EGL_NONE; >> + dri2_add_config(disp, dri2_dpy->driver_configs[i], >> + i + 1, EGL_WINDOW_BIT, attr_list, NULL); > > > The platform doesn't support eglCreateWindowSurface, so why do you set > EGL_WINDOW_BIT here? Is this to work around some limitiation of > dri2_add_config? > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev