[Mesa-dev] [PATCH v2 2/8] egl: add EGL_MESA_device_software support

[Mathias Fröhlich]

Hi Emil,

Some comments inline below:

On Tuesday, 4 September 2018 20:32:59 CEST Emil Velikov wrote:
> From: Emil Velikov <emil.velikov at collabora.com>
> 
> Add a plain software device, which is always available.
> 
> We can safely assign it as the first/initial device in _eglGlobals,
> although we ensure that's the case with a handful of _eglDeviceSupports
> checks throughout the code.
> 
> v2:
>  - s/_eglFindDevice/_eglAddDevice/ (Eric)
>  - s/_eglLookupAllDevices/_eglRefreshDeviceList/ (Eric)
>  - move ^^ helpers into a earlier patch (Eric, Mathias)
>  - set the SW device on _eglGlobal init. (Eric)
>  - add a number of _eglDeviceSupports checks (Mathias)
>  - split Device/Display attach to a separate patch
> 
> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> ---
>  src/egl/main/egldevice.c  | 27 +++++++++++++++++++++++++++
>  src/egl/main/egldevice.h  |  4 +++-
>  src/egl/main/eglglobals.c |  1 +
>  3 files changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/src/egl/main/egldevice.c b/src/egl/main/egldevice.c
> index bbc9f2060d1..1d67fd71191 100644
> --- a/src/egl/main/egldevice.c
> +++ b/src/egl/main/egldevice.c
> @@ -37,6 +37,8 @@ struct _egl_device {
>     _EGLDevice *Next;
>  
>     const char *extensions;
> +
> +   EGLBoolean MESA_device_software;
>  };
>  
>  void
> @@ -47,6 +49,12 @@ _eglFiniDevice(void)
>     /* atexit function is called with global mutex locked */
>  
>     dev_list = _eglGlobal.DeviceList;
> +
> +   /* The first device is on-stack allocated SW device */

May be I name that wrong as non native english, but 'on-stack'
would be something different for me. The stack is more or less
the function local allocation scope.

The sw device is much more a 'static allocated SW device'.

Right?




The following asserts just flag in some of my tests:

> +   assert(!dev_list);
> +   assert(!_eglDeviceSupports(dev_list, _EGL_DEVICE_SOFTWARE));

Looking at the comment above I assume you mean:

   assert(dev_list);
   assert(_eglDeviceSupports(dev_list, _EGL_DEVICE_SOFTWARE));




> +   dev_list = dev_list->Next;
> +
>     while (dev_list) {
>        /* pop list head */
>        dev = dev_list;
> @@ -74,6 +82,11 @@ _eglCheckDeviceHandle(EGLDeviceEXT device)
>     return (cur != NULL);
>  }
>  
> +_EGLDevice _eglSoftwareDevice = {
> +   .extensions = "EGL_MESA_device_software",
> +   .MESA_device_software = EGL_TRUE,
> +};
> +
>  /* Adds a device in DeviceList, if needed for the given fd.
>   *
>   * If a software device, the fd is ignored.
> @@ -84,6 +97,13 @@ _eglAddDevice(int fd, bool software)
>     _EGLDevice *dev;
>  
>     mtx_lock(_eglGlobal.Mutex);
> +   dev = _eglGlobal.DeviceList;
> +
> +   /* The first device is always software */
> +   assert(!dev);
> +   assert(!_eglDeviceSupports(dev, _EGL_DEVICE_SOFTWARE));

The same two asserts with the probably inverted logic like above.



> +   if (software)
> +      goto out;
>  
>     dev = NULL;
>  
> @@ -96,6 +116,8 @@ EGLBoolean
>  _eglDeviceSupports(_EGLDevice *dev, _EGLDeviceExtension ext)
>  {
>     switch (ext) {
> +   case _EGL_DEVICE_SOFTWARE:
> +      return dev->MESA_device_software;
>     default:
>        assert(0);
>        return EGL_FALSE;
> @@ -140,6 +162,11 @@ _eglRefreshDeviceList(void)
>  
>     dev = _eglGlobal.DeviceList;
>  
> +   /* The first device is always software */
> +   assert(!dev);
> +   assert(!_eglDeviceSupports(dev, _EGL_DEVICE_SOFTWARE));

... and again.

best
Mathias

> +   count++;
> +
>     return count;
>  }
>  
> diff --git a/src/egl/main/egldevice.h b/src/egl/main/egldevice.h
> index 72d250393f4..74b5ddeee5c 100644
> --- a/src/egl/main/egldevice.h
> +++ b/src/egl/main/egldevice.h
> @@ -38,6 +38,8 @@
>  extern "C" {
>  #endif
>  
> +extern _EGLDevice _eglSoftwareDevice;
> +
>  void
>  _eglFiniDevice(void);
>  
> @@ -57,7 +59,7 @@ _EGLDevice *
>  _eglAddDevice(int fd, bool software);
>  
>  enum _egl_device_extension {
> -   EGL_FOOBAR,
> +   _EGL_DEVICE_SOFTWARE,
>  };
>  
>  typedef enum _egl_device_extension _EGLDeviceExtension;
> diff --git a/src/egl/main/eglglobals.c b/src/egl/main/eglglobals.c
> index ac8bb3f328a..db81fcaf2b5 100644
> --- a/src/egl/main/eglglobals.c
> +++ b/src/egl/main/eglglobals.c
> @@ -52,6 +52,7 @@ struct _egl_global _eglGlobal =
>  {
>     .Mutex = &_eglGlobalMutex,
>     .DisplayList = NULL,
> +   .DeviceList = &_eglSoftwareDevice,
>     .NumAtExitCalls = 3,
>     .AtExitCalls = {
>        /* default AtExitCalls, called in reverse order */
>