[Mesa-dev] [RFC 00/10] Let us welcome EGLDevice

[Mathias Fröhlich]

Hi Emil,

On Friday, 3 August 2018 14:44:16 CEST Emil Velikov wrote:
> Hi all,
> 
> This series implements the following extensions:
>  - EGL_EXT_device_base
>  - EGL_MESA_device_software
>  - EGL_EXT_device_drm
>  - EGL_platform_device
> 
> As you know the APIs are used to enumerate, select and use EGLDevices.
> The second extension (proposed by Ajax) defines a 'software' device,
> alongside the existing DRM ones.
> 
> While there are many usecases for this work, my primary interest is
> allowing device selection, wrt testing. To achieve the goal, we would
> need to finalise EGL_EXT_explicit_device (also proposed by Ajax).
> 
> Piglit patches will be sent out shortly.
> 
> Any feedback is greatly appreciated.

>From a higher point of view the approach looks good.

To sum up, you basically associate an _EGLDevice with each _EGLDisplay
where the _EGLDevice only contains the meta information where to
find the device and the _EGLDisplay later contains open file descriptors
to work with ...

Nevertheless, running the tests and proof of concept programs that I used
back in the day brought up some questions and one crash.

At first the Crash: The attached eglcontext-pbuffer.c which goes the pbuffer approach
instead of going surfaceless just dumps core in pbuffer creation using
the patch series. I believe that it is legal what the program does, but may
be you want to double check that too.

Then if I use the patch series on an account that has no DISPLAY set and no
'display server' running, eglInitialize fails in device_probe_device due to at first
opening the '.../card0' device and bailing out when this does not work.
Means the current patch series goes via opening the primary node which
shall not be accessible by everyone and from that derives the rendernode
device which is finally used for _EGLDisplay initialization.
Can we alternatively go directly to the rendernode in some way?

For patch #7, can you explain why dri already provides the right format?
It's probably correct, but I am missing some bits of that context creation
big picture to give a review.

And finally, in patch #2 you mention that you want to avoid larger patches
and the announced extensions are not yet working as expected.
May be you can announce the extensions in a separate patch that follows
all the implementation patches? That probably helps people that want to
bisect something using piglit.

thanks for approaching this topic!

best
Mathias


> 
> Thanks
> Emil
> 
> Emil Velikov (9):
>   egl: add simple EGL_EXT_device_base implementation
>   egl: add EGL_MESA_device_software support
>   egl: add EGL_EXT_device_drm support
>   meson: egl: group dri2 bits separately from haiku
>   egl/surfaceless: inline surfaceless_alloc_image()
>   egl/surfaceless: honour the format passed to getBuffers
>   egl/surfaceless: remove no longer used dri2_egl_surface::visual
>   egl: add optional plat_opt to _eglFindDisplay()
>   egl: add EGL_platform_device support
> 
> Jonny Lamb (1):
>   egl: add initial boilerplate for EGL_EXT_device_base
> 
>  src/egl/Makefile.am                         |   3 +
>  src/egl/Makefile.sources                    |   2 +
>  src/egl/drivers/dri2/egl_dri2.c             |   3 +
>  src/egl/drivers/dri2/egl_dri2.h             |  13 +-
>  src/egl/drivers/dri2/platform_android.c     |   9 +
>  src/egl/drivers/dri2/platform_device.c      | 380 ++++++++++++++++++++
>  src/egl/drivers/dri2/platform_drm.c         |   9 +
>  src/egl/drivers/dri2/platform_surfaceless.c |  37 +-
>  src/egl/drivers/dri2/platform_wayland.c     |  18 +
>  src/egl/drivers/dri2/platform_x11.c         |  27 ++
>  src/egl/drivers/haiku/egl_haiku.cpp         |   8 +
>  src/egl/main/eglapi.c                       |  79 +++-
>  src/egl/main/egldevice.c                    | 306 ++++++++++++++++
>  src/egl/main/egldevice.h                    |  83 +++++
>  src/egl/main/egldisplay.c                   | 119 +++++-
>  src/egl/main/egldisplay.h                   |  14 +-
>  src/egl/main/eglentrypoint.h                |   4 +
>  src/egl/main/eglglobals.c                   |  10 +-
>  src/egl/main/eglglobals.h                   |   2 +
>  src/egl/main/egltypedefs.h                  |   2 +
>  src/egl/meson.build                         |  68 ++--
>  21 files changed, 1120 insertions(+), 76 deletions(-)
>  create mode 100644 src/egl/drivers/dri2/platform_device.c
>  create mode 100644 src/egl/main/egldevice.c
>  create mode 100644 src/egl/main/egldevice.h
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: eglcontext-pbuffer.c
Type: text/x-csrc
Size: 7848 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180828/5148c6d0/attachment.c>