[PATCH v3 weston] compositor-drm: Ignore non-KMS devices

[Emil Velikov]

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 <daniels at collabora.com> 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 <daniels at collabora.com>
> Reported-by: Thierry Reding <treding at nvidia.com>
> Reported-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  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