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

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(-)

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
>

>> -#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

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