On Tue, 23 Jan 2018 22:15:47 -0500 nerdopolis <bluescreen_aven...@verizon.net> wrote:
> This adds a function to detect the first framebuffer device in the > current seat. Instead of hardcoding /dev/fb0, detect the device > with udev, favoring the boot_vga device, and falling back to the > first framebuffer device in the seat if there is none. This is very > similar to what compositor-drm does to find display devices > --- > libweston/compositor-fbdev.c | 77 > ++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 74 insertions(+), 3 deletions(-) > Hi, the commit subject is quite long. It would suffice to have "compositor-fbdev: detect the first fb device in the seat". This patch has a good idea and mostly good implementation, but there are some memory handling issues. > diff --git a/libweston/compositor-fbdev.c b/libweston/compositor-fbdev.c > index 39668aa8..8cf0922e 100644 > --- a/libweston/compositor-fbdev.c > +++ b/libweston/compositor-fbdev.c > @@ -711,6 +711,71 @@ fbdev_restore(struct weston_compositor *compositor) > weston_launcher_restore(compositor->launcher); > } > > +static char * > +find_framebuffer_device(struct fbdev_backend *b, const char *seat) > +{ > + struct udev_enumerate *e; > + struct udev_list_entry *entry; > + const char *path, *device_seat; > + char *fb_device, *find_device, *id; > + struct udev_device *device, *pci; > + > + e = udev_enumerate_new(b->udev); > + udev_enumerate_add_match_subsystem(e, "graphics"); > + udev_enumerate_add_match_sysname(e, "fb[0-9]*"); > + > + udev_enumerate_scan_devices(e); > + fb_device = NULL; > + udev_list_entry_foreach(entry, udev_enumerate_get_list_entry(e)) { > + bool is_boot_vga = false; > + > + path = udev_list_entry_get_name(entry); > + device = udev_device_new_from_syspath(b->udev, path); > + find_device = udev_device_get_devnode(device); I don't know if it is ok to call udev_device_get_devnode(NULL), so this call would be better after the NULL check. > + if (!device) > + continue; > + device_seat = udev_device_get_property_value(device, "ID_SEAT"); > + if (!device_seat) > + device_seat = default_seat; > + if (strcmp(device_seat, seat)) { > + udev_device_unref(device); > + continue; > + } > + > + pci = udev_device_get_parent_with_subsystem_devtype(device, > + "pci", NULL); > + if (pci) { > + id = udev_device_get_sysattr_value(pci, "boot_vga"); > + if (id && !strcmp(id, "1")) > + is_boot_vga = true; > + } > + > + /* If a framebuffer device was found, and this device isn't > + * the boot-VGA device, don't use it. */ > + if (!is_boot_vga && fb_device) { > + udev_device_unref(device); > + continue; > + } > + > + /* There can only be one boot_vga device. Try to use it > + * at all costs. */ > + if (is_boot_vga) { > + fb_device = strdup(find_device); If fb_device was already set on the previous iteration, this will leak the old string. > + udev_device_unref(device); > + break; > + } > + > + /* Per the (!is_boot_vga && fb_device) test above, only > + * trump existing saved devices with boot-VGA devices, so if > + * the test ends up here, this must be the first device seen. */ > + assert(!fb_device); > + fb_device = find_device; This is leaking 'device', right? And if you do unref 'device', then 'find_device' and therefore 'fb_device' will become pointers to freed memory. It might be good to cross-check with the function in compositor-drm.c and change the type of fb_device to struct udev_device * and do the strdup() just before returning if anything was found. > + } > + > + udev_enumerate_unref(e); > + return fb_device; > +} > + > static struct fbdev_backend * > fbdev_backend_create(struct weston_compositor *compositor, > struct weston_fbdev_backend_config *param) > @@ -743,6 +808,11 @@ fbdev_backend_create(struct weston_compositor > *compositor, > goto out_compositor; > } > > + if (!param->device) > + param->device = strdup(find_framebuffer_device(backend, > seat_id)); > + if (!param->device) The only way strdup() is documented to return NULL is if it runs out of memory. It would be better for find_framebuffer_device() to return a freshly allocate string instead of having to strdup() here, or NULL on failure. Looks like it does that already too. > + param->device = strdup("/dev/fb0"); > + > /* Set up the TTY. */ > backend->session_listener.notify = session_notify; > wl_signal_add(&compositor->session_signal, > @@ -769,12 +839,15 @@ fbdev_backend_create(struct weston_compositor > *compositor, > if (fbdev_output_create(backend, param->device) < 0) > goto out_launcher; > > + free(param->device); > + > udev_input_init(&backend->input, compositor, backend->udev, > seat_id, param->configure_device); > > return backend; > > out_launcher: > + free(param->device); > weston_launcher_destroy(compositor->launcher); > > out_udev: > @@ -790,10 +863,8 @@ out_compositor: > static void > config_init_to_defaults(struct weston_fbdev_backend_config *config) > { > - /* TODO: Ideally, available frame buffers should be enumerated using > - * udev, rather than passing a device node in as a parameter. */ > config->tty = 0; /* default to current tty */ > - config->device = "/dev/fb0"; /* default frame buffer */ > + config->device = NULL; > config->seat_id = NULL; > } > Thanks, pq
pgpu4d0l40gRV.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel