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

[nerdopolis]

On Monday, January 22, 2018 4:50:35 AM EST Pekka Paalanen wrote:
> On Fri, 29 Dec 2017 13:31:51 -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, use udev to find the
> > first framebuffer device in the seat.
> > ---
> >  libweston/compositor-fbdev.c | 45 +++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 42 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libweston/compositor-fbdev.c b/libweston/compositor-fbdev.c
> > index 39668aa8..34e6c2f3 100644
> > --- a/libweston/compositor-fbdev.c
> > +++ b/libweston/compositor-fbdev.c
> > @@ -711,6 +711,42 @@ 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;
> > +	struct udev_device *device;
> > +
> > +	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)) {
> > +
> > +		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)) {
> > +			fb_device = udev_device_get_devnode(device);
> > +			udev_enumerate_unref(e);
> 
> Hi,
> 
> seems like this should be udev_device_unref() instead, otherwise
> 'e' is unreffed twice and 'device' is leaked.
> 
Patch 7 improves this function somewhat, but I don't really know how to squash 5 and 7 into
5 without squashing 6... ...I guess I could just copy and paste that manually...

With patch 7 applied on top of this, is the only issue that I just need to do
                       fb_device = strdup(find_device);
                       udev_device_unref(device);
to be able to unref device ? 

> > +			break;
> > +		}
> > +		udev_device_unref(device);
> > +	}
> > +
> > +	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 +779,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");
> 
> Missing spaces around '=' on both lines.
> 
> The assigned string is leaked, there is no free() for it. However, one
> must take care to not free() the string that came as an argument via
> weston_fbdev_backend_config struct.
> 
Not sure what I need to do here, regarding the free() TBH.
The issue is with the param->device=strdup("/dev/fb0"); line correct?
Is the issue that the hardcoded "/dev/fb0" string that is strdup'ed stuck in memory?

> > +
> >  	/* Set up the TTY. */
> >  	backend->session_listener.notify = session_notify;
> >  	wl_signal_add(&compositor->session_signal,
> > @@ -790,10 +831,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;
> >  }
> >  
> 
> Otherwise looks good to me.
> 
> 
> Thanks,
> pq
>