[PATCH] fbdev: Add an fbdev compositor backend using pixman and evdev
Philip Withnall
philip at tecnocode.co.uk
Fri Jan 18 02:45:27 PST 2013
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
> >
> > diff --git a/configure.ac b/configure.ac
> > index 64505dd..57a444b 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -154,6 +154,16 @@ fi
> > AM_CONDITIONAL(INSTALL_RPI_COMPOSITOR, test "x$have_bcm_host" = "xyes")
> >
> >
> > +AC_ARG_ENABLE([fbdev-compositor], [ --enable-fbdev-compositor],,
> > + enable_fbdev_compositor=yes)
> > +AM_CONDITIONAL([ENABLE_FBDEV_COMPOSITOR],
> > + [test x$enable_fbdev_compositor = xyes])
> > +AS_IF([test x$enable_fbdev_compositor = xyes], [
> > + AC_DEFINE([BUILD_FBDEV_COMPOSITOR], [1], [Build the fbdev compositor])
> > + PKG_CHECK_MODULES([FBDEV_COMPOSITOR], [libudev >= 136 mtdev >= 1.1.0])
> > +])
> > +
> > +
> > AC_ARG_WITH(cairo-glesv2,
> > AS_HELP_STRING([--with-cairo-glesv2],
> > [Use GLESv2 cairo instead of full GL]))
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index cbfa911..efa819d 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -84,7 +84,8 @@ module_LTLIBRARIES = \
> > $(x11_backend) \
> > $(drm_backend) \
> > $(wayland_backend) \
> > - $(headless_backend)
> > + $(headless_backend) \
> > + $(fbdev_backend)
> >
> > # Do not install, since the binary produced via autotools is unusable.
> > # The real backend is built by the Android build system.
> > @@ -195,6 +196,29 @@ headless_backend_la_CFLAGS = \
> > headless_backend_la_SOURCES = compositor-headless.c
> > endif
> >
> > +if ENABLE_FBDEV_COMPOSITOR
> > +fbdev_backend = fbdev-backend.la
> > +fbdev_backend_la_LDFLAGS = -module -avoid-version
> > +fbdev_backend_la_LIBADD = \
> > + $(COMPOSITOR_LIBS) \
> > + $(FBDEV_COMPOSITOR_LIBS) \
> > + ../shared/libshared.la \
> > + $(NULL)
>
> What does that $(NULL) stuff do?
$(NULL) means nothing (IIRC it expands to the empty string, or a space,
or something meaningless), but allows all the files/libraries/whatever
to be listed on separate lines with their backslashes. This reduces
churn in git diffs and merges.
> > +fbdev_backend_la_CFLAGS = \
> > + $(COMPOSITOR_CFLAGS) \
> > + $(FBDEV_COMPOSITOR_CFLAGS) \
> > + $(PIXMAN_CFLAGS) \
> > + $(GCC_CFLAGS) \
> > + $(NULL)
>
> And here
>
> > +fbdev_backend_la_SOURCES = \
> > + compositor-fbdev.c \
> > + tty.c \
> > + evdev.c \
> > + evdev.h \
> > + evdev-touchpad.c \
> > + $(NULL)
>
> and here
>
> > +endif
> > +
> > if ENABLE_DESKTOP_SHELL
> > desktop_shell = desktop-shell.la
> > desktop_shell_la_LDFLAGS = -module -avoid-version
> > diff --git a/src/compositor-fbdev.c b/src/compositor-fbdev.c
> > new file mode 100644
> > index 0000000..f81534b
> > --- /dev/null
> > +++ b/src/compositor-fbdev.c
> > @@ -0,0 +1,828 @@
> > +/*
> > + * Copyright © 2008-2011 Kristian Høgsberg
> > + * Copyright © 2011 Intel Corporation
> > + * Copyright © 2012 Raspberry Pi Foundation
> > + * Copyright © 2013 Philip Withnall
> > + *
> > + * Permission to use, copy, modify, distribute, and sell this software and
> > + * its documentation for any purpose is hereby granted without fee, provided
> > + * that the above copyright notice appear in all copies and that both that
> > + * copyright notice and this permission notice appear in supporting
> > + * documentation, and that the name of the copyright holders not be used in
> > + * advertising or publicity pertaining to distribution of the software
> > + * without specific, written prior permission. The copyright holders make
> > + * no representations about the suitability of this software for any
> > + * purpose. It is provided "as is" without express or implied warranty.
> > + *
> > + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> > + * SOFTWARE, INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
> > + * FITNESS, IN NO EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY
> > + * SPECIAL, INDIRECT OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER
> > + * RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF
> > + * CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
> > + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > + */
> > +
> > +#define _GNU_SOURCE
> > +
> > +#include <errno.h>
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <math.h>
> > +#include <sys/mman.h>
> > +#include <sys/types.h>
> > +#include <fcntl.h>
> > +#include <unistd.h>
> > +#include <linux/fb.h>
> > +
> > +#include <libudev.h>
> > +
> > +#include "config.h"
> > +
> > +#include "compositor.h"
> > +#include "pixman-renderer.h"
> > +#include "evdev.h"
> > +
> > +/* Enabling this debugging incurs a significant performance hit */
> > +#if 0
> > +#define DBG(...) \
> > + weston_log(__VA_ARGS__)
> > +#else
> > +#define DBG(...) do {} while (0)
> > +#endif
>
> I'd actually recommend to add global debugging helpers to weston and
> avoid doing this per-backend. But maybe it's ok if only used for
> debugging. Kristian should comment on that.
I copied DBG from the rpi compositor (which is what the fbdev compositor
is heavily based on). It was only actually used once in the fbdev
compositor, so I've removed it.
> > +
> > +struct fbdev_compositor;
> > +struct fbdev_output;
>
> I don't think this forward declaration is needed.
Removed.
> > +
> > +struct fbdev_output {
> > + struct fbdev_compositor *compositor;
> > + struct weston_output base;
> > +
> > + struct weston_mode mode;
> > + struct wl_event_source *finish_frame_timer;
> > +
> > + /* Frame buffer details. */
> > + struct fb_fix_screeninfo fixed_info;
> > + struct fb_var_screeninfo variable_info;
> > + void *fb;
> > + size_t fb_len;
> > +
> > + /* pixman details. */
> > + pixman_image_t *hw_surface;
> > + pixman_image_t *shadow_surface;
> > + void *shadow_buf;
> > + uint8_t depth;
> > +};
> > +
> > +struct fbdev_seat {
> > + struct weston_seat base;
> > + struct wl_list devices_list;
> > +
> > + struct udev_monitor *udev_monitor;
> > + struct wl_event_source *udev_monitor_source;
> > + char *seat_id;
> > +};
> > +
> > +struct fbdev_compositor {
> > + struct weston_compositor base;
> > + uint32_t prev_state;
> > +
> > + struct udev *udev;
> > + struct tty *tty;
> > +};
> > +
> > +struct fbdev_parameters {
> > + int tty;
> > + char *device;
> > +};
> > +
> > +static inline struct fbdev_output *
> > +to_fbdev_output(struct weston_output *base)
> > +{
> > + return container_of(base, struct fbdev_output, base);
> > +}
> > +
> > +static inline struct fbdev_seat *
> > +to_fbdev_seat(struct weston_seat *base)
> > +{
> > + return container_of(base, struct fbdev_seat, base);
> > +}
> > +
> > +static inline struct fbdev_compositor *
> > +to_fbdev_compositor(struct weston_compositor *base)
> > +{
> > + return container_of(base, struct fbdev_compositor, base);
> > +}
> > +
> > +static void
> > +fbdev_output_repaint(struct weston_output *base, pixman_region32_t *damage)
> > +{
> > + struct fbdev_output *output = to_fbdev_output(base);
> > + struct weston_compositor *ec = output->base.compositor;
> > + pixman_box32_t *rects;
> > + int nrects, i, src_x, src_y, x1, y1, x2, y2, width, height;
> > +
> > + /* Repaint the damaged region onto the back buffer. */
> > + pixman_renderer_output_set_buffer(base, output->shadow_surface);
> > + ec->renderer->repaint_output(base, damage);
> > +
> > + /* Transform and composite onto the frame buffer. */
> > + width = pixman_image_get_width(output->shadow_surface);
> > + height = pixman_image_get_height(output->shadow_surface);
> > + rects = pixman_region32_rectangles(damage, &nrects);
> > +
> > + for (i = 0; i < nrects; i++) {
> > + switch (base->transform) {
> > + default:
> > + case WL_OUTPUT_TRANSFORM_NORMAL:
> > + x1 = rects[i].x1;
> > + x2 = rects[i].x2;
> > + y1 = rects[i].y1;
> > + y2 = rects[i].y2;
> > + break;
> > + case WL_OUTPUT_TRANSFORM_180:
> > + x1 = width - rects[i].x2;
> > + x2 = width - rects[i].x1;
> > + y1 = height - rects[i].y2;
> > + y2 = height - rects[i].y1;
> > + break;
> > + case WL_OUTPUT_TRANSFORM_90:
> > + x1 = height - rects[i].y2;
> > + x2 = height - rects[i].y1;
> > + y1 = rects[i].x1;
> > + y2 = rects[i].x2;
> > + break;
> > + case WL_OUTPUT_TRANSFORM_270:
> > + x1 = rects[i].y1;
> > + x2 = rects[i].y2;
> > + y1 = width - rects[i].x2;
> > + y2 = width - rects[i].x1;
> > + break;
> > + }
> > + src_x = x1;
> > + src_y = y1;
> > +
> > + pixman_image_composite32(PIXMAN_OP_SRC,
> > + output->shadow_surface, /* src */
> > + NULL /* mask */,
> > + output->hw_surface, /* dest */
> > + src_x, src_y, /* src_x, src_y */
> > + 0, 0, /* mask_x, mask_y */
> > + x1, y1, /* dest_x, dest_y */
> > + x2 - x1, /* width */
> > + y2 - y1 /* height */);
>
> Why not pixman_blt()? You don't do any compositing here, it's a simply
> copy, right?
As Vasily says, compositing is needed when doing rotations.
> > + }
> > +
> > + /* Update the damage region. */
> > + pixman_region32_subtract(&ec->primary_plane.damage,
> > + &ec->primary_plane.damage, damage);
> > +
> > + /* Schedule the end of the frame. TODO: Sync this to FB clock? */
> > + wl_event_source_timer_update(output->finish_frame_timer, 10);
>
> Yeah, don't do VSync for fbdev. You could do FB_ACTIVATE_VBL or
> FBIO_WAITFORVSYNC but I don't think that we should do that for fbdev.
> If users want tear-free output, they should use DRM devices. The fbdev
> API is crappy. FBIO_WAITFORVSYNC is blocking so not suitable here and
> FB_ACTIVATE_VBL requires panning (FBIOPAN_DISPLAY) and that is
> horribly broken in most kernel drivers.
>
> I recommend just using a shadow buffer and blitting the stuff on
> repaint. The page-flip should simply be timed to the refresh-rate that
> we read below in fbdev_output_create.
I've added a version of the code you suggested for calculating the
refresh rate, and used it here, setting the timer period to (1000000 /
refresh_rate). I've also adapted your feedback into a comment explaining
why vsync isn't being used.
> > +}
> > +
> > +static int
> > +finish_frame_handler(void *data)
> > +{
> > + struct fbdev_output *output = data;
> > + uint32_t msec;
> > + struct timeval tv;
> > +
> > + gettimeofday(&tv, NULL);
> > + msec = tv.tv_sec * 1000 + tv.tv_usec / 1000;
> > + weston_output_finish_frame(&output->base, msec);
> > +
> > + return 1;
> > +}
> > +
> > +static int
> > +fbdev_frame_buffer_create(struct fbdev_output *output, const char *fb_dev)
> > +{
> > + int retval = -1;
> > + int fd = -1;
> > +
> > + /* Open the frame buffer device. */
> > + fd = open (fb_dev, O_RDWR);
>
> O_CLOEXEC please even though this is opened only for the time of this
> function. But you never know whether there might be other threads
> forking at the same time. Better be safe here.
Whoops. Added.
> > + if (fd < 0) {
> > + weston_log("Failed to open frame buffer device ‘%s’: %s\n",
> > + fb_dev, strerror(errno));
> > + goto err;
> > + }
> > +
> > + /* Grab the screen info. */
> > + if (ioctl(fd, FBIOGET_FSCREENINFO, &output->fixed_info) < 0 ||
> > + ioctl(fd, FBIOGET_VSCREENINFO, &output->variable_info) < 0) {
> > + weston_log("Failed to get frame buffer info: %s\n",
> > + strerror(errno));
> > + goto err;
> > + }
> > +
> > + /* Map the frame buffer. */
> > + output->fb = mmap (NULL, output->fixed_info.smem_len,
> > + PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>
> Why PROT_READ? We should never read the framebuffer data.
A mistake. Removed.
> > + if (output->fb == NULL) {
>
> Check for MAP_FAILED instead of NULL please.
Whoops. Fixed.
> > + weston_log("Failed to mmap frame buffer: %s\n",
> > + strerror(errno));
> > + goto err;
> > + }
> > +
> > + /* Success! */
> > + output->fb_len = output->fixed_info.smem_len;
> > + retval = 0;
> > +
> > +err:
> > + if (fd >= 0)
> > + close (fd);
> > +
> > + return retval;
> > +}
> > +
> > +static void
> > +fbdev_frame_buffer_destroy(struct fbdev_output *output)
> > +{
> > + if (munmap(output->fb, output->fb_len) < 0)
> > + weston_log("Failed to munmap frame buffer: %s\n",
> > + strerror(errno));
> > +
> > + output->fb = NULL;
> > + output->fb_len = 0;
> > +}
> > +
> > +static void fbdev_output_destroy(struct weston_output *base);
> > +
> > +static int
> > +fbdev_output_create(struct fbdev_compositor *compositor,
> > + struct fbdev_parameters *param)
> > +{
> > + struct fbdev_output *output;
> > + pixman_transform_t transform;
> > + int shadow_width, shadow_height;
> > + int width, height;
> > + pixman_format_code_t pixman_format;
> > + struct wl_event_loop *loop;
> > +
> > + output = calloc(1, sizeof *output);
> > + if (!output)
> > + return -1;
> > +
> > + output->compositor = compositor;
> > +
> > + /* Create the frame buffer. */
> > + if (fbdev_frame_buffer_create(output, param->device) < 0) {
> > + weston_log("Creating frame buffer failed.\n");
> > + goto out_free;
> > + }
> > +
> > + /* Check its properties. TODO: Try a bit harder to support other
> > + * formats, including setting the preferred format in the hardware. */
> > + if (output->variable_info.bits_per_pixel != 32 ||
> > + output->variable_info.red.offset != 16 ||
> > + output->variable_info.red.length != 8 ||
> > + output->variable_info.red.msb_right != 0 ||
> > + output->variable_info.green.offset != 8 ||
> > + output->variable_info.green.length != 8 ||
> > + output->variable_info.green.msb_right != 0 ||
> > + output->variable_info.blue.offset != 0 ||
> > + output->variable_info.blue.length != 8 ||
> > + output->variable_info.blue.msb_right != 0) {
> > + weston_log("Frame buffer doesn't support x8r8g8b8 format.\n");
> > + goto out_fb;
> > + }
>
> I would recommend setting a "xrgb32 = true" flag for the output here
> and adding a fallback to the renderer path that converts xrgb32 input
> to the output-device like this:
>
> uint32_t pixel = input, out;
> uint8_t r, g, b;
>
> if (fbdev->xrgb32) {
> out = pixel;
> } else {
> r = (pixel >> 16) & 0xff;
> g = (pixel >> 8) & 0xff;
> b = (pixel >> 0) & 0xff;
> out = (r >> (8 - fbdev->len_r)) << fbdev->off_r;
> out |= (g >> (8 - fbdev->len_g)) << fbdev->off_g;
> out |= (b >> (8 - fbdev->len_b)) << fbdev->off_b;
> }
>
> This converts pixel (xrgb32) to the required output format. ALthough
> this requires the output format to be 32bits max (which sounds
> reasonable). You would have to replace the pixman_composite() with a
> for() loop to copy the data then.
>
> For instance the udlfb (DisplayLink) driver only provides rgb16
> formats so only allowing xrgb32 seems a little bit too restricting. We
> could also add dithering but that's not required now I think.
>
> The xrgb32 field guarantees a faster path for common xrgb32 devices.
> Especially if we do dithering we should special case xrgb32 to speed
> it up.
>
> Or maybe just add support for some more pixman formats. On the other
> hand, we can also add that later so xrgb32 may be ok for now.
I've added support for constructing the pixman_format_code_t from the
variable and fixed frame buffer info. That should allow support for a
wider variety of formats. The main missing ones at the moment are
non-true-colour formats (greyscale/monochrome/etc.) and big-endian
formats (MSB on the right).
Given the right pixman_format_code_t, pixman should be able to do the
conversion internally when compositing, I think.
> > +
> > + pixman_format = PIXMAN_x8r8g8b8;
>
> Please also check for fixed_info->visual == FB_VISUAL_TRUECOLOR
> otherwise the red/green/blue length/offset values are not guaranteed
> to be what you expect here.
Fixed.
> > +
> > + output->base.repaint = fbdev_output_repaint;
> > + output->base.destroy = fbdev_output_destroy;
> > + output->base.assign_planes = NULL;
> > + output->base.set_backlight = NULL;
> > + output->base.set_dpms = NULL;
> > + output->base.switch_mode = NULL;
> > +
> > + /* only one static mode in list */
> > + output->mode.flags =
> > + WL_OUTPUT_MODE_CURRENT | WL_OUTPUT_MODE_PREFERRED;
> > + output->mode.width = output->variable_info.xres;
> > + output->mode.height = output->variable_info.yres;
> > + output->mode.refresh = 60000; /* 60kHz, TODO: this is a guess */
>
> You can actually try to compute that value from the fbdev information:
>
> /* calculate monitor rate, default is 60 Hz */
> uint64_t quot;
> quot = (vinfo->upper_margin + vinfo->lower_margin + vinfo->yres);
> quot *= (vinfo->left_margin + vinfo->right_margin + vinfo->xres);
> quot *= vinfo->pixclock;
> if (quot) {
> quot = 1000000000000000LLU / quot;
> if (!quot)
> quot = 0;
> else if (quot > 200000)
> quot = 200000
> output->mode.refresh = quot;
> } else {
> output->mode.refresh = 60 * 1000;
> }
I've added this code, as mentioned above. Thanks.
> > + wl_list_init(&output->base.mode_list);
> > + wl_list_insert(&output->base.mode_list, &output->mode.link);
> > +
> > + output->base.current = &output->mode;
> > + output->base.origin = &output->mode;
> > + output->base.subpixel = WL_OUTPUT_SUBPIXEL_UNKNOWN;
> > + output->base.make = "fbdev";
> > + output->base.model = output->fixed_info.id;
> > +
> > + weston_output_init(&output->base, &compositor->base,
> > + 0, 0, output->variable_info.width,
> > + output->variable_info.height,
> > + WL_OUTPUT_TRANSFORM_NORMAL);
>
> Error checking?
Error checking what? None of the functions in the above block return
error values as far as I can see.
> > + /* Create a pixman image to wrap the memory mapped frame buffer. */
> > + width = output->variable_info.xres;
> > + height = output->variable_info.yres;
> > +
> > + output->hw_surface =
> > + pixman_image_create_bits(pixman_format, width, height,
> > + output->fb,
> > + output->fixed_info.line_length);
>
> Error checking?
Added.
> > +
> > + pixman_transform_init_identity(&transform);
> > + switch (output->base.transform) {
> > + default:
> > + case WL_OUTPUT_TRANSFORM_NORMAL:
> > + shadow_width = width;
> > + shadow_height = height;
> > + pixman_transform_rotate(&transform,
> > + NULL, 0, 0);
> > + pixman_transform_translate(&transform, NULL,
> > + 0, 0);
> > + break;
> > + case WL_OUTPUT_TRANSFORM_180:
> > + shadow_width = width;
> > + shadow_height = height;
> > + pixman_transform_rotate(&transform,
> > + NULL, -pixman_fixed_1, 0);
> > + pixman_transform_translate(NULL, &transform,
> > + pixman_int_to_fixed(shadow_width),
> > + pixman_int_to_fixed(shadow_height));
> > + break;
> > + case WL_OUTPUT_TRANSFORM_270:
> > + shadow_width = height;
> > + shadow_height = width;
> > + pixman_transform_rotate(&transform,
> > + NULL, 0, pixman_fixed_1);
> > + pixman_transform_translate(&transform,
> > + NULL,
> > + pixman_int_to_fixed(shadow_width),
> > + 0);
> > + break;
> > + case WL_OUTPUT_TRANSFORM_90:
> > + shadow_width = height;
> > + shadow_height = width;
> > + pixman_transform_rotate(&transform,
> > + NULL, 0, -pixman_fixed_1);
> > + pixman_transform_translate(&transform,
> > + NULL,
> > + 0,
> > + pixman_int_to_fixed(shadow_height));
> > + break;
> > + }
> > + output->shadow_buf = malloc(width * height * (output->variable_info.bits_per_pixel / 8));
> > + output->shadow_surface = pixman_image_create_bits(pixman_format, shadow_width, shadow_height,
> > + output->shadow_buf, shadow_width * (output->variable_info.bits_per_pixel / 8));
>
> Error checking?
Added.
> > + /* No need in transform for normal output */
> > + if (output->base.transform != WL_OUTPUT_TRANSFORM_NORMAL)
> > + pixman_image_set_transform(output->shadow_surface, &transform);
> > +
> > + if (pixman_renderer_output_create(&output->base) < 0)
> > + goto out_output;
> > +
> > + loop = wl_display_get_event_loop(compositor->base.wl_display);
> > + output->finish_frame_timer =
> > + wl_event_loop_add_timer(loop, finish_frame_handler, output);
> > +
> > + wl_list_insert(compositor->base.output_list.prev, &output->base.link);
> > +
> > + weston_log("fbdev output %d×%d px\n",
> > + output->mode.width, output->mode.height);
> > + weston_log_continue(STAMP_SPACE "guessing %d Hz and 96 dpi\n",
> > + output->mode.refresh / 1000);
> > +
> > + return 0;
> > +
> > +out_output:
> > + weston_output_destroy(&output->base);
> > +out_fb:
> > + fbdev_frame_buffer_destroy(output);
> > +out_free:
> > + free(output);
> > +
> > + return -1;
> > +}
> > +
> > +static void
> > +fbdev_output_destroy(struct weston_output *base)
> > +{
> > + struct fbdev_output *output = to_fbdev_output(base);
> > +
> > + DBG("%s\n", __func__);
> > +
> > + pixman_renderer_output_destroy(base);
> > +
> > + pixman_image_unref(output->hw_surface);
> > + output->hw_surface = NULL;
> > +
> > + pixman_image_unref(output->shadow_surface);
> > + output->shadow_surface = NULL;
> > +
> > + fbdev_frame_buffer_destroy(output);
> > +
> > + wl_list_remove(&output->base.link);
> > + weston_output_destroy(&output->base);
> > +
> > + free(output);
> > +}
> > +
> > +static void
> > +fbdev_led_update(struct weston_seat *seat_base, enum weston_led leds)
> > +{
> > + struct fbdev_seat *seat = to_fbdev_seat(seat_base);
> > + struct evdev_device *device;
> > +
> > + wl_list_for_each(device, &seat->devices_list, link)
> > + evdev_led_update(device, leds);
> > +}
> > +
> > +static const char default_seat[] = "seat0";
> > +
> > +static void
> > +device_added(struct udev_device *udev_device, struct fbdev_seat *master)
> > +{
> > + struct evdev_device *device;
> > + const char *devnode;
> > + const char *device_seat;
> > + int fd;
> > +
> > + device_seat = udev_device_get_property_value(udev_device, "ID_SEAT");
> > + if (!device_seat)
> > + device_seat = default_seat;
> > +
> > + if (strcmp(device_seat, master->seat_id))
> > + return;
> > +
> > + devnode = udev_device_get_devnode(udev_device);
>
> devnode might be NULL.
Added error handling. This code was copied from the rpi backend, so that
will also need updating.
> > +
> > + /* Use non-blocking mode so that we can loop on read on
> > + * evdev_device_data() until all events on the fd are
> > + * read. mtdev_get() also expects this. */
> > + fd = open(devnode, O_RDWR | O_NONBLOCK | O_CLOEXEC);
> > + if (fd < 0) {
> > + weston_log("opening input device '%s' failed.\n", devnode);
> > + return;
> > + }
> > +
> > + device = evdev_device_create(&master->base, devnode, fd);
> > + if (!device) {
> > + close(fd);
> > + weston_log("not using input device '%s'.\n", devnode);
> > + return;
> > + }
> > +
> > + wl_list_insert(master->devices_list.prev, &device->link);
> > +}
> > +
> > +static void
> > +evdev_add_devices(struct udev *udev, struct weston_seat *seat_base)
> > +{
> > + struct fbdev_seat *seat = to_fbdev_seat(seat_base);
> > + struct udev_enumerate *e;
> > + struct udev_list_entry *entry;
> > + struct udev_device *device;
> > + const char *path, *sysname;
> > +
> > + e = udev_enumerate_new(udev);
> > + udev_enumerate_add_match_subsystem(e, "input");
> > + udev_enumerate_scan_devices(e);
>
> Please do error checking on all those functions here.
Added. Again, this code was copied from the rpi backend, so that will
need updating.
> > + udev_list_entry_foreach(entry, udev_enumerate_get_list_entry(e)) {
> > + path = udev_list_entry_get_name(entry);
> > + device = udev_device_new_from_syspath(udev, path);
> > +
> > + sysname = udev_device_get_sysname(device);
> > + if (strncmp("event", sysname, 5) != 0) {
> > + udev_device_unref(device);
> > + continue;
> > + }
> > +
> > + device_added(device, seat);
> > +
> > + udev_device_unref(device);
> > + }
> > + udev_enumerate_unref(e);
> > +
> > + evdev_notify_keyboard_focus(&seat->base, &seat->devices_list);
> > +
> > + if (wl_list_empty(&seat->devices_list)) {
> > + weston_log(
> > + "warning: no input devices on entering Weston. "
> > + "Possible causes:\n"
> > + "\t- no permissions to read /dev/input/event*\n"
> > + "\t- seats misconfigured "
> > + "(Weston backend option 'seat', "
> > + "udev device property ID_SEAT)\n");
> > + }
> > +}
> > +
> > +static int
> > +evdev_udev_handler(int fd, uint32_t mask, void *data)
> > +{
> > + struct fbdev_seat *seat = data;
> > + struct udev_device *udev_device;
> > + struct evdev_device *device, *next;
> > + const char *action;
> > + const char *devnode;
> > +
> > + udev_device = udev_monitor_receive_device(seat->udev_monitor);
> > + if (!udev_device)
> > + return 1;
> > +
> > + action = udev_device_get_action(udev_device);
> > + if (!action)
> > + goto out;
> > +
> > + if (strncmp("event", udev_device_get_sysname(udev_device), 5) != 0)
> > + goto out;
> > +
> > + if (!strcmp(action, "add")) {
> > + device_added(udev_device, seat);
> > + }
> > + else if (!strcmp(action, "remove")) {
> > + devnode = udev_device_get_devnode(udev_device);
> > + wl_list_for_each_safe(device, next, &seat->devices_list, link)
> > + if (!strcmp(device->devnode, devnode)) {
> > + weston_log("input device %s, %s removed\n",
> > + device->devname, device->devnode);
> > + evdev_device_destroy(device);
> > + break;
> > + }
> > + }
> > +
> > +out:
> > + udev_device_unref(udev_device);
> > +
> > + return 0;
> > +}
> > +
> > +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.
> > +
> > + if (udev_monitor_enable_receiving(master->udev_monitor)) {
> > + weston_log("udev: failed to bind the udev monitor\n");
> > + udev_monitor_unref(master->udev_monitor);
> > + return 0;
> > + }
> > +
> > + loop = wl_display_get_event_loop(c->wl_display);
> > + fd = udev_monitor_get_fd(master->udev_monitor);
> > + master->udev_monitor_source =
> > + wl_event_loop_add_fd(loop, fd, WL_EVENT_READABLE,
> > + evdev_udev_handler, master);
> > + if (!master->udev_monitor_source) {
> > + udev_monitor_unref(master->udev_monitor);
> > + return 0;
> > + }
> > +
> > + return 1;
> > +}
> > +
> > +static void
> > +evdev_disable_udev_monitor(struct weston_seat *seat_base)
> > +{
> > + struct fbdev_seat *seat = to_fbdev_seat(seat_base);
> > +
> > + if (!seat->udev_monitor)
> > + return;
> > +
> > + udev_monitor_unref(seat->udev_monitor);
> > + seat->udev_monitor = NULL;
> > + wl_event_source_remove(seat->udev_monitor_source);
> > + seat->udev_monitor_source = NULL;
> > +}
> > +
> > +static void
> > +evdev_input_create(struct weston_compositor *c, struct udev *udev,
> > + const char *seat_id)
> > +{
> > + struct fbdev_seat *seat;
> > +
> > + seat = malloc(sizeof *seat);
> > + if (seat == NULL)
> > + return;
> > +
> > + memset(seat, 0, sizeof *seat);
> > + weston_seat_init(&seat->base, c);
> > + seat->base.led_update = fbdev_led_update;
> > +
> > + wl_list_init(&seat->devices_list);
> > + seat->seat_id = strdup(seat_id);
>
> Check for (!seat->seat_id) please.
Fixed. Copied from the rpi backend again.
> > + if (!evdev_enable_udev_monitor(udev, &seat->base)) {
> > + free(seat->seat_id);
> > + free(seat);
> > + return;
> > + }
> > +
> > + evdev_add_devices(udev, &seat->base);
> > +}
> > +
> > +static void
> > +evdev_remove_devices(struct weston_seat *seat_base)
> > +{
> > + struct fbdev_seat *seat = to_fbdev_seat(seat_base);
> > + struct evdev_device *device, *next;
> > +
> > + wl_list_for_each_safe(device, next, &seat->devices_list, link)
> > + evdev_device_destroy(device);
> > +
> > + if (seat->base.seat.keyboard)
> > + notify_keyboard_focus_out(&seat->base);
> > +}
> > +
> > +static void
> > +evdev_input_destroy(struct weston_seat *seat_base)
> > +{
> > + struct fbdev_seat *seat = to_fbdev_seat(seat_base);
> > +
> > + evdev_remove_devices(seat_base);
> > + evdev_disable_udev_monitor(&seat->base);
> > +
> > + weston_seat_release(seat_base);
> > + free(seat->seat_id);
> > + free(seat);
> > +}
> > +
> > +static void
> > +fbdev_compositor_destroy(struct weston_compositor *base)
> > +{
> > + struct fbdev_compositor *compositor = to_fbdev_compositor(base);
> > + struct weston_seat *seat, *next;
> > +
> > + /* Destroy all inputs. */
> > + wl_list_for_each_safe(seat, next, &compositor->base.seat_list, link)
> > + evdev_input_destroy(seat);
> > +
> > + /* Destroy the output. */
> > + weston_compositor_shutdown(&compositor->base);
> > +
> > + /* Chain up. */
> > + compositor->base.renderer->destroy(&compositor->base);
> > + tty_destroy(compositor->tty);
> > +
> > + free(compositor);
> > +}
> > +
> > +static void
> > +vt_func(struct weston_compositor *base, int event)
> > +{
> > + struct fbdev_compositor *compositor = to_fbdev_compositor(base);
> > + struct weston_seat *seat;
> > + struct weston_output *output;
> > +
> > + switch (event) {
> > + case TTY_ENTER_VT:
> > + weston_log("entering VT\n");
> > + compositor->base.focus = 1;
> > + compositor->base.state = compositor->prev_state;
> > + weston_compositor_damage_all(&compositor->base);
> > + wl_list_for_each(seat, &compositor->base.seat_list, link) {
> > + evdev_add_devices(compositor->udev, seat);
> > + evdev_enable_udev_monitor(compositor->udev, seat);
> > + }
> > + break;
> > + case TTY_LEAVE_VT:
> > + weston_log("leaving VT\n");
> > + wl_list_for_each(seat, &compositor->base.seat_list, link) {
> > + evdev_disable_udev_monitor(seat);
> > + evdev_remove_devices(seat);
> > + }
> > +
> > + compositor->base.focus = 0;
> > + compositor->prev_state = compositor->base.state;
> > + compositor->base.state = WESTON_COMPOSITOR_SLEEPING;
> > +
> > + /* If we have a repaint scheduled (from the idle handler), make
> > + * sure we cancel that so we don't try to pageflip when we're
> > + * vt switched away. The SLEEPING state will prevent
> > + * further attemps at repainting. When we switch
> > + * back, we schedule a repaint, which will process
> > + * pending frame callbacks. */
> > +
> > + wl_list_for_each(output,
> > + &compositor->base.output_list, link) {
> > + output->repaint_needed = 0;
> > + }
> > +
> > + break;
> > + };
> > +}
> > +
> > +static void
> > +fbdev_restore(struct weston_compositor *base)
> > +{
> > + struct fbdev_compositor *compositor = to_fbdev_compositor(base);
> > +
> > + tty_reset(compositor->tty);
> > +}
> > +
> > +static void
> > +switch_vt_binding(struct wl_seat *seat, uint32_t time, uint32_t key, void *data)
> > +{
> > + struct fbdev_compositor *ec = data;
> > +
> > + tty_activate_vt(ec->tty, key - KEY_F1 + 1);
> > +}
> > +
> > +static struct weston_compositor *
> > +fbdev_compositor_create(struct wl_display *display, int argc, char *argv[],
> > + const char *config_file, struct fbdev_parameters *param)
> > +{
> > + struct fbdev_compositor *compositor;
> > + const char *seat = default_seat;
> > + uint32_t key;
> > +
> > + weston_log("initializing fbdev backend\n");
> > +
> > + compositor = calloc(1, sizeof *compositor);
> > + if (compositor == NULL)
> > + return NULL;
> > +
> > + if (weston_compositor_init(&compositor->base, display, argc, argv,
> > + config_file) < 0)
> > + goto out_free;
> > +
> > + compositor->udev = udev_new();
> > + if (compositor->udev == NULL) {
> > + weston_log("Failed to initialize udev context.\n");
> > + goto out_compositor;
> > + }
> > +
> > + /* Set up the TTY. */
> > + compositor->tty = tty_create(&compositor->base, vt_func, param->tty);
> > + if (!compositor->tty) {
> > + weston_log("Failed to initialize tty.\n");
> > + goto out_udev;
> > + }
> > +
> > + compositor->base.destroy = fbdev_compositor_destroy;
> > + compositor->base.restore = fbdev_restore;
> > +
> > + compositor->base.focus = 1;
> > + compositor->prev_state = WESTON_COMPOSITOR_ACTIVE;
> > +
> > + for (key = KEY_F1; key < KEY_F9; key++)
> > + weston_compositor_add_key_binding(&compositor->base, key,
> > + MODIFIER_CTRL | MODIFIER_ALT,
> > + switch_vt_binding,
> > + compositor);
> > +
> > + if (pixman_renderer_init(&compositor->base) < 0)
> > + goto out_tty;
> > +
> > + if (fbdev_output_create(compositor, param) < 0)
> > + goto out_pixman;
> > +
> > + evdev_input_create(&compositor->base, compositor->udev, seat);
> > +
> > + return &compositor->base;
> > +
> > +out_pixman:
> > + compositor->base.renderer->destroy(&compositor->base);
> > +
> > +out_tty:
> > + tty_destroy(compositor->tty);
> > +
> > +out_udev:
> > + udev_unref(compositor->udev);
> > +
> > +out_compositor:
> > + weston_compositor_shutdown(&compositor->base);
> > +
> > +out_free:
> > + free(compositor);
> > +
> > + return NULL;
> > +}
> > +
> > +WL_EXPORT struct weston_compositor *
> > +backend_init(struct wl_display *display, int argc, char *argv[],
> > + const char *config_file)
> > +{
> > + struct fbdev_parameters param = {
> > + .tty = 0, /* default to current tty */
> > + .device = "/dev/fb0", /* default frame buffer */
>
> 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.
Philip
> > + };
> > +
> > + const struct weston_option fbdev_options[] = {
> > + { WESTON_OPTION_INTEGER, "tty", 0, ¶m.tty },
> > + { WESTON_OPTION_STRING, "device", 0, ¶m.device },
> > + };
> > +
> > + parse_options(fbdev_options, ARRAY_LENGTH(fbdev_options), argc, argv);
> > +
> > + return fbdev_compositor_create(display, argc, argv, config_file,
> > + ¶m);
> > +}
> > --
> > 1.7.11.7
> >
> >
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 230 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20130118/fe04a2b8/attachment-0001.pgp>
More information about the wayland-devel
mailing list