[PATCH v8 5/6] compositor-fbdev: detect the first fb device in the seat

[Pekka Paalanen]

On Wed, 27 Jun 2018 20:44:21 -0400
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 | 83 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 80 insertions(+), 3 deletions(-)
> 
> diff --git a/libweston/compositor-fbdev.c b/libweston/compositor-fbdev.c
> index e3777495..616300dc 100644
> --- a/libweston/compositor-fbdev.c
> +++ b/libweston/compositor-fbdev.c
> @@ -777,6 +777,77 @@ session_notify(struct wl_listener *listener, void *data)
>  	}
>  }

Hi,

now this patch triggers the following compiler warnings:

  CC       libweston/fbdev_backend_la-compositor-fbdev.lo
/home/pq/git/weston/libweston/compositor-fbdev.c: In function ‘find_framebuffer_device’:
/home/pq/git/weston/libweston/compositor-fbdev.c:848:9: warning: return discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
  return fb_device_path;
         ^~~~~~~~~~~~~~
/home/pq/git/weston/libweston/compositor-fbdev.c: In function ‘weston_backend_init’:
/home/pq/git/weston/libweston/compositor-fbdev.c:885:5: warning: ‘fb_device_path’ may be used uninitialized in this function [-Wmaybe-uninitialized]
  if (!param->device)
     ^
/home/pq/git/weston/libweston/compositor-fbdev.c:785:40: note: ‘fb_device_path’ was declared here
  const char *path, *device_seat, *id, *fb_device_path;
                                        ^~~~~~~~~~~~~~


>  
> +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, *id, *fb_device_path;

fb_device_path should be declared without const, because it holds a
malloc()'d (strdup()'d) string we create here. Also the function return
type is without const which leads to the warning.

It also needs to be initialized, so that we return NULL if no fb
devices are found at all.

> +	struct udev_device *device, *fb_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);
> +		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) {
> +			if (fb_device)
> +				udev_device_unref(fb_device);
> +			fb_device = 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 = device;
> +	}
> +
> +	udev_enumerate_unref(e);
> +
> +	if (fb_device)
> +	{

Opening brace needs to be on the previous line.

> +		fb_device_path=strdup(udev_device_get_devnode(fb_device));

Missing spaces around =.

> +		udev_device_unref(fb_device);
> +	}
> +
> +	return fb_device_path;
> +}
> +
>  static struct fbdev_backend *
>  fbdev_backend_create(struct weston_compositor *compositor,
>                       struct weston_fbdev_backend_config *param)
> @@ -809,6 +880,11 @@ fbdev_backend_create(struct weston_compositor *compositor,
>  		goto out_compositor;
>  	}
>  
> +	if (!param->device)
> +		param->device = find_framebuffer_device(backend, seat_id);
> +	if (!param->device)
> +		param->device = strdup("/dev/fb0");

I wonder, if we don't find any fb device in find_framebuffer_device(),
should we still use /dev/fb0? Either it did not exist in the first
place, or it might be assigned to a different seat.

Other than all the little details I mentioned, this looks good now.


Thanks,
pq

> +
>  	/* Set up the TTY. */
>  	backend->session_listener.notify = session_notify;
>  	wl_signal_add(&compositor->session_signal,
> @@ -835,12 +911,15 @@ fbdev_backend_create(struct weston_compositor *compositor,
>  	if (!fbdev_head_create(backend, param->device))
>  		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:
> @@ -856,10 +935,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;
>  }
>  

-------------- 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/20180629/c89e56d7/attachment.sig>