Hi Emil,
Thanks for the review!
On 10 February 2017 at 19:51, Emil Velikov <[email protected]> wrote:
> On 10 February 2017 at 17:43, Daniel Stone <[email protected]> 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
_______________________________________________
wayland-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
