[Mesa-dev] [PATCH 1/2] egl/dri2: implement platform_null (v2).
Stéphane Marchesin
stephane.marchesin at gmail.com
Fri Apr 3 19:18:35 PDT 2015
On Fri, Apr 3, 2015 at 1:35 PM, Chad Versace <chad.versace at 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 <hshi at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list