Hi Rob, Thanks for sending v3. Please see few more comments inline.
On Sun, Jun 10, 2018 at 2:28 AM Robert Foss <robert.f...@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. > > Signed-off-by: Robert Foss <robert.f...@collabora.com> > --- > > Changes since v2: > - Switch from drmGetDevices2 to manual renderD node iteration > - Add probe_res enum to communicate probing results better > - Avoid using _eglError() in internal static functions > - Avoid actually loading the driver while probing, just verify > that it exists. > - Replace strlen call with the assumed length PROPERTY_VALUE_MAX [snip] > +static probe_ret_t > +droid_probe_device(_EGLDisplay *disp, int fd, char *vendor) > +{ > + int ret; > + > + drmVersionPtr ver = drmGetVersion(fd); > + if (!ver) > + return probe_error; > + > + if (vendor != NULL && ver->name != NULL && > + strncmp(vendor, ver->name, PROPERTY_VALUE_MAX) != 0) { Shouldn't we fail the match if vendor != NULL, but ver->name == NULL? i.e. if (vendor && (!ver->name || strcmp(...)) { ... > + ret = probe_filtered_out; > + goto cleanup; > + } > + > + > + if (!droid_probe_driver(fd)) { > + ret = probe_no_driver; > + goto cleanup; > + } > + > + ret = probe_success; > + > +cleanup: > + drmFreeVersion(ver); > + return ret; > +} > + > +static int > +droid_open_device(_EGLDisplay *disp) > +{ > + const int MAX_DRM_DEVICES = 32; > + int prop_set, num_devices; > + int fd = -1, fallback_fd = -1; > + > + char *vendor_name = NULL; > + char vendor_buf[PROPERTY_VALUE_MAX]; > + if (property_get("drm.gpu.vendor_name", vendor_buf, NULL) > 0); > + vendor_name = vendor_buf; > + > + const char *drm_dir_name = "/dev/dri"; > + DIR *sysdir = opendir(drm_dir_name); > + if (!sysdir) > + return -errno; > + > + struct dirent *dent; > + while ((dent = readdir(sysdir))) { > + char dev_path[128]; > + char *render_dev_prefix = "renderD"; > + size_t prefix_len = strlen(render_dev_prefix); If a const array like below is used instead const char render_dev_prefix[] = "renderD"; you can just use sizeof(render_dev_prefix), without the need for strlen(). > + > + if (strncmp(render_dev_prefix, dent->d_name, prefix_len) != 0) > + continue; > + > + sprintf(dev_path, "%s/%s", drm_dir_name, dent->d_name); snprintf(dev_path, sizeof(dev_path), ...); > + fd = loader_open_device(dev_path); > + if (fd == -1) { nit: Perhaps fd < 0, just to be safe? > + _eglLog(_EGL_WARNING, "%s() Failed to open DRM device %s", > + __func__, dev_path); > + continue; > + } > + > + int ret = droid_probe_device(disp, fd, vendor_name); > + switch (ret) { > + case probe_success: > + goto success; > + case probe_filtered_out: > + goto allow_fallback; The 2 lines of code at the "allow_fallback" label could be just moved here. > + case probe_error: > + case probe_no_driver: Do we need 2 separate cases for these? Just one "probe_fail" should be enough. > + goto next; If we move the fallback handling to "probe_filtered_out" case, we could remove the "next" label too and simply "break" here. > + } > + > +allow_fallback: > + if (fallback_fd == -1) > + fallback_fd = fd; > +next: > + if (fallback_fd != fd) > + close(fd); > + fd = -1; > + continue; > + } > + > +success: > + closedir(sysdir); > + > + if (fallback_fd < 0 && fd < 0) { > + _eglLog(_EGL_WARNING, "Failed to open any DRM device"); > + return -1; > + } > + > + if (fd < 0) { > + _eglLog(_EGL_WARNING, "Failed to open desired DRM device, using > fallback"); > + return fallback_fd; > + } > + > + close(fallback_fd); > + return fd; > +} [Leaving context for readability.] Best regards, Tomasz _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev