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

[Tomasz Figa]

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.

Best regards,
Tomasz