[2/2] egl: Don't add hardware device if there is no render node.

Submitted by Mathias Fröhlich on June 6, 2019, 11:10 a.m.

Details

Message ID 704c6e755a0808c0b5f9ce2a1bc227d0f5e728a5.1559818218.git.Mathias.Froehlich@gmx.net
State New
Headers show
Series "Followups on the just landed EGL_platform_device." ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Mathias Fröhlich June 6, 2019, 11:10 a.m.
From: Mathias Fröhlich <Mathias.Froehlich@gmx.net>

Do not offer a hardware drm backed egl device if no render node
is available. The current implementation will fail on this
egl device. On top it issues a warning that is actually missleading.
There are finally more error paths that can fail on the way to a
hardware backed egl device. Fixing all of them would kind of require
opening the drm device and see if there is a usable driver associated
with the device. The taken approach avoids a full probe and fixes at
least this kind of problem on kvm virtualization hosts I observe here.

Signed-off-by: Mathias Fröhlich <Mathias.Froehlich@web.de>
---
 src/egl/main/egldevice.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

--
2.21.0

Patch hide | download patch | download mbox

diff --git a/src/egl/main/egldevice.c b/src/egl/main/egldevice.c
index 76b8960fa5b..6b791383af5 100644
--- a/src/egl/main/egldevice.c
+++ b/src/egl/main/egldevice.c
@@ -105,12 +105,16 @@  _EGLDevice _eglSoftwareDevice = {
  * Negative value on error, zero if newly added, one if already in list.
  */
 static int
-_eglAddDRMDevice(drmDevicePtr device, _EGLDevice **out_dev)
+_eglAddDRMDevice(drmDevicePtr device, bool also_primary, _EGLDevice **out_dev)
 {
    _EGLDevice *dev;
+   int wanted_nodes = 1 << DRM_NODE_RENDER;

-   if ((device->available_nodes & (1 << DRM_NODE_PRIMARY |
-                                   1 << DRM_NODE_RENDER)) == 0)
+   /* In case we have an fd given initially, we can accept a primary node */
+   if (also_primary)
+      wanted_nodes |= 1 << DRM_NODE_PRIMARY;
+
+   if ((device->available_nodes & wanted_nodes) == 0)
       return -1;

    dev = _eglGlobal.DeviceList;
@@ -176,7 +180,7 @@  _eglAddDevice(int fd, bool software)
    }

    /* Device is not added - error or already present */
-   if (_eglAddDRMDevice(device, &dev) != 0)
+   if (_eglAddDRMDevice(device, true, &dev) != 0)
       drmFreeDevice(&device);
 #else
    _eglLog(_EGL_FATAL, "Driver bug: Built without libdrm, yet looking for HW device");
@@ -273,7 +277,7 @@  _eglRefreshDeviceList(void)

    num_devs = drmGetDevices2(0, devices, ARRAY_SIZE(devices));
    for (int i = 0; i < num_devs; i++) {
-      ret = _eglAddDRMDevice(devices[i], NULL);
+      ret = _eglAddDRMDevice(devices[i], false, NULL);

       /* Device is not added - error or already present */
       if (ret != 0)

Comments

On Thu, 6 Jun 2019 at 12:10, <Mathias.Froehlich@gmx.net> wrote:
>
> From: Mathias Fröhlich <Mathias.Froehlich@gmx.net>
>
> Do not offer a hardware drm backed egl device if no render node
> is available.
As far as I can see current implementation does _not_ add the DRM
device if its missing render node (and a primary one).
Looking at the change below, it's effectively making the primary node optional.

Hence the comment does not alight with the code - old and new. Can you
elaborate?

I have not thought exactly how primary node-less DRM will work out
esp. since the EGL_EXT_device_drm extension explicitly mentions one.

-Emil
Hi Emil,

On Friday, 7 June 2019 15:43:48 CEST Emil Velikov wrote:
> On Thu, 6 Jun 2019 at 12:10, <Mathias.Froehlich@gmx.net> wrote:
> >
> > From: Mathias Fröhlich <Mathias.Froehlich@gmx.net>
> >
> > Do not offer a hardware drm backed egl device if no render node
> > is available.
> As far as I can see current implementation does _not_ add the DRM
> device if its missing render node (and a primary one).
> Looking at the change below, it's effectively making the primary node optional.
> 
> Hence the comment does not alight with the code - old and new. Can you
> elaborate?

The currently pushed implementation asks drmGetDevices2 for a list of
devices and all devices with either a render node or a master node or both
are added as hardware device. This check is the bitmasking test with
device->available_nodes that you have put near the top of _eglAddDRMDevice.
So, if there is no render node the hardware device is added.
Later on the filename of the render node as returned from
_eglGetDRMDeviceRenderNode is opened which does not succeed in that case.
egInitialize fails then which is not nice.

Past my change, a pure hardware device is not added if there is no render node.
That is decided by this above mentioned bitmask test.

The codepath that adds devices via _eglAddDevice from all the platforms is
untouched as this still uses the same bitmask as before.
I did not check this code path specifically above the call to _eglAddDevice
as this patch does not change the behavior of this case.

best

Mathias

> 
> I have not thought exactly how primary node-less DRM will work out
> esp. since the EGL_EXT_device_drm extension explicitly mentions one.
> 
> -Emil
>
On Monday, 10 June 2019 12:11:40 CEST Mathias Fröhlich wrote:
> Hi Emil,
> 
> On Friday, 7 June 2019 15:43:48 CEST Emil Velikov wrote:
> > On Thu, 6 Jun 2019 at 12:10, <Mathias.Froehlich@gmx.net> wrote:
> > >
> > > From: Mathias Fröhlich <Mathias.Froehlich@gmx.net>
> > >
> > > Do not offer a hardware drm backed egl device if no render node
> > > is available.
> > As far as I can see current implementation does _not_ add the DRM
> > device if its missing render node (and a primary one).
> > Looking at the change below, it's effectively making the primary node optional.
> > 
> > Hence the comment does not alight with the code - old and new. Can you
> > elaborate?
> 
> The currently pushed implementation asks drmGetDevices2 for a list of
> devices and all devices with either a render node or a master node or both
> are added as hardware device. This check is the bitmasking test with
> device->available_nodes that you have put near the top of _eglAddDRMDevice.
> So, if there is no render node the hardware device is added.
> Later on the filename of the render node as returned from
> _eglGetDRMDeviceRenderNode is opened which does not succeed in that case.
> egInitialize fails then which is not nice.
> 
> Past my change, a pure hardware device is not added if there is no render node.
> That is decided by this above mentioned bitmask test.
> 
> The codepath that adds devices via _eglAddDevice from all the platforms is
> untouched as this still uses the same bitmask as before.
> I did not check this code path specifically above the call to _eglAddDevice
> as this patch does not change the behavior of this case.
> 
> best
> 
> Mathias
> 
> > 
> > I have not thought exactly how primary node-less DRM will work out
> > esp. since the EGL_EXT_device_drm extension explicitly mentions one.

Emil,

ok, now I see. You also want the master node and bail out if that is not there.

The problem is that the current EGLDevice code *needs* a user accessible
render node to function, but does not enforce it to be there. It just says if we
have either a render node or a master node or both go ahead.

Your bitmaks test needs to read

int wanted_nodes =(1 << DRM_NODE_PRIMARY |
                                   1 << DRM_NODE_RENDER);
if ((device->available_nodes & wanted_nodes) != wanted_nodes)
   return -1;

if you want to have *both* bits set for a hardware device.
Means with your current pushed code the master node is 'optional' as well!!

From the use case that mostly drove the device extension
originally - put the EGL_EXT_device_drm extension away - do make sense
once you have a user accessible render node device file with a mesa driver
that the loader can access. No matter if there is a master device file and no matter
what (historic?) statement about a master node device file the
EGL_EXT_device_drm makes ...

Also, if the master node is returned from libdrm, it is returned from the
string query function. Is that sufficient?

Can we make the EGL_EXT_device_drm optional then? Means render node
accessible by user and driver name returned by libdrm results in an EGLdevice
ready to use.

best

Mathias

> > 
> > -Emil
> > 
> 
> 
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>