Patchwork [20/24] xen: re-enable refresh interval reporting for xenfb

login
register
mail settings
Submitter Gerd Hoffmann
Date April 4, 2013, 7:29 a.m.
Message ID <1365060546-24638-21-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/233679/
State New
Headers show

Comments

Gerd Hoffmann - April 4, 2013, 7:29 a.m.
xenfb informs the guest about the gui refresh interval so it can avoid
pointless work.  That logic was temporarely disabled for the
DisplayState reorganization.  Restore it now, with a proper interface
for it.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/xenfb.c           |   47 ++++++++++++++++-------------------------------
 include/ui/console.h |    1 +
 ui/console.c         |    6 ++++++
 3 files changed, 23 insertions(+), 31 deletions(-)
Stefano Stabellini - April 4, 2013, 5:06 p.m.
On Thu, 4 Apr 2013, Gerd Hoffmann wrote:
> xenfb informs the guest about the gui refresh interval so it can avoid
> pointless work.  That logic was temporarely disabled for the
> DisplayState reorganization.  Restore it now, with a proper interface
> for it.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/xenfb.c           |   47 ++++++++++++++++-------------------------------
>  include/ui/console.h |    1 +
>  ui/console.c         |    6 ++++++
>  3 files changed, 23 insertions(+), 31 deletions(-)
> 
> diff --git a/hw/xenfb.c b/hw/xenfb.c
> index f2af7eb..6344f11 100644
> --- a/hw/xenfb.c
> +++ b/hw/xenfb.c
> @@ -646,7 +646,7 @@ static void xenfb_guest_copy(struct XenFB *xenfb, int x, int y, int w, int h)
>      dpy_gfx_update(xenfb->c.con, x, y, w, h);
>  }
>  
> -#if 0 /* def XENFB_TYPE_REFRESH_PERIOD */
> +#ifdef XENFB_TYPE_REFRESH_PERIOD
>  static int xenfb_queue_full(struct XenFB *xenfb)
>  {
>      struct xenfb_page *page = xenfb->c.page;
> @@ -705,37 +705,7 @@ static void xenfb_update(void *opaque)
>          return;
>  
>      if (xenfb->feature_update) {
> -#if 0 /* XENFB_TYPE_REFRESH_PERIOD */
> -        struct DisplayChangeListener *l;
> -        int period = 99999999;
> -        int idle = 1;
> -
> -	if (xenfb_queue_full(xenfb))
> -	    return;
> -
> -        QLIST_FOREACH(l, &xenfb->c.ds->listeners, next) {
> -            if (l->idle)
> -                continue;
> -            idle = 0;
> -            if (!l->gui_timer_interval) {
> -                if (period > GUI_REFRESH_INTERVAL)
> -                    period = GUI_REFRESH_INTERVAL;
> -            } else {
> -                if (period > l->gui_timer_interval)
> -                    period = l->gui_timer_interval;
> -            }
> -        }
> -        if (idle)
> -	    period = XENFB_NO_REFRESH;
> -
> -	if (xenfb->refresh_period != period) {
> -	    xenfb_send_refresh_period(xenfb, period);
> -	    xenfb->refresh_period = period;
> -            xen_be_printf(&xenfb->c.xendev, 1, "refresh period: %d\n", period);
> -	}
> -#else
>  	; /* nothing */
> -#endif
>      } else {
>  	/* we don't get update notifications, thus use the
>  	 * sledge hammer approach ... */

You might as well remove the if () nothing; case.


> @@ -785,6 +755,20 @@ static void xenfb_update(void *opaque)
>      xenfb->up_fullscreen = 0;
>  }
>  
> +static void xenfb_update_interval(void *opaque, uint64_t interval)
> +{
> +    struct XenFB *xenfb = opaque;
> +
> +    if (xenfb->feature_update) {
> +#ifdef XENFB_TYPE_REFRESH_PERIOD
> +        if (xenfb_queue_full(xenfb)) {
> +            return;
> +        }
> +        xenfb_send_refresh_period(xenfb, interval);

Shouldn't we be updating xenfb->refresh_period here?
And shouldn't we call xenfb_send_refresh_period only if interval !=
xenfb->refresh_period?
On the other hand if refresh_period is not useful anymore, shouldn't we
remove it from struct XenFB?



> +#endif
> +    }
> +}
> +
>  /* QEMU display state changed, so refresh the framebuffer copy */
>  static void xenfb_invalidate(void *opaque)
>  {
> @@ -980,6 +964,7 @@ struct XenDevOps xen_framebuffer_ops = {
>  static const GraphicHwOps xenfb_ops = {
>      .invalidate  = xenfb_invalidate,
>      .gfx_update  = xenfb_update,
> +    .update_interval = xenfb_update_interval,
>  };
>  
>  /*
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 3cb0018..800f458 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -272,6 +272,7 @@ typedef struct GraphicHwOps {
>      void (*invalidate)(void *opaque);
>      void (*gfx_update)(void *opaque);
>      void (*text_update)(void *opaque, console_ch_t *text);
> +    void (*update_interval)(void *opaque, uint64_t interval);
>  } GraphicHwOps;
>  
>  QemuConsole *graphic_console_init(const GraphicHwOps *ops,
> diff --git a/ui/console.c b/ui/console.c
> index be89747..ead5d35 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -182,6 +182,7 @@ static void gui_update(void *opaque)
>      uint64_t dcl_interval;
>      DisplayState *ds = opaque;
>      DisplayChangeListener *dcl;
> +    int i;
>  
>      ds->refreshing = true;
>      dpy_refresh(ds);
> @@ -196,6 +197,11 @@ static void gui_update(void *opaque)
>      }
>      if (ds->update_interval != interval) {
>          ds->update_interval = interval;
> +        for (i = 0; i < nb_consoles; i++) {
> +            if (consoles[i]->hw_ops->update_interval) {
> +                consoles[i]->hw_ops->update_interval(consoles[i]->hw, interval);
> +            }
> +        }
>          trace_console_refresh(interval);
>      }
>      ds->last_update = qemu_get_clock_ms(rt_clock);
> -- 
> 1.7.9.7
>
Gerd Hoffmann - April 5, 2013, 7:28 a.m.
>> -#else
>>  	; /* nothing */
>> -#endif
>>      } else {
>>  	/* we don't get update notifications, thus use the
>>  	 * sledge hammer approach ... */
> 
> You might as well remove the if () nothing; case.

Yep, will do.

>> @@ -785,6 +755,20 @@ static void xenfb_update(void *opaque)
>>      xenfb->up_fullscreen = 0;
>>  }
>>  
>> +static void xenfb_update_interval(void *opaque, uint64_t interval)
>> +{
>> +    struct XenFB *xenfb = opaque;
>> +
>> +    if (xenfb->feature_update) {
>> +#ifdef XENFB_TYPE_REFRESH_PERIOD
>> +        if (xenfb_queue_full(xenfb)) {
>> +            return;
>> +        }
>> +        xenfb_send_refresh_period(xenfb, interval);
> 
> Shouldn't we be updating xenfb->refresh_period here? And shouldn't we
> call xenfb_send_refresh_period only if interval != 
> xenfb->refresh_period?

> On the other hand if refresh_period is not useful anymore, shouldn't
> we remove it from struct XenFB?

xenfb_update_interval is only called when interval changes, which I
think means we don't need refresh_period any more, correct?

cheers,
  Gerd
Stefano Stabellini - April 5, 2013, 10:07 a.m.
On Fri, 5 Apr 2013, Gerd Hoffmann wrote:
> >> -#else
> >>  	; /* nothing */
> >> -#endif
> >>      } else {
> >>  	/* we don't get update notifications, thus use the
> >>  	 * sledge hammer approach ... */
> > 
> > You might as well remove the if () nothing; case.
> 
> Yep, will do.
> 
> >> @@ -785,6 +755,20 @@ static void xenfb_update(void *opaque)
> >>      xenfb->up_fullscreen = 0;
> >>  }
> >>  
> >> +static void xenfb_update_interval(void *opaque, uint64_t interval)
> >> +{
> >> +    struct XenFB *xenfb = opaque;
> >> +
> >> +    if (xenfb->feature_update) {
> >> +#ifdef XENFB_TYPE_REFRESH_PERIOD
> >> +        if (xenfb_queue_full(xenfb)) {
> >> +            return;
> >> +        }
> >> +        xenfb_send_refresh_period(xenfb, interval);
> > 
> > Shouldn't we be updating xenfb->refresh_period here? And shouldn't we
> > call xenfb_send_refresh_period only if interval != 
> > xenfb->refresh_period?
> 
> > On the other hand if refresh_period is not useful anymore, shouldn't
> > we remove it from struct XenFB?
> 
> xenfb_update_interval is only called when interval changes, which I
> think means we don't need refresh_period any more, correct?
 
that's right

Patch

diff --git a/hw/xenfb.c b/hw/xenfb.c
index f2af7eb..6344f11 100644
--- a/hw/xenfb.c
+++ b/hw/xenfb.c
@@ -646,7 +646,7 @@  static void xenfb_guest_copy(struct XenFB *xenfb, int x, int y, int w, int h)
     dpy_gfx_update(xenfb->c.con, x, y, w, h);
 }
 
-#if 0 /* def XENFB_TYPE_REFRESH_PERIOD */
+#ifdef XENFB_TYPE_REFRESH_PERIOD
 static int xenfb_queue_full(struct XenFB *xenfb)
 {
     struct xenfb_page *page = xenfb->c.page;
@@ -705,37 +705,7 @@  static void xenfb_update(void *opaque)
         return;
 
     if (xenfb->feature_update) {
-#if 0 /* XENFB_TYPE_REFRESH_PERIOD */
-        struct DisplayChangeListener *l;
-        int period = 99999999;
-        int idle = 1;
-
-	if (xenfb_queue_full(xenfb))
-	    return;
-
-        QLIST_FOREACH(l, &xenfb->c.ds->listeners, next) {
-            if (l->idle)
-                continue;
-            idle = 0;
-            if (!l->gui_timer_interval) {
-                if (period > GUI_REFRESH_INTERVAL)
-                    period = GUI_REFRESH_INTERVAL;
-            } else {
-                if (period > l->gui_timer_interval)
-                    period = l->gui_timer_interval;
-            }
-        }
-        if (idle)
-	    period = XENFB_NO_REFRESH;
-
-	if (xenfb->refresh_period != period) {
-	    xenfb_send_refresh_period(xenfb, period);
-	    xenfb->refresh_period = period;
-            xen_be_printf(&xenfb->c.xendev, 1, "refresh period: %d\n", period);
-	}
-#else
 	; /* nothing */
-#endif
     } else {
 	/* we don't get update notifications, thus use the
 	 * sledge hammer approach ... */
@@ -785,6 +755,20 @@  static void xenfb_update(void *opaque)
     xenfb->up_fullscreen = 0;
 }
 
+static void xenfb_update_interval(void *opaque, uint64_t interval)
+{
+    struct XenFB *xenfb = opaque;
+
+    if (xenfb->feature_update) {
+#ifdef XENFB_TYPE_REFRESH_PERIOD
+        if (xenfb_queue_full(xenfb)) {
+            return;
+        }
+        xenfb_send_refresh_period(xenfb, interval);
+#endif
+    }
+}
+
 /* QEMU display state changed, so refresh the framebuffer copy */
 static void xenfb_invalidate(void *opaque)
 {
@@ -980,6 +964,7 @@  struct XenDevOps xen_framebuffer_ops = {
 static const GraphicHwOps xenfb_ops = {
     .invalidate  = xenfb_invalidate,
     .gfx_update  = xenfb_update,
+    .update_interval = xenfb_update_interval,
 };
 
 /*
diff --git a/include/ui/console.h b/include/ui/console.h
index 3cb0018..800f458 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -272,6 +272,7 @@  typedef struct GraphicHwOps {
     void (*invalidate)(void *opaque);
     void (*gfx_update)(void *opaque);
     void (*text_update)(void *opaque, console_ch_t *text);
+    void (*update_interval)(void *opaque, uint64_t interval);
 } GraphicHwOps;
 
 QemuConsole *graphic_console_init(const GraphicHwOps *ops,
diff --git a/ui/console.c b/ui/console.c
index be89747..ead5d35 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -182,6 +182,7 @@  static void gui_update(void *opaque)
     uint64_t dcl_interval;
     DisplayState *ds = opaque;
     DisplayChangeListener *dcl;
+    int i;
 
     ds->refreshing = true;
     dpy_refresh(ds);
@@ -196,6 +197,11 @@  static void gui_update(void *opaque)
     }
     if (ds->update_interval != interval) {
         ds->update_interval = interval;
+        for (i = 0; i < nb_consoles; i++) {
+            if (consoles[i]->hw_ops->update_interval) {
+                consoles[i]->hw_ops->update_interval(consoles[i]->hw, interval);
+            }
+        }
         trace_console_refresh(interval);
     }
     ds->last_update = qemu_get_clock_ms(rt_clock);