Re: [Qemu-devel] [PATCH 7/9] fbdev: move to pixman

[Stefano Stabellini]

On Wed, 19 Sep 2012, Gerd Hoffmann wrote:
> Stop reinventing the wheel, use the pixman library for raster ops.
> Also add setdata callback.
> 
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  configure  |   12 ++++
>  ui/fbdev.c |  182 
> +++++++++++++++++++++++++++++++++++++-----------------------
>  2 files changed, 124 insertions(+), 70 deletions(-)
> 
> diff --git a/configure b/configure
> index c4ba338..d10ff78 100755
> --- a/configure
> +++ b/configure
> @@ -148,6 +148,7 @@ docs=""
>  fdt=""
>  nptl=""
>  sdl=""
> +pixman=""
>  fbdev="no"
>  virtfs=""
>  vnc="yes"
> @@ -2153,6 +2154,17 @@ else
>      exit 1
>  fi
>  
> +if $pkg_config pixman-1 > /dev/null 2>&1
> +then
> +    pixman="yes"
> +    pixman_cflags=`$pkg_config --cflags pixman-1 2>/dev/null`
> +    pixman_libs=`$pkg_config --libs pixman-1 2>/dev/null`
> +    QEMU_CFLAGS="$QEMU_CFLAGS $pixman_cflags"
> +    libs_softmmu="$libs_softmmu $pixman_libs"
> +else
> +    fbdev="no"
> +fi
> +
>  ##########################################
>  # libcap probe
>  
> diff --git a/ui/fbdev.c b/ui/fbdev.c
> index 6dfbacc..d55850e 100644
> --- a/ui/fbdev.c
> +++ b/ui/fbdev.c
> @@ -23,11 +23,12 @@
>  #include <linux/vt.h>
>  #include <linux/fb.h>
>  
> +#include <pixman.h>
> +
>  #include "qemu-common.h"
>  #include "console.h"
>  #include "keymaps.h"
>  #include "sysemu.h"
> -#include "pflib.h"
>  
>  /*
>   * must be last so we get the linux input layer
> @@ -70,19 +71,82 @@ static bool                       key_down[KEY_CNT];
>  #define FB_ACQ_REQ   3
>  static int fb_switch_state;
>  
> -/* qdev windup */
> +/* qemu windup */
>  static DisplayChangeListener      *dcl;
> -static QemuPfConv                 *conv;
> -static PixelFormat                fbpf;
>  static int                        resize_screen;
>  static int                        redraw_screen;
>  static int                        cx, cy, cw, ch;
>  static Notifier                   exit_notifier;
> +static pixman_image_t             *surface;
> +static pixman_image_t             *framebuffer;
> +static pixman_transform_t         transform;
> +static pixman_region16_t          dirty;
>  
>  /* fwd decls */
>  static int fbdev_activate_vt(int tty, int vtno, bool wait);
>  
>  /* -------------------------------------------------------------------- */
> +/* pixman helpers                                                       */
> +
> +static int pixman_shifts_to_type(int rshift, int gshift, int bshift)
> +{
> +    int type = PIXMAN_TYPE_OTHER;
> +
> +    if (rshift > gshift && gshift > bshift) {
> +        if (bshift == 0) {
> +            type = PIXMAN_TYPE_ARGB;
> +        } else {
> +#if PIXMAN_VERSION >= PIXMAN_VERSION_ENCODE(0, 21, 8)
> +            type = PIXMAN_TYPE_RGBA;
> +#endif
> +        }
> +    } else if (rshift < gshift && gshift < bshift) {
> +        if (rshift == 0) {
> +            type = PIXMAN_TYPE_ABGR;
> +        } else {
> +            type = PIXMAN_TYPE_BGRA;
> +        }
> +    }
> +    return type;
> +}
> +
> +static pixman_image_t *pixman_from_displaystate(DisplayState *ds)
> +{
> +    PixelFormat *pf = &ds->surface->pf;
> +    pixman_format_code_t format;
> +    pixman_image_t *image;
> +    int type;
> +
> +    type = pixman_shifts_to_type(pf->rshift, pf->gshift, pf->bshift);
> +    format = PIXMAN_FORMAT(pf->bits_per_pixel, type,
> +                           pf->abits, pf->rbits, pf->gbits, pf->bbits);
> +    image = pixman_image_create_bits(format, ds_get_width(ds),
> +                                     ds_get_height(ds),
> +                                     (void *)ds_get_data(ds),
> +                                     ds_get_linesize(ds));
> +    return image;
> +}
> +
> +static pixman_image_t *pixman_from_framebuffer(void)
> +{
> +    pixman_format_code_t format;
> +    pixman_image_t *image;
> +    int type;
> +
> +    type = pixman_shifts_to_type(fb_var.red.offset,
> +                                 fb_var.green.offset,
> +                                 fb_var.blue.offset);
> +    format = PIXMAN_FORMAT(fb_var.bits_per_pixel, type,
> +                           fb_var.transp.length,
> +                           fb_var.red.length,
> +                           fb_var.green.length,
> +                           fb_var.blue.length);
> +    image = pixman_image_create_bits(format, fb_var.xres, fb_var.yres,
> +                                     (void *)fb_mem, fb_fix.line_length);
> +    return image;
> +}
> +
> +/* -------------------------------------------------------------------- */
>  /* mouse                                                                */
>  
>  static void read_mouse(void *opaque)
> @@ -529,6 +593,17 @@ static void fbdev_cleanup(void)
>  {
>      trace_fbdev_cleanup();
>  
> +    /* release pixman stuff */
> +    pixman_region_fini(&dirty);
> +    if (framebuffer) {
> +        pixman_image_unref(framebuffer);
> +        framebuffer = NULL;
> +    }
> +    if (surface) {
> +        pixman_image_unref(surface);
> +        surface = NULL;
> +    }
> +
>      /* restore console */
>      if (fb_mem != NULL) {
>          munmap(fb_mem, fb_fix.smem_len+fb_mem_offset);
> @@ -682,36 +757,8 @@ static int fbdev_init(const char *device, Error **err)
>      start_mediumraw(tty);
>      qemu_set_fd_handler(tty, read_mediumraw, NULL, NULL);
>  
> -    /* create PixelFormat from fbdev structs */
> -    fbpf.bits_per_pixel  = fb_var.bits_per_pixel;
> -    fbpf.bytes_per_pixel = (fb_var.bits_per_pixel+7)/8;
> -    fbpf.depth           = fb_var.bits_per_pixel == 32
> -        ? 24 : fb_var.bits_per_pixel;
> -    fbpf.rshift          = fb_var.red.offset;
> -    fbpf.rbits           = fb_var.red.length;
> -    fbpf.gshift          = fb_var.green.offset;
> -    fbpf.gbits           = fb_var.green.length;
> -    fbpf.bshift          = fb_var.blue.offset;
> -    fbpf.bbits           = fb_var.blue.length;
> -    fbpf.ashift          = fb_var.transp.offset;
> -    fbpf.abits           = fb_var.transp.length;
> -
> -    if (fbpf.rbits) {
> -        fbpf.rmax   = (1 << fbpf.rbits) - 1;
> -        fbpf.rmask  = fbpf.rmax << fbpf.rshift;
> -    }
> -    if (fbpf.gbits) {
> -        fbpf.gmax   = (1 << fbpf.gbits) - 1;
> -        fbpf.gmask  = fbpf.gmax << fbpf.gshift;
> -    }
> -    if (fbpf.bbits) {
> -        fbpf.bmax   = (1 << fbpf.bbits) - 1;
> -        fbpf.bmask  = fbpf.bmax << fbpf.bshift;
> -    }
> -    if (fbpf.abits) {
> -        fbpf.amax   = (1 << fbpf.abits) - 1;
> -        fbpf.amask  = fbpf.amax << fbpf.ashift;
> -    }
> +    framebuffer = pixman_from_framebuffer();
> +    pixman_region_init(&dirty);
>      return 0;
>  
>  err_early:
> @@ -819,36 +866,15 @@ static int fbdev_switch_init(void)
>  /* -------------------------------------------------------------------- */
>  /* rendering                                                            */
>  
> -static void fbdev_render(DisplayState *ds, int x, int y, int w, int h)
> +static void fbdev_render(DisplayState *ds)
>  {
> -    uint8_t *dst;
> -    uint8_t *src;
> -    int line;
> +    assert(surface);
>  
> -    if (!conv) {
> -        return;
> -    }
> -
> -    src = ds_get_data(ds) + y * ds_get_linesize(ds)
> -        + x * ds_get_bytes_per_pixel(ds);
> -    dst = fb_mem + y * fb_fix.line_length
> -        + x * fbpf.bytes_per_pixel;
> -
> -    dst += cy * fb_fix.line_length;
> -    dst += cx * fbpf.bytes_per_pixel;
> -
> -    if (h > fb_var.yres - y) {
> -        h = fb_var.yres - y;
> -    }
> -    if (w > fb_var.xres - x) {
> -        w = fb_var.xres - x;
> -    }
> -
> -    for (line = y; line < y+h; line++) {
> -        qemu_pf_conv_run(conv, dst, src, w);
> -        dst += fb_fix.line_length;
> -        src += ds_get_linesize(ds);
> -    }
> +    pixman_image_set_clip_region(surface, &dirty);
> +    pixman_image_composite(PIXMAN_OP_SRC, surface, NULL, framebuffer,
> +                           0, 0, 0, 0, 0, 0, fb_var.xres, fb_var.yres);
> +    pixman_region_fini(&dirty);
> +    pixman_region_init(&dirty);
>  }
>  
>  /* -------------------------------------------------------------------- */
> @@ -872,14 +898,16 @@ static void fbdev_update(DisplayState *ds, int x, int 
> y, int w, int h)
>          if (ds_get_height(ds) < fb_var.yres) {
>              cy = (fb_var.yres - ds_get_height(ds)) / 2;
>          }
> -
> -        if (conv) {
> -            qemu_pf_conv_put(conv);
> -        }
> -        conv = qemu_pf_conv_get(&fbpf, &ds->surface->pf);
> -        if (conv == NULL) {
> -            fprintf(stderr, "fbdev: unsupported PixelFormat conversion\n");
> +        if (surface) {
> +            pixman_image_unref(surface);
>          }
> +        surface = pixman_from_displaystate(ds);

Am I reading this right? Are you creating a new pixman surface in every
call to fbdev_update?


> +        pixman_transform_init_identity(&transform);
> +        pixman_transform_translate(&transform, NULL,
> +                                   pixman_int_to_fixed(-cx),
> +                                   pixman_int_to_fixed(-cy));
> +        pixman_image_set_transform(surface, &transform);
>      }
>  
>      if (redraw_screen) {
> @@ -889,7 +917,7 @@ static void fbdev_update(DisplayState *ds, int x, int y, 
> int w, int h)
>          x = 0; y = 0; w = ds_get_width(ds); h = ds_get_height(ds);
>      }
>  
> -    fbdev_render(ds, x, y, w, h);
> +    pixman_region_union_rect(&dirty, &dirty, x, y, w, h);
>  }
>  
>  static void fbdev_resize(DisplayState *ds)
> @@ -898,6 +926,15 @@ static void fbdev_resize(DisplayState *ds)
>      redraw_screen++;
>  }
>  
> +static void fbdev_setdata(DisplayState *ds)
> +{
> +    if (surface) {
> +        pixman_image_unref(surface);
> +    }
> +    surface = pixman_from_displaystate(ds);
> +    redraw_screen++;
> +}
> +
>  static void fbdev_refresh(DisplayState *ds)
>  {
>      switch (fb_switch_state) {
> @@ -916,6 +953,10 @@ static void fbdev_refresh(DisplayState *ds)
>      if (redraw_screen) {
>          fbdev_update(ds, 0, 0, 0, 0);
>      }
> +
> +    if (pixman_region_not_empty(&dirty)) {
> +        fbdev_render(ds);
> +    }

Why are you using fbdev_refresh for rendering instead of fbdev_update?
>From consistency with sdl and vnc as well as the semantics of these
callbacks I think it would be better to do the rendering from
fbdev_update and just call vga_hw_update here.


>  }
>  
>  static void fbdev_exit_notifier(Notifier *notifier, void *data)
> @@ -942,6 +983,7 @@ int fbdev_display_init(DisplayState *ds, const char 
> *device, Error **err)
>      dcl = g_new0(DisplayChangeListener, 1);
>      dcl->dpy_update  = fbdev_update;
>      dcl->dpy_resize  = fbdev_resize;
> +    dcl->dpy_setdata = fbdev_setdata;
>      dcl->dpy_refresh = fbdev_refresh;
>      register_displaychangelistener(ds, dcl);

Pixman or non-pixman, I still think that this could benefit from
implementing a DisplayAllocator interface: it would avoid a memcpy
whenever there is no need for scaling and pixel conversions.
It should be able to make visible difference from the usability
perspective.