[PATCH 00/14] hw/display: Omnibus cleanups

[PATCH 00/14] hw/display: Omnibus cleanups

[Philippe Mathieu-Daudé]

Hi Gerd, for your convenience I joined all the hw/display/
patches I sent recently altogether in a series.

Regards,

Phil.

Philippe Mathieu-Daudé (14):
  hw/display/edid: Add missing 'qdev-properties.h' header
  hw/display/cg3: Convert debug printf()s to trace events
  hw/display/cirrus_vga: Convert debug printf() to trace event
  hw/display/cirrus_vga: Use qemu_log_mask(UNIMP) instead of debug
    printf
  hw/display/cirrus_vga: Use qemu_log_mask(ERROR) instead of debug
    printf
  hw/display/cirrus_vga: Convert debug printf() to trace event
  hw/display/dpcd: Fix memory region size
  hw/display/dpcd: Convert debug printf()s to trace events
  hw/display/xlnx_dp: Replace disabled DPRINTF() by error_report()
  hw/display/vmware_vga: Replace printf() calls by qemu_log_mask(ERROR)
  hw/display/vmware_vga: Let the PCI device own its I/O MemoryRegion
  hw/display/exynos4210_fimd: Use qemu_log_mask(GUEST_ERROR)
  hw/display/omap_dss: Replace fprintf() call by
    qemu_log_mask(LOG_UNIMP)
  hw/display/pxa2xx_lcd: Replace printf() call by qemu_log_mask()

 include/hw/display/edid.h    |   1 +
 hw/display/cg3.c             |  14 ++---
 hw/display/cirrus_vga.c      | 119 +++++++++++++++--------------------
 hw/display/dpcd.c            |  20 ++----
 hw/display/exynos4210_fimd.c |  46 +++++++++-----
 hw/display/omap_dss.c        |   2 +-
 hw/display/pxa2xx_lcd.c      |  26 ++++----
 hw/display/vmware_vga.c      |  18 ++++--
 hw/display/xlnx_dp.c         |  14 +++--
 hw/display/trace-events      |  10 +++
 10 files changed, 136 insertions(+), 134 deletions(-)

-- 
2.21.3

[PATCH 01/14] hw/display/edid: Add missing 'qdev-properties.h' header

[Philippe Mathieu-Daudé]

When trying to consume the DEFINE_EDID_PROPERTIES() macro
by including "hw/display/edid.h", we get this build failure:

  include/hw/display/edid.h:24:5: error: implicit declaration of
  function ‘DEFINE_PROP_UINT32’ [-Werror=implicit-function-declaration]
     24 |     DEFINE_PROP_UINT32("xres", _state, _edid_info.prefx, 0),    \
        |     ^~~~~~~~~~~~~~~~~~

Headers should be self-contained, and one shouldn't have to
dig to find the missing headers.
In this case "hw/qdev-properties.h" is missing. Add it.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/display/edid.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/hw/display/edid.h b/include/hw/display/edid.h
index ff99dc0a05..23371ee82c 100644
--- a/include/hw/display/edid.h
+++ b/include/hw/display/edid.h
@@ -2,6 +2,7 @@
 #define EDID_H
 
 #include "qom/object.h"
+#include "hw/qdev-properties.h"
 
 typedef struct qemu_edid_info {
     const char *vendor; /* http://www.uefi.org/pnp_id_list */
-- 
2.21.3

Re: [PATCH 01/14] hw/display/edid: Add missing 'qdev-properties.h' header

[Alistair Francis]

On Mon, May 25, 2020 at 11:23 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> When trying to consume the DEFINE_EDID_PROPERTIES() macro
> by including "hw/display/edid.h", we get this build failure:
>
>   include/hw/display/edid.h:24:5: error: implicit declaration of
>   function ‘DEFINE_PROP_UINT32’ [-Werror=implicit-function-declaration]
>      24 |     DEFINE_PROP_UINT32("xres", _state, _edid_info.prefx, 0),    \
>         |     ^~~~~~~~~~~~~~~~~~
>
> Headers should be self-contained, and one shouldn't have to
> dig to find the missing headers.
> In this case "hw/qdev-properties.h" is missing. Add it.
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  include/hw/display/edid.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/include/hw/display/edid.h b/include/hw/display/edid.h
> index ff99dc0a05..23371ee82c 100644
> --- a/include/hw/display/edid.h
> +++ b/include/hw/display/edid.h
> @@ -2,6 +2,7 @@
>  #define EDID_H
>
>  #include "qom/object.h"
> +#include "hw/qdev-properties.h"
>
>  typedef struct qemu_edid_info {
>      const char *vendor; /* http://www.uefi.org/pnp_id_list */
> --
> 2.21.3
>
>

[PATCH 02/14] hw/display/cg3: Convert debug printf()s to trace events

[Philippe Mathieu-Daudé]

Convert DPRINTF() to trace events and remove ifdef'ry.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/cg3.c        | 14 ++++----------
 hw/display/trace-events |  4 ++++
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index f7f1c199ce..7cbe6e56ff 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -35,6 +35,7 @@
 #include "hw/qdev-properties.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "trace.h"
 
 /* Change to 1 to enable debugging */
 #define DEBUG_CG3 0
@@ -63,12 +64,6 @@
 #define CG3_VRAM_SIZE 0x100000
 #define CG3_VRAM_OFFSET 0x800000
 
-#define DPRINTF(fmt, ...) do { \
-    if (DEBUG_CG3) { \
-        printf("CG3: " fmt , ## __VA_ARGS__); \
-    } \
-} while (0)
-
 #define TYPE_CG3 "cgthree"
 #define CG3(obj) OBJECT_CHECK(CG3State, (obj), TYPE_CG3)
 
@@ -195,7 +190,8 @@ static uint64_t cg3_reg_read(void *opaque, hwaddr addr, unsigned size)
         val = 0;
         break;
     }
-    DPRINTF("read %02x from reg %" HWADDR_PRIx "\n", val, addr);
+    trace_cg3_read(addr, val, size);
+
     return val;
 }
 
@@ -206,9 +202,7 @@ static void cg3_reg_write(void *opaque, hwaddr addr, uint64_t val,
     uint8_t regval;
     int i;
 
-    DPRINTF("write %" PRIx64 " to reg %" HWADDR_PRIx " size %d\n",
-            val, addr, size);
-
+    trace_cg3_write(addr, val, size);
     switch (addr) {
     case CG3_REG_BT458_ADDR:
         s->dac_index = val;
diff --git a/hw/display/trace-events b/hw/display/trace-events
index e6e22bef88..47b2b168ae 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -151,3 +151,7 @@ artist_vram_write(unsigned int size, uint64_t addr, uint64_t val) "%u 0x%"PRIx64
 artist_fill_window(unsigned int start_x, unsigned int start_y, unsigned int width, unsigned int height, uint32_t op, uint32_t ctlpln) "start=%ux%u length=%ux%u op=0x%08x ctlpln=0x%08x"
 artist_block_move(unsigned int start_x, unsigned int start_y, unsigned int dest_x, unsigned int dest_y, unsigned int width, unsigned int height) "source %ux%u -> dest %ux%u size %ux%u"
 artist_draw_line(unsigned int start_x, unsigned int start_y, unsigned int end_x, unsigned int end_y) "%ux%u %ux%u"
+
+# cg3.c
+cg3_read(uint32_t addr, uint32_t val, unsigned size) "read addr:0x%06"PRIx32" val:0x%08"PRIx32" size:%u"
+cg3_write(uint32_t addr, uint32_t val, unsigned size) "write addr:0x%06"PRIx32" val:0x%08"PRIx32" size:%u"
-- 
2.21.3

Re: [PATCH 02/14] hw/display/cg3: Convert debug printf()s to trace events

[Alistair Francis]

On Mon, May 25, 2020 at 11:23 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Convert DPRINTF() to trace events and remove ifdef'ry.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/cg3.c        | 14 ++++----------
>  hw/display/trace-events |  4 ++++
>  2 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/hw/display/cg3.c b/hw/display/cg3.c
> index f7f1c199ce..7cbe6e56ff 100644
> --- a/hw/display/cg3.c
> +++ b/hw/display/cg3.c
> @@ -35,6 +35,7 @@
>  #include "hw/qdev-properties.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> +#include "trace.h"
>
>  /* Change to 1 to enable debugging */
>  #define DEBUG_CG3 0
> @@ -63,12 +64,6 @@
>  #define CG3_VRAM_SIZE 0x100000
>  #define CG3_VRAM_OFFSET 0x800000
>
> -#define DPRINTF(fmt, ...) do { \
> -    if (DEBUG_CG3) { \
> -        printf("CG3: " fmt , ## __VA_ARGS__); \
> -    } \
> -} while (0)
> -
>  #define TYPE_CG3 "cgthree"
>  #define CG3(obj) OBJECT_CHECK(CG3State, (obj), TYPE_CG3)
>
> @@ -195,7 +190,8 @@ static uint64_t cg3_reg_read(void *opaque, hwaddr addr, unsigned size)
>          val = 0;
>          break;
>      }
> -    DPRINTF("read %02x from reg %" HWADDR_PRIx "\n", val, addr);
> +    trace_cg3_read(addr, val, size);
> +
>      return val;
>  }
>
> @@ -206,9 +202,7 @@ static void cg3_reg_write(void *opaque, hwaddr addr, uint64_t val,
>      uint8_t regval;
>      int i;
>
> -    DPRINTF("write %" PRIx64 " to reg %" HWADDR_PRIx " size %d\n",
> -            val, addr, size);
> -
> +    trace_cg3_write(addr, val, size);
>      switch (addr) {
>      case CG3_REG_BT458_ADDR:
>          s->dac_index = val;
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index e6e22bef88..47b2b168ae 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -151,3 +151,7 @@ artist_vram_write(unsigned int size, uint64_t addr, uint64_t val) "%u 0x%"PRIx64
>  artist_fill_window(unsigned int start_x, unsigned int start_y, unsigned int width, unsigned int height, uint32_t op, uint32_t ctlpln) "start=%ux%u length=%ux%u op=0x%08x ctlpln=0x%08x"
>  artist_block_move(unsigned int start_x, unsigned int start_y, unsigned int dest_x, unsigned int dest_y, unsigned int width, unsigned int height) "source %ux%u -> dest %ux%u size %ux%u"
>  artist_draw_line(unsigned int start_x, unsigned int start_y, unsigned int end_x, unsigned int end_y) "%ux%u %ux%u"
> +
> +# cg3.c
> +cg3_read(uint32_t addr, uint32_t val, unsigned size) "read addr:0x%06"PRIx32" val:0x%08"PRIx32" size:%u"
> +cg3_write(uint32_t addr, uint32_t val, unsigned size) "write addr:0x%06"PRIx32" val:0x%08"PRIx32" size:%u"
> --
> 2.21.3
>
>

[PATCH 03/14] hw/display/cirrus_vga: Convert debug printf() to trace event

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/cirrus_vga.c | 4 +---
 hw/display/trace-events | 1 +
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 1f29731ffe..33ccdde000 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -1512,9 +1512,7 @@ static int cirrus_vga_read_gr(CirrusVGAState * s, unsigned reg_index)
 static void
 cirrus_vga_write_gr(CirrusVGAState * s, unsigned reg_index, int reg_value)
 {
-#if defined(DEBUG_BITBLT) && 0
-    printf("gr%02x: %02x\n", reg_index, reg_value);
-#endif
+    trace_vga_cirrus_write_gr(reg_index, reg_value);
     switch (reg_index) {
     case 0x00:			// Standard VGA, BGCOLOR 0x000000ff
 	s->vga.gr[reg_index] = reg_value & gr_mask[reg_index];
diff --git a/hw/display/trace-events b/hw/display/trace-events
index 47b2b168ae..c3043e4ced 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -133,6 +133,7 @@ vga_vbe_write(uint32_t index, uint32_t val) "index 0x%x, val 0x%x"
 vga_cirrus_read_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
 vga_cirrus_write_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
 vga_cirrus_write_blt(uint32_t offset, uint32_t val) "offset 0x%x, val 0x%x"
+vga_cirrus_write_gr(uint8_t index, uint8_t val) "GR addr 0x%02x, val 0x%02x"
 
 # sii9022.c
 sii9022_read_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val 0x%02x"
-- 
2.21.3

Re: [PATCH 03/14] hw/display/cirrus_vga: Convert debug printf() to trace event

[Alistair Francis]

On Mon, May 25, 2020 at 11:25 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/cirrus_vga.c | 4 +---
>  hw/display/trace-events | 1 +
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 1f29731ffe..33ccdde000 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -1512,9 +1512,7 @@ static int cirrus_vga_read_gr(CirrusVGAState * s, unsigned reg_index)
>  static void
>  cirrus_vga_write_gr(CirrusVGAState * s, unsigned reg_index, int reg_value)
>  {
> -#if defined(DEBUG_BITBLT) && 0
> -    printf("gr%02x: %02x\n", reg_index, reg_value);
> -#endif
> +    trace_vga_cirrus_write_gr(reg_index, reg_value);
>      switch (reg_index) {
>      case 0x00:                 // Standard VGA, BGCOLOR 0x000000ff
>         s->vga.gr[reg_index] = reg_value & gr_mask[reg_index];
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index 47b2b168ae..c3043e4ced 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -133,6 +133,7 @@ vga_vbe_write(uint32_t index, uint32_t val) "index 0x%x, val 0x%x"
>  vga_cirrus_read_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
>  vga_cirrus_write_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
>  vga_cirrus_write_blt(uint32_t offset, uint32_t val) "offset 0x%x, val 0x%x"
> +vga_cirrus_write_gr(uint8_t index, uint8_t val) "GR addr 0x%02x, val 0x%02x"
>
>  # sii9022.c
>  sii9022_read_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val 0x%02x"
> --
> 2.21.3
>
>

[PATCH 04/14] hw/display/cirrus_vga: Use qemu_log_mask(UNIMP) instead of debug printf

[Philippe Mathieu-Daudé]

Replace some debug printf() calls by qemu_log_mask(LOG_UNIMP),
and add a new one in cirrus_linear_bitblt_read().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/cirrus_vga.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 33ccdde000..f9f837b850 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -35,6 +35,7 @@
 #include "qemu/osdep.h"
 #include "qemu/module.h"
 #include "qemu/units.h"
+#include "qemu/log.h"
 #include "sysemu/reset.h"
 #include "qapi/error.h"
 #include "trace.h"
@@ -905,9 +906,8 @@ static int cirrus_bitblt_cputovideo(CirrusVGAState * s)
 static int cirrus_bitblt_videotocpu(CirrusVGAState * s)
 {
     /* XXX */
-#ifdef DEBUG_BITBLT
-    printf("cirrus: bitblt (video to cpu) is not implemented yet\n");
-#endif
+    qemu_log_mask(LOG_UNIMP,
+                  "cirrus: bitblt (video to cpu) is not implemented\n");
     return 0;
 }
 
@@ -989,9 +989,8 @@ static void cirrus_bitblt_start(CirrusVGAState * s)
 	 cirrus_blt_mode & (CIRRUS_BLTMODE_MEMSYSSRC |
 			    CIRRUS_BLTMODE_MEMSYSDEST))
 	== (CIRRUS_BLTMODE_MEMSYSSRC | CIRRUS_BLTMODE_MEMSYSDEST)) {
-#ifdef DEBUG_BITBLT
-	printf("cirrus: bitblt - memory-to-memory copy is requested\n");
-#endif
+        qemu_log_mask(LOG_UNIMP,
+                      "cirrus: bitblt - memory-to-memory copy requested\n");
 	goto bitblt_ignore;
     }
 
@@ -2412,6 +2411,9 @@ static uint64_t cirrus_linear_bitblt_read(void *opaque,
 
     /* XXX handle bitblt */
     (void)s;
+    qemu_log_mask(LOG_UNIMP,
+                  "cirrus: linear bitblt is not implemented\n");
+
     return 0xff;
 }
 
-- 
2.21.3

Re: [PATCH 04/14] hw/display/cirrus_vga: Use qemu_log_mask(UNIMP) instead of debug printf

[Alistair Francis]

On Mon, May 25, 2020 at 11:25 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Replace some debug printf() calls by qemu_log_mask(LOG_UNIMP),
> and add a new one in cirrus_linear_bitblt_read().
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/cirrus_vga.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 33ccdde000..f9f837b850 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -35,6 +35,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/module.h"
>  #include "qemu/units.h"
> +#include "qemu/log.h"
>  #include "sysemu/reset.h"
>  #include "qapi/error.h"
>  #include "trace.h"
> @@ -905,9 +906,8 @@ static int cirrus_bitblt_cputovideo(CirrusVGAState * s)
>  static int cirrus_bitblt_videotocpu(CirrusVGAState * s)
>  {
>      /* XXX */
> -#ifdef DEBUG_BITBLT
> -    printf("cirrus: bitblt (video to cpu) is not implemented yet\n");
> -#endif
> +    qemu_log_mask(LOG_UNIMP,
> +                  "cirrus: bitblt (video to cpu) is not implemented\n");
>      return 0;
>  }
>
> @@ -989,9 +989,8 @@ static void cirrus_bitblt_start(CirrusVGAState * s)
>          cirrus_blt_mode & (CIRRUS_BLTMODE_MEMSYSSRC |
>                             CIRRUS_BLTMODE_MEMSYSDEST))
>         == (CIRRUS_BLTMODE_MEMSYSSRC | CIRRUS_BLTMODE_MEMSYSDEST)) {
> -#ifdef DEBUG_BITBLT
> -       printf("cirrus: bitblt - memory-to-memory copy is requested\n");
> -#endif
> +        qemu_log_mask(LOG_UNIMP,
> +                      "cirrus: bitblt - memory-to-memory copy requested\n");
>         goto bitblt_ignore;
>      }
>
> @@ -2412,6 +2411,9 @@ static uint64_t cirrus_linear_bitblt_read(void *opaque,
>
>      /* XXX handle bitblt */
>      (void)s;
> +    qemu_log_mask(LOG_UNIMP,
> +                  "cirrus: linear bitblt is not implemented\n");
> +
>      return 0xff;
>  }
>
> --
> 2.21.3
>
>

[PATCH 05/14] hw/display/cirrus_vga: Use qemu_log_mask(ERROR) instead of debug printf

[Philippe Mathieu-Daudé]

Replace some debug printf() calls by qemu_log_mask(LOG_GUEST_ERROR).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/cirrus_vga.c | 77 ++++++++++++++++++-----------------------
 1 file changed, 33 insertions(+), 44 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index f9f837b850..76e2dc5bb6 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -978,9 +978,8 @@ static void cirrus_bitblt_start(CirrusVGAState * s)
 	s->cirrus_blt_pixelwidth = 4;
 	break;
     default:
-#ifdef DEBUG_BITBLT
-	printf("cirrus: bitblt - pixel width is unknown\n");
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: bitblt - pixel width is unknown\n");
 	goto bitblt_ignore;
     }
     s->cirrus_blt_mode &= ~CIRRUS_BLTMODE_PIXELWIDTHMASK;
@@ -1037,7 +1036,9 @@ static void cirrus_bitblt_start(CirrusVGAState * s)
         } else {
 	    if (s->cirrus_blt_mode & CIRRUS_BLTMODE_TRANSPARENTCOMP) {
 		if (s->cirrus_blt_pixelwidth > 2) {
-		    printf("src transparent without colorexpand must be 8bpp or 16bpp\n");
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "cirrus: src transparent without colorexpand "
+                          "must be 8bpp or 16bpp\n");
 		    goto bitblt_ignore;
 		}
 		if (s->cirrus_blt_mode & CIRRUS_BLTMODE_BACKWARDS) {
@@ -1135,10 +1136,9 @@ static uint32_t cirrus_get_bpp16_depth(CirrusVGAState * s)
 	ret = 16;
 	break;			/* XGA HiColor */
     default:
-#ifdef DEBUG_CIRRUS
-	printf("cirrus: invalid DAC value %x in 16bpp\n",
-	       (s->cirrus_hidden_dac_data & 0xf));
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: invalid DAC value 0x%x in 16bpp\n",
+                      (s->cirrus_hidden_dac_data & 0xf));
 	ret = 15;		/* XXX */
 	break;
     }
@@ -1307,11 +1307,9 @@ static int cirrus_vga_read_sr(CirrusVGAState * s)
 #endif
 	return s->vga.sr[s->vga.sr_index];
     default:
-#ifdef DEBUG_CIRRUS
-	printf("cirrus: inport sr_index %02x\n", s->vga.sr_index);
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: inport sr_index 0x%02x\n", s->vga.sr_index);
 	return 0xff;
-	break;
     }
 }
 
@@ -1400,10 +1398,9 @@ static void cirrus_vga_write_sr(CirrusVGAState * s, uint32_t val)
         cirrus_update_memory_access(s);
         break;
     default:
-#ifdef DEBUG_CIRRUS
-	printf("cirrus: outport sr_index %02x, sr_value %02x\n",
-               s->vga.sr_index, val);
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: outport sr_index 0x%02x, sr_value 0x%02x\n",
+                      s->vga.sr_index, val);
 	break;
     }
 }
@@ -1501,9 +1498,8 @@ static int cirrus_vga_read_gr(CirrusVGAState * s, unsigned reg_index)
     if (reg_index < 0x3a) {
 	return s->vga.gr[reg_index];
     } else {
-#ifdef DEBUG_CIRRUS
-	printf("cirrus: inport gr_index %02x\n", reg_index);
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: inport gr_index 0x%02x\n", reg_index);
 	return 0xff;
     }
 }
@@ -1590,10 +1586,9 @@ cirrus_vga_write_gr(CirrusVGAState * s, unsigned reg_index, int reg_value)
 	cirrus_write_bitblt(s, reg_value);
 	break;
     default:
-#ifdef DEBUG_CIRRUS
-	printf("cirrus: outport gr_index %02x, gr_value %02x\n", reg_index,
-	       reg_value);
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: outport gr_index 0x%02x, gr_value 0x%02x\n",
+                      reg_index, reg_value);
 	break;
     }
 }
@@ -1648,9 +1643,8 @@ static int cirrus_vga_read_cr(CirrusVGAState * s, unsigned reg_index)
 	return s->vga.ar_index & 0x3f;
 	break;
     default:
-#ifdef DEBUG_CIRRUS
-	printf("cirrus: inport cr_index %02x\n", reg_index);
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: inport cr_index 0x%02x\n", reg_index);
 	return 0xff;
     }
 }
@@ -1721,10 +1715,9 @@ static void cirrus_vga_write_cr(CirrusVGAState * s, int reg_value)
 	break;
     case 0x25:			// Part Status
     default:
-#ifdef DEBUG_CIRRUS
-	printf("cirrus: outport cr_index %02x, cr_value %02x\n",
-               s->vga.cr_index, reg_value);
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: outport cr_index 0x%02x, cr_value 0x%02x\n",
+                      s->vga.cr_index, reg_value);
 	break;
     }
 }
@@ -1834,9 +1827,8 @@ static uint8_t cirrus_mmio_blt_read(CirrusVGAState * s, unsigned address)
 	value = cirrus_vga_read_gr(s, 0x31);
 	break;
     default:
-#ifdef DEBUG_CIRRUS
-	printf("cirrus: mmio read - address 0x%04x\n", address);
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: mmio read - address 0x%04x\n", address);
 	break;
     }
 
@@ -1946,10 +1938,9 @@ static void cirrus_mmio_blt_write(CirrusVGAState * s, unsigned address,
 	cirrus_vga_write_gr(s, 0x31, value);
 	break;
     default:
-#ifdef DEBUG_CIRRUS
-	printf("cirrus: mmio write - addr 0x%04x val 0x%02x (ignored)\n",
-	       address, value);
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: mmio write - addr 0x%04x val 0x%02x (ignored)\n",
+                      address, value);
 	break;
     }
 }
@@ -2047,9 +2038,8 @@ static uint64_t cirrus_vga_mem_read(void *opaque,
 	}
     } else {
 	val = 0xff;
-#ifdef DEBUG_CIRRUS
-	printf("cirrus: mem_readb " TARGET_FMT_plx "\n", addr);
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: mem_readb 0x" TARGET_FMT_plx "\n", addr);
     }
     return val;
 }
@@ -2112,10 +2102,9 @@ static void cirrus_vga_mem_write(void *opaque,
 	    cirrus_mmio_blt_write(s, addr & 0xff, mem_value);
 	}
     } else {
-#ifdef DEBUG_CIRRUS
-        printf("cirrus: mem_writeb " TARGET_FMT_plx " value 0x%02" PRIu64 "\n", addr,
-               mem_value);
-#endif
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "cirrus: mem_writeb 0x" TARGET_FMT_plx " "
+                      "value 0x%02" PRIu64 "\n", addr, mem_value);
     }
 }
 
-- 
2.21.3

Re: [PATCH 05/14] hw/display/cirrus_vga: Use qemu_log_mask(ERROR) instead of debug printf

[Alistair Francis]

On Mon, May 25, 2020 at 11:27 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Replace some debug printf() calls by qemu_log_mask(LOG_GUEST_ERROR).
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/cirrus_vga.c | 77 ++++++++++++++++++-----------------------
>  1 file changed, 33 insertions(+), 44 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index f9f837b850..76e2dc5bb6 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -978,9 +978,8 @@ static void cirrus_bitblt_start(CirrusVGAState * s)
>         s->cirrus_blt_pixelwidth = 4;
>         break;
>      default:
> -#ifdef DEBUG_BITBLT
> -       printf("cirrus: bitblt - pixel width is unknown\n");
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: bitblt - pixel width is unknown\n");
>         goto bitblt_ignore;
>      }
>      s->cirrus_blt_mode &= ~CIRRUS_BLTMODE_PIXELWIDTHMASK;
> @@ -1037,7 +1036,9 @@ static void cirrus_bitblt_start(CirrusVGAState * s)
>          } else {
>             if (s->cirrus_blt_mode & CIRRUS_BLTMODE_TRANSPARENTCOMP) {
>                 if (s->cirrus_blt_pixelwidth > 2) {
> -                   printf("src transparent without colorexpand must be 8bpp or 16bpp\n");
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "cirrus: src transparent without colorexpand "
> +                          "must be 8bpp or 16bpp\n");
>                     goto bitblt_ignore;
>                 }
>                 if (s->cirrus_blt_mode & CIRRUS_BLTMODE_BACKWARDS) {
> @@ -1135,10 +1136,9 @@ static uint32_t cirrus_get_bpp16_depth(CirrusVGAState * s)
>         ret = 16;
>         break;                  /* XGA HiColor */
>      default:
> -#ifdef DEBUG_CIRRUS
> -       printf("cirrus: invalid DAC value %x in 16bpp\n",
> -              (s->cirrus_hidden_dac_data & 0xf));
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: invalid DAC value 0x%x in 16bpp\n",
> +                      (s->cirrus_hidden_dac_data & 0xf));
>         ret = 15;               /* XXX */
>         break;
>      }
> @@ -1307,11 +1307,9 @@ static int cirrus_vga_read_sr(CirrusVGAState * s)
>  #endif
>         return s->vga.sr[s->vga.sr_index];
>      default:
> -#ifdef DEBUG_CIRRUS
> -       printf("cirrus: inport sr_index %02x\n", s->vga.sr_index);
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: inport sr_index 0x%02x\n", s->vga.sr_index);
>         return 0xff;
> -       break;
>      }
>  }
>
> @@ -1400,10 +1398,9 @@ static void cirrus_vga_write_sr(CirrusVGAState * s, uint32_t val)
>          cirrus_update_memory_access(s);
>          break;
>      default:
> -#ifdef DEBUG_CIRRUS
> -       printf("cirrus: outport sr_index %02x, sr_value %02x\n",
> -               s->vga.sr_index, val);
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: outport sr_index 0x%02x, sr_value 0x%02x\n",
> +                      s->vga.sr_index, val);
>         break;
>      }
>  }
> @@ -1501,9 +1498,8 @@ static int cirrus_vga_read_gr(CirrusVGAState * s, unsigned reg_index)
>      if (reg_index < 0x3a) {
>         return s->vga.gr[reg_index];
>      } else {
> -#ifdef DEBUG_CIRRUS
> -       printf("cirrus: inport gr_index %02x\n", reg_index);
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: inport gr_index 0x%02x\n", reg_index);
>         return 0xff;
>      }
>  }
> @@ -1590,10 +1586,9 @@ cirrus_vga_write_gr(CirrusVGAState * s, unsigned reg_index, int reg_value)
>         cirrus_write_bitblt(s, reg_value);
>         break;
>      default:
> -#ifdef DEBUG_CIRRUS
> -       printf("cirrus: outport gr_index %02x, gr_value %02x\n", reg_index,
> -              reg_value);
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: outport gr_index 0x%02x, gr_value 0x%02x\n",
> +                      reg_index, reg_value);
>         break;
>      }
>  }
> @@ -1648,9 +1643,8 @@ static int cirrus_vga_read_cr(CirrusVGAState * s, unsigned reg_index)
>         return s->vga.ar_index & 0x3f;
>         break;
>      default:
> -#ifdef DEBUG_CIRRUS
> -       printf("cirrus: inport cr_index %02x\n", reg_index);
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: inport cr_index 0x%02x\n", reg_index);
>         return 0xff;
>      }
>  }
> @@ -1721,10 +1715,9 @@ static void cirrus_vga_write_cr(CirrusVGAState * s, int reg_value)
>         break;
>      case 0x25:                 // Part Status
>      default:
> -#ifdef DEBUG_CIRRUS
> -       printf("cirrus: outport cr_index %02x, cr_value %02x\n",
> -               s->vga.cr_index, reg_value);
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: outport cr_index 0x%02x, cr_value 0x%02x\n",
> +                      s->vga.cr_index, reg_value);
>         break;
>      }
>  }
> @@ -1834,9 +1827,8 @@ static uint8_t cirrus_mmio_blt_read(CirrusVGAState * s, unsigned address)
>         value = cirrus_vga_read_gr(s, 0x31);
>         break;
>      default:
> -#ifdef DEBUG_CIRRUS
> -       printf("cirrus: mmio read - address 0x%04x\n", address);
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: mmio read - address 0x%04x\n", address);
>         break;
>      }
>
> @@ -1946,10 +1938,9 @@ static void cirrus_mmio_blt_write(CirrusVGAState * s, unsigned address,
>         cirrus_vga_write_gr(s, 0x31, value);
>         break;
>      default:
> -#ifdef DEBUG_CIRRUS
> -       printf("cirrus: mmio write - addr 0x%04x val 0x%02x (ignored)\n",
> -              address, value);
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: mmio write - addr 0x%04x val 0x%02x (ignored)\n",
> +                      address, value);
>         break;
>      }
>  }
> @@ -2047,9 +2038,8 @@ static uint64_t cirrus_vga_mem_read(void *opaque,
>         }
>      } else {
>         val = 0xff;
> -#ifdef DEBUG_CIRRUS
> -       printf("cirrus: mem_readb " TARGET_FMT_plx "\n", addr);
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: mem_readb 0x" TARGET_FMT_plx "\n", addr);
>      }
>      return val;
>  }
> @@ -2112,10 +2102,9 @@ static void cirrus_vga_mem_write(void *opaque,
>             cirrus_mmio_blt_write(s, addr & 0xff, mem_value);
>         }
>      } else {
> -#ifdef DEBUG_CIRRUS
> -        printf("cirrus: mem_writeb " TARGET_FMT_plx " value 0x%02" PRIu64 "\n", addr,
> -               mem_value);
> -#endif
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "cirrus: mem_writeb 0x" TARGET_FMT_plx " "
> +                      "value 0x%02" PRIu64 "\n", addr, mem_value);
>      }
>  }
>
> --
> 2.21.3
>
>

[PATCH 06/14] hw/display/cirrus_vga: Convert debug printf() to trace event

[Philippe Mathieu-Daudé]

Convert the final bit of DEBUG_BITBLT to a tracepoint.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/cirrus_vga.c | 24 ++++++++++--------------
 hw/display/trace-events |  1 +
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
index 76e2dc5bb6..92c197cdde 100644
--- a/hw/display/cirrus_vga.c
+++ b/hw/display/cirrus_vga.c
@@ -53,7 +53,6 @@
  */
 
 //#define DEBUG_CIRRUS
-//#define DEBUG_BITBLT
 
 /***************************************
  *
@@ -950,19 +949,16 @@ static void cirrus_bitblt_start(CirrusVGAState * s)
     s->cirrus_blt_dstaddr &= s->cirrus_addr_mask;
     s->cirrus_blt_srcaddr &= s->cirrus_addr_mask;
 
-#ifdef DEBUG_BITBLT
-    printf("rop=0x%02x mode=0x%02x modeext=0x%02x w=%d h=%d dpitch=%d spitch=%d daddr=0x%08x saddr=0x%08x writemask=0x%02x\n",
-           blt_rop,
-           s->cirrus_blt_mode,
-           s->cirrus_blt_modeext,
-           s->cirrus_blt_width,
-           s->cirrus_blt_height,
-           s->cirrus_blt_dstpitch,
-           s->cirrus_blt_srcpitch,
-           s->cirrus_blt_dstaddr,
-           s->cirrus_blt_srcaddr,
-           s->vga.gr[0x2f]);
-#endif
+    trace_vga_cirrus_bitblt_start(blt_rop,
+                                  s->cirrus_blt_mode,
+                                  s->cirrus_blt_modeext,
+                                  s->cirrus_blt_width,
+                                  s->cirrus_blt_height,
+                                  s->cirrus_blt_dstpitch,
+                                  s->cirrus_blt_srcpitch,
+                                  s->cirrus_blt_dstaddr,
+                                  s->cirrus_blt_srcaddr,
+                                  s->vga.gr[0x2f]);
 
     switch (s->cirrus_blt_mode & CIRRUS_BLTMODE_PIXELWIDTHMASK) {
     case CIRRUS_BLTMODE_PIXELWIDTH8:
diff --git a/hw/display/trace-events b/hw/display/trace-events
index c3043e4ced..bb089a5f5e 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -134,6 +134,7 @@ vga_cirrus_read_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
 vga_cirrus_write_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
 vga_cirrus_write_blt(uint32_t offset, uint32_t val) "offset 0x%x, val 0x%x"
 vga_cirrus_write_gr(uint8_t index, uint8_t val) "GR addr 0x%02x, val 0x%02x"
+vga_cirrus_bitblt_start(uint8_t blt_rop, uint8_t blt_mode, uint8_t blt_modeext, int blt_width, int blt_height, int blt_dstpitch, int blt_srcpitch, uint32_t blt_dstaddr, uint32_t blt_srcaddr, uint8_t gr_val) "rop=0x%02x mode=0x%02x modeext=0x%02x w=%d h=%d dpitch=%d spitch=%d daddr=0x%08"PRIx32" saddr=0x%08"PRIx32" writemask=0x%02x"
 
 # sii9022.c
 sii9022_read_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val 0x%02x"
-- 
2.21.3

Re: [PATCH 06/14] hw/display/cirrus_vga: Convert debug printf() to trace event

[Alistair Francis]

On Mon, May 25, 2020 at 11:25 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Convert the final bit of DEBUG_BITBLT to a tracepoint.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/cirrus_vga.c | 24 ++++++++++--------------
>  hw/display/trace-events |  1 +
>  2 files changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c
> index 76e2dc5bb6..92c197cdde 100644
> --- a/hw/display/cirrus_vga.c
> +++ b/hw/display/cirrus_vga.c
> @@ -53,7 +53,6 @@
>   */
>
>  //#define DEBUG_CIRRUS
> -//#define DEBUG_BITBLT
>
>  /***************************************
>   *
> @@ -950,19 +949,16 @@ static void cirrus_bitblt_start(CirrusVGAState * s)
>      s->cirrus_blt_dstaddr &= s->cirrus_addr_mask;
>      s->cirrus_blt_srcaddr &= s->cirrus_addr_mask;
>
> -#ifdef DEBUG_BITBLT
> -    printf("rop=0x%02x mode=0x%02x modeext=0x%02x w=%d h=%d dpitch=%d spitch=%d daddr=0x%08x saddr=0x%08x writemask=0x%02x\n",
> -           blt_rop,
> -           s->cirrus_blt_mode,
> -           s->cirrus_blt_modeext,
> -           s->cirrus_blt_width,
> -           s->cirrus_blt_height,
> -           s->cirrus_blt_dstpitch,
> -           s->cirrus_blt_srcpitch,
> -           s->cirrus_blt_dstaddr,
> -           s->cirrus_blt_srcaddr,
> -           s->vga.gr[0x2f]);
> -#endif
> +    trace_vga_cirrus_bitblt_start(blt_rop,
> +                                  s->cirrus_blt_mode,
> +                                  s->cirrus_blt_modeext,
> +                                  s->cirrus_blt_width,
> +                                  s->cirrus_blt_height,
> +                                  s->cirrus_blt_dstpitch,
> +                                  s->cirrus_blt_srcpitch,
> +                                  s->cirrus_blt_dstaddr,
> +                                  s->cirrus_blt_srcaddr,
> +                                  s->vga.gr[0x2f]);
>
>      switch (s->cirrus_blt_mode & CIRRUS_BLTMODE_PIXELWIDTHMASK) {
>      case CIRRUS_BLTMODE_PIXELWIDTH8:
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index c3043e4ced..bb089a5f5e 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -134,6 +134,7 @@ vga_cirrus_read_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
>  vga_cirrus_write_io(uint32_t addr, uint32_t val) "addr 0x%x, val 0x%x"
>  vga_cirrus_write_blt(uint32_t offset, uint32_t val) "offset 0x%x, val 0x%x"
>  vga_cirrus_write_gr(uint8_t index, uint8_t val) "GR addr 0x%02x, val 0x%02x"
> +vga_cirrus_bitblt_start(uint8_t blt_rop, uint8_t blt_mode, uint8_t blt_modeext, int blt_width, int blt_height, int blt_dstpitch, int blt_srcpitch, uint32_t blt_dstaddr, uint32_t blt_srcaddr, uint8_t gr_val) "rop=0x%02x mode=0x%02x modeext=0x%02x w=%d h=%d dpitch=%d spitch=%d daddr=0x%08"PRIx32" saddr=0x%08"PRIx32" writemask=0x%02x"
>
>  # sii9022.c
>  sii9022_read_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val 0x%02x"
> --
> 2.21.3
>
>

[PATCH 07/14] hw/display/dpcd: Fix memory region size

[Philippe Mathieu-Daudé]

The memory region size is 512K.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/dpcd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/display/dpcd.c b/hw/display/dpcd.c
index 170545c605..0c1b7b35fb 100644
--- a/hw/display/dpcd.c
+++ b/hw/display/dpcd.c
@@ -1,5 +1,5 @@
 /*
- * dpcd.c
+ * Xilinx Display Port Control Data
  *
  *  Copyright (C) 2015 : GreenSocs Ltd
  *      http://www.greensocs.com/ , email: info@greensocs.com
@@ -137,7 +137,7 @@ static void dpcd_init(Object *obj)
 {
     DPCDState *s = DPCD(obj);
 
-    memory_region_init_io(&s->iomem, obj, &aux_ops, s, TYPE_DPCD, 0x7FFFF);
+    memory_region_init_io(&s->iomem, obj, &aux_ops, s, TYPE_DPCD, 0x80000);
     aux_init_mmio(AUX_SLAVE(obj), &s->iomem);
 }
 
-- 
2.21.3

Re: [PATCH 07/14] hw/display/dpcd: Fix memory region size

[Alistair Francis]

On Mon, May 25, 2020 at 11:27 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The memory region size is 512K.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/dpcd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/dpcd.c b/hw/display/dpcd.c
> index 170545c605..0c1b7b35fb 100644
> --- a/hw/display/dpcd.c
> +++ b/hw/display/dpcd.c
> @@ -1,5 +1,5 @@
>  /*
> - * dpcd.c
> + * Xilinx Display Port Control Data
>   *
>   *  Copyright (C) 2015 : GreenSocs Ltd
>   *      http://www.greensocs.com/ , email: info@greensocs.com
> @@ -137,7 +137,7 @@ static void dpcd_init(Object *obj)
>  {
>      DPCDState *s = DPCD(obj);
>
> -    memory_region_init_io(&s->iomem, obj, &aux_ops, s, TYPE_DPCD, 0x7FFFF);
> +    memory_region_init_io(&s->iomem, obj, &aux_ops, s, TYPE_DPCD, 0x80000);
>      aux_init_mmio(AUX_SLAVE(obj), &s->iomem);
>  }
>
> --
> 2.21.3
>
>

[PATCH 08/14] hw/display/dpcd: Convert debug printf()s to trace events

[Philippe Mathieu-Daudé]

Convert DPRINTF() to trace events and remove ifdef'ry.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/dpcd.c       | 16 +++-------------
 hw/display/trace-events |  4 ++++
 2 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/hw/display/dpcd.c b/hw/display/dpcd.c
index 0c1b7b35fb..64463654a1 100644
--- a/hw/display/dpcd.c
+++ b/hw/display/dpcd.c
@@ -32,16 +32,7 @@
 #include "hw/misc/auxbus.h"
 #include "migration/vmstate.h"
 #include "hw/display/dpcd.h"
-
-#ifndef DEBUG_DPCD
-#define DEBUG_DPCD 0
-#endif
-
-#define DPRINTF(fmt, ...) do {                                                 \
-    if (DEBUG_DPCD) {                                                          \
-        qemu_log("dpcd: " fmt, ## __VA_ARGS__);                                \
-    }                                                                          \
-} while (0)
+#include "trace.h"
 
 #define DPCD_READABLE_AREA                      0x600
 
@@ -70,8 +61,8 @@ static uint64_t dpcd_read(void *opaque, hwaddr offset, unsigned size)
                                        offset);
         ret = 0;
     }
+    trace_dpcd_read(offset, ret);
 
-    DPRINTF("read 0x%" PRIX8 " @0x%" HWADDR_PRIX "\n", ret, offset);
     return ret;
 }
 
@@ -80,8 +71,7 @@ static void dpcd_write(void *opaque, hwaddr offset, uint64_t value,
 {
     DPCDState *e = DPCD(opaque);
 
-    DPRINTF("write 0x%" PRIX8 " @0x%" HWADDR_PRIX "\n", (uint8_t)value, offset);
-
+    trace_dpcd_write(offset, value);
     if (offset < DPCD_READABLE_AREA) {
         e->dpcd_info[offset] = value;
     } else {
diff --git a/hw/display/trace-events b/hw/display/trace-events
index bb089a5f5e..72d4c9812c 100644
--- a/hw/display/trace-events
+++ b/hw/display/trace-events
@@ -157,3 +157,7 @@ artist_draw_line(unsigned int start_x, unsigned int start_y, unsigned int end_x,
 # cg3.c
 cg3_read(uint32_t addr, uint32_t val, unsigned size) "read addr:0x%06"PRIx32" val:0x%08"PRIx32" size:%u"
 cg3_write(uint32_t addr, uint32_t val, unsigned size) "write addr:0x%06"PRIx32" val:0x%08"PRIx32" size:%u"
+
+# dpcd.c
+dpcd_read(uint32_t addr, uint8_t val) "read addr:0x%"PRIx32" val:0x%02x"
+dpcd_write(uint32_t addr, uint8_t val) "write addr:0x%"PRIx32" val:0x%02x"
-- 
2.21.3

Re: [PATCH 08/14] hw/display/dpcd: Convert debug printf()s to trace events

[Alistair Francis]

On Mon, May 25, 2020 at 11:25 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Convert DPRINTF() to trace events and remove ifdef'ry.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/dpcd.c       | 16 +++-------------
>  hw/display/trace-events |  4 ++++
>  2 files changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/hw/display/dpcd.c b/hw/display/dpcd.c
> index 0c1b7b35fb..64463654a1 100644
> --- a/hw/display/dpcd.c
> +++ b/hw/display/dpcd.c
> @@ -32,16 +32,7 @@
>  #include "hw/misc/auxbus.h"
>  #include "migration/vmstate.h"
>  #include "hw/display/dpcd.h"
> -
> -#ifndef DEBUG_DPCD
> -#define DEBUG_DPCD 0
> -#endif
> -
> -#define DPRINTF(fmt, ...) do {                                                 \
> -    if (DEBUG_DPCD) {                                                          \
> -        qemu_log("dpcd: " fmt, ## __VA_ARGS__);                                \
> -    }                                                                          \
> -} while (0)
> +#include "trace.h"
>
>  #define DPCD_READABLE_AREA                      0x600
>
> @@ -70,8 +61,8 @@ static uint64_t dpcd_read(void *opaque, hwaddr offset, unsigned size)
>                                         offset);
>          ret = 0;
>      }
> +    trace_dpcd_read(offset, ret);
>
> -    DPRINTF("read 0x%" PRIX8 " @0x%" HWADDR_PRIX "\n", ret, offset);
>      return ret;
>  }
>
> @@ -80,8 +71,7 @@ static void dpcd_write(void *opaque, hwaddr offset, uint64_t value,
>  {
>      DPCDState *e = DPCD(opaque);
>
> -    DPRINTF("write 0x%" PRIX8 " @0x%" HWADDR_PRIX "\n", (uint8_t)value, offset);
> -
> +    trace_dpcd_write(offset, value);
>      if (offset < DPCD_READABLE_AREA) {
>          e->dpcd_info[offset] = value;
>      } else {
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index bb089a5f5e..72d4c9812c 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -157,3 +157,7 @@ artist_draw_line(unsigned int start_x, unsigned int start_y, unsigned int end_x,
>  # cg3.c
>  cg3_read(uint32_t addr, uint32_t val, unsigned size) "read addr:0x%06"PRIx32" val:0x%08"PRIx32" size:%u"
>  cg3_write(uint32_t addr, uint32_t val, unsigned size) "write addr:0x%06"PRIx32" val:0x%08"PRIx32" size:%u"
> +
> +# dpcd.c
> +dpcd_read(uint32_t addr, uint8_t val) "read addr:0x%"PRIx32" val:0x%02x"
> +dpcd_write(uint32_t addr, uint8_t val) "write addr:0x%"PRIx32" val:0x%02x"
> --
> 2.21.3
>
>

[PATCH 09/14] hw/display/xlnx_dp: Replace disabled DPRINTF() by error_report()

[Philippe Mathieu-Daudé]

DPRINTF() calls are disabled by default, so when unexpected
data is used, the whole process abort without information.

Display a bit of information with error_report() before crashing.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/xlnx_dp.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
index 3e5fb44e06..8d940cd8d1 100644
--- a/hw/display/xlnx_dp.c
+++ b/hw/display/xlnx_dp.c
@@ -1,5 +1,5 @@
 /*
- * xlnx_dp.c
+ * Xilinx Display Port
  *
  *  Copyright (C) 2015 : GreenSocs Ltd
  *      http://www.greensocs.com/ , email: info@greensocs.com
@@ -24,6 +24,7 @@
 
 #include "qemu/osdep.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "hw/display/xlnx_dp.h"
@@ -465,7 +466,7 @@ static uint8_t xlnx_dp_aux_pop_tx_fifo(XlnxDPState *s)
     uint8_t ret;
 
     if (fifo8_is_empty(&s->tx_fifo)) {
-        DPRINTF("tx_fifo underflow..\n");
+        error_report("%s: TX_FIFO underflow", __func__);
         abort();
     }
     ret = fifo8_pop(&s->tx_fifo);
@@ -525,6 +526,7 @@ static void xlnx_dp_aux_set_command(XlnxDPState *s, uint32_t value)
         qemu_log_mask(LOG_UNIMP, "xlnx_dp: Write i2c status not implemented\n");
         break;
     default:
+        error_report("%s: invalid command: %u", __func__, cmd);
         abort();
     }
 
@@ -631,8 +633,8 @@ static void xlnx_dp_change_graphic_fmt(XlnxDPState *s)
         s->g_plane.format = PIXMAN_b8g8r8;
         break;
     default:
-        DPRINTF("error: unsupported graphic format %u.\n",
-                s->avbufm_registers[AV_BUF_FORMAT] & DP_GRAPHIC_MASK);
+        error_report("%s: unsupported graphic format %u", __func__,
+                     s->avbufm_registers[AV_BUF_FORMAT] & DP_GRAPHIC_MASK);
         abort();
     }
 
@@ -647,8 +649,8 @@ static void xlnx_dp_change_graphic_fmt(XlnxDPState *s)
         s->v_plane.format = PIXMAN_x8b8g8r8;
         break;
     default:
-        DPRINTF("error: unsupported video format %u.\n",
-                s->avbufm_registers[AV_BUF_FORMAT] & DP_NL_VID_FMT_MASK);
+        error_report("%s: unsupported video format %u", __func__,
+                     s->avbufm_registers[AV_BUF_FORMAT] & DP_NL_VID_FMT_MASK);
         abort();
     }
 
-- 
2.21.3

Re: [PATCH 09/14] hw/display/xlnx_dp: Replace disabled DPRINTF() by error_report()

[Edgar E. Iglesias]

On Tue, May 26, 2020 at 08:22:47AM +0200, Philippe Mathieu-Daudé wrote:
> DPRINTF() calls are disabled by default, so when unexpected
> data is used, the whole process abort without information.
> 
> Display a bit of information with error_report() before crashing.

Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>



> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/display/xlnx_dp.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 3e5fb44e06..8d940cd8d1 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -1,5 +1,5 @@
>  /*
> - * xlnx_dp.c
> + * Xilinx Display Port
>   *
>   *  Copyright (C) 2015 : GreenSocs Ltd
>   *      http://www.greensocs.com/ , email: info@greensocs.com
> @@ -24,6 +24,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qemu/error-report.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "hw/display/xlnx_dp.h"
> @@ -465,7 +466,7 @@ static uint8_t xlnx_dp_aux_pop_tx_fifo(XlnxDPState *s)
>      uint8_t ret;
>  
>      if (fifo8_is_empty(&s->tx_fifo)) {
> -        DPRINTF("tx_fifo underflow..\n");
> +        error_report("%s: TX_FIFO underflow", __func__);
>          abort();
>      }
>      ret = fifo8_pop(&s->tx_fifo);
> @@ -525,6 +526,7 @@ static void xlnx_dp_aux_set_command(XlnxDPState *s, uint32_t value)
>          qemu_log_mask(LOG_UNIMP, "xlnx_dp: Write i2c status not implemented\n");
>          break;
>      default:
> +        error_report("%s: invalid command: %u", __func__, cmd);
>          abort();
>      }
>  
> @@ -631,8 +633,8 @@ static void xlnx_dp_change_graphic_fmt(XlnxDPState *s)
>          s->g_plane.format = PIXMAN_b8g8r8;
>          break;
>      default:
> -        DPRINTF("error: unsupported graphic format %u.\n",
> -                s->avbufm_registers[AV_BUF_FORMAT] & DP_GRAPHIC_MASK);
> +        error_report("%s: unsupported graphic format %u", __func__,
> +                     s->avbufm_registers[AV_BUF_FORMAT] & DP_GRAPHIC_MASK);
>          abort();
>      }
>  
> @@ -647,8 +649,8 @@ static void xlnx_dp_change_graphic_fmt(XlnxDPState *s)
>          s->v_plane.format = PIXMAN_x8b8g8r8;
>          break;
>      default:
> -        DPRINTF("error: unsupported video format %u.\n",
> -                s->avbufm_registers[AV_BUF_FORMAT] & DP_NL_VID_FMT_MASK);
> +        error_report("%s: unsupported video format %u", __func__,
> +                     s->avbufm_registers[AV_BUF_FORMAT] & DP_NL_VID_FMT_MASK);
>          abort();
>      }
>  
> -- 
> 2.21.3
> 

Re: [PATCH 09/14] hw/display/xlnx_dp: Replace disabled DPRINTF() by error_report()

[Alistair Francis]

On Mon, May 25, 2020 at 11:29 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> DPRINTF() calls are disabled by default, so when unexpected
> data is used, the whole process abort without information.
>
> Display a bit of information with error_report() before crashing.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/xlnx_dp.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/hw/display/xlnx_dp.c b/hw/display/xlnx_dp.c
> index 3e5fb44e06..8d940cd8d1 100644
> --- a/hw/display/xlnx_dp.c
> +++ b/hw/display/xlnx_dp.c
> @@ -1,5 +1,5 @@
>  /*
> - * xlnx_dp.c
> + * Xilinx Display Port
>   *
>   *  Copyright (C) 2015 : GreenSocs Ltd
>   *      http://www.greensocs.com/ , email: info@greensocs.com
> @@ -24,6 +24,7 @@
>
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qemu/error-report.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "hw/display/xlnx_dp.h"
> @@ -465,7 +466,7 @@ static uint8_t xlnx_dp_aux_pop_tx_fifo(XlnxDPState *s)
>      uint8_t ret;
>
>      if (fifo8_is_empty(&s->tx_fifo)) {
> -        DPRINTF("tx_fifo underflow..\n");
> +        error_report("%s: TX_FIFO underflow", __func__);
>          abort();
>      }
>      ret = fifo8_pop(&s->tx_fifo);
> @@ -525,6 +526,7 @@ static void xlnx_dp_aux_set_command(XlnxDPState *s, uint32_t value)
>          qemu_log_mask(LOG_UNIMP, "xlnx_dp: Write i2c status not implemented\n");
>          break;
>      default:
> +        error_report("%s: invalid command: %u", __func__, cmd);
>          abort();
>      }
>
> @@ -631,8 +633,8 @@ static void xlnx_dp_change_graphic_fmt(XlnxDPState *s)
>          s->g_plane.format = PIXMAN_b8g8r8;
>          break;
>      default:
> -        DPRINTF("error: unsupported graphic format %u.\n",
> -                s->avbufm_registers[AV_BUF_FORMAT] & DP_GRAPHIC_MASK);
> +        error_report("%s: unsupported graphic format %u", __func__,
> +                     s->avbufm_registers[AV_BUF_FORMAT] & DP_GRAPHIC_MASK);
>          abort();
>      }
>
> @@ -647,8 +649,8 @@ static void xlnx_dp_change_graphic_fmt(XlnxDPState *s)
>          s->v_plane.format = PIXMAN_x8b8g8r8;
>          break;
>      default:
> -        DPRINTF("error: unsupported video format %u.\n",
> -                s->avbufm_registers[AV_BUF_FORMAT] & DP_NL_VID_FMT_MASK);
> +        error_report("%s: unsupported video format %u", __func__,
> +                     s->avbufm_registers[AV_BUF_FORMAT] & DP_NL_VID_FMT_MASK);
>          abort();
>      }
>
> --
> 2.21.3
>
>

[PATCH 10/14] hw/display/vmware_vga: Replace printf() calls by qemu_log_mask(ERROR)

[Philippe Mathieu-Daudé]

Avoid flooding stdio by converting printf() calls to
qemu_log_mask(GUEST_ERROR), which are disabled by default.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/vmware_vga.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 58ea82e3e5..5c0fc49d9d 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -26,6 +26,7 @@
 #include "qemu/module.h"
 #include "qemu/units.h"
 #include "qapi/error.h"
+#include "qemu/log.h"
 #include "hw/loader.h"
 #include "trace.h"
 #include "ui/vnc.h"
@@ -953,7 +954,8 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
             ret = s->scratch[s->index - SVGA_SCRATCH_BASE];
             break;
         }
-        printf("%s: Bad register %02x\n", __func__, s->index);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Bad register %02x\n", __func__, s->index);
         ret = 0;
         break;
     }
@@ -1002,7 +1004,8 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
             s->new_width = value;
             s->invalidated = 1;
         } else {
-            printf("%s: Bad width: %i\n", __func__, value);
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Bad width: %i\n", __func__, value);
         }
         break;
 
@@ -1011,13 +1014,15 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
             s->new_height = value;
             s->invalidated = 1;
         } else {
-            printf("%s: Bad height: %i\n", __func__, value);
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Bad height: %i\n", __func__, value);
         }
         break;
 
     case SVGA_REG_BITS_PER_PIXEL:
         if (value != 32) {
-            printf("%s: Bad bits per pixel: %i bits\n", __func__, value);
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "%s: Bad bits per pixel: %i bits\n", __func__, value);
             s->config = 0;
             s->invalidated = 1;
         }
@@ -1082,7 +1087,8 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
             s->scratch[s->index - SVGA_SCRATCH_BASE] = value;
             break;
         }
-        printf("%s: Bad register %02x\n", __func__, s->index);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: Bad register %02x\n", __func__, s->index);
     }
 }
 
-- 
2.21.3

Re: [PATCH 10/14] hw/display/vmware_vga: Replace printf() calls by qemu_log_mask(ERROR)

[Alistair Francis]

On Mon, May 25, 2020 at 11:32 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Avoid flooding stdio by converting printf() calls to
> qemu_log_mask(GUEST_ERROR), which are disabled by default.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/vmware_vga.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index 58ea82e3e5..5c0fc49d9d 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -26,6 +26,7 @@
>  #include "qemu/module.h"
>  #include "qemu/units.h"
>  #include "qapi/error.h"
> +#include "qemu/log.h"
>  #include "hw/loader.h"
>  #include "trace.h"
>  #include "ui/vnc.h"
> @@ -953,7 +954,8 @@ static uint32_t vmsvga_value_read(void *opaque, uint32_t address)
>              ret = s->scratch[s->index - SVGA_SCRATCH_BASE];
>              break;
>          }
> -        printf("%s: Bad register %02x\n", __func__, s->index);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad register %02x\n", __func__, s->index);
>          ret = 0;
>          break;
>      }
> @@ -1002,7 +1004,8 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
>              s->new_width = value;
>              s->invalidated = 1;
>          } else {
> -            printf("%s: Bad width: %i\n", __func__, value);
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: Bad width: %i\n", __func__, value);
>          }
>          break;
>
> @@ -1011,13 +1014,15 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
>              s->new_height = value;
>              s->invalidated = 1;
>          } else {
> -            printf("%s: Bad height: %i\n", __func__, value);
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: Bad height: %i\n", __func__, value);
>          }
>          break;
>
>      case SVGA_REG_BITS_PER_PIXEL:
>          if (value != 32) {
> -            printf("%s: Bad bits per pixel: %i bits\n", __func__, value);
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "%s: Bad bits per pixel: %i bits\n", __func__, value);
>              s->config = 0;
>              s->invalidated = 1;
>          }
> @@ -1082,7 +1087,8 @@ static void vmsvga_value_write(void *opaque, uint32_t address, uint32_t value)
>              s->scratch[s->index - SVGA_SCRATCH_BASE] = value;
>              break;
>          }
> -        printf("%s: Bad register %02x\n", __func__, s->index);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: Bad register %02x\n", __func__, s->index);
>      }
>  }
>
> --
> 2.21.3
>
>

[PATCH 11/14] hw/display/vmware_vga: Let the PCI device own its I/O MemoryRegion

[Philippe Mathieu-Daudé]

To avoid the orphan I/O memory region being added in the /unattached
QOM container, register the PCI device as its owner.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC: This might break migration
---
 hw/display/vmware_vga.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 5c0fc49d9d..2579f6b218 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1306,7 +1306,7 @@ static void pci_vmsvga_realize(PCIDevice *dev, Error **errp)
     dev->config[PCI_LATENCY_TIMER] = 0x40;
     dev->config[PCI_INTERRUPT_LINE] = 0xff;          /* End */
 
-    memory_region_init_io(&s->io_bar, NULL, &vmsvga_io_ops, &s->chip,
+    memory_region_init_io(&s->io_bar, OBJECT(dev), &vmsvga_io_ops, &s->chip,
                           "vmsvga-io", 0x10);
     memory_region_set_flush_coalesced(&s->io_bar);
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &s->io_bar);
-- 
2.21.3

Re: [PATCH 11/14] hw/display/vmware_vga: Let the PCI device own its I/O MemoryRegion

[Gerd Hoffmann]

On Tue, May 26, 2020 at 08:22:49AM +0200, Philippe Mathieu-Daudé wrote:
> To avoid the orphan I/O memory region being added in the /unattached
> QOM container, register the PCI device as its owner.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC: This might break migration

--verbose please.  This doesn't touch the live migration data stream?

take care,
  Gerd

Re: [PATCH 11/14] hw/display/vmware_vga: Let the PCI device own its I/O MemoryRegion

[Philippe Mathieu-Daudé]

On 5/26/20 10:01 AM, Gerd Hoffmann wrote:
> On Tue, May 26, 2020 at 08:22:49AM +0200, Philippe Mathieu-Daudé wrote:
>> To avoid the orphan I/O memory region being added in the /unattached
>> QOM container, register the PCI device as its owner.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> RFC: This might break migration
> 
> --verbose please.  This doesn't touch the live migration data stream?

Oops, this is an I/O region... I was confused by this warning form Peter:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg681970.html

But it is restricted to RAM regions, so this patch is harmless.

Re: [PATCH 11/14] hw/display/vmware_vga: Let the PCI device own its I/O MemoryRegion

[Gerd Hoffmann]

On Tue, May 26, 2020 at 10:16:19AM +0200, Philippe Mathieu-Daudé wrote:
> On 5/26/20 10:01 AM, Gerd Hoffmann wrote:
> > On Tue, May 26, 2020 at 08:22:49AM +0200, Philippe Mathieu-Daudé wrote:
> >> To avoid the orphan I/O memory region being added in the /unattached
> >> QOM container, register the PCI device as its owner.
> >>
> >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> ---
> >> RFC: This might break migration
> > 
> > --verbose please.  This doesn't touch the live migration data stream?
> 
> Oops, this is an I/O region... I was confused by this warning form Peter:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg681970.html
> 
> But it is restricted to RAM regions, so this patch is harmless.

Yes, for ram regions this is a problem, this is why vga has
global_vmstate.  No problem for io.  Series looks fine then.

take care,
  Gerd

[PATCH 12/14] hw/display/exynos4210_fimd: Use qemu_log_mask(GUEST_ERROR)

[Philippe Mathieu-Daudé]

Replace DPRINT_ERROR() by qemu_log_mask(GUEST_ERROR).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/exynos4210_fimd.c | 46 +++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c
index 1c0266ce9f..4b7286b7c9 100644
--- a/hw/display/exynos4210_fimd.c
+++ b/hw/display/exynos4210_fimd.c
@@ -31,6 +31,7 @@
 #include "ui/pixel_ops.h"
 #include "qemu/bswap.h"
 #include "qemu/module.h"
+#include "qemu/log.h"
 
 /* Debug messages configuration */
 #define EXYNOS4210_FIMD_DEBUG              0
@@ -39,20 +40,15 @@
 #if EXYNOS4210_FIMD_DEBUG == 0
     #define DPRINT_L1(fmt, args...)       do { } while (0)
     #define DPRINT_L2(fmt, args...)       do { } while (0)
-    #define DPRINT_ERROR(fmt, args...)    do { } while (0)
 #elif EXYNOS4210_FIMD_DEBUG == 1
     #define DPRINT_L1(fmt, args...) \
         do {fprintf(stderr, "QEMU FIMD: "fmt, ## args); } while (0)
     #define DPRINT_L2(fmt, args...)       do { } while (0)
-    #define DPRINT_ERROR(fmt, args...)  \
-        do {fprintf(stderr, "QEMU FIMD ERROR: "fmt, ## args); } while (0)
 #else
     #define DPRINT_L1(fmt, args...) \
         do {fprintf(stderr, "QEMU FIMD: "fmt, ## args); } while (0)
     #define DPRINT_L2(fmt, args...) \
         do {fprintf(stderr, "QEMU FIMD: "fmt, ## args); } while (0)
-    #define DPRINT_ERROR(fmt, args...)  \
-        do {fprintf(stderr, "QEMU FIMD ERROR: "fmt, ## args); } while (0)
 #endif
 
 #if EXYNOS4210_FIMD_MODE_TRACE == 0
@@ -1108,7 +1104,7 @@ static inline int fimd_get_buffer_id(Exynos4210fimdWindow *w)
     case FIMD_WINCON_BUF2_STAT:
         return 2;
     default:
-        DPRINT_ERROR("Non-existent buffer index\n");
+        qemu_log_mask(LOG_GUEST_ERROR, "FIMD: Non-existent buffer index\n");
         return 0;
     }
 }
@@ -1160,20 +1156,24 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
 
     if (int128_get64(w->mem_section.size) != w->fb_len ||
             !memory_region_is_ram(w->mem_section.mr)) {
-        DPRINT_ERROR("Failed to find window %u framebuffer region\n", win);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "FIMD: Failed to find window %u framebuffer region\n",
+                      win);
         goto error_return;
     }
 
     w->host_fb_addr = cpu_physical_memory_map(fb_start_addr, &fb_mapped_len,
                                               false);
     if (!w->host_fb_addr) {
-        DPRINT_ERROR("Failed to map window %u framebuffer\n", win);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "FIMD: Failed to map window %u framebuffer\n", win);
         goto error_return;
     }
 
     if (fb_mapped_len != w->fb_len) {
-        DPRINT_ERROR("Window %u mapped framebuffer length is less then "
-                "expected\n", win);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "FIMD: Window %u mapped framebuffer length is less than "
+                      "expected\n", win);
         cpu_physical_memory_unmap(w->host_fb_addr, fb_mapped_len, 0, 0);
         goto error_return;
     }
@@ -1490,7 +1490,9 @@ static void exynos4210_fimd_write(void *opaque, hwaddr offset,
             break;
         case 3:
             if (w != 1 && w != 2) {
-                DPRINT_ERROR("Bad write offset 0x%08x\n", offset);
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "FIMD: Bad write offset 0x%08"HWADDR_PRIx"\n",
+                              offset);
                 return;
             }
             s->window[w].osdsize = val;
@@ -1624,7 +1626,9 @@ static void exynos4210_fimd_write(void *opaque, hwaddr offset,
         break;
     case FIMD_VIDW0ADD0_B2 ... FIMD_VIDW4ADD0_B2:
         if (offset & 0x0004) {
-            DPRINT_ERROR("bad write offset 0x%08x\n", offset);
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "FIMD: bad write offset 0x%08"HWADDR_PRIx"\n",
+                          offset);
             break;
         }
         w = (offset - FIMD_VIDW0ADD0_B2) >> 3;
@@ -1638,14 +1642,18 @@ static void exynos4210_fimd_write(void *opaque, hwaddr offset,
         break;
     case FIMD_SHD_ADD0_START ... FIMD_SHD_ADD0_END:
         if (offset & 0x0004) {
-            DPRINT_ERROR("bad write offset 0x%08x\n", offset);
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "FIMD: bad write offset 0x%08"HWADDR_PRIx"\n",
+                          offset);
             break;
         }
         s->window[(offset - FIMD_SHD_ADD0_START) >> 3].shadow_buf_start = val;
         break;
     case FIMD_SHD_ADD1_START ... FIMD_SHD_ADD1_END:
         if (offset & 0x0004) {
-            DPRINT_ERROR("bad write offset 0x%08x\n", offset);
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "FIMD: bad write offset 0x%08"HWADDR_PRIx"\n",
+                          offset);
             break;
         }
         s->window[(offset - FIMD_SHD_ADD1_START) >> 3].shadow_buf_end = val;
@@ -1665,7 +1673,8 @@ static void exynos4210_fimd_write(void *opaque, hwaddr offset,
         s->window[w].palette[i] = val;
         break;
     default:
-        DPRINT_ERROR("bad write offset 0x%08x\n", offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "FIMD: bad write offset 0x%08"HWADDR_PRIx"\n", offset);
         break;
     }
 }
@@ -1715,7 +1724,9 @@ static uint64_t exynos4210_fimd_read(void *opaque, hwaddr offset,
             break;
         case 3:
             if (w != 1 && w != 2) {
-                DPRINT_ERROR("bad read offset 0x%08x\n", offset);
+                qemu_log_mask(LOG_GUEST_ERROR,
+                              "FIMD: bad read offset 0x%08"HWADDR_PRIx"\n",
+                              offset);
                 return 0xBAADBAAD;
             }
             ret = s->window[w].osdsize;
@@ -1809,7 +1820,8 @@ static uint64_t exynos4210_fimd_read(void *opaque, hwaddr offset,
         return s->window[w].palette[i];
     }
 
-    DPRINT_ERROR("bad read offset 0x%08x\n", offset);
+    qemu_log_mask(LOG_GUEST_ERROR,
+                  "FIMD: bad read offset 0x%08"HWADDR_PRIx"\n", offset);
     return 0xBAADBAAD;
 }
 
-- 
2.21.3

Re: [PATCH 12/14] hw/display/exynos4210_fimd: Use qemu_log_mask(GUEST_ERROR)

[Alistair Francis]

On Mon, May 25, 2020 at 11:34 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Replace DPRINT_ERROR() by qemu_log_mask(GUEST_ERROR).
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/exynos4210_fimd.c | 46 +++++++++++++++++++++++-------------
>  1 file changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/hw/display/exynos4210_fimd.c b/hw/display/exynos4210_fimd.c
> index 1c0266ce9f..4b7286b7c9 100644
> --- a/hw/display/exynos4210_fimd.c
> +++ b/hw/display/exynos4210_fimd.c
> @@ -31,6 +31,7 @@
>  #include "ui/pixel_ops.h"
>  #include "qemu/bswap.h"
>  #include "qemu/module.h"
> +#include "qemu/log.h"
>
>  /* Debug messages configuration */
>  #define EXYNOS4210_FIMD_DEBUG              0
> @@ -39,20 +40,15 @@
>  #if EXYNOS4210_FIMD_DEBUG == 0
>      #define DPRINT_L1(fmt, args...)       do { } while (0)
>      #define DPRINT_L2(fmt, args...)       do { } while (0)
> -    #define DPRINT_ERROR(fmt, args...)    do { } while (0)
>  #elif EXYNOS4210_FIMD_DEBUG == 1
>      #define DPRINT_L1(fmt, args...) \
>          do {fprintf(stderr, "QEMU FIMD: "fmt, ## args); } while (0)
>      #define DPRINT_L2(fmt, args...)       do { } while (0)
> -    #define DPRINT_ERROR(fmt, args...)  \
> -        do {fprintf(stderr, "QEMU FIMD ERROR: "fmt, ## args); } while (0)
>  #else
>      #define DPRINT_L1(fmt, args...) \
>          do {fprintf(stderr, "QEMU FIMD: "fmt, ## args); } while (0)
>      #define DPRINT_L2(fmt, args...) \
>          do {fprintf(stderr, "QEMU FIMD: "fmt, ## args); } while (0)
> -    #define DPRINT_ERROR(fmt, args...)  \
> -        do {fprintf(stderr, "QEMU FIMD ERROR: "fmt, ## args); } while (0)
>  #endif
>
>  #if EXYNOS4210_FIMD_MODE_TRACE == 0
> @@ -1108,7 +1104,7 @@ static inline int fimd_get_buffer_id(Exynos4210fimdWindow *w)
>      case FIMD_WINCON_BUF2_STAT:
>          return 2;
>      default:
> -        DPRINT_ERROR("Non-existent buffer index\n");
> +        qemu_log_mask(LOG_GUEST_ERROR, "FIMD: Non-existent buffer index\n");
>          return 0;
>      }
>  }
> @@ -1160,20 +1156,24 @@ static void fimd_update_memory_section(Exynos4210fimdState *s, unsigned win)
>
>      if (int128_get64(w->mem_section.size) != w->fb_len ||
>              !memory_region_is_ram(w->mem_section.mr)) {
> -        DPRINT_ERROR("Failed to find window %u framebuffer region\n", win);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "FIMD: Failed to find window %u framebuffer region\n",
> +                      win);
>          goto error_return;
>      }
>
>      w->host_fb_addr = cpu_physical_memory_map(fb_start_addr, &fb_mapped_len,
>                                                false);
>      if (!w->host_fb_addr) {
> -        DPRINT_ERROR("Failed to map window %u framebuffer\n", win);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "FIMD: Failed to map window %u framebuffer\n", win);
>          goto error_return;
>      }
>
>      if (fb_mapped_len != w->fb_len) {
> -        DPRINT_ERROR("Window %u mapped framebuffer length is less then "
> -                "expected\n", win);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "FIMD: Window %u mapped framebuffer length is less than "
> +                      "expected\n", win);
>          cpu_physical_memory_unmap(w->host_fb_addr, fb_mapped_len, 0, 0);
>          goto error_return;
>      }
> @@ -1490,7 +1490,9 @@ static void exynos4210_fimd_write(void *opaque, hwaddr offset,
>              break;
>          case 3:
>              if (w != 1 && w != 2) {
> -                DPRINT_ERROR("Bad write offset 0x%08x\n", offset);
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "FIMD: Bad write offset 0x%08"HWADDR_PRIx"\n",
> +                              offset);
>                  return;
>              }
>              s->window[w].osdsize = val;
> @@ -1624,7 +1626,9 @@ static void exynos4210_fimd_write(void *opaque, hwaddr offset,
>          break;
>      case FIMD_VIDW0ADD0_B2 ... FIMD_VIDW4ADD0_B2:
>          if (offset & 0x0004) {
> -            DPRINT_ERROR("bad write offset 0x%08x\n", offset);
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "FIMD: bad write offset 0x%08"HWADDR_PRIx"\n",
> +                          offset);
>              break;
>          }
>          w = (offset - FIMD_VIDW0ADD0_B2) >> 3;
> @@ -1638,14 +1642,18 @@ static void exynos4210_fimd_write(void *opaque, hwaddr offset,
>          break;
>      case FIMD_SHD_ADD0_START ... FIMD_SHD_ADD0_END:
>          if (offset & 0x0004) {
> -            DPRINT_ERROR("bad write offset 0x%08x\n", offset);
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "FIMD: bad write offset 0x%08"HWADDR_PRIx"\n",
> +                          offset);
>              break;
>          }
>          s->window[(offset - FIMD_SHD_ADD0_START) >> 3].shadow_buf_start = val;
>          break;
>      case FIMD_SHD_ADD1_START ... FIMD_SHD_ADD1_END:
>          if (offset & 0x0004) {
> -            DPRINT_ERROR("bad write offset 0x%08x\n", offset);
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "FIMD: bad write offset 0x%08"HWADDR_PRIx"\n",
> +                          offset);
>              break;
>          }
>          s->window[(offset - FIMD_SHD_ADD1_START) >> 3].shadow_buf_end = val;
> @@ -1665,7 +1673,8 @@ static void exynos4210_fimd_write(void *opaque, hwaddr offset,
>          s->window[w].palette[i] = val;
>          break;
>      default:
> -        DPRINT_ERROR("bad write offset 0x%08x\n", offset);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "FIMD: bad write offset 0x%08"HWADDR_PRIx"\n", offset);
>          break;
>      }
>  }
> @@ -1715,7 +1724,9 @@ static uint64_t exynos4210_fimd_read(void *opaque, hwaddr offset,
>              break;
>          case 3:
>              if (w != 1 && w != 2) {
> -                DPRINT_ERROR("bad read offset 0x%08x\n", offset);
> +                qemu_log_mask(LOG_GUEST_ERROR,
> +                              "FIMD: bad read offset 0x%08"HWADDR_PRIx"\n",
> +                              offset);
>                  return 0xBAADBAAD;
>              }
>              ret = s->window[w].osdsize;
> @@ -1809,7 +1820,8 @@ static uint64_t exynos4210_fimd_read(void *opaque, hwaddr offset,
>          return s->window[w].palette[i];
>      }
>
> -    DPRINT_ERROR("bad read offset 0x%08x\n", offset);
> +    qemu_log_mask(LOG_GUEST_ERROR,
> +                  "FIMD: bad read offset 0x%08"HWADDR_PRIx"\n", offset);
>      return 0xBAADBAAD;
>  }
>
> --
> 2.21.3
>
>

[PATCH 13/14] hw/display/omap_dss: Replace fprintf() call by qemu_log_mask(LOG_UNIMP)

[Philippe Mathieu-Daudé]

Replace fprintf() call by qemu_log_mask(LOG_UNIMP), which is
disabled by default. This avoid flooding the terminal when
fuzzing the device.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/omap_dss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/display/omap_dss.c b/hw/display/omap_dss.c
index 32dc0d6aa7..21fde58a26 100644
--- a/hw/display/omap_dss.c
+++ b/hw/display/omap_dss.c
@@ -619,7 +619,7 @@ static void omap_rfbi_transfer_start(struct omap_dss_s *s)
     if (s->rfbi.control & (1 << 1)) {				/* BYPASS */
         /* TODO: in non-Bypass mode we probably need to just assert the
          * DRQ and wait for DMA to write the pixels.  */
-        fprintf(stderr, "%s: Bypass mode unimplemented\n", __func__);
+        qemu_log_mask(LOG_UNIMP, "%s: Bypass mode unimplemented\n", __func__);
         return;
     }
 
-- 
2.21.3

Re: [PATCH 13/14] hw/display/omap_dss: Replace fprintf() call by qemu_log_mask(LOG_UNIMP)

[Alistair Francis]

On Mon, May 25, 2020 at 11:35 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Replace fprintf() call by qemu_log_mask(LOG_UNIMP), which is
> disabled by default. This avoid flooding the terminal when
> fuzzing the device.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/omap_dss.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/display/omap_dss.c b/hw/display/omap_dss.c
> index 32dc0d6aa7..21fde58a26 100644
> --- a/hw/display/omap_dss.c
> +++ b/hw/display/omap_dss.c
> @@ -619,7 +619,7 @@ static void omap_rfbi_transfer_start(struct omap_dss_s *s)
>      if (s->rfbi.control & (1 << 1)) {                          /* BYPASS */
>          /* TODO: in non-Bypass mode we probably need to just assert the
>           * DRQ and wait for DMA to write the pixels.  */
> -        fprintf(stderr, "%s: Bypass mode unimplemented\n", __func__);
> +        qemu_log_mask(LOG_UNIMP, "%s: Bypass mode unimplemented\n", __func__);
>          return;
>      }
>
> --
> 2.21.3
>
>

[PATCH 14/14] hw/display/pxa2xx_lcd: Replace printf() call by qemu_log_mask()

[Philippe Mathieu-Daudé]

Replace printf() calls by qemu_log_mask(UNIMP), which is
disabled by default. This avoid flooding the terminal when
fuzzing the device.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/display/pxa2xx_lcd.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
index d5f2e82a4e..ff90104b80 100644
--- a/hw/display/pxa2xx_lcd.c
+++ b/hw/display/pxa2xx_lcd.c
@@ -426,9 +426,10 @@ static void pxa2xx_lcdc_write(void *opaque, hwaddr offset,
         if ((s->control[0] & LCCR0_ENB) && !(value & LCCR0_ENB))
             s->status[0] |= LCSR0_QD;
 
-        if (!(s->control[0] & LCCR0_LCDT) && (value & LCCR0_LCDT))
-            printf("%s: internal frame buffer unsupported\n", __func__);
-
+        if (!(s->control[0] & LCCR0_LCDT) && (value & LCCR0_LCDT)) {
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: internal frame buffer unsupported\n", __func__);
+        }
         if ((s->control[3] & LCCR3_API) &&
                 (value & LCCR0_ENB) && !(value & LCCR0_LCDT))
             s->status[0] |= LCSR0_ABC;
@@ -462,9 +463,9 @@ static void pxa2xx_lcdc_write(void *opaque, hwaddr offset,
         break;
 
     case OVL1C1:
-        if (!(s->ovl1c[0] & OVLC1_EN) && (value & OVLC1_EN))
-            printf("%s: Overlay 1 not supported\n", __func__);
-
+        if (!(s->ovl1c[0] & OVLC1_EN) && (value & OVLC1_EN)) {
+            qemu_log_mask(LOG_UNIMP, "%s: Overlay 1 not supported\n", __func__);
+        }
         s->ovl1c[0] = value & 0x80ffffff;
         s->dma_ch[1].up = (value & OVLC1_EN) || (s->control[0] & LCCR0_SDS);
         break;
@@ -474,9 +475,9 @@ static void pxa2xx_lcdc_write(void *opaque, hwaddr offset,
         break;
 
     case OVL2C1:
-        if (!(s->ovl2c[0] & OVLC1_EN) && (value & OVLC1_EN))
-            printf("%s: Overlay 2 not supported\n", __func__);
-
+        if (!(s->ovl2c[0] & OVLC1_EN) && (value & OVLC1_EN)) {
+            qemu_log_mask(LOG_UNIMP, "%s: Overlay 2 not supported\n", __func__);
+        }
         s->ovl2c[0] = value & 0x80ffffff;
         s->dma_ch[2].up = !!(value & OVLC1_EN);
         s->dma_ch[3].up = !!(value & OVLC1_EN);
@@ -488,9 +489,10 @@ static void pxa2xx_lcdc_write(void *opaque, hwaddr offset,
         break;
 
     case CCR:
-        if (!(s->ccr & CCR_CEN) && (value & CCR_CEN))
-            printf("%s: Hardware cursor unimplemented\n", __func__);
-
+        if (!(s->ccr & CCR_CEN) && (value & CCR_CEN)) {
+            qemu_log_mask(LOG_UNIMP,
+                          "%s: Hardware cursor unimplemented\n", __func__);
+        }
         s->ccr = value & 0x81ffffe7;
         s->dma_ch[5].up = !!(value & CCR_CEN);
         break;
-- 
2.21.3

Re: [PATCH 14/14] hw/display/pxa2xx_lcd: Replace printf() call by qemu_log_mask()

[Alistair Francis]

On Mon, May 25, 2020 at 11:36 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Replace printf() calls by qemu_log_mask(UNIMP), which is
> disabled by default. This avoid flooding the terminal when
> fuzzing the device.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/display/pxa2xx_lcd.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/hw/display/pxa2xx_lcd.c b/hw/display/pxa2xx_lcd.c
> index d5f2e82a4e..ff90104b80 100644
> --- a/hw/display/pxa2xx_lcd.c
> +++ b/hw/display/pxa2xx_lcd.c
> @@ -426,9 +426,10 @@ static void pxa2xx_lcdc_write(void *opaque, hwaddr offset,
>          if ((s->control[0] & LCCR0_ENB) && !(value & LCCR0_ENB))
>              s->status[0] |= LCSR0_QD;
>
> -        if (!(s->control[0] & LCCR0_LCDT) && (value & LCCR0_LCDT))
> -            printf("%s: internal frame buffer unsupported\n", __func__);
> -
> +        if (!(s->control[0] & LCCR0_LCDT) && (value & LCCR0_LCDT)) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: internal frame buffer unsupported\n", __func__);
> +        }
>          if ((s->control[3] & LCCR3_API) &&
>                  (value & LCCR0_ENB) && !(value & LCCR0_LCDT))
>              s->status[0] |= LCSR0_ABC;
> @@ -462,9 +463,9 @@ static void pxa2xx_lcdc_write(void *opaque, hwaddr offset,
>          break;
>
>      case OVL1C1:
> -        if (!(s->ovl1c[0] & OVLC1_EN) && (value & OVLC1_EN))
> -            printf("%s: Overlay 1 not supported\n", __func__);
> -
> +        if (!(s->ovl1c[0] & OVLC1_EN) && (value & OVLC1_EN)) {
> +            qemu_log_mask(LOG_UNIMP, "%s: Overlay 1 not supported\n", __func__);
> +        }
>          s->ovl1c[0] = value & 0x80ffffff;
>          s->dma_ch[1].up = (value & OVLC1_EN) || (s->control[0] & LCCR0_SDS);
>          break;
> @@ -474,9 +475,9 @@ static void pxa2xx_lcdc_write(void *opaque, hwaddr offset,
>          break;
>
>      case OVL2C1:
> -        if (!(s->ovl2c[0] & OVLC1_EN) && (value & OVLC1_EN))
> -            printf("%s: Overlay 2 not supported\n", __func__);
> -
> +        if (!(s->ovl2c[0] & OVLC1_EN) && (value & OVLC1_EN)) {
> +            qemu_log_mask(LOG_UNIMP, "%s: Overlay 2 not supported\n", __func__);
> +        }
>          s->ovl2c[0] = value & 0x80ffffff;
>          s->dma_ch[2].up = !!(value & OVLC1_EN);
>          s->dma_ch[3].up = !!(value & OVLC1_EN);
> @@ -488,9 +489,10 @@ static void pxa2xx_lcdc_write(void *opaque, hwaddr offset,
>          break;
>
>      case CCR:
> -        if (!(s->ccr & CCR_CEN) && (value & CCR_CEN))
> -            printf("%s: Hardware cursor unimplemented\n", __func__);
> -
> +        if (!(s->ccr & CCR_CEN) && (value & CCR_CEN)) {
> +            qemu_log_mask(LOG_UNIMP,
> +                          "%s: Hardware cursor unimplemented\n", __func__);
> +        }
>          s->ccr = value & 0x81ffffe7;
>          s->dma_ch[5].up = !!(value & CCR_CEN);
>          break;
> --
> 2.21.3
>
>

Re: [PATCH 00/14] hw/display: Omnibus cleanups

[Gerd Hoffmann]

On Tue, May 26, 2020 at 08:22:38AM +0200, Philippe Mathieu-Daudé wrote:
> Hi Gerd, for your convenience I joined all the hw/display/
> patches I sent recently altogether in a series.

Nice service.  I was about to collect them from my inbox after sending
out the audio pull req ...

thanks,
  Gerd

Re: [PATCH 00/14] hw/display: Omnibus cleanups

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200526062252.19852-1-f4bug@amsat.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200526062252.19852-1-f4bug@amsat.org
Subject: [PATCH 00/14] hw/display: Omnibus cleanups
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Switched to a new branch 'test'
8960790 hw/display/pxa2xx_lcd: Replace printf() call by qemu_log_mask()
d0b4cb9 hw/display/omap_dss: Replace fprintf() call by qemu_log_mask(LOG_UNIMP)
d21b1b3 hw/display/exynos4210_fimd: Use qemu_log_mask(GUEST_ERROR)
e16fcb5 hw/display/vmware_vga: Let the PCI device own its I/O MemoryRegion
6aeaa83 hw/display/vmware_vga: Replace printf() calls by qemu_log_mask(ERROR)
67b03be hw/display/xlnx_dp: Replace disabled DPRINTF() by error_report()
0ea30c7 hw/display/dpcd: Convert debug printf()s to trace events
870c71a hw/display/dpcd: Fix memory region size
8386530 hw/display/cirrus_vga: Convert debug printf() to trace event
6a156a8 hw/display/cirrus_vga: Use qemu_log_mask(ERROR) instead of debug printf
7bddcc1 hw/display/cirrus_vga: Use qemu_log_mask(UNIMP) instead of debug printf
3655560 hw/display/cirrus_vga: Convert debug printf() to trace event
5da1d7d hw/display/cg3: Convert debug printf()s to trace events
5e15cf3 hw/display/edid: Add missing 'qdev-properties.h' header

=== OUTPUT BEGIN ===
1/14 Checking commit 5e15cf344f0d (hw/display/edid: Add missing 'qdev-properties.h' header)
2/14 Checking commit 5da1d7d709c4 (hw/display/cg3: Convert debug printf()s to trace events)
3/14 Checking commit 3655560a1af9 (hw/display/cirrus_vga: Convert debug printf() to trace event)
4/14 Checking commit 7bddcc1ffc45 (hw/display/cirrus_vga: Use qemu_log_mask(UNIMP) instead of debug printf)
5/14 Checking commit 6a156a8c7bfd (hw/display/cirrus_vga: Use qemu_log_mask(ERROR) instead of debug printf)
ERROR: suspect code indent for conditional statements (16, 12)
#31: FILE: hw/display/cirrus_vga.c:1038:
                if (s->cirrus_blt_pixelwidth > 2) {
+            qemu_log_mask(LOG_GUEST_ERROR,

total: 1 errors, 0 warnings, 156 lines checked

Patch 5/14 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

6/14 Checking commit 8386530a9c63 (hw/display/cirrus_vga: Convert debug printf() to trace event)
7/14 Checking commit 870c71a9a32f (hw/display/dpcd: Fix memory region size)
8/14 Checking commit 0ea30c7609df (hw/display/dpcd: Convert debug printf()s to trace events)
9/14 Checking commit 67b03be888c2 (hw/display/xlnx_dp: Replace disabled DPRINTF() by error_report())
10/14 Checking commit 6aeaa836d719 (hw/display/vmware_vga: Replace printf() calls by qemu_log_mask(ERROR))
11/14 Checking commit e16fcb57bdee (hw/display/vmware_vga: Let the PCI device own its I/O MemoryRegion)
12/14 Checking commit d21b1b38fe80 (hw/display/exynos4210_fimd: Use qemu_log_mask(GUEST_ERROR))
13/14 Checking commit d0b4cb9ec764 (hw/display/omap_dss: Replace fprintf() call by qemu_log_mask(LOG_UNIMP))
14/14 Checking commit 896079058bf6 (hw/display/pxa2xx_lcd: Replace printf() call by qemu_log_mask())
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200526062252.19852-1-f4bug@amsat.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com