Re: Experimental patch to enable Deferred FPS mode for the qemu qxl driver.

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 01/28/2013 09:46 PM, Jeremy White wrote:
> When I decided to try to understand what was going on with Gnome
> fallback, I hacked the Deferred FPS mode into working with a qemu
> system.
>
> This is not quite ready for prime time, but should make for
> interesting testing.
>
> My rather biased sense is that it's an improvement over the normal mode.
>
> Cheers,
>
> Jeremy
>
>
> ---
>   configure.ac      |   15 ++++++---------
>   src/Makefile.am   |    2 ++
>   src/dfps.c        |   40 +++++++++++++++++++++++++++++++++++++++-
>   src/dfps.h        |    1 +
>   src/qxl.h         |   11 +++++------
>   src/qxl_driver.c  |   31 ++++++++++---------------------
>   src/qxl_surface.c |    5 ++++-
>   7 files changed, 67 insertions(+), 38 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 48904a2..1aba1c1 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -92,16 +92,13 @@ AC_ARG_ENABLE(xspice,
>      fi
>   ])
>
> -if test "x$enable_xspice" = "xyes"; then
> -    PKG_CHECK_MODULES([SPICE], [spice-server >= 0.6.3],
> -    [
> -        AC_SUBST(SPICE_CFLAGS)
> -        AC_SUBST(SPICE_LIBS)
> -    ],
> +PKG_CHECK_MODULES([SPICE], [spice-server >= 0.6.3],
> +[
> +    AC_SUBST(SPICE_CFLAGS)
> +    AC_SUBST(SPICE_LIBS)
> +],
>   )
> -else
> -    enable_xspice=no
> -fi
> +
>   AM_CONDITIONAL(BUILD_XSPICE, test "x$enable_xspice" = "xyes")
>   AM_CONDITIONAL(BUILD_QXL, test "x$enable_qxl" = "xyes")

Hmm, this seems like some debugging / wip work left over ?

> diff --git a/src/Makefile.am b/src/Makefile.am
> index 6950ba5..08674b2 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -32,6 +32,7 @@ AM_CFLAGS = $(SPICE_PROTOCOL_CFLAGS) $(XORG_CFLAGS) $(PCIACCESS_CFLAGS) $(CWARNF
>   if BUILD_QXL
>   qxl_drv_la_LTLIBRARIES = qxl_drv.la
>   qxl_drv_la_LDFLAGS = -module -avoid-version
> +qxl_drv_la_CFLAGS = $(AM_CFLAGS) $(SPICE_CFLAGS)
>   qxl_drv_ladir = @moduledir@/drivers
>
>   qxl_drv_la_LIBADD = uxa/libuxa.la
> @@ -51,6 +52,7 @@ qxl_drv_la_SOURCES =				\
>   	qxl_option_helpers.c		\
>   	qxl_option_helpers.h		\
>   	qxl_edid.c			\
> +	dfps.c				        \
>   	compat-api.h
>   endif
>
> diff --git a/src/dfps.c b/src/dfps.c
> index e5a2273..5fd6964 100644
> --- a/src/dfps.c
> +++ b/src/dfps.c
> @@ -53,6 +53,44 @@ struct dfps_info_t
>       GCPtr       pgc;
>   };
>
> +typedef struct SpiceTimer {
> +    OsTimerPtr xorg_timer;
> +    SpiceTimerFunc func;
> +    void *opaque; // also stored in xorg_timer, but needed for timer_start
> +} Timer;
> +
> +static CARD32 xorg_timer_callback(
> +    OsTimerPtr xorg_timer,
> +    CARD32 time,
> +    pointer arg)
> +{
> +    SpiceTimer *timer = (SpiceTimer*)arg;
> +
> +    timer->func(timer->opaque);
> +    return 0; // if non zero xorg does a TimerSet, we don't want that.
> +}
> +
> +static SpiceTimer* timer_add(SpiceTimerFunc func, void *opaque)
> +{
> +    SpiceTimer *timer = calloc(sizeof(SpiceTimer), 1);
> +
> +    timer->xorg_timer = TimerSet(NULL, 0, 1e9 /* TODO: infinity? */, xorg_timer_callback, timer);
> +    timer->func = func;
> +    timer->opaque = opaque;
> +    return timer;
> +}
> +
> +static void timer_start(SpiceTimer *timer, uint32_t ms)
> +{
> +    TimerSet(timer->xorg_timer, 0 /* flags */, ms, xorg_timer_callback, timer);
> +}
> +
> +void dfps_start_ticker(qxl_screen_t *qxl)
> +{
> +    qxl->frames_timer = timer_add(dfps_ticker, qxl);
> +    timer_start(qxl->frames_timer, 1000 / qxl->deferred_fps);
> +}
> +
>   void dfps_ticker(void *opaque)
>   {
>       qxl_screen_t *qxl = (qxl_screen_t *) opaque;
> @@ -68,7 +106,7 @@ void dfps_ticker(void *opaque)
>           RegionUninit(&info->updated_region);
>           RegionInit(&info->updated_region, NULL, 0);
>       }
> -    qxl->core->timer_start(qxl->frames_timer, 1000 / qxl->deferred_fps);
> +    timer_start(qxl->frames_timer, 1000 / qxl->deferred_fps);
>   }
>
>
> diff --git a/src/dfps.h b/src/dfps.h
> index ea38a46..3f3336a 100644
> --- a/src/dfps.h
> +++ b/src/dfps.h
> @@ -24,6 +24,7 @@
>
>   typedef struct dfps_info_t dfps_info_t;
>
> +void dfps_start_ticker(qxl_screen_t *qxl);
>   void dfps_ticker(void *opaque);
>   void dfps_set_uxa_functions(qxl_screen_t *qxl, ScreenPtr screen);
>
> diff --git a/src/qxl.h b/src/qxl.h
> index 8e2802a..c301e0a 100644
> --- a/src/qxl.h
> +++ b/src/qxl.h
> @@ -26,9 +26,7 @@
>   #include <stdint.h>
>
>   #include <spice/qxl_dev.h>
> -#ifdef XSPICE
>   #include <spice.h>
> -#endif
>
>   #include "compiler.h"
>   #include "xf86.h"
> @@ -104,6 +102,7 @@ enum {
>       OPTION_ENABLE_FALLBACK_CACHE,
>       OPTION_ENABLE_SURFACES,
>       OPTION_NUM_HEADS,
> +    OPTION_SPICE_DEFERRED_FPS,
>   #ifdef XSPICE
>       OPTION_SPICE_PORT,
>       OPTION_SPICE_TLS_PORT,
> @@ -127,7 +126,6 @@ enum {
>       OPTION_SPICE_TLS_CIPHERS,
>       OPTION_SPICE_CACERT_FILE,
>       OPTION_SPICE_DH_FILE,
> -    OPTION_SPICE_DEFERRED_FPS,
>       OPTION_SPICE_EXIT_ON_DISCONNECT,
>   #endif
>       OPTION_COUNT,
> @@ -237,12 +235,13 @@ struct _qxl_screen_t
>       int				enable_fallback_cache;
>       int				enable_surfaces;
>
> +    SpiceCoreInterface *core;
> +    SpiceTimer *        frames_timer;
> +
>   #ifdef XSPICE
>       /* XSpice specific */
>       struct QXLRom		shadow_rom;    /* Parameter RAM */
>       SpiceServer *       spice_server;
> -    SpiceCoreInterface *core;
> -    SpiceTimer *        frames_timer;
>
>       QXLWorker *         worker;
>       int                 worker_running;
> @@ -266,8 +265,8 @@ struct _qxl_screen_t
>           uint8_t        *data, *flipped;
>       } guest_primary;
>
> -    uint32_t           deferred_fps;
>   #endif /* XSPICE */
> +    uint32_t           deferred_fps;
>   };
>
>   typedef struct qxl_output_private {
> diff --git a/src/qxl_driver.c b/src/qxl_driver.c
> index e1893dc..e951ccc 100644
> --- a/src/qxl_driver.c
> +++ b/src/qxl_driver.c
> @@ -44,20 +44,19 @@
>
>   #include "mspace.h"
>
> +#include <spice.h>
>   #include "qxl.h"
>   #include "assert.h"
>   #include "qxl_option_helpers.h"
>   #include <spice/protocol.h>
>
> -#ifdef XSPICE
>   #include "spiceqxl_driver.h"
> +#include "spiceqxl_spice_server.h"
>   #include "spiceqxl_main_loop.h"
>   #include "spiceqxl_display.h"
>   #include "spiceqxl_inputs.h"
>   #include "spiceqxl_io_port.h"
> -#include "spiceqxl_spice_server.h"
>   #include "dfps.h"
> -#endif /* XSPICE */
>
>   extern void compat_init_scrn (ScrnInfoPtr);
>
> @@ -79,6 +78,8 @@ const OptionInfoRec DefaultOptions[] =
>         "EnableSurfaces",           OPTV_BOOLEAN, { 0 }, TRUE },
>       { OPTION_NUM_HEADS,
>         "NumHeads",                 OPTV_INTEGER, { 4 }, FALSE },
> +    { OPTION_SPICE_DEFERRED_FPS,
> +      "SpiceDeferredFPS",         OPTV_INTEGER, { 15 }, FALSE},
>   #ifdef XSPICE
>       { OPTION_SPICE_PORT,
>         "SpicePort",                OPTV_INTEGER,   {5900}, FALSE },
> @@ -125,8 +126,6 @@ const OptionInfoRec DefaultOptions[] =
>         "SpiceCacertFile",          OPTV_STRING,    {0}, FALSE},
>       { OPTION_SPICE_DH_FILE,
>         "SpiceDhFile",              OPTV_STRING,    {0}, FALSE},
> -    { OPTION_SPICE_DEFERRED_FPS,
> -      "SpiceDeferredFPS",         OPTV_INTEGER,   {0}, FALSE},
>       { OPTION_SPICE_EXIT_ON_DISCONNECT,
>         "SpiceExitOnDisconnect",    OPTV_BOOLEAN,   {0}, FALSE},
>   #endif
> @@ -988,9 +987,7 @@ qxl_resize_primary_to_virtual (qxl_screen_t *qxl)
>       {
>   	PixmapPtr root = pScreen->GetScreenPixmap (pScreen);
>
> -#ifdef XSPICE
>           if (qxl->deferred_fps <= 0)
> -#endif
>           {
>               qxl_surface_t *surf;
>
> @@ -1203,9 +1200,7 @@ qxl_create_screen_resources (ScreenPtr pScreen)
>
>       pPixmap = pScreen->GetScreenPixmap (pScreen);
>
> -#ifdef XSPICE
>       if (qxl->deferred_fps <= 0)
> -#endif
>       {
>           set_screen_pixmap_header (pScreen);
>
> @@ -1680,11 +1675,9 @@ setup_uxa (qxl_screen_t *qxl, ScreenPtr screen)
>       qxl->uxa->uxa_major = 1;
>       qxl->uxa->uxa_minor = 0;
>
> -#ifdef XSPICE
>       if (qxl->deferred_fps)
>           dfps_set_uxa_functions(qxl, screen);
>       else
> -#endif
>           set_uxa_functions(qxl, screen);
>
>       if (!uxa_driver_init (screen, qxl->uxa))
> @@ -1722,11 +1715,6 @@ spiceqxl_screen_init (ScrnInfoPtr pScrn, qxl_screen_t *qxl)
>   	qxl_add_spice_display_interface (qxl);
>   	qxl->worker->start (qxl->worker);
>   	qxl->worker_running = TRUE;
> -        if (qxl->deferred_fps)
> -        {
> -            qxl->frames_timer = qxl->core->timer_add(dfps_ticker, qxl);
> -            qxl->core->timer_start(qxl->frames_timer, 1000 / qxl->deferred_fps);
> -        }
>       }
>       qxl->spice_server = qxl->spice_server;
>   }
> @@ -1757,7 +1745,7 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL)
>       VisualPtr      visual;
>
>       CHECK_POINT ();
> -
> +
>       assert (qxl->pScrn == pScrn);
>
>       if (!qxl_map_memory (qxl, pScrn->scrnIndex))
> @@ -1888,6 +1876,11 @@ qxl_screen_init (SCREEN_INIT_ARGS_DECL)
>       /* bounds" */
>       xf86RandR12SetTransformSupport (pScreen, TRUE);
>
> +    if (qxl->deferred_fps)
> +    {
> +        dfps_start_ticker(qxl);
> +    }
> +
>       return TRUE;
>
>   out:
> @@ -1940,9 +1933,7 @@ qxl_leave_vt (VT_FUNC_ARGS_DECL)
>
>       pScrn->EnableDisableFBAccess (XF86_SCRN_ARG (pScrn), FALSE);
>
> -#ifdef XSPICE
>       if (qxl->deferred_fps <= 0)
> -#endif
>           qxl->vt_surfaces = qxl_surface_cache_evacuate_all (qxl->surface_cache);
>
>       ioport_write (qxl, QXL_IO_RESET, 0);
> @@ -2431,13 +2422,11 @@ qxl_pre_init (ScrnInfoPtr pScrn, int flags)
>       qxl->num_heads =
>           get_int_option (qxl->options, OPTION_NUM_HEADS, "QXL_NUM_HEADS");
>
> -#ifdef XSPICE
>       qxl->deferred_fps = get_int_option(qxl->options, OPTION_SPICE_DEFERRED_FPS, "XSPICE_DEFERRED_FPS");
>       if (qxl->deferred_fps > 0)
>           xf86DrvMsg(scrnIndex, X_INFO, "Deferred FPS: %d\n", qxl->deferred_fps);
>       else
>           xf86DrvMsg(scrnIndex, X_INFO, "Deferred Frames: Disabled\n");
> -#endif
>
>       xf86DrvMsg (scrnIndex, X_INFO, "Offscreen Surfaces: %s\n",
>                   qxl->enable_surfaces ? "Enabled" : "Disabled");

And the below chunk looks like it should be in a separate commit, with a commit
message explaining it in more detail.

> diff --git a/src/qxl_surface.c b/src/qxl_surface.c
> index 3da9079..f6de4d0 100644
> --- a/src/qxl_surface.c
> +++ b/src/qxl_surface.c
> @@ -1137,7 +1137,10 @@ qxl_surface_upload_primary_regions(qxl_screen_t *qxl, PixmapPtr pixmap, RegionRe
>
>       while (n_boxes--)
>       {
> -        upload_one_primary_region(qxl, pixmap, boxes);
> +        if (boxes->x2 > boxes->x1 && boxes->y2 > boxes->y1)
> +            upload_one_primary_region(qxl, pixmap, boxes);
> +        else
> +            ErrorF("JPW skipping boxes %d, %d to %d, %d\n", boxes->x1, boxes->y1, boxes->x2, boxes->y2);
>           boxes++;
>       }
>   }

Regards,

Hans
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel