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

[Mathias Fröhlich]

On Tuesday, 28 August 2018 16:22:33 CEST Emil Velikov wrote:
> > 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 ...
> 
> It's the other way around - I associate each EGLDIsplay with a given
> EGLDevice. The EGL_EXT_device_enumeration spec removed that (imho) bogus
> relation:
> 
>     "Because the EGLDeviceEXT is a property of <dpy>, any use of an
>     associated EGLDeviceEXT after <dpy> has been terminated gives
>     undefined results. Querying an EGL_DEVICE_EXT from <dpy> after a
>     call to eglTerminate() (and subsequent re-initialization) may
>     return a different value."

Ok, wording difference. The _EGLDisplay has a member that points to an 
_EGLDevice. So each display has a device.
But the list of _EGLDevices exists independent of the list of _EGLDisplays.
Sounds plausible to me.

> > 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.
> 
> Off the top of my head, I think that's due to eglCreatePbufferSurface
> instead of eglCreatePlatformPbufferSurfaceEXT.
> I think we could wire that legacy API up, although the whole thing is
> _really_ fiddly.
>
> See my piglit patch 85e3b32b3260184853ec20b938574c26a0f39dd8 for some
> details.

Can you explain that in more detail?

Don't we have a initialized _EGLDisplay in our hands that does not require any 
further 'guessing of the underlying platform'.

May be I am missing the latest development in this area, but I could not find 
any pointer to eglCreatePlatformPbufferSurfaceEXT in the wild. May be you can 
help out here?

Anyhow, IMO it must not crash with a null pointer dereference even with the 
first patch series.

> > 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?
> 
> I think we could. Can you elaborate a bit more or provide an example
> on the topic though.
> I'm quite curious what's happening here - is there no card0 node,
> open() fails, other?

It's just the file permissions:

$ ls -l /dev/dri/{card*,render*}
crw-rw----+ 1 root video  226,   0 Aug 30 08:28 /dev/dri/card0
crw-rw-rw-  1 root render 226, 128 Aug 30 08:28 /dev/dri/renderD128

which is pretty much what I expect from the basic idea of render nodes.
As long as you are the one logged into the console you can access card0 via an 
dynamically added acl. But if you are not logged into the console, which is at 
least one of the major driving use cases of the device enumeration extension, 
the current implementation fails with opening card0.


> > 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.
> 
> The format used when to create a surface is passed to the driver,
> which then calls back the loader with the exact same one.
> Admittedly there's a ton of conversion back and forth (within the
> driver) so it's not always clear.
> 
> Pretty much every other platform respects the provided format, hence
> my cleanup patches ;-)
> That said, I'm perfectly fine with pulling those patches (and updating
> platform_device.c) if it will make things easier.

Thanks for that explanation.
I see, it is the getBuffers call that routes that back into the loader.
This is to align with the behavior of loader_dri3_get_buffers.
I am quite sure there are others here that can way better judge about possible 
sideeffects of such changes. The change sounds plausible to me and if this 
would be the only thing that holds back EGLDevices I can give my name for 
that...

> > 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.
> 
> It's actually patch 1/10 which "enables" the extensions.
Yes, may be put that hunk into a patch 3.5/10.
Then the swrast device would just fulfill all the needs at first and 
everything should bisect piglit runs fine.

> You're right though - we could merge the 1/10 boilerplate with 2/10
> and enable the extensions once the SW _and_ DRM extensions have
> landed.
> Might even a) split the helpers from 3/10 into a prep patch and b) add
> the DRM _eglFindDevice instances of 3/10 into the DRM patch - 4/10.

I am fine with that plan, as it avoids false negatives in bisecting using 
piglit results.


> Another thing that comes to mind is ... minimise the first device is
> always SW assumption.
> That would involve adding a couple of _eglDeviceSupports(dev,
> _EGL_DEVICE_DRM) calls. It should make things less magical and clearer
> for the next EGL device extension.
Yes, I would also appreciate that.
I did not want to put that as a requirement, but if you have less places that 
needs to know about the swrast device and its position in the list of 
_EGLDevices it would be great!

> FTR swrast with platform_device is still WIP - I've been working
> fixing up swrast recently. It requires a ton of refactoring and fixes
> ;-)
> 
> > thanks for approaching this topic!
> 
> Thank you for helping out with review and questions.
> 
> I'll try to find time and respin the series tomorrow.

Well, basically I lost my development platform for mesa on monday. The 
replacement ssd is on the way, but until then, I am basically bound to write 
mails instead of compiling and testing ...
So, no hurry from my side ...

Best

Mathias