[Mesa-dev] [PATCH 1/2] egl/dri2: implement platform_null (v2).

[Emil Velikov]

On 04/04/15 02:18, Stéphane Marchesin wrote:
> 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.
> 
Seconded - the way we manage/open the device nodes is not too pretty.
Although I would kindly urge you against going the libudev road.

We had some problems with using it previously (some people ship another
version of the library), plus it results in more code :\

Cheers,
Emil