Hey Tomasz,
Thanks for the quick feedback.
On 2018-06-14 08:30, Tomasz Figa wrote:
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(...)) { ...
Yeah, overall that if-case is too much. I'll split out the NULL check separate
check that return an error.
+ 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().
Ack.
+
+ 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), ...);
Ack.
+ fd = loader_open_device(dev_path);
+ if (fd == -1) {
nit: Perhaps fd < 0, just to be safe?
Ack.
+ _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.
Ack.
+ case probe_error:
+ case probe_no_driver:
Do we need 2 separate cases for these? Just one "probe_fail" should be enough.
Ack.
+ goto next;
If we move the fallback handling to "probe_filtered_out" case, we
could remove the "next" label too and simply "break" here.
Ack.
+ }
+
+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
