egl: Don't add hardware device if there is no render node v2.

Submitted by Mathias Fröhlich on June 17, 2019, 10:36 a.m.

Details

Message ID d22d9b0dead110c70df0c8b909a4c5cf837ae48d.1560767564.git.Mathias.Froehlich@gmx.net
State New
Headers show
Series "Followups on the just landed EGL_platform_device." ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

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


Emil,

that one probably matches your original intent then.

please review
Thanks

Mathias


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 | 4 ++--
 1 file changed, 2 insertions(+), 2 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..99d8a6c1886 100644
--- a/src/egl/main/egldevice.c
+++ b/src/egl/main/egldevice.c
@@ -108,9 +108,9 @@  static int
 _eglAddDRMDevice(drmDevicePtr device, _EGLDevice **out_dev)
 {
    _EGLDevice *dev;
+   const int wanted_nodes = 1 << DRM_NODE_RENDER | 1 << DRM_NODE_PRIMARY;

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

    dev = _eglGlobal.DeviceList;

Comments

On 2019/06/17, Mathias.Froehlich@gmx.net wrote:
> From: Mathias Fröhlich <Mathias.Froehlich@gmx.net>
> 
> 
> Emil,
> 
> that one probably matches your original intent then.
> 
> please review
> Thanks
> 
> Mathias
> 
> 
> 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.
> 
Thanks

Fixes: dbb4457d985 ("egl: add EGL_EXT_device_drm support")
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

-Emil
On Mon, Jun 17, 2019 at 6:37 AM <Mathias.Froehlich@gmx.net> wrote:
>
> From: Mathias Fröhlich <Mathias.Froehlich@gmx.net>
>
>
> Emil,
>
> that one probably matches your original intent then.
>
> please review
> Thanks
>
> Mathias
>
>
> 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 | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/egl/main/egldevice.c b/src/egl/main/egldevice.c
> index 76b8960fa5b..99d8a6c1886 100644
> --- a/src/egl/main/egldevice.c
> +++ b/src/egl/main/egldevice.c
> @@ -108,9 +108,9 @@ static int
>  _eglAddDRMDevice(drmDevicePtr device, _EGLDevice **out_dev)
>  {
>     _EGLDevice *dev;
> +   const int wanted_nodes = 1 << DRM_NODE_RENDER | 1 << DRM_NODE_PRIMARY;

Why check for primary? Why require both at this point? I guess the
original intent was to require render *or* primary... but some devices
will not have a primary node if they don't have any outputs...

>
> -   if ((device->available_nodes & (1 << DRM_NODE_PRIMARY |
> -                                   1 << DRM_NODE_RENDER)) == 0)
> +   if ((device->available_nodes & wanted_nodes) != wanted_nodes)
>        return -1;
>
>     dev = _eglGlobal.DeviceList;
> --
> 2.21.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Mon, 17 Jun 2019 at 19:16, Ilia Mirkin <imirkin@alum.mit.edu> wrote:
>
> On Mon, Jun 17, 2019 at 6:37 AM <Mathias.Froehlich@gmx.net> wrote:
> >
> > From: Mathias Fröhlich <Mathias.Froehlich@gmx.net>
> >
> >
> > Emil,
> >
> > that one probably matches your original intent then.
> >
> > please review
> > Thanks
> >
> > Mathias
> >
> >
> > 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 | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/egl/main/egldevice.c b/src/egl/main/egldevice.c
> > index 76b8960fa5b..99d8a6c1886 100644
> > --- a/src/egl/main/egldevice.c
> > +++ b/src/egl/main/egldevice.c
> > @@ -108,9 +108,9 @@ static int
> >  _eglAddDRMDevice(drmDevicePtr device, _EGLDevice **out_dev)
> >  {
> >     _EGLDevice *dev;
> > +   const int wanted_nodes = 1 << DRM_NODE_RENDER | 1 << DRM_NODE_PRIMARY;
>
> Why check for primary? Why require both at this point? I guess the
> original intent was to require render *or* primary... but some devices
> will not have a primary node if they don't have any outputs...
>
My original intent was to require both, although I got it wrong.

Why we require both - simple the spec more or less mandates a card/primary node.
Since there an be other software running hence getting authenticated
will be a pita and primary nodes are not accessible* render node seems
like the only robust solution.

HTH
Emil
* There's a bit more to it:
Users not in the video group and w/o systemd - opening the primary
node will fail with EPERM.
Platforms running systemd - logind relaxes the primary node
permission, only when a local user has logged in.
On Monday, 17 June 2019 19:33:26 CEST Emil Velikov wrote:
> On 2019/06/17, Mathias.Froehlich@gmx.net wrote:
> > From: Mathias Fröhlich <Mathias.Froehlich@gmx.net>
> > 
> > 
> > Emil,
> > 
> > that one probably matches your original intent then.
> > 
> > please review
> > Thanks
> > 
> > Mathias
> > 
> > 
> > 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.
> > 
> Thanks
> 
> Fixes: dbb4457d985 ("egl: add EGL_EXT_device_drm support")
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

Thanks!
Pushed that!

best
Mathias

> 
> -Emil
>