Hi Daniel, Not a huge expert on this code, so take the following with a grain of salt.
On 10 February 2017 at 17:43, Daniel Stone <[email protected]> wrote: > Given that we can have render-only devices, or vgem in a class of its > own, ignore any non-KMS devices in compositor-drm's device selection. > For x86 platforms, this is mostly a non-issue since we look at the udev > boot_vga issue, but other architectures which lack this, and have > multiple KMS devices present, will hit this. > > Signed-off-by: Daniel Stone <[email protected]> > Reported-by: Thierry Reding <[email protected]> > Reported-by: Daniel Vetter <[email protected]> > --- > libweston/compositor-drm.c | 88 > +++++++++++++++++++++++++++++----------------- > 1 file changed, 55 insertions(+), 33 deletions(-) > > Resend/ping. Given that this breaks actual real-world setups (non-PCI > setups shipping vgem), this would be great to have in the release. > > diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c > index 2a80c6d79..c3fcd78f3 100644 > --- a/libweston/compositor-drm.c > +++ b/libweston/compositor-drm.c > @@ -1506,36 +1506,15 @@ on_drm_input(int fd, uint32_t mask, void *data) > } > > static int > -init_drm(struct drm_backend *b, struct udev_device *device) > +init_kms_caps(struct drm_backend *b) > { > - const char *filename, *sysnum; > uint64_t cap; > - int fd, ret; > + int ret; > clockid_t clk_id; > > - sysnum = udev_device_get_sysnum(device); > - if (sysnum) > - b->drm.id = atoi(sysnum); > - if (!sysnum || b->drm.id < 0) { > - weston_log("cannot get device sysnum\n"); > - return -1; > - } > - > - filename = udev_device_get_devnode(device); > - fd = weston_launcher_open(b->compositor->launcher, filename, O_RDWR); > - if (fd < 0) { > - /* Probably permissions error */ > - weston_log("couldn't open %s, skipping\n", > - udev_device_get_devnode(device)); > - return -1; > - } > - > - weston_log("using %s\n", filename); > - > - b->drm.fd = fd; > - b->drm.filename = strdup(filename); > + weston_log("using %s\n", b->drm.filename); > > - ret = drmGetCap(fd, DRM_CAP_TIMESTAMP_MONOTONIC, &cap); > + ret = drmGetCap(b->drm.fd, DRM_CAP_TIMESTAMP_MONOTONIC, &cap); > if (ret == 0 && cap == 1) > clk_id = CLOCK_MONOTONIC; > else > @@ -1547,13 +1526,13 @@ init_drm(struct drm_backend *b, struct udev_device > *device) > return -1; > } > > - ret = drmGetCap(fd, DRM_CAP_CURSOR_WIDTH, &cap); > + ret = drmGetCap(b->drm.fd, DRM_CAP_CURSOR_WIDTH, &cap); > if (ret == 0) > b->cursor_width = cap; > else > b->cursor_width = 64; > > - ret = drmGetCap(fd, DRM_CAP_CURSOR_HEIGHT, &cap); > + ret = drmGetCap(b->drm.fd, DRM_CAP_CURSOR_HEIGHT, &cap); > if (ret == 0) > b->cursor_height = cap; > else > @@ -2939,6 +2918,49 @@ session_notify(struct wl_listener *listener, void > *data) > }; > } > > +static bool > +drm_device_is_kms(struct drm_backend *b, struct udev_device *device) > +{ > + const char *filename = udev_device_get_devnode(device); > + const char *sysnum = udev_device_get_sysnum(device); > + drmModeRes *res; > + int fd; > + > + if (!filename) > + return false; > + > + fd = weston_launcher_open(b->compositor->launcher, filename, O_RDWR); > + if (fd < 0) > + return false; > + > + res = drmModeGetResources(fd); > + if (!res) > + goto out_fd; > + > + if (res->count_crtcs <= 0 || res->count_connectors <= 0 || > + res->count_encoders <= 0) > + goto out_res; > + > + if (sysnum) > + b->drm.id = atoi(sysnum); > + if (!sysnum || b->drm.id < 0) { > + weston_log("couldn't get sysnum for device %s\n", > + b->drm.filename); > + goto out_res; > + } > + > + b->drm.fd = fd; > + b->drm.filename = strdup(filename); > + As we find a KMS device, we keep the launcher open, store the fd and dup the filename... > + return true; > + > +out_res: > + drmModeFreeResources(res); > +out_fd: > + weston_launcher_close(b->compositor->launcher, fd); Mildly related: there's a bunch of these weston_launcher_close() missing through the codebase. But I'm not sure what' the best location where they should be called. > + return false; > +} > + > /* > * Find primary GPU > * Some systems may have multiple DRM devices attached to a single seat. This > @@ -2973,6 +2995,10 @@ find_primary_gpu(struct drm_backend *b, const char > *seat) > continue; > } > > + /* Make sure this device is actually capable of modesetting. > */ > + if (!drm_device_is_kms(b, device)) > + continue; > + > pci = udev_device_get_parent_with_subsystem_devtype(device, > "pci", NULL); > if (pci) { ... yet here we continue to probe the boot_vga flag (if PCI device) and eventually continue with the next card node. I.e. imho the order is wrong (we should honour boot_vga first?) and we leak quite a bit. Perhaps something roughly like the following will be better ? udev_for_each... { if (!seat_matches()) { unref(device); continue; } if (boot_vga_is_one()) break; if (drm_device_is_kms()) break; unref(device); } unref(e); return device; -Emil _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
