[PATCH v6 5/6] libweston: fbdev: Attempt to detect the first framebuffer device in the seat. instead of defaulting to /dev/fb0

[Pekka Paalanen]

On Tue, 23 Jan 2018 22:15:47 -0500
nerdopolis <bluescreen_avenger at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180612/4faff8b8/attachment.sig>