[Spice-devel] [xf86-video-qxl] Make the Deferred FPS mode available in all cases, not just XSPICE.

[Hans de Goede]

Hi,

On 03/27/2013 04:12 PM, Jeremy White wrote:
> Signed-off-by: Jeremy White <jwhite at codeweavers.com>
> ---
>   configure.ac     |   18 +++++++++---------
>   src/Makefile.am  |    2 ++
>   src/dfps.c       |   40 +++++++++++++++++++++++++++++++++++++++-
>   src/dfps.h       |    1 +
>   src/qxl.h        |   12 +++++-------
>   src/qxl_driver.c |   23 ++++++-----------------
>   src/qxl_uxa.c    |    2 --
>   7 files changed, 62 insertions(+), 36 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 48904a2..eef20e0 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -91,17 +91,17 @@ AC_ARG_ENABLE(xspice,
>           enable_xspice=no
>      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)
> -    ],
> -)
> -else
> +if test "x$enable_xspice" = x; then
>       enable_xspice=no
>   fi
> +
> +PKG_CHECK_MODULES([SPICE], [spice-server >= 0.6.3],
> +[
> +    AC_SUBST(SPICE_CFLAGS)
> +    AC_SUBST(SPICE_LIBS)
> +],
> +)
> +
>   AM_CONDITIONAL(BUILD_XSPICE, test "x$enable_xspice" = "xyes")
>   AM_CONDITIONAL(BUILD_QXL, test "x$enable_qxl" = "xyes")
>


Hmm, if I read this correctly, this means that building just the xorg-driver now has
grown a buildtime (I surely hope it is buildtime only!) dependency on spice-server,
I don't really like that.


> diff --git a/src/Makefile.am b/src/Makefile.am
> index 8632297..eea19c1 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
> @@ -56,6 +57,7 @@ qxl_drv_la_SOURCES =				\
>   	qxl_uxa.c			\
>   	qxl_ums_mode.c                  \
>   	qxl_io.c                        \
> +	dfps.c				\
>   	compat-api.h
>   endif
>
> diff --git a/src/dfps.c b/src/dfps.c
> index 6ac29f9..b57519b 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 c26ea8f..a49f020 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"
> @@ -105,6 +103,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,
> @@ -128,7 +127,6 @@ enum {
>       OPTION_SPICE_TLS_CIPHERS,
>       OPTION_SPICE_CACERT_FILE,
>       OPTION_SPICE_DH_FILE,
> -    OPTION_SPICE_DEFERRED_FPS,
>       OPTION_SPICE_EXIT_ON_DISCONNECT,
>       OPTION_SPICE_PLAYBACK_FIFO_DIR,
>   #endif
> @@ -275,12 +273,13 @@ struct _qxl_screen_t
>       int				enable_fallback_cache;
>       int				enable_surfaces;
>
> +    SpiceCoreInterface *core;
> +    SpiceTimer *        frames_timer;
> +

I guess this is why the build time dependency is needed. But since
you now define a custom struct for this in dfps.c, can't we just
have a "struct SpiceTimer;" somewhere, without needing to build-time
depend on spice-server ? Also is it just me, or do the dfps.c
changes break xspice mode ?

Regards,

Hans