[PATCH] fbdev: Add an fbdev compositor backend using pixman and evdev

[Pekka Paalanen]

On Fri, 18 Jan 2013 10:45:27 +0000
Philip Withnall <philip at tecnocode.co.uk> wrote:

> Hi David,
> 
> Thanks for the review. Replies inline, and a new patch coming up soon.
> 
> On Wed, 2013-01-16 at 21:22 +0100, David Herrmann wrote:
> > Hi Philip
> > 
> > I like the simple design of this. I don't think we need to share much
> > of the code as it's pretty simple.
> > 
> > I haven't checked all of it, but some comments below.
> > 
> > Thanks
> > David
> > 
> > On Tue, Jan 15, 2013 at 3:08 PM, Philip Withnall <philip at tecnocode.co.uk> wrote:
> > > This is an initial version of an fbdev backend for Weston. I don't
> > > consider it polished; I'm just looking for rough feedback at the
> > > moment. The work is also available as a gitorious branch if anyone
> > > prefers that:
> > > https://gitorious.org/~pwithnall/weston/pwithnalls-weston/commits/compositor-fbdev
> > >
> > > One concern I have is that a lot of the input/evdev code was copied
> > > verbatim from the rpi backend. It should probably be factored out into
> > > evdev.c or something.
> > >
> > >
> > > Initial version of a frame buffer backend using pixman to render to fbdev.
> > > This has been tested against nouveaufb but nothing else. Much of the code
> > > came straight from the rpi backend (and copyright has been attributed
> > > accordingly).
> > >
> > > The behaviour of this backend on less modern frame buffers has yet to be
> > > tested.
> > >
> > > Signed-off-by: Philip Withnall <philip at tecnocode.co.uk>
> > > ---
> > >  configure.ac           |  10 +
> > >  src/Makefile.am        |  26 +-
> > >  src/compositor-fbdev.c | 828 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 863 insertions(+), 1 deletion(-)
> > >  create mode 100644 src/compositor-fbdev.c
...
> > > +static int
> > > +evdev_enable_udev_monitor(struct udev *udev, struct weston_seat *seat_base)
> > > +{
> > > +       struct fbdev_seat *master = to_fbdev_seat(seat_base);
> > > +       struct wl_event_loop *loop;
> > > +       struct weston_compositor *c = master->base.compositor;
> > > +       int fd;
> > > +
> > > +       master->udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
> > > +       if (!master->udev_monitor) {
> > > +               weston_log("udev: failed to create the udev monitor\n");
> > > +               return 0;
> > > +       }
> > > +
> > > +       udev_monitor_filter_add_match_subsystem_devtype(master->udev_monitor,
> > > +                                                       "input", NULL);
> > 
> > Please check for return value here, otherwise you might get all
> > devices if this match-criteria cannot be added.
> 
> Fixed. Again, this was copied from rpi.
> 

I think most of the code mentioned here was originally copied from the
DRM backend to the rpi backend, at least I do not recall working with
udev API much. So it's likely that all backends using udev will need
the added checks. Looks like sharing that code could have some benefits.

> > 
> > Why not discover that device via udev? That would avoid any hard-coded
> > paths. And you could do an fbdev_output_create() on all fbdev devices
> > you find.
> 
> Good idea. It's not on my list of priorities though, since I (completely
> selfishly) put together this backend for my own use on FreeBSD, which
> doesn't have udev.

Ah, I've been wondering about udev on FreeBSD a bit. You would
probably want two flavours of fbdev backend, one with udev and one
without (or with something else). Do you have any plans on maintaining
a FreeBSD/fbdev backend? Would it be something worth getting in Weston
upstream?

The Android backend was perhaps helping you, since it didn't have
udev available, either, and required the shared input code to support
that.


Thanks,
pq