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

[Daniel Stone]

Hi Emil,
Thanks for the review!

On 10 February 2017 at 19:51, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 10 February 2017 at 17:43, Daniel Stone <daniels at collabora.com> wrote:
>> +static bool
>> +drm_device_is_kms(struct drm_backend *b, struct udev_device *device)
>> +{
>> +       [...]
>> +
>> +       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.

It'd be interesting to know where they are. We need to be really
careful with close, since logind only lets us open a given device once
per session - hence the awkward ownership dance in
drm_device_is_kms().

>> @@ -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.

You're totally right here.

> 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);
> }

Hmm. We don't want to break on the first KMS device we see, because it
might not be the boot_vga device. I'll send a v3 now which I think is
probably the best compromise.

Cheers,
Daniel