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

[Emil Velikov]

On 30 August 2018 at 10:54, Mathias Fröhlich <Mathias.Froehlich at gmx.net> wrote:
> 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.
>
Precisely.

>> > 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'.
>
Correct. We had a number of bugs (one of which breaking MT apps) that
makes me cautious.

> 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?
>
You're correct - there isn't one. I should have checked the spec
because speaking.

> Anyhow, IMO it must not crash with a null pointer dereference even with the
> first patch series.
>
Agreed - will need to take a closer look why things crash, while the
piglit test works.
Might even add a piglit subtest.

>> > 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.
>
IIRC when someone gets logged in systemd/logind/others sets up the ACL
-  both ownership and permissions.
So an open(/dev/dri/card0) will be fine - be that from tty or the DE
terminal emulator.

Can you share a brief how-to reproduce?

>
>> > 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...
>
There is some extremely minuscule chance of that change causing issues.
So I'll just pull those out - making the series a bit shorter ;-)

>> > 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.
>
Almost - about half the EGL platforms support swrast. Since all
platforms support DRM I'm planning to move the enablement after that.

>> 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 ...
>
Ouch. Hope you have backups for your important bits.

-Emil