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

[Emil Velikov]

On 14 February 2017 at 13:49, 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>
> Cc: Emil Velikov <emil.velikov at collabora.com>
> Reported-by: Thierry Reding <treding at nvidia.com>
> Reported-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
>  libweston/compositor-drm.c | 142 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 100 insertions(+), 42 deletions(-)
>
> v5: Document find_primary_gpu() and fix FD leaks if we happen to find a
>     suitable non-boot-VGA device before later finding a boot-VGA device.
>
Seems like my initial assumption of is_kms being optional was off.

There's a few minor comments below, but I'm split how much of a
blocker they are.
Either way:
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

> +       if (sysnum)
> +               b->drm.id = atoi(sysnum);
Overwriting the .id in the error case seems wrong, since we'll stomp
over the valid data from the previous device.

> +       if (!sysnum || b->drm.id < 0) {
> +               weston_log("couldn't get sysnum for device %s\n",
> +                          b->drm.filename);
"filename" only


> +               goto out_res;
> +       }
> +
> +       /* We can be called successfully on multiple devices; if we have,
> +        * clean up old entries. */
> +       if (b->drm.fd)
Use "b->drm.fd >= 0" ?

> +               weston_launcher_close(b->compositor->launcher, b->drm.fd);
> +       free(b->drm.filename);
> +
One might need to check that the b-> fields are initialised properly.


> +               /* Make sure this device is actually capable of modesetting;
> +                * if this call succeeds, b->drm.{fd,filename} will be set. */
... will be set and the old ones will be freed/closed.

-Emil