[Mesa-dev] [PATCH v2] egl/android: rework device probing

[Tomasz Figa]

On Mon, Sep 3, 2018 at 2:45 PM Tomasz Figa <tfiga at chromium.org> wrote:
>
> Hi Emil,
>
> On Sat, Sep 1, 2018 at 2:03 AM Emil Velikov <emil.l.velikov at gmail.com> wrote:
> >
> > From: Emil Velikov <emil.velikov at collabora.com>
> >
>
> Thanks for the patch! Please see my comments below.
>
> [snip]
> > @@ -1214,10 +1215,13 @@ droid_open_device_drm_gralloc(struct dri2_egl_display *dri2_dpy)
> >                                            &fd);
> >     if (err || fd < 0) {
> >        _eglLog(_EGL_WARNING, "fail to get drm fd");
> > -      fd = -1;
> > +      return EGL_FALSE;
> >     }
> >
> > -   return (fd >= 0) ? fcntl(fd, F_DUPFD_CLOEXEC, 3) : -1;
> > +   if (dri2_dpy->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3) < 0)
>
> This will assign the boolean result of the comparison to dri2_dpy->fd.
> To avoid parenthesis hell, I'd rewrite this to:
>
> dri2_dpy->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
> if (dri2_dpy->fd < 0)
>    return EGL_FALSE;
>
> > +     return EGL_FALSE;
> > +
> > +   return droid_probe_device(disp);
> >  }
> >  #endif /* HAVE_DRM_GRALLOC */
> >
> > @@ -1365,7 +1369,7 @@ static const __DRIextension *droid_image_loader_extensions[] = {
> >  EGLBoolean
> >  droid_load_driver(_EGLDisplay *disp)
>
> Not related to this patch, but I guess we could fix it up, while at
> it. Fails to build with:
>
> src/egl/drivers/dri2/platform_android
> .c:1369:1: error: no previous prototype for function
> 'droid_load_driver' [-Werror,-Wmissing-prototypes]
> droid_load_driver(_EGLDisplay *disp)
> ^
>
> The function should be static.
>
> >  {
> > -   struct dri2_egl_display *dri2_dpy = disp->DriverData;
> > +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> >     const char *err;
> >
> >     dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
> > @@ -1404,6 +1408,17 @@ error:
> >     return false;
> >  }
> [snip]
> > +static EGLBoolean
> >  droid_open_device(_EGLDisplay *disp)
> >  {
> >  #define MAX_DRM_DEVICES 32
> > +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> >     drmDevicePtr device, devices[MAX_DRM_DEVICES] = { NULL };
> >     int prop_set, num_devices;
>
> Not related to this patch, but prop_set is unused. We could add a
> fixup in this patch, while reworking this already.
>
> I'm going to test it on Chrome OS with the fixups above applied.

The probing itself seems to be working fine, but it looks like there
is some EGL image regression in master, which I don't have time to
investigate.

Tested without the vendor property set, with both cases of the desired
device being probed in first or some later iteration. In both cases
the initialization succeeded and I could see the right GL renderer
string.

Tested-by: Tomasz Figa <tfiga at chromium.org>

Best regards,
Tomasz