Re: [Qemu-devel] [PATCH v2 1/3] spice: drop dprint() debug logging

[Gerd Hoffmann]

> > @@ -413,8 +392,10 @@ void qemu_spice_display_switch(SimpleSpiceDisplay *ssd,
> >  surface_height(surface) == pixman_image_get_height(ssd->surface) &&
> >  surface_format(surface) == pixman_image_get_format(ssd->surface)) {
> >  /* no-resize fast path: just swap backing store */
> > -dprint(1, "%s/%d: fast (%dx%d)\n", __func__, ssd->qxl.id,
> > -   surface_width(surface), surface_height(surface));
> > +trace_qemu_spice_display_surface(ssd->qxl.id,
> > + surface_width(surface),
> > + surface_height(surface),
> > + true);
> >  qemu_mutex_lock(>lock);
> >  ssd->ds = surface;
> >  pixman_image_unref(ssd->surface);
> > @@ -427,11 +408,10 @@ void qemu_spice_display_switch(SimpleSpiceDisplay 
> > *ssd,
> >  }
> >
> >  /* full mode switch */
> > -dprint(1, "%s/%d: full (%dx%d -> %dx%d)\n", __func__, ssd->qxl.id,
> > -   ssd->surface ? pixman_image_get_width(ssd->surface)  : 0,
> > -   ssd->surface ? pixman_image_get_height(ssd->surface) : 0,
> > -   surface ? surface_width(surface)  : 0,
> > -   surface ? surface_height(surface) : 0);
> > +trace_qemu_spice_display_surface(ssd->qxl.id,
> > + surface_width(surface),
> > + surface_height(surface),
> > + false);
> 
> You are now assuming surface is always != NULL, but
> dpy_gfx_replace_surface() for example, has assert( || surface == NULL)
> before calling dpy_gfx_switch().

Oops.  Wasn't intentional (cut & paste from above).  Will fix.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 1/3] spice: drop dprint() debug logging

[Marc-André Lureau]

Hi

On Tue, Mar 6, 2018 at 9:38 AM, Gerd Hoffmann  wrote:
> Some calls are deleted, some are converted into tracepoints.
>
> Signed-off-by: Gerd Hoffmann 
> ---
>  ui/spice-display.c | 75 
> --
>  ui/trace-events|  9 +++
>  2 files changed, 31 insertions(+), 53 deletions(-)
>
> diff --git a/ui/spice-display.c b/ui/spice-display.c
> index 98ccdfb687..f3ae6beb3d 100644
> --- a/ui/spice-display.c
> +++ b/ui/spice-display.c
> @@ -26,20 +26,8 @@
>
>  #include "ui/spice-display.h"
>
> -static int debug = 0;
>  bool spice_opengl;
>
> -static void GCC_FMT_ATTR(2, 3) dprint(int level, const char *fmt, ...)
> -{
> -va_list args;
> -
> -if (level <= debug) {
> -va_start(args, fmt);
> -vfprintf(stderr, fmt, args);
> -va_end(args);
> -}
> -}
> -
>  int qemu_spice_rect_is_empty(const QXLRect* r)
>  {
>  return r->top == r->bottom || r->left == r->right;
> @@ -322,8 +310,6 @@ void qemu_spice_create_host_memslot(SimpleSpiceDisplay 
> *ssd)
>  {
>  QXLDevMemSlot memslot;
>
> -dprint(1, "%s/%d:\n", __func__, ssd->qxl.id);
> -
>  memset(, 0, sizeof(memslot));
>  memslot.slot_group_id = MEMSLOT_GROUP_HOST;
>  memslot.virt_end = ~0;
> @@ -347,10 +333,6 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay 
> *ssd)
>  ssd->buf = g_malloc(ssd->bufsize);
>  }
>
> -dprint(1, "%s/%d: %ux%u (size %" PRIu64 "/%d)\n", __func__, ssd->qxl.id,
> -   surface_width(ssd->ds), surface_height(ssd->ds),
> -   surface_size, ssd->bufsize);
> -
>  surface.format = SPICE_SURFACE_FMT_32_xRGB;
>  surface.width  = surface_width(ssd->ds);
>  surface.height = surface_height(ssd->ds);
> @@ -366,8 +348,6 @@ void qemu_spice_create_host_primary(SimpleSpiceDisplay 
> *ssd)
>
>  void qemu_spice_destroy_host_primary(SimpleSpiceDisplay *ssd)
>  {
> -dprint(1, "%s/%d:\n", __func__, ssd->qxl.id);
> -
>  qemu_spice_destroy_primary_surface(ssd, 0, QXL_SYNC);
>  }
>
> @@ -389,8 +369,7 @@ void qemu_spice_display_update(SimpleSpiceDisplay *ssd,
>  {
>  QXLRect update_area;
>
> -dprint(2, "%s/%d: x %d y %d w %d h %d\n", __func__,
> -   ssd->qxl.id, x, y, w, h);
> +trace_qemu_spice_display_update(ssd->qxl.id, x, y, w, h);
>  update_area.left = x,
>  update_area.right = x + w;
>  update_area.top = y;
> @@ -413,8 +392,10 @@ void qemu_spice_display_switch(SimpleSpiceDisplay *ssd,
>  surface_height(surface) == pixman_image_get_height(ssd->surface) &&
>  surface_format(surface) == pixman_image_get_format(ssd->surface)) {
>  /* no-resize fast path: just swap backing store */
> -dprint(1, "%s/%d: fast (%dx%d)\n", __func__, ssd->qxl.id,
> -   surface_width(surface), surface_height(surface));
> +trace_qemu_spice_display_surface(ssd->qxl.id,
> + surface_width(surface),
> + surface_height(surface),
> + true);
>  qemu_mutex_lock(>lock);
>  ssd->ds = surface;
>  pixman_image_unref(ssd->surface);
> @@ -427,11 +408,10 @@ void qemu_spice_display_switch(SimpleSpiceDisplay *ssd,
>  }
>
>  /* full mode switch */
> -dprint(1, "%s/%d: full (%dx%d -> %dx%d)\n", __func__, ssd->qxl.id,
> -   ssd->surface ? pixman_image_get_width(ssd->surface)  : 0,
> -   ssd->surface ? pixman_image_get_height(ssd->surface) : 0,
> -   surface ? surface_width(surface)  : 0,
> -   surface ? surface_height(surface) : 0);
> +trace_qemu_spice_display_surface(ssd->qxl.id,
> + surface_width(surface),
> + surface_height(surface),
> + false);

You are now assuming surface is always != NULL, but
dpy_gfx_replace_surface() for example, has assert( || surface == NULL)
before calling dpy_gfx_switch(). If such invariants is added, it would
be nice to document it in DisplayChangeListenerOps declaration.

>
>  memset(>dirty, 0, sizeof(ssd->dirty));
>  if (ssd->surface) {
> @@ -495,7 +475,6 @@ void qemu_spice_cursor_refresh_bh(void *opaque)
>
>  void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
>  {
> -dprint(3, "%s/%d:\n", __func__, ssd->qxl.id);
>  graphic_hw_update(ssd->dcl.con);
>
>  qemu_mutex_lock(>lock);
> @@ -505,10 +484,10 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
>  }
>  qemu_mutex_unlock(>lock);
>
> +trace_qemu_spice_display_refresh(ssd->qxl.id, ssd->notify);
>  if (ssd->notify) {
>  ssd->notify = 0;
>  qemu_spice_wakeup(ssd);
> -dprint(2, "%s/%d: notify\n", __func__, ssd->qxl.id);
>  }
>  }
>
> @@ -516,21 +495,17 @@ void qemu_spice_display_refresh(SimpleSpiceDisplay *ssd)
>
>  static void interface_attach_worker(QXLInstance *sin,