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

2018-09-04 Thread Tomasz Figa 
On Mon, Sep 3, 2018 at 3:25 PM Tomasz Figa  wrote:
>
> On Mon, Sep 3, 2018 at 2:45 PM Tomasz Figa  wrote:
> >
> > Hi Emil,
> >
> > On Sat, Sep 1, 2018 at 2:03 AM Emil Velikov  
> > wrote:
> > >
> > > From: Emil Velikov 
> > >
> >
> > 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)
> > >);
> > > 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.

Never mind, it looks like I had libdrm in some weird state. After a
complete clean build, it worked fine.

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-09-03 Thread Tapani Pälli 

I've tested that Android Celadon continues to work with these changes ..

Tested-by: Tapani Pälli 

On 08/31/2018 07:59 PM, Emil Velikov wrote:

From: Emil Velikov 

Unlike the other platforms, here we aim do guess if the device that we
somewhat arbitrarily picked, is supported or not.

In particular: when a vendor is _not_ requested we loop through all
devices, picking the first one which can create a DRI screen.

When a vendor is requested - we use that and do _not_ fall-back to any
other device.

The former seems a bit fiddly, but considering EGL_EXT_explicit_device and
EGL_MESA_query_renderer are MIA, this is the best we can do for the
moment.

With those (proposed) extensions userspace will be able to create a
separate EGL display for each device, query device details and make the
conscious decision which one to use.

v2:
  - update droid_open_device_drm_gralloc()
  - set the dri2_dpy->fd before using it
  - return a EGLBoolean for droid_{probe,open}_device*
  - do not warn on droid_load_driver failure (Tomasz)
  - plug mem leak on dri2_create_screen failure (Tomasz)
  - fixup function name typo (Tomasz, Rob)

Cc: Robert Foss 
Cc: Tomasz Figa 
Cc: Mauro Rossi 
Signed-off-by: Emil Velikov 
---
Rob, I choose not to keep your r-b since the patch has changed quite a
bit.

Mauro, please check that this version doesn't break the drm_gralloc case.

Thanks
Emil
---
  src/egl/drivers/dri2/platform_android.c | 116 +++-
  1 file changed, 73 insertions(+), 43 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index 1f9fe27ab85..0586633a6db 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1203,9 +1203,10 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
  }
  
  #ifdef HAVE_DRM_GRALLOC

-static int
-droid_open_device_drm_gralloc(struct dri2_egl_display *dri2_dpy)
+static EGLBoolean
+droid_open_device_drm_gralloc(_EGLDisplay *disp)
  {
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
 int fd = -1, err = -EINVAL;
  
 if (dri2_dpy->gralloc->perform)

@@ -1214,10 +1215,13 @@ droid_open_device_drm_gralloc(struct dri2_egl_display 
*dri2_dpy)
);
 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)
+ 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)
  {
-   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;
  }
  
+static void

+droid_unload_driver(_EGLDisplay *disp)
+{
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
+
+   dlclose(dri2_dpy->driver);
+   dri2_dpy->driver = NULL;
+   free(dri2_dpy->driver_name);
+   dri2_dpy->driver_name = NULL;
+}
+
  static int
  droid_filter_device(_EGLDisplay *disp, int fd, const char *vendor)
  {
@@ -1420,13 +1435,31 @@ droid_filter_device(_EGLDisplay *disp, int fd, const 
char *vendor)
 return 0;
  }
  
-static int

+static EGLBoolean
+droid_probe_device(_EGLDisplay *disp)
+{
+  /* Check that the device is supported, by attempting to:
+   * - load the dri module
+   * - and, create a screen
+   */
+   if (!droid_load_driver(disp))
+  return EGL_FALSE;
+
+   if (!dri2_create_screen(disp)) {
+  _eglLog(_EGL_WARNING, "DRI2: failed to create screen");
+  droid_unload_driver(disp);
+  return EGL_FALSE;
+   }
+   return EGL_TRUE;
+}
+
+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;
-   int fd = -1, fallback_fd = -1;
  
 char *vendor_name = NULL;

 char vendor_buf[PROPERTY_VALUE_MAX];
@@ -1436,7 +1469,7 @@ droid_open_device(_EGLDisplay *disp)
  
 num_devices = drmGetDevices2(0, devices, ARRAY_SIZE(devices));

 if (num_devices < 0)
-  return num_devices;
+  return EGL_FALSE;
  
 for (int i = 0; i < num_devices; i++) {

device = devices[i];
@@ -1444,41 +1477,49 @@ droid_open_device(_EGLDisplay *disp)
if (!(device->available_nodes & (1 << DRM_NODE_RENDER)))
   continue;
  
-  fd = loader_open_device(device->nodes[DRM_NODE_RENDER]);

-  if (fd < 0) {
+  dri2_dpy->fd = loader_open_device(device->nodes[DRM_NODE_RENDER]);
+  if (dri2_dpy->fd < 0) {
   _eglLog(_EGL_WARNING, "%s() Failed to

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

2018-09-03 Thread Eric Engestrom 
On Monday, 2018-09-03 08:40:47 +0100, Eric Engestrom wrote:
> On Friday, 2018-08-31 17:59:10 +0100, Emil Velikov wrote:
> > From: Emil Velikov 
> > 
> > Unlike the other platforms, here we aim do guess if the device that we
> > somewhat arbitrarily picked, is supported or not.
> > 
> > In particular: when a vendor is _not_ requested we loop through all
> > devices, picking the first one which can create a DRI screen.
> > 
> > When a vendor is requested - we use that and do _not_ fall-back to any
> > other device.
> > 
> > The former seems a bit fiddly, but considering EGL_EXT_explicit_device and
> > EGL_MESA_query_renderer are MIA, this is the best we can do for the
> > moment.
> > 
> > With those (proposed) extensions userspace will be able to create a
> > separate EGL display for each device, query device details and make the
> > conscious decision which one to use.
> > 
> > v2:
> >  - update droid_open_device_drm_gralloc()
> >  - set the dri2_dpy->fd before using it
> >  - return a EGLBoolean for droid_{probe,open}_device*
> >  - do not warn on droid_load_driver failure (Tomasz)
> >  - plug mem leak on dri2_create_screen failure (Tomasz)
> >  - fixup function name typo (Tomasz, Rob)
> > 
> > Cc: Robert Foss 
> > Cc: Tomasz Figa 
> > Cc: Mauro Rossi 
> > Signed-off-by: Emil Velikov 
> > ---
> > Rob, I choose not to keep your r-b since the patch has changed quite a
> > bit.
> > 
> > Mauro, please check that this version doesn't break the drm_gralloc case.
> > 
> > Thanks
> > Emil
> > ---
> >  src/egl/drivers/dri2/platform_android.c | 116 +++-
> >  1 file changed, 73 insertions(+), 43 deletions(-)
> > 
> > diff --git a/src/egl/drivers/dri2/platform_android.c 
> > b/src/egl/drivers/dri2/platform_android.c
> > index 1f9fe27ab85..0586633a6db 100644
> > --- a/src/egl/drivers/dri2/platform_android.c
> > +++ b/src/egl/drivers/dri2/platform_android.c
> > @@ -1203,9 +1203,10 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
> > _EGLDisplay *dpy)
> >  }
> >  
> >  #ifdef HAVE_DRM_GRALLOC
> > -static int
> > -droid_open_device_drm_gralloc(struct dri2_egl_display *dri2_dpy)
> > +static EGLBoolean
> > +droid_open_device_drm_gralloc(_EGLDisplay *disp)
> >  {
> > +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> > int fd = -1, err = -EINVAL;
> >  
> > if (dri2_dpy->gralloc->perform)
> > @@ -1214,10 +1215,13 @@ droid_open_device_drm_gralloc(struct 
> > dri2_egl_display *dri2_dpy)
> >);
> > 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 is doing `dpy->fd = (dup() < 0)`.
> 
> You could add parentheses to fix it, or simply split into
>   dpy->fd = dup();
>   if (dpy->fd < 0)
> 
> (voting for the latter)

Sorry, ignore this, I just saw that Tomasz pointed it out as well :]

(I should really read other people's replies before saying anything)

> 
> > + 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)
> >  {
> > -   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;
> >  }
> >  
> > +static void
> > +droid_unload_driver(_EGLDisplay *disp)
> > +{
> > +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> > +
> > +   dlclose(dri2_dpy->driver);
> > +   dri2_dpy->driver = NULL;
> > +   free(dri2_dpy->driver_name);
> > +   dri2_dpy->driver_name = NULL;
> > +}
> > +
> >  static int
> >  droid_filter_device(_EGLDisplay *disp, int fd, const char *vendor)
> >  {
> > @@ -1420,13 +1435,31 @@ droid_filter_device(_EGLDisplay *disp, int fd, 
> > const char *vendor)
> > return 0;
> >  }
> >  
> > -static int
> > +static EGLBoolean
> > +droid_probe_device(_EGLDisplay *disp)
> > +{
> > +  /* Check that the device is supported, by attempting to:
> > +   * - load the dri module
> > +   * - and, create a screen
> > +   */
> > +   if (!droid_load_driver(disp))
> > +  return EGL_FALSE;
> > +
> > +   if (!dri2_create_screen(disp)) {
> > +  _eglLog(_EGL_WARNING, "DRI2: failed to create screen");
> > +  droid_unload_driver(disp);
> > +  return EGL_FALSE;
> > +   }
> > +   return EGL_TRUE;
> > +}
> > +
> > +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,

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

2018-09-03 Thread Eric Engestrom 
On Friday, 2018-08-31 17:59:10 +0100, Emil Velikov wrote:
> From: Emil Velikov 
> 
> Unlike the other platforms, here we aim do guess if the device that we
> somewhat arbitrarily picked, is supported or not.
> 
> In particular: when a vendor is _not_ requested we loop through all
> devices, picking the first one which can create a DRI screen.
> 
> When a vendor is requested - we use that and do _not_ fall-back to any
> other device.
> 
> The former seems a bit fiddly, but considering EGL_EXT_explicit_device and
> EGL_MESA_query_renderer are MIA, this is the best we can do for the
> moment.
> 
> With those (proposed) extensions userspace will be able to create a
> separate EGL display for each device, query device details and make the
> conscious decision which one to use.
> 
> v2:
>  - update droid_open_device_drm_gralloc()
>  - set the dri2_dpy->fd before using it
>  - return a EGLBoolean for droid_{probe,open}_device*
>  - do not warn on droid_load_driver failure (Tomasz)
>  - plug mem leak on dri2_create_screen failure (Tomasz)
>  - fixup function name typo (Tomasz, Rob)
> 
> Cc: Robert Foss 
> Cc: Tomasz Figa 
> Cc: Mauro Rossi 
> Signed-off-by: Emil Velikov 
> ---
> Rob, I choose not to keep your r-b since the patch has changed quite a
> bit.
> 
> Mauro, please check that this version doesn't break the drm_gralloc case.
> 
> Thanks
> Emil
> ---
>  src/egl/drivers/dri2/platform_android.c | 116 +++-
>  1 file changed, 73 insertions(+), 43 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_android.c 
> b/src/egl/drivers/dri2/platform_android.c
> index 1f9fe27ab85..0586633a6db 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -1203,9 +1203,10 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
> _EGLDisplay *dpy)
>  }
>  
>  #ifdef HAVE_DRM_GRALLOC
> -static int
> -droid_open_device_drm_gralloc(struct dri2_egl_display *dri2_dpy)
> +static EGLBoolean
> +droid_open_device_drm_gralloc(_EGLDisplay *disp)
>  {
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> int fd = -1, err = -EINVAL;
>  
> if (dri2_dpy->gralloc->perform)
> @@ -1214,10 +1215,13 @@ droid_open_device_drm_gralloc(struct dri2_egl_display 
> *dri2_dpy)
>);
> 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 is doing `dpy->fd = (dup() < 0)`.

You could add parentheses to fix it, or simply split into
  dpy->fd = dup();
  if (dpy->fd < 0)

(voting for the latter)

> + 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)
>  {
> -   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;
>  }
>  
> +static void
> +droid_unload_driver(_EGLDisplay *disp)
> +{
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> +
> +   dlclose(dri2_dpy->driver);
> +   dri2_dpy->driver = NULL;
> +   free(dri2_dpy->driver_name);
> +   dri2_dpy->driver_name = NULL;
> +}
> +
>  static int
>  droid_filter_device(_EGLDisplay *disp, int fd, const char *vendor)
>  {
> @@ -1420,13 +1435,31 @@ droid_filter_device(_EGLDisplay *disp, int fd, const 
> char *vendor)
> return 0;
>  }
>  
> -static int
> +static EGLBoolean
> +droid_probe_device(_EGLDisplay *disp)
> +{
> +  /* Check that the device is supported, by attempting to:
> +   * - load the dri module
> +   * - and, create a screen
> +   */
> +   if (!droid_load_driver(disp))
> +  return EGL_FALSE;
> +
> +   if (!dri2_create_screen(disp)) {
> +  _eglLog(_EGL_WARNING, "DRI2: failed to create screen");
> +  droid_unload_driver(disp);
> +  return EGL_FALSE;
> +   }
> +   return EGL_TRUE;
> +}
> +
> +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;
> -   int fd = -1, fallback_fd = -1;
>  
> char *vendor_name = NULL;
> char vendor_buf[PROPERTY_VALUE_MAX];
> @@ -1436,7 +1469,7 @@ droid_open_device(_EGLDisplay *disp)
>  
> num_devices = drmGetDevices2(0, devices, ARRAY_SIZE(devices));
> if (num_devices < 0)
> -  return num_devices;
> +  return EGL_FALSE;
>  
> for (int i = 0; i < num_devices; i++) {
>device = devices[i];
> @@ -1444,41 +1477,49 @@ droid_open_device(_EGLDisplay *disp)
>if

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

2018-09-03 Thread Tomasz Figa 
On Mon, Sep 3, 2018 at 2:45 PM Tomasz Figa  wrote:
>
> Hi Emil,
>
> On Sat, Sep 1, 2018 at 2:03 AM Emil Velikov  wrote:
> >
> > From: Emil Velikov 
> >
>
> 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)
> >);
> > 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 

Best regards,
Tomasz
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-09-02 Thread Tomasz Figa 
Hi Emil,

On Sat, Sep 1, 2018 at 2:03 AM Emil Velikov  wrote:
>
> From: Emil Velikov 
>

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)
>);
> 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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

2018-08-31 Thread Emil Velikov 
From: Emil Velikov 

Unlike the other platforms, here we aim do guess if the device that we
somewhat arbitrarily picked, is supported or not.

In particular: when a vendor is _not_ requested we loop through all
devices, picking the first one which can create a DRI screen.

When a vendor is requested - we use that and do _not_ fall-back to any
other device.

The former seems a bit fiddly, but considering EGL_EXT_explicit_device and
EGL_MESA_query_renderer are MIA, this is the best we can do for the
moment.

With those (proposed) extensions userspace will be able to create a
separate EGL display for each device, query device details and make the
conscious decision which one to use.

v2:
 - update droid_open_device_drm_gralloc()
 - set the dri2_dpy->fd before using it
 - return a EGLBoolean for droid_{probe,open}_device*
 - do not warn on droid_load_driver failure (Tomasz)
 - plug mem leak on dri2_create_screen failure (Tomasz)
 - fixup function name typo (Tomasz, Rob)

Cc: Robert Foss 
Cc: Tomasz Figa 
Cc: Mauro Rossi 
Signed-off-by: Emil Velikov 
---
Rob, I choose not to keep your r-b since the patch has changed quite a
bit.

Mauro, please check that this version doesn't break the drm_gralloc case.

Thanks
Emil
---
 src/egl/drivers/dri2/platform_android.c | 116 +++-
 1 file changed, 73 insertions(+), 43 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index 1f9fe27ab85..0586633a6db 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1203,9 +1203,10 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
 }
 
 #ifdef HAVE_DRM_GRALLOC
-static int
-droid_open_device_drm_gralloc(struct dri2_egl_display *dri2_dpy)
+static EGLBoolean
+droid_open_device_drm_gralloc(_EGLDisplay *disp)
 {
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
int fd = -1, err = -EINVAL;
 
if (dri2_dpy->gralloc->perform)
@@ -1214,10 +1215,13 @@ droid_open_device_drm_gralloc(struct dri2_egl_display 
*dri2_dpy)
   );
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)
+ 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)
 {
-   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;
 }
 
+static void
+droid_unload_driver(_EGLDisplay *disp)
+{
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
+
+   dlclose(dri2_dpy->driver);
+   dri2_dpy->driver = NULL;
+   free(dri2_dpy->driver_name);
+   dri2_dpy->driver_name = NULL;
+}
+
 static int
 droid_filter_device(_EGLDisplay *disp, int fd, const char *vendor)
 {
@@ -1420,13 +1435,31 @@ droid_filter_device(_EGLDisplay *disp, int fd, const 
char *vendor)
return 0;
 }
 
-static int
+static EGLBoolean
+droid_probe_device(_EGLDisplay *disp)
+{
+  /* Check that the device is supported, by attempting to:
+   * - load the dri module
+   * - and, create a screen
+   */
+   if (!droid_load_driver(disp))
+  return EGL_FALSE;
+
+   if (!dri2_create_screen(disp)) {
+  _eglLog(_EGL_WARNING, "DRI2: failed to create screen");
+  droid_unload_driver(disp);
+  return EGL_FALSE;
+   }
+   return EGL_TRUE;
+}
+
+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;
-   int fd = -1, fallback_fd = -1;
 
char *vendor_name = NULL;
char vendor_buf[PROPERTY_VALUE_MAX];
@@ -1436,7 +1469,7 @@ droid_open_device(_EGLDisplay *disp)
 
num_devices = drmGetDevices2(0, devices, ARRAY_SIZE(devices));
if (num_devices < 0)
-  return num_devices;
+  return EGL_FALSE;
 
for (int i = 0; i < num_devices; i++) {
   device = devices[i];
@@ -1444,41 +1477,49 @@ droid_open_device(_EGLDisplay *disp)
   if (!(device->available_nodes & (1 << DRM_NODE_RENDER)))
  continue;
 
-  fd = loader_open_device(device->nodes[DRM_NODE_RENDER]);
-  if (fd < 0) {
+  dri2_dpy->fd = loader_open_device(device->nodes[DRM_NODE_RENDER]);
+  if (dri2_dpy->fd < 0) {
  _eglLog(_EGL_WARNING, "%s() Failed to open DRM device %s",
  __func__, device->nodes[DRM_NODE_RENDER]);
  continue;
   }
 
-  if (vendor_name && droid_filter_device(disp, fd, vendor_name)) {
- /* Match