[Mesa-dev] [PATCH v2 3/8] egl: add EGL_EXT_device_drm support

[Mathias Fröhlich]

Hi Emil,

There are some comments inline below:

On Tuesday, 4 September 2018 20:33:00 CEST Emil Velikov wrote:
> From: Emil Velikov <emil.velikov at collabora.com>
> 
> Add implementation based around the drmDevice API. As such it's only
> available only when building with libdrm. With the latter already a
> requirement when using !SW code paths in the platform code.
> 
> Note: the current code will work if a device is hot-plugged. Yet
> hot-unplugged is not implemented, since I have no ways of testing it.
> 
> v2:
>  - ddd some _eglDeviceSupports checks
>  - require DRM_NODE_RENDER
>  - add _eglGetDRMDeviceRenderNode helper
> 
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
>  src/egl/main/egldevice.c | 113 +++++++++++++++++++++++++++++++++++++++
>  src/egl/main/egldevice.h |   4 ++
>  2 files changed, 117 insertions(+)
> 
> diff --git a/src/egl/main/egldevice.c b/src/egl/main/egldevice.c
> index 1d67fd71191..6bd584e025b 100644
> --- a/src/egl/main/egldevice.c
> +++ b/src/egl/main/egldevice.c
> @@ -25,10 +25,14 @@
>   *
>   **************************************************************************/
>  
> +#ifdef HAVE_LIBDRM
> +#include <xf86drm.h>
> +#endif
>  #include "util/macros.h"
>  
>  #include "eglcurrent.h"
>  #include "egldevice.h"
> +#include "egllog.h"
>  #include "eglglobals.h"
>  #include "egltypedefs.h"
>  
> @@ -39,6 +43,11 @@ struct _egl_device {
>     const char *extensions;
>  
>     EGLBoolean MESA_device_software;
> +   EGLBoolean EXT_device_drm;
> +
> +#ifdef HAVE_LIBDRM
> +   drmDevicePtr device;
> +#endif
>  };
>  
>  void
> @@ -60,6 +69,10 @@ _eglFiniDevice(void)
>        dev = dev_list;
>        dev_list = dev_list->Next;
>  
> +#ifdef HAVE_LIBDRM
> +      assert(!_eglDeviceSupports(dev_list, _EGL_DEVICE_DRM));

You probably mean dev instead of dev_list also the ! is probably wrong:

    assert(_eglDeviceSupports(dev, _EGL_DEVICE_DRM));


> +      drmFreeDevice(&dev->device);
> +#endif
>        free(dev);
>     }
>  
> @@ -87,6 +100,55 @@ _EGLDevice _eglSoftwareDevice = {
>     .MESA_device_software = EGL_TRUE,
>  };
>  
> +#ifdef HAVE_LIBDRM
> +/*
> + * Negative value on error, zero if newly added, one if already in list.
> + */
> +static int
> +_eglAddDRMDevice(drmDevicePtr device, _EGLDevice **out_dev)
> +{
> +   _EGLDevice *dev;
> +
> +   if ((device->available_nodes & (1 << DRM_NODE_PRIMARY |
> +                                   1 << DRM_NODE_RENDER)) == 0)
> +      return -1;
> +
> +   dev = _eglGlobal.DeviceList;
> +
> +   /* The first device is always software */
> +   assert(!dev);
> +   assert(!_eglDeviceSupports(dev, _EGL_DEVICE_SOFTWARE));
> +
> +   while (dev->Next) {
> +      dev = dev->Next;
> +
> +      assert(!_eglDeviceSupports(dev_list, _EGL_DEVICE_DRM));

That one does not compile with assertions enabled.
There is no dev_list.
Also the ! is probably wrong.






> +      if (drmDevicesEqual(device, dev->device) != 0) {
> +         if (out_dev)
> +            *out_dev = dev;
> +         return 1;
> +      }
> +   }
> +
> +   dev->Next = calloc(1, sizeof(_EGLDevice));
> +   if (!dev->Next) {
> +      if (out_dev)
> +         *out_dev = NULL;
> +      return -1;
> +   }
> +
> +   dev = dev->Next;
> +   dev->extensions = "EGL_EXT_device_drm";
> +   dev->EXT_device_drm = EGL_TRUE;
> +   dev->device = device;
> +
> +   if (out_dev)
> +      *out_dev = dev;
> +
> +   return 0;
> +}
> +#endif
> +
>  /* Adds a device in DeviceList, if needed for the given fd.
>   *
>   * If a software device, the fd is ignored.
> @@ -105,7 +167,21 @@ _eglAddDevice(int fd, bool software)
>     if (software)
>        goto out;
>  
> +#ifdef HAVE_LIBDRM
> +   drmDevicePtr device;
> +
> +   if (drmGetDevice2(fd, 0, &device) != 0) {
> +      dev = NULL;
> +      goto out;
> +   }
> +
> +   /* Device is not added - error or already present */
> +   if (_eglAddDRMDevice(device, &dev) != 0)
> +      drmFreeDevice(&device);
> +#else
> +   _eglLog(_EGL_FATAL, "Driver bug: Built without libdrm, yet looking for HW device");
>     dev = NULL;
> +#endif
>  
>  out:
>     mtx_unlock(_eglGlobal.Mutex);
> @@ -118,12 +194,26 @@ _eglDeviceSupports(_EGLDevice *dev, _EGLDeviceExtension ext)
>     switch (ext) {
>     case _EGL_DEVICE_SOFTWARE:
>        return dev->MESA_device_software;
> +   case _EGL_DEVICE_DRM:
> +      return dev->EXT_device_drm;
>     default:
>        assert(0);
>        return EGL_FALSE;
>     };
>  }
>  
> +/* Ideally we'll have an extension which passes the render node,
> + * instead of the card one + magic.
> + *
> + * Then we can move this in _eglQueryDeviceStringEXT below. Until then
> + * keep it separate.
> + */
> +const char *
> +_eglGetDRMDeviceRenderNode(_EGLDevice *dev)
> +{
> +   return dev->device->nodes[DRM_NODE_RENDER];
> +}
> +
>  EGLBoolean
>  _eglQueryDeviceAttribEXT(_EGLDevice *dev, EGLint attribute,
>                           EGLAttrib *value)
> @@ -141,6 +231,12 @@ _eglQueryDeviceStringEXT(_EGLDevice *dev, EGLint name)
>     switch (name) {
>     case EGL_EXTENSIONS:
>        return dev->extensions;
> +#ifdef HAVE_LIBDRM
> +   case EGL_DRM_DEVICE_FILE_EXT:
> +      if (_eglDeviceSupports(dev, _EGL_DEVICE_DRM))
> +         return dev->device->nodes[DRM_NODE_PRIMARY];
... we probably want 
return _eglGetDRMDeviceRenderNode(dev);

best
Mathias

> +      /* fall through */
> +#endif
>     default:
>        _eglError(EGL_BAD_PARAMETER, "eglQueryDeviceStringEXT");
>        return NULL;
> @@ -167,6 +263,23 @@ _eglRefreshDeviceList(void)
>     assert(!_eglDeviceSupports(dev, _EGL_DEVICE_SOFTWARE));
>     count++;
>  
> +#ifdef HAVE_LIBDRM
> +   drmDevicePtr devices[64];
> +   int num_devs, ret;
> +
> +   num_devs = drmGetDevices2(0, devices, ARRAY_SIZE(devices));
> +   for (int i = 0; i < num_devs; i++) {
> +      ret = _eglAddDRMDevice(devices[i], NULL);
> +
> +      /* Device is not added - error or already present */
> +      if (ret != 0)
> +         drmFreeDevice(&devices[i]);
> +
> +      if (ret >= 0)
> +         count++;
> +   }
> +#endif
> +
>     return count;
>  }
>  
> diff --git a/src/egl/main/egldevice.h b/src/egl/main/egldevice.h
> index 74b5ddeee5c..ddcdcd17f5a 100644
> --- a/src/egl/main/egldevice.h
> +++ b/src/egl/main/egldevice.h
> @@ -60,6 +60,7 @@ _eglAddDevice(int fd, bool software);
>  
>  enum _egl_device_extension {
>     _EGL_DEVICE_SOFTWARE,
> +   _EGL_DEVICE_DRM,
>  };
>  
>  typedef enum _egl_device_extension _EGLDeviceExtension;
> @@ -67,6 +68,9 @@ typedef enum _egl_device_extension _EGLDeviceExtension;
>  EGLBoolean
>  _eglDeviceSupports(_EGLDevice *dev, _EGLDeviceExtension ext);
>  
> +const char *
> +_eglGetDRMDeviceRenderNode(_EGLDevice *dev);
> +
>  EGLBoolean
>  _eglQueryDeviceAttribEXT(_EGLDevice *dev, EGLint attribute,
>                           EGLAttrib *value);
>