[Mesa-dev] [RFC] egl/android: Add DRM node probing and filtering

[Robert Foss]

On 04/20/2018 06:41 AM, Tomasz Figa wrote:
> Hi Rob,
> 
> On Thu, Apr 19, 2018 at 1:03 AM Robert Foss <robert.foss at collabora.com>
> wrote:
> 
>> This patch both adds support for probing & filtering DRM nodes
>> and switches away from using the GRALLOC_MODULE_PERFORM_GET_DRM_FD
>> gralloc call.
> 
>> Currently the filtering is based just on the driver name,
>> and the desired name is supplied using the "drm.gpu.vendor_name"
>> Android property.
> 
>> The filtering itself is done using the newly introduced
>> libdrm drmHandleMatch() call.
> 
>> Signed-off-by: Robert Foss <robert.foss at collabora.com>
>> ---
> 
>> This patch is based on[1], which contains a new libdrm function,
>> called drmHandleMatch(), which allows for matching an opened
>> drm node handle against some desired properties.
> 
>> A choice that was made for this patch was to add support for
>> falling back to to DRM nodes that have failed the filtering,
>> if no node passes the filter.
>> If this wouldn't be useful to anyone, I would suggest ripping it
>> out since it is a little bit ugly&complex.
> 
> This is actually not only useful, but quite of a requirement for Chrome OS,
> where we use the same Android image for devices with different GPU vendors.
> (Technically we could hack it up by injecting some per-board properties,
> but it would be painful from maintenance point of view.)

Ack, I'll remove the fallback.

> 
> But actually, the way it is implemented in this patch will not work. The
> code assumes that the first DRM node that could be opened
> (loader_open_device()) is fine, but if you look at what we do in our
> Chromium patch [2], it is actually necessary to check if Mesa includes a
> driver for it (loader_get_driver_for_fd()).
> 
> [2]
> https://chromium-review.googlesource.com/c/chromiumos/third_party/mesa/+/780797

Ack, I'll add a check for a driver existing.

> 
> 
> 
>> [1] https://www.spinics.net/lists/dri-devel/msg172497.html
> 
> 
>>    src/egl/drivers/dri2/platform_android.c | 72
> ++++++++++++++++++++++++++++-----
>>    1 file changed, 62 insertions(+), 10 deletions(-)
> 
>> diff --git a/src/egl/drivers/dri2/platform_android.c
> b/src/egl/drivers/dri2/platform_android.c
>> index 7f1a496ea24..0b082fe5dcc 100644
>> --- a/src/egl/drivers/dri2/platform_android.c
>> +++ b/src/egl/drivers/dri2/platform_android.c
>> @@ -27,6 +27,7 @@
>>     * DEALINGS IN THE SOFTWARE.
>>     */
> 
>> +#include <cutils/properties.h>
>>    #include <errno.h>
>>    #include <dlfcn.h>
>>    #include <fcntl.h>
>> @@ -1117,18 +1118,69 @@ droid_add_configs_for_visuals(_EGLDriver *drv,
> _EGLDisplay *dpy)
>>    static int
>>    droid_open_device(struct dri2_egl_display *dri2_dpy)
>>    {
>> -   int fd = -1, err = -EINVAL;
>> -
>> -   if (dri2_dpy->gralloc->perform)
>> -         err = dri2_dpy->gralloc->perform(dri2_dpy->gralloc,
>> -
>    GRALLOC_MODULE_PERFORM_GET_DRM_FD,
>> -                                          &fd);
>> -   if (err || fd < 0) {
>> -      _eglLog(_EGL_WARNING, "fail to get drm fd");
>> -      fd = -1;
>> +   int prop_set, num_devices, ret;
>> +   const int node_type = DRM_NODE_RENDER;
>> +   int fd = -1, fallback_fd = -1;
>> +
>> +   const int MAX_DRM_DEVICES = 32;
>> +   char vendor_name[PROPERTY_VALUE_MAX];
>> +   prop_set = property_get("drm.gpu.vendor_name", vendor_name, NULL);
>> +   drmVersion ver_filter;
>> +   ver_filter.name = vendor_name;
>> +
>> +   drmDevicePtr devices[MAX_DRM_DEVICES];
>> +   num_devices = drmGetDevices2(0, devices, MAX_DRM_DEVICES);
>> +   if (num_devices < 0) {
>> +      _eglLog(_EGL_WARNING, "Failed to find any DRM devices");
>> +      return -1;
>> +   }
>> +
>> +   for (int i = 0; i < num_devices; i++) {
>> +      /* Filter out DRM_NODE_ types we aren't interested in */
>> +      if (!(devices[i]->available_nodes & (1 << node_type))) {
>> +         continue;
>> +      }
>> +
>> +      /* Open DRM node FD */
>> +      fd = loader_open_device(devices[i]->nodes[node_type]);
>> +      if (fd == -1 && errno == EINVAL) {
>> +         _eglLog(_EGL_WARNING, "%s()  node #%d failed to open",
> __func__, i);
>> +         continue;
>> +      }
> 
> What should happen if (fd == -1 && errno != EINVAL)? I don't think we
> should run the code below in such case.
> 
>> +
>> +      /* See if FD matches the driver vendor we want */
>> +      if (prop_set && !drmHandleMatch(fd, &ver_filter, NULL)){
>> +         _eglLog(_EGL_WARNING, "%s()  node #%d  FD=%d does not match
> filter", __func__, i , fd);
> 
> How about printing the actual node (i.e. devices[i]->nodes[node_type]),
> rather than index in devices array?
> 
> Also, should it be _EGL_DEBUG? I'd expect it to be a normal event, which
> should not concern the user.
> 
>> +         goto next;
>> +      }
>> +
>> +      /* Successfully found matching FD */
>> +      close(fallback_fd);
>> +      fallback_fd = -1;
>> +      break;
>> +
>> +next:
>> +      if (fallback_fd == -1) {
>> +         fallback_fd = fd;
>> +         fd = -1;
>> +      } else {
>> +         close(fd);
>> +         fd = -1;
>> +      }
>> +      continue;
>> +   }
> 
> How about simplifying the code a bit:
> 
> +   for (int i = 0; i < num_devices; i++) {
> +      /* Filter out DRM_NODE_ types we aren't interested in */
> +      if (!(devices[i]->available_nodes & (1 << node_type))) {
> +         continue;
> +      }
> +
> +      /* Open DRM node FD */
> +      fd = loader_open_device(devices[i]->nodes[node_type]);
> +      if (fd < 0) {
> +         _eglLog(_EGL_WARNING, "%s()  node #%d failed to open", __func__,
> i);
> +         continue;
> +      }
> +
> +      if (fallback_fd < 0)
> +         fallback_fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
> +
> +      /* See if FD matches the driver vendor we want */
> +      if (!prop_set || drmHandleMatch(fd, &ver_filter, NULL))
> +         /* Successfully found matching FD */
> +         break;
> +
> +      _eglLog(_EGL_WARNING, "%s()  node #%d  FD=%d does not match filter",
> __func__, i , fd);
> +      close(fd);
> +      fd = -1;
> +   }
> +
> +   if (fd >= 0) {
> +      close(fallback_fd);
> +      fallback_fd = -1;
> +   } else {
> +      _eglLog(_EGL_WARNING, "Failed to open desired DRM handle, using
> fallback");
> +      fd = fallback_fd;
> +   }
>> +
>> +   if (fd < 0) {
>> +      _eglLog(_EGL_WARNING, "Failed to open desired DRM handle, using
> fallback");
>> +      return fallback_fd;
>>       }
> 
> Best regards,
> Tomasz
>