[Qemu-devel] [PATCH 0/5] Misc sm501 improvements

[Qemu-devel] [PATCH 0/5] Misc sm501 improvements

[BALATON Zoltan]

These are fixes to sm501 emulation that were found while testing with
AmigaOS 4.1FE (AmigaOS 4 is known to work on real hardware but we have
no way to verify it against hardware). While this is mainly to improve
sam460ex emulation this is now a separate series with the sm501
specific patches because that's also used on SH emulation. I hope
someone can review and merge this.

BALATON Zoltan (3):
  sm501: Implement i2c part for reading monitor EDID
  sm501: Set updated region dirty after 2D operation
  sm501: Fix support for non-zero frame buffer start address

Sebastian Bauer (2):
  sm501: Perform a full update after palette change
  sm501: Use values from the pitch register for 2d operations.

 default-configs/ppc-softmmu.mak    |   1 +
 default-configs/ppcemb-softmmu.mak |   1 +
 default-configs/sh4-softmmu.mak    |   2 +
 default-configs/sh4eb-softmmu.mak  |   2 +
 hw/display/sm501.c                 | 166 +++++++++++++++++++++++++++++++++++--
 5 files changed, 163 insertions(+), 9 deletions(-)

-- 
2.7.6

[Qemu-devel] [PATCH 5/5] sm501: Fix support for non-zero frame buffer start address

[BALATON Zoltan]

Display updates and drawing hardware cursor did not work when frame
buffer address was non-zero. Fix this by taking the frame buffer
address into account in these cases. This fixes screen dragging on
AmigaOS. Based on patch by Sebastian Bauer.

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index e426d2f..acc26f6 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -578,6 +578,11 @@ static uint32_t get_local_mem_size_index(uint32_t size)
     return index;
 }
 
+static ram_addr_t get_fb_addr(SM501State *s, int crt)
+{
+    return crt ? s->dc_crt_fb_addr : s->dc_panel_fb_addr;
+}
+
 static inline int get_width(SM501State *s, int crt)
 {
     int width = crt ? s->dc_crt_h_total : s->dc_panel_h_total;
@@ -680,7 +685,8 @@ static inline void hwc_invalidate(SM501State *s, int crt)
     start *= w * bpp;
     end *= w * bpp;
 
-    memory_region_set_dirty(&s->local_mem_region, start, end - start);
+    memory_region_set_dirty(&s->local_mem_region,
+                            get_fb_addr(s, crt) + start, end - start);
 }
 
 static void sm501_2d_operation(SM501State *s)
@@ -1577,7 +1583,7 @@ static void sm501_update_display(void *opaque)
     draw_hwc_line_func *draw_hwc_line = NULL;
     int full_update = 0;
     int y_start = -1;
-    ram_addr_t offset = 0;
+    ram_addr_t offset;
     uint32_t *palette;
     uint8_t hwc_palette[3 * 3];
     uint8_t *hwc_src = NULL;
@@ -1634,9 +1640,10 @@ static void sm501_update_display(void *opaque)
     }
 
     /* draw each line according to conditions */
+    offset = get_fb_addr(s, crt);
     snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
               offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
-    for (y = 0, offset = 0; y < height; y++, offset += width * src_bpp) {
+    for (y = 0; y < height; y++, offset += width * src_bpp) {
         int update, update_hwc;
 
         /* check if hardware cursor is enabled and we're within its range */
-- 
2.7.6

Re: [Qemu-devel] [PATCH 5/5] sm501: Fix support for non-zero frame buffer start address

[BALATON Zoltan]

On Thu, 21 Jun 2018, BALATON Zoltan wrote:
> Display updates and drawing hardware cursor did not work when frame
> buffer address was non-zero. Fix this by taking the frame buffer
> address into account in these cases. This fixes screen dragging on
> AmigaOS. Based on patch by Sebastian Bauer.
>
> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> hw/display/sm501.c | 13 ++++++++++---
> 1 file changed, 10 insertions(+), 3 deletions(-)

I've just noticed this seems to break Linux frambuffer for some reason. I 
got this backtrace:

#0  raise () from /lib64/libc.so.6
#1  abort () from /lib64/libc.so.6
#2  qemu_get_ram_block (addr=2685665280) at exec.c:1296
#3  tlb_reset_dirty_range_all (start=2685665280, length=256000) at exec.c:1330
#4  cpu_physical_memory_snapshot_and_clear_dirty (start=2685665280, length=256000, client=0) at exec.c:1420
#5  memory_region_snapshot_and_clear_dirty (mr=0x555556b42f20, addr=2147483648, size=256000, client=0)
     at memory.c:2017
#6  sm501_update_display (opaque=0x555556b42f00) at hw/display/sm501.c:1644
#7  graphic_hw_update (con=0x555556c3e660) at ui/console.c:267
#8  sdl2_2d_refresh (dcl=0x555557076120) at ui/sdl2-2d.c:128
#9  dpy_refresh (s=0x555556c3e5f0) at ui/console.c:1658
#10 gui_update (opaque=0x555556c3e5f0) at ui/console.c:205
#11 timerlist_run_timers (timer_list=0x55555691a7e0) at util/qemu-timer.c:536
#12 qemu_clock_run_timers (type=QEMU_CLOCK_REALTIME) at util/qemu-timer.c:547
#13 qemu_clock_run_all_timers () at util/qemu-timer.c:674
#14 main_loop_wait (nonblocking=0) at util/main-loop.c:503
#15 main_loop () at vl.c:1848
#16 main (argc=17, argv=0x7fffffffdf68, envp=0x7fffffffdff8) at vl.c:4600

with this error:

Bad ram offset a0140000

after sm501 driver in Linux is set up. Any ideas? Maybe something is not 
initialised correctly?

Regards,
BALATON Zoltan

> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index e426d2f..acc26f6 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -578,6 +578,11 @@ static uint32_t get_local_mem_size_index(uint32_t size)
>     return index;
> }
>
> +static ram_addr_t get_fb_addr(SM501State *s, int crt)
> +{
> +    return crt ? s->dc_crt_fb_addr : s->dc_panel_fb_addr;
> +}
> +
> static inline int get_width(SM501State *s, int crt)
> {
>     int width = crt ? s->dc_crt_h_total : s->dc_panel_h_total;
> @@ -680,7 +685,8 @@ static inline void hwc_invalidate(SM501State *s, int crt)
>     start *= w * bpp;
>     end *= w * bpp;
>
> -    memory_region_set_dirty(&s->local_mem_region, start, end - start);
> +    memory_region_set_dirty(&s->local_mem_region,
> +                            get_fb_addr(s, crt) + start, end - start);
> }
>
> static void sm501_2d_operation(SM501State *s)
> @@ -1577,7 +1583,7 @@ static void sm501_update_display(void *opaque)
>     draw_hwc_line_func *draw_hwc_line = NULL;
>     int full_update = 0;
>     int y_start = -1;
> -    ram_addr_t offset = 0;
> +    ram_addr_t offset;
>     uint32_t *palette;
>     uint8_t hwc_palette[3 * 3];
>     uint8_t *hwc_src = NULL;
> @@ -1634,9 +1640,10 @@ static void sm501_update_display(void *opaque)
>     }
>
>     /* draw each line according to conditions */
> +    offset = get_fb_addr(s, crt);
>     snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
>               offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
> -    for (y = 0, offset = 0; y < height; y++, offset += width * src_bpp) {
> +    for (y = 0; y < height; y++, offset += width * src_bpp) {
>         int update, update_hwc;
>
>         /* check if hardware cursor is enabled and we're within its range */
>

[Qemu-devel] [PATCH 1/5] sm501: Implement i2c part for reading monitor EDID

[BALATON Zoltan]

Emulate the i2c part of SM501 which is used to access the EDID info
from a monitor.

The vmstate structure is changed and its version is increased but
SM501 is only used on SH and PPC sam460ex machines that don't support
cross-version migration.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 default-configs/ppc-softmmu.mak    |   1 +
 default-configs/ppcemb-softmmu.mak |   1 +
 default-configs/sh4-softmmu.mak    |   2 +
 default-configs/sh4eb-softmmu.mak  |   2 +
 hw/display/sm501.c                 | 136 +++++++++++++++++++++++++++++++++++--
 5 files changed, 138 insertions(+), 4 deletions(-)

diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
index b8b0526..e131e24 100644
--- a/default-configs/ppc-softmmu.mak
+++ b/default-configs/ppc-softmmu.mak
@@ -24,6 +24,7 @@ CONFIG_ETSEC=y
 # For Sam460ex
 CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
+CONFIG_DDC=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
 CONFIG_BITBANG_I2C=y
diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
index 37af193..ac44f15 100644
--- a/default-configs/ppcemb-softmmu.mak
+++ b/default-configs/ppcemb-softmmu.mak
@@ -17,6 +17,7 @@ CONFIG_XILINX=y
 CONFIG_XILINX_ETHLITE=y
 CONFIG_USB_EHCI_SYSBUS=y
 CONFIG_SM501=y
+CONFIG_DDC=y
 CONFIG_IDE_SII3112=y
 CONFIG_I2C=y
 CONFIG_BITBANG_I2C=y
diff --git a/default-configs/sh4-softmmu.mak b/default-configs/sh4-softmmu.mak
index 546d855..caeccd5 100644
--- a/default-configs/sh4-softmmu.mak
+++ b/default-configs/sh4-softmmu.mak
@@ -9,6 +9,8 @@ CONFIG_PFLASH_CFI02=y
 CONFIG_SH4=y
 CONFIG_IDE_MMIO=y
 CONFIG_SM501=y
+CONFIG_I2C=y
+CONFIG_DDC=y
 CONFIG_ISA_TESTDEV=y
 CONFIG_I82378=y
 CONFIG_I8259=y
diff --git a/default-configs/sh4eb-softmmu.mak b/default-configs/sh4eb-softmmu.mak
index 2d3fd49..53b9cd7 100644
--- a/default-configs/sh4eb-softmmu.mak
+++ b/default-configs/sh4eb-softmmu.mak
@@ -9,6 +9,8 @@ CONFIG_PFLASH_CFI02=y
 CONFIG_SH4=y
 CONFIG_IDE_MMIO=y
 CONFIG_SM501=y
+CONFIG_I2C=y
+CONFIG_DDC=y
 CONFIG_ISA_TESTDEV=y
 CONFIG_I82378=y
 CONFIG_I8259=y
diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index ca0840f..0625cf5 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "qemu/cutils.h"
 #include "qapi/error.h"
+#include "qemu/log.h"
 #include "qemu-common.h"
 #include "cpu.h"
 #include "hw/hw.h"
@@ -34,6 +35,8 @@
 #include "hw/devices.h"
 #include "hw/sysbus.h"
 #include "hw/pci/pci.h"
+#include "hw/i2c/i2c.h"
+#include "hw/i2c/i2c-ddc.h"
 #include "qemu/range.h"
 #include "ui/pixel_ops.h"
 
@@ -471,10 +474,12 @@ typedef struct SM501State {
     MemoryRegion local_mem_region;
     MemoryRegion mmio_region;
     MemoryRegion system_config_region;
+    MemoryRegion i2c_region;
     MemoryRegion disp_ctrl_region;
     MemoryRegion twoD_engine_region;
     uint32_t last_width;
     uint32_t last_height;
+    I2CBus *i2c_bus;
 
     /* mmio registers */
     uint32_t system_control;
@@ -487,6 +492,11 @@ typedef struct SM501State {
     uint32_t misc_timing;
     uint32_t power_mode_control;
 
+    uint8_t i2c_byte_count;
+    uint8_t i2c_status;
+    uint8_t i2c_addr;
+    uint8_t i2c_data[16];
+
     uint32_t uart0_ier;
     uint32_t uart0_lcr;
     uint32_t uart0_mcr;
@@ -897,6 +907,107 @@ static const MemoryRegionOps sm501_system_config_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+static uint64_t sm501_i2c_read(void *opaque, hwaddr addr, unsigned size)
+{
+    SM501State *s = (SM501State *)opaque;
+    uint8_t ret = 0;
+
+    switch (addr) {
+    case SM501_I2C_BYTE_COUNT:
+        ret = s->i2c_byte_count;
+        break;
+    case SM501_I2C_STATUS:
+        ret = s->i2c_status;
+        break;
+    case SM501_I2C_SLAVE_ADDRESS:
+        ret = s->i2c_addr;
+        break;
+    case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
+        ret = s->i2c_data[addr - SM501_I2C_DATA];
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register read."
+                      " addr=0x%" HWADDR_PRIx "\n", addr);
+    }
+
+    SM501_DPRINTF("sm501 i2c regs : read addr=%" HWADDR_PRIx " val=%x\n",
+                  addr, ret);
+    return ret;
+}
+
+static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
+                            unsigned size)
+{
+    SM501State *s = (SM501State *)opaque;
+    SM501_DPRINTF("sm501 i2c regs : write addr=%" HWADDR_PRIx
+                  " val=%" PRIx64 "\n", addr, value);
+
+    switch (addr) {
+    case SM501_I2C_BYTE_COUNT:
+        s->i2c_byte_count = value & 0xf;
+        break;
+    case SM501_I2C_CONTROL:
+        if (value & 1) {
+            if (value & 4) {
+                int res = i2c_start_transfer(s->i2c_bus,
+                                             s->i2c_addr >> 1,
+                                             s->i2c_addr & 1);
+                s->i2c_status |= (res ? 1 << 2 : 0);
+                if (!res) {
+                    int i;
+                    SM501_DPRINTF("sm501 i2c : transferring %d bytes to 0x%x\n",
+                                  s->i2c_byte_count + 1, s->i2c_addr >> 1);
+                    for (i = 0; i <= s->i2c_byte_count; i++) {
+                        res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
+                                            !(s->i2c_addr & 1));
+                        if (res) {
+                            SM501_DPRINTF("sm501 i2c : transfer failed"
+                                          " i=%d, res=%d\n", i, res);
+                            s->i2c_status |= (res ? 1 << 2 : 0);
+                            return;
+                        }
+                    }
+                    if (i) {
+                        SM501_DPRINTF("sm501 i2c : transferred %d bytes\n", i);
+                        s->i2c_status = 8;
+                    }
+                }
+            } else {
+                SM501_DPRINTF("sm501 i2c : end transfer\n");
+                i2c_end_transfer(s->i2c_bus);
+                s->i2c_status &= ~4;
+            }
+        }
+        break;
+    case SM501_I2C_RESET:
+            s->i2c_status &= ~4;
+        break;
+    case SM501_I2C_SLAVE_ADDRESS:
+        s->i2c_addr = value & 0xff;
+        break;
+    case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
+        s->i2c_data[addr - SM501_I2C_DATA] = value & 0xff;
+        break;
+    default:
+        qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register write. "
+                      "addr=0x%" HWADDR_PRIx " val=%" PRIx64 "\n", addr, value);
+    }
+}
+
+static const MemoryRegionOps sm501_i2c_ops = {
+    .read = sm501_i2c_read,
+    .write = sm501_i2c_write,
+    .valid = {
+        .min_access_size = 1,
+        .max_access_size = 4,
+    },
+    .impl = {
+        .min_access_size = 1,
+        .max_access_size = 1,
+    },
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
 static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
 {
     SM501State *s = (SM501State *)opaque;
@@ -1577,6 +1688,10 @@ static void sm501_reset(SM501State *s)
     s->irq_mask = 0;
     s->misc_timing = 0;
     s->power_mode_control = 0;
+    s->i2c_byte_count = 0;
+    s->i2c_status = 0;
+    s->i2c_addr = 0;
+    memset(s->i2c_data, 0, 16);
     s->dc_panel_control = 0x00010000; /* FIFO level 3 */
     s->dc_video_control = 0;
     s->dc_crt_control = 0x00010000;
@@ -1615,6 +1730,11 @@ static void sm501_init(SM501State *s, DeviceState *dev,
     memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
     s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);
 
+    /* i2c */
+    s->i2c_bus = i2c_init_bus(dev, "sm501.i2c");
+    I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
+    i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
+
     /* mmio */
     memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
     memory_region_init_io(&s->system_config_region, OBJECT(dev),
@@ -1622,6 +1742,9 @@ static void sm501_init(SM501State *s, DeviceState *dev,
                           "sm501-system-config", 0x6c);
     memory_region_add_subregion(&s->mmio_region, SM501_SYS_CONFIG,
                                 &s->system_config_region);
+    memory_region_init_io(&s->i2c_region, OBJECT(dev), &sm501_i2c_ops, s,
+                          "sm501-i2c", 0x14);
+    memory_region_add_subregion(&s->mmio_region, SM501_I2C, &s->i2c_region);
     memory_region_init_io(&s->disp_ctrl_region, OBJECT(dev),
                           &sm501_disp_ctrl_ops, s,
                           "sm501-disp-ctrl", 0x1000);
@@ -1705,6 +1828,11 @@ static const VMStateDescription vmstate_sm501_state = {
         VMSTATE_UINT32(twoD_destination_base, SM501State),
         VMSTATE_UINT32(twoD_alpha, SM501State),
         VMSTATE_UINT32(twoD_wrap, SM501State),
+        /* Added in version 2 */
+        VMSTATE_UINT8(i2c_byte_count, SM501State),
+        VMSTATE_UINT8(i2c_status, SM501State),
+        VMSTATE_UINT8(i2c_addr, SM501State),
+        VMSTATE_UINT8_ARRAY(i2c_data, SM501State, 16),
         VMSTATE_END_OF_LIST()
      }
 };
@@ -1770,8 +1898,8 @@ static void sm501_reset_sysbus(DeviceState *dev)
 
 static const VMStateDescription vmstate_sm501_sysbus = {
     .name = TYPE_SYSBUS_SM501,
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_STRUCT(state, SM501SysBusState, 1,
                        vmstate_sm501_state, SM501State),
@@ -1843,8 +1971,8 @@ static void sm501_reset_pci(DeviceState *dev)
 
 static const VMStateDescription vmstate_sm501_pci = {
     .name = TYPE_PCI_SM501,
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
         VMSTATE_STRUCT(state, SM501PCIState, 1,
-- 
2.7.6

Re: [Qemu-devel] [PATCH 1/5] sm501: Implement i2c part for reading monitor EDID

[Philippe Mathieu-Daudé]

Hi Zoltan,

On 06/21/2018 05:08 AM, BALATON Zoltan wrote:
> Emulate the i2c part of SM501 which is used to access the EDID info
> from a monitor.
> 
> The vmstate structure is changed and its version is increased but
> SM501 is only used on SH and PPC sam460ex machines that don't support
> cross-version migration.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  default-configs/ppc-softmmu.mak    |   1 +
>  default-configs/ppcemb-softmmu.mak |   1 +
>  default-configs/sh4-softmmu.mak    |   2 +
>  default-configs/sh4eb-softmmu.mak  |   2 +
>  hw/display/sm501.c                 | 136 +++++++++++++++++++++++++++++++++++--
>  5 files changed, 138 insertions(+), 4 deletions(-)
> 
> diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> index b8b0526..e131e24 100644
> --- a/default-configs/ppc-softmmu.mak
> +++ b/default-configs/ppc-softmmu.mak
> @@ -24,6 +24,7 @@ CONFIG_ETSEC=y
>  # For Sam460ex
>  CONFIG_USB_EHCI_SYSBUS=y
>  CONFIG_SM501=y
> +CONFIG_DDC=y
>  CONFIG_IDE_SII3112=y
>  CONFIG_I2C=y
>  CONFIG_BITBANG_I2C=y
> diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> index 37af193..ac44f15 100644
> --- a/default-configs/ppcemb-softmmu.mak
> +++ b/default-configs/ppcemb-softmmu.mak
> @@ -17,6 +17,7 @@ CONFIG_XILINX=y
>  CONFIG_XILINX_ETHLITE=y
>  CONFIG_USB_EHCI_SYSBUS=y
>  CONFIG_SM501=y
> +CONFIG_DDC=y
>  CONFIG_IDE_SII3112=y
>  CONFIG_I2C=y
>  CONFIG_BITBANG_I2C=y
> diff --git a/default-configs/sh4-softmmu.mak b/default-configs/sh4-softmmu.mak
> index 546d855..caeccd5 100644
> --- a/default-configs/sh4-softmmu.mak
> +++ b/default-configs/sh4-softmmu.mak
> @@ -9,6 +9,8 @@ CONFIG_PFLASH_CFI02=y
>  CONFIG_SH4=y
>  CONFIG_IDE_MMIO=y
>  CONFIG_SM501=y
> +CONFIG_I2C=y
> +CONFIG_DDC=y
>  CONFIG_ISA_TESTDEV=y
>  CONFIG_I82378=y
>  CONFIG_I8259=y
> diff --git a/default-configs/sh4eb-softmmu.mak b/default-configs/sh4eb-softmmu.mak
> index 2d3fd49..53b9cd7 100644
> --- a/default-configs/sh4eb-softmmu.mak
> +++ b/default-configs/sh4eb-softmmu.mak
> @@ -9,6 +9,8 @@ CONFIG_PFLASH_CFI02=y
>  CONFIG_SH4=y
>  CONFIG_IDE_MMIO=y
>  CONFIG_SM501=y
> +CONFIG_I2C=y
> +CONFIG_DDC=y
>  CONFIG_ISA_TESTDEV=y
>  CONFIG_I82378=y
>  CONFIG_I8259=y
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index ca0840f..0625cf5 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -26,6 +26,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/cutils.h"
>  #include "qapi/error.h"
> +#include "qemu/log.h"
>  #include "qemu-common.h"
>  #include "cpu.h"
>  #include "hw/hw.h"
> @@ -34,6 +35,8 @@
>  #include "hw/devices.h"
>  #include "hw/sysbus.h"
>  #include "hw/pci/pci.h"
> +#include "hw/i2c/i2c.h"
> +#include "hw/i2c/i2c-ddc.h"
>  #include "qemu/range.h"
>  #include "ui/pixel_ops.h"
>  
> @@ -471,10 +474,12 @@ typedef struct SM501State {
>      MemoryRegion local_mem_region;
>      MemoryRegion mmio_region;
>      MemoryRegion system_config_region;
> +    MemoryRegion i2c_region;
>      MemoryRegion disp_ctrl_region;
>      MemoryRegion twoD_engine_region;
>      uint32_t last_width;
>      uint32_t last_height;
> +    I2CBus *i2c_bus;
>  
>      /* mmio registers */
>      uint32_t system_control;
> @@ -487,6 +492,11 @@ typedef struct SM501State {
>      uint32_t misc_timing;
>      uint32_t power_mode_control;
>  
> +    uint8_t i2c_byte_count;
> +    uint8_t i2c_status;
> +    uint8_t i2c_addr;
> +    uint8_t i2c_data[16];

Using:
       uint8_t i2c_regs[0x20];

Would ease vmstate management and further refactors.

> +
>      uint32_t uart0_ier;
>      uint32_t uart0_lcr;
>      uint32_t uart0_mcr;
> @@ -897,6 +907,107 @@ static const MemoryRegionOps sm501_system_config_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  

To help code review you can use the "hw/registerfields.h" API:

FIELD(CTRL, ENABLE,       0, 1)
FIELD(CTRL, BUS_SPEED,    1, 1)
FIELD(CTRL, START,        2, 1)
FIELD(CTRL, INT_ENABLE,   4, 1)
FIELD(CTRL, INT_ACK,      5, 1)
FIELD(CTRL, REPEAT_START, 6, 1)

FIELD(STATUS, BUSY,       0, 1)
FIELD(STATUS, ACK,        1, 1)
FIELD(STATUS, ERROR,      2, 1)
FIELD(STATUS, COMPLETE,   3, 1)

FIELD(RESET, ERROR,       2, 1)

> +static uint64_t sm501_i2c_read(void *opaque, hwaddr addr, unsigned size)
> +{
> +    SM501State *s = (SM501State *)opaque;
> +    uint8_t ret = 0;
> +
> +    switch (addr) {
> +    case SM501_I2C_BYTE_COUNT:
> +        ret = s->i2c_byte_count;
> +        break;
> +    case SM501_I2C_STATUS:
> +        ret = s->i2c_status;
> +        break;
> +    case SM501_I2C_SLAVE_ADDRESS:
> +        ret = s->i2c_addr;
> +        break;
> +    case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
> +        ret = s->i2c_data[addr - SM501_I2C_DATA];
> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register read."
> +                      " addr=0x%" HWADDR_PRIx "\n", addr);
> +    }
> +
> +    SM501_DPRINTF("sm501 i2c regs : read addr=%" HWADDR_PRIx " val=%x\n",
> +                  addr, ret);

At some point we should replace the SM501_DPRINTF() macro by trace events.

> +    return ret;
> +}
> +
> +static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
> +                            unsigned size)
> +{
> +    SM501State *s = (SM501State *)opaque;
> +    SM501_DPRINTF("sm501 i2c regs : write addr=%" HWADDR_PRIx
> +                  " val=%" PRIx64 "\n", addr, value);
> +
> +    switch (addr) {
> +    case SM501_I2C_BYTE_COUNT:
> +        s->i2c_byte_count = value & 0xf;
> +        break;
> +    case SM501_I2C_CONTROL:
> +        if (value & 1) {

               value & R_CTRL_ENABLE_MASK

> +            if (value & 4) {

                   value & R_CTRL_START_MASK

> +                int res = i2c_start_transfer(s->i2c_bus,
> +                                             s->i2c_addr >> 1,
> +                                             s->i2c_addr & 1);
> +                s->i2c_status |= (res ? 1 << 2 : 0);

Hmm if you previously had the ERROR bit set, you don't clear it.

                   s->i2c_status = FIELD_DP32(s->i2c_status, STATUS,
                                              ERROR, res);

> +                if (!res) {
> +                    int i;
> +                    SM501_DPRINTF("sm501 i2c : transferring %d bytes to 0x%x\n",
> +                                  s->i2c_byte_count + 1, s->i2c_addr >> 1);
> +                    for (i = 0; i <= s->i2c_byte_count; i++) {
> +                        res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
> +                                            !(s->i2c_addr & 1));
> +                        if (res) {
> +                            SM501_DPRINTF("sm501 i2c : transfer failed"
> +                                          " i=%d, res=%d\n", i, res);
> +                            s->i2c_status |= (res ? 1 << 2 : 0);

Ditto.

This could be a goto transfer_error at end of func.

> +                            return;
> +                        }
> +                    }
> +                    if (i) {
> +                        SM501_DPRINTF("sm501 i2c : transferred %d bytes\n", i);
> +                        s->i2c_status = 8;

                           s->i2c_status = R_STATUS_COMPLETE_MASK;

> +                    }
> +                }
> +            } else {
> +                SM501_DPRINTF("sm501 i2c : end transfer\n");
> +                i2c_end_transfer(s->i2c_bus);
> +                s->i2c_status &= ~4;

                   s->i2c_status &= ~R_STATUS_ERROR_MASK;

> +            }
> +        }
> +        break;
> +    case SM501_I2C_RESET:
> +            s->i2c_status &= ~4;

I understand "0 clear" as:

               if (FIELD_EX32(value, RESET, ERROR) == 0) {
                   s->i2c_status &= ~R_STATUS_ERROR_MASK;
               }

also:

               value &= ~R_RESET_ERROR_MASK;
               if (value) {
                   qemu_log_mask(LOG_GUEST_ERROR,
                                 "sm501_i2c_write(RESET) bad value...",
                                 value);
               }

> +        break;
> +    case SM501_I2C_SLAVE_ADDRESS:
> +        s->i2c_addr = value & 0xff;

Implicit mask, fine.

> +        break;
> +    case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
> +        s->i2c_data[addr - SM501_I2C_DATA] = value & 0xff;

Ditto.

> +        break;
> +    default:
> +        qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register write. "
> +                      "addr=0x%" HWADDR_PRIx " val=%" PRIx64 "\n", addr, value);
> +    }
> +}
> +
> +static const MemoryRegionOps sm501_i2c_ops = {
> +    .read = sm501_i2c_read,
> +    .write = sm501_i2c_write,
> +    .valid = {
> +        .min_access_size = 1,
> +        .max_access_size = 4,

Per "Table 9-1: GPIO and I2C Register Summary" valid.max_access_size is
1 (8-bit registers) for registers 0, 1, 2, 3;
and 4 for registers 4 to 15.

To keep things simple you could add in sm501_i2c_read/sm501_i2c_write
prologue:

       if (addr < 4 && size > 1) {
           qemu_log_mask(LOG_GUEST_ERROR, ...
           return ...
       }

> +    },
> +    .impl = {
> +        .min_access_size = 1,
> +        .max_access_size = 1,
> +    },
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};
> +
>  static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
>  {
>      SM501State *s = (SM501State *)opaque;
> @@ -1577,6 +1688,10 @@ static void sm501_reset(SM501State *s)
>      s->irq_mask = 0;
>      s->misc_timing = 0;
>      s->power_mode_control = 0;
> +    s->i2c_byte_count = 0;
> +    s->i2c_status = 0;
> +    s->i2c_addr = 0;
> +    memset(s->i2c_data, 0, 16);
>      s->dc_panel_control = 0x00010000; /* FIFO level 3 */
>      s->dc_video_control = 0;
>      s->dc_crt_control = 0x00010000;
> @@ -1615,6 +1730,11 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>      memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
>      s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);
>  
> +    /* i2c */
> +    s->i2c_bus = i2c_init_bus(dev, "sm501.i2c");

       /* ddc */

> +    I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
> +    i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);

I think this is board/screen related but that's way easier this way.

> +
>      /* mmio */
>      memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
>      memory_region_init_io(&s->system_config_region, OBJECT(dev),
> @@ -1622,6 +1742,9 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>                            "sm501-system-config", 0x6c);
>      memory_region_add_subregion(&s->mmio_region, SM501_SYS_CONFIG,
>                                  &s->system_config_region);
> +    memory_region_init_io(&s->i2c_region, OBJECT(dev), &sm501_i2c_ops, s,
> +                          "sm501-i2c", 0x14);

Per "Figure 9-2: GPIO / I2C Master Register Space" the region size is 0x20.

> +    memory_region_add_subregion(&s->mmio_region, SM501_I2C, &s->i2c_region);
>      memory_region_init_io(&s->disp_ctrl_region, OBJECT(dev),
>                            &sm501_disp_ctrl_ops, s,
>                            "sm501-disp-ctrl", 0x1000);
> @@ -1705,6 +1828,11 @@ static const VMStateDescription vmstate_sm501_state = {
>          VMSTATE_UINT32(twoD_destination_base, SM501State),
>          VMSTATE_UINT32(twoD_alpha, SM501State),
>          VMSTATE_UINT32(twoD_wrap, SM501State),
> +        /* Added in version 2 */
> +        VMSTATE_UINT8(i2c_byte_count, SM501State),
> +        VMSTATE_UINT8(i2c_status, SM501State),
> +        VMSTATE_UINT8(i2c_addr, SM501State),
> +        VMSTATE_UINT8_ARRAY(i2c_data, SM501State, 16),
>          VMSTATE_END_OF_LIST()
>       }
>  };
> @@ -1770,8 +1898,8 @@ static void sm501_reset_sysbus(DeviceState *dev)
>  
>  static const VMStateDescription vmstate_sm501_sysbus = {
>      .name = TYPE_SYSBUS_SM501,
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
>          VMSTATE_STRUCT(state, SM501SysBusState, 1,
>                         vmstate_sm501_state, SM501State),
> @@ -1843,8 +1971,8 @@ static void sm501_reset_pci(DeviceState *dev)
>  
>  static const VMStateDescription vmstate_sm501_pci = {
>      .name = TYPE_PCI_SM501,
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
>          VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
>          VMSTATE_STRUCT(state, SM501PCIState, 1,
> 

Fixing "0 clear" and region size to 0x20:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Regards,

Phil.

Re: [Qemu-devel] [PATCH 1/5] sm501: Implement i2c part for reading monitor EDID

[BALATON Zoltan]

Hello,

Thanks for review, see some questions below.

On Fri, 22 Jun 2018, Philippe Mathieu-Daudé wrote:
> On 06/21/2018 05:08 AM, BALATON Zoltan wrote:
>> Emulate the i2c part of SM501 which is used to access the EDID info
>> from a monitor.
>>
>> The vmstate structure is changed and its version is increased but
>> SM501 is only used on SH and PPC sam460ex machines that don't support
>> cross-version migration.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  default-configs/ppc-softmmu.mak    |   1 +
>>  default-configs/ppcemb-softmmu.mak |   1 +
>>  default-configs/sh4-softmmu.mak    |   2 +
>>  default-configs/sh4eb-softmmu.mak  |   2 +
>>  hw/display/sm501.c                 | 136 +++++++++++++++++++++++++++++++++++--
>>  5 files changed, 138 insertions(+), 4 deletions(-)
>>
[...]
>> @@ -487,6 +492,11 @@ typedef struct SM501State {
>>      uint32_t misc_timing;
>>      uint32_t power_mode_control;
>>
>> +    uint8_t i2c_byte_count;
>> +    uint8_t i2c_status;
>> +    uint8_t i2c_addr;
>> +    uint8_t i2c_data[16];
>
> Using:
>       uint8_t i2c_regs[0x20];
>
> Would ease vmstate management and further refactors.

Hmm, all other regions have named variables like this and not indexed 
arrays so that would not fit the current way device is implemented.

>> +
>>      uint32_t uart0_ier;
>>      uint32_t uart0_lcr;
>>      uint32_t uart0_mcr;
>> @@ -897,6 +907,107 @@ static const MemoryRegionOps sm501_system_config_ops = {
>>      .endianness = DEVICE_LITTLE_ENDIAN,
>>  };
>>
>
> To help code review you can use the "hw/registerfields.h" API:
>
> FIELD(CTRL, ENABLE,       0, 1)
> FIELD(CTRL, BUS_SPEED,    1, 1)
> FIELD(CTRL, START,        2, 1)
> FIELD(CTRL, INT_ENABLE,   4, 1)
> FIELD(CTRL, INT_ACK,      5, 1)
> FIELD(CTRL, REPEAT_START, 6, 1)
>
> FIELD(STATUS, BUSY,       0, 1)
> FIELD(STATUS, ACK,        1, 1)
> FIELD(STATUS, ERROR,      2, 1)
> FIELD(STATUS, COMPLETE,   3, 1)
>
> FIELD(RESET, ERROR,       2, 1)
>
>> +static uint64_t sm501_i2c_read(void *opaque, hwaddr addr, unsigned size)
>> +{
>> +    SM501State *s = (SM501State *)opaque;
>> +    uint8_t ret = 0;
>> +
>> +    switch (addr) {
>> +    case SM501_I2C_BYTE_COUNT:
>> +        ret = s->i2c_byte_count;
>> +        break;
>> +    case SM501_I2C_STATUS:
>> +        ret = s->i2c_status;
>> +        break;
>> +    case SM501_I2C_SLAVE_ADDRESS:
>> +        ret = s->i2c_addr;
>> +        break;
>> +    case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
>> +        ret = s->i2c_data[addr - SM501_I2C_DATA];
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register read."
>> +                      " addr=0x%" HWADDR_PRIx "\n", addr);
>> +    }
>> +
>> +    SM501_DPRINTF("sm501 i2c regs : read addr=%" HWADDR_PRIx " val=%x\n",
>> +                  addr, ret);
>
> At some point we should replace the SM501_DPRINTF() macro by trace events.

This and to convert to use registerfields as suggested above could be done 
in separate clean up, not in this patch. That's why I've followed existing 
usage of SM501_DPRINTF here.

>> +    return ret;
>> +}
>> +
>> +static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
>> +                            unsigned size)
>> +{
>> +    SM501State *s = (SM501State *)opaque;
>> +    SM501_DPRINTF("sm501 i2c regs : write addr=%" HWADDR_PRIx
>> +                  " val=%" PRIx64 "\n", addr, value);
>> +
>> +    switch (addr) {
>> +    case SM501_I2C_BYTE_COUNT:
>> +        s->i2c_byte_count = value & 0xf;
>> +        break;
>> +    case SM501_I2C_CONTROL:
>> +        if (value & 1) {
>
>               value & R_CTRL_ENABLE_MASK
>
>> +            if (value & 4) {
>
>                   value & R_CTRL_START_MASK
>
>> +                int res = i2c_start_transfer(s->i2c_bus,
>> +                                             s->i2c_addr >> 1,
>> +                                             s->i2c_addr & 1);
>> +                s->i2c_status |= (res ? 1 << 2 : 0);
>
> Hmm if you previously had the ERROR bit set, you don't clear it.
>
>                   s->i2c_status = FIELD_DP32(s->i2c_status, STATUS,
>                                              ERROR, res);
>
>> +                if (!res) {
>> +                    int i;
>> +                    SM501_DPRINTF("sm501 i2c : transferring %d bytes to 0x%x\n",
>> +                                  s->i2c_byte_count + 1, s->i2c_addr >> 1);
>> +                    for (i = 0; i <= s->i2c_byte_count; i++) {
>> +                        res = i2c_send_recv(s->i2c_bus, &s->i2c_data[i],
>> +                                            !(s->i2c_addr & 1));
>> +                        if (res) {
>> +                            SM501_DPRINTF("sm501 i2c : transfer failed"
>> +                                          " i=%d, res=%d\n", i, res);
>> +                            s->i2c_status |= (res ? 1 << 2 : 0);
>
> Ditto.
>
> This could be a goto transfer_error at end of func.

Where do you see in spec or elsewhere when should the error bit be 
cleared?

>
>> +                            return;
>> +                        }
>> +                    }
>> +                    if (i) {
>> +                        SM501_DPRINTF("sm501 i2c : transferred %d bytes\n", i);
>> +                        s->i2c_status = 8;
>
>                           s->i2c_status = R_STATUS_COMPLETE_MASK;
>
>> +                    }
>> +                }
>> +            } else {
>> +                SM501_DPRINTF("sm501 i2c : end transfer\n");
>> +                i2c_end_transfer(s->i2c_bus);
>> +                s->i2c_status &= ~4;
>
>                   s->i2c_status &= ~R_STATUS_ERROR_MASK;
>
>> +            }
>> +        }
>> +        break;
>> +    case SM501_I2C_RESET:
>> +            s->i2c_status &= ~4;
>
> I understand "0 clear" as:
>
>               if (FIELD_EX32(value, RESET, ERROR) == 0) {
>                   s->i2c_status &= ~R_STATUS_ERROR_MASK;
>               }
>
> also:
>
>               value &= ~R_RESET_ERROR_MASK;
>               if (value) {
>                   qemu_log_mask(LOG_GUEST_ERROR,
>                                 "sm501_i2c_write(RESET) bad value...",
>                                 value);
>               }

In my copy of the spec all other bits are reserved (should contain 0) and 
error bit is write only.

>> +        break;
>> +    case SM501_I2C_SLAVE_ADDRESS:
>> +        s->i2c_addr = value & 0xff;
>
> Implicit mask, fine.
>
>> +        break;
>> +    case SM501_I2C_DATA ... SM501_I2C_DATA + 15:
>> +        s->i2c_data[addr - SM501_I2C_DATA] = value & 0xff;
>
> Ditto.
>
>> +        break;
>> +    default:
>> +        qemu_log_mask(LOG_UNIMP, "sm501 i2c : not implemented register write. "
>> +                      "addr=0x%" HWADDR_PRIx " val=%" PRIx64 "\n", addr, value);
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps sm501_i2c_ops = {
>> +    .read = sm501_i2c_read,
>> +    .write = sm501_i2c_write,
>> +    .valid = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 4,
>
> Per "Table 9-1: GPIO and I2C Register Summary" valid.max_access_size is
> 1 (8-bit registers) for registers 0, 1, 2, 3;
> and 4 for registers 4 to 15.
>
> To keep things simple you could add in sm501_i2c_read/sm501_i2c_write
> prologue:
>
>       if (addr < 4 && size > 1) {
>           qemu_log_mask(LOG_GUEST_ERROR, ...
>           return ...
>       }

Actually the copy I have lists all i2c regs as 8 bits in table 9-1 so 
maybe I should have .max_access_size = 1 instead. Or where have you seen 
access size 4 for regs >= 4 ?

>> +    },
>> +    .impl = {
>> +        .min_access_size = 1,
>> +        .max_access_size = 1,
>> +    },
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>>  static uint32_t sm501_palette_read(void *opaque, hwaddr addr)
>>  {
>>      SM501State *s = (SM501State *)opaque;
>> @@ -1577,6 +1688,10 @@ static void sm501_reset(SM501State *s)
>>      s->irq_mask = 0;
>>      s->misc_timing = 0;
>>      s->power_mode_control = 0;
>> +    s->i2c_byte_count = 0;
>> +    s->i2c_status = 0;
>> +    s->i2c_addr = 0;
>> +    memset(s->i2c_data, 0, 16);
>>      s->dc_panel_control = 0x00010000; /* FIFO level 3 */
>>      s->dc_video_control = 0;
>>      s->dc_crt_control = 0x00010000;
>> @@ -1615,6 +1730,11 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>>      memory_region_set_log(&s->local_mem_region, true, DIRTY_MEMORY_VGA);
>>      s->local_mem = memory_region_get_ram_ptr(&s->local_mem_region);
>>
>> +    /* i2c */
>> +    s->i2c_bus = i2c_init_bus(dev, "sm501.i2c");
>
>       /* ddc */
>
>> +    I2CDDCState *ddc = I2CDDC(qdev_create(BUS(s->i2c_bus), TYPE_I2CDDC));
>> +    i2c_set_slave_address(I2C_SLAVE(ddc), 0x50);
>
> I think this is board/screen related but that's way easier this way.
>
>> +
>>      /* mmio */
>>      memory_region_init(&s->mmio_region, OBJECT(dev), "sm501.mmio", MMIO_SIZE);
>>      memory_region_init_io(&s->system_config_region, OBJECT(dev),
>> @@ -1622,6 +1742,9 @@ static void sm501_init(SM501State *s, DeviceState *dev,
>>                            "sm501-system-config", 0x6c);
>>      memory_region_add_subregion(&s->mmio_region, SM501_SYS_CONFIG,
>>                                  &s->system_config_region);
>> +    memory_region_init_io(&s->i2c_region, OBJECT(dev), &sm501_i2c_ops, s,
>> +                          "sm501-i2c", 0x14);
>
> Per "Figure 9-2: GPIO / I2C Master Register Space" the region size is 0x20.

But registers are only defined for 0x40-0x53 so what should the rest of 
this region contain?

Regards,
BALATON Zoltan

>> +    memory_region_add_subregion(&s->mmio_region, SM501_I2C, &s->i2c_region);
>>      memory_region_init_io(&s->disp_ctrl_region, OBJECT(dev),
>>                            &sm501_disp_ctrl_ops, s,
>>                            "sm501-disp-ctrl", 0x1000);
>> @@ -1705,6 +1828,11 @@ static const VMStateDescription vmstate_sm501_state = {
>>          VMSTATE_UINT32(twoD_destination_base, SM501State),
>>          VMSTATE_UINT32(twoD_alpha, SM501State),
>>          VMSTATE_UINT32(twoD_wrap, SM501State),
>> +        /* Added in version 2 */
>> +        VMSTATE_UINT8(i2c_byte_count, SM501State),
>> +        VMSTATE_UINT8(i2c_status, SM501State),
>> +        VMSTATE_UINT8(i2c_addr, SM501State),
>> +        VMSTATE_UINT8_ARRAY(i2c_data, SM501State, 16),
>>          VMSTATE_END_OF_LIST()
>>       }
>>  };
>> @@ -1770,8 +1898,8 @@ static void sm501_reset_sysbus(DeviceState *dev)
>>
>>  static const VMStateDescription vmstate_sm501_sysbus = {
>>      .name = TYPE_SYSBUS_SM501,
>> -    .version_id = 1,
>> -    .minimum_version_id = 1,
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_STRUCT(state, SM501SysBusState, 1,
>>                         vmstate_sm501_state, SM501State),
>> @@ -1843,8 +1971,8 @@ static void sm501_reset_pci(DeviceState *dev)
>>
>>  static const VMStateDescription vmstate_sm501_pci = {
>>      .name = TYPE_PCI_SM501,
>> -    .version_id = 1,
>> -    .minimum_version_id = 1,
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>>      .fields = (VMStateField[]) {
>>          VMSTATE_PCI_DEVICE(parent_obj, SM501PCIState),
>>          VMSTATE_STRUCT(state, SM501PCIState, 1,
>>
>
> Fixing "0 clear" and region size to 0x20:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Regards,
>
> Phil.
>
>

[Qemu-devel] [PATCH 2/5] sm501: Perform a full update after palette change

[BALATON Zoltan]

From: Sebastian Bauer <mail@sebastianbauer.info>

Changing the palette of a color index has as an immediate effect on
all pixels with the corresponding index on real hardware. Performing a
full update after a palette change is a simple way to emulate this
effect.

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---

Notes:
    v4: Updated commit message

 hw/display/sm501.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 0625cf5..a2ee6e3 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -479,6 +479,7 @@ typedef struct SM501State {
     MemoryRegion twoD_engine_region;
     uint32_t last_width;
     uint32_t last_height;
+    uint32_t do_full_update; /* perform a full update next time */
     I2CBus *i2c_bus;
 
     /* mmio registers */
@@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
 
     assert(range_covers_byte(0, 0x400 * 3, addr));
     *(uint32_t *)&s->dc_palette[addr] = value;
+    s->do_full_update = 1;
 }
 
 static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
@@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
         full_update = 1;
     }
 
+    /* someone else requested a full update */
+    if (s->do_full_update) {
+        s->do_full_update = 0;
+        full_update = 1;
+    }
+
     /* draw each line according to conditions */
     snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
               offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
-- 
2.7.6

Re: [Qemu-devel] [PATCH 2/5] sm501: Perform a full update after palette change

[Philippe Mathieu-Daudé]

On 06/21/2018 05:08 AM, BALATON Zoltan wrote:
> From: Sebastian Bauer <mail@sebastianbauer.info>
> 
> Changing the palette of a color index has as an immediate effect on
> all pixels with the corresponding index on real hardware. Performing a
> full update after a palette change is a simple way to emulate this
> effect.
> 
> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> 
> Notes:
>     v4: Updated commit message
> 
>  hw/display/sm501.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 0625cf5..a2ee6e3 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -479,6 +479,7 @@ typedef struct SM501State {
>      MemoryRegion twoD_engine_region;
>      uint32_t last_width;
>      uint32_t last_height;
> +    uint32_t do_full_update; /* perform a full update next time */

Can this be a bool?

>      I2CBus *i2c_bus;
>  
>      /* mmio registers */
> @@ -1032,6 +1033,7 @@ static void sm501_palette_write(void *opaque, hwaddr addr,
>  
>      assert(range_covers_byte(0, 0x400 * 3, addr));
>      *(uint32_t *)&s->dc_palette[addr] = value;
> +    s->do_full_update = 1;
>  }
>  
>  static uint64_t sm501_disp_ctrl_read(void *opaque, hwaddr addr,
> @@ -1620,6 +1622,12 @@ static void sm501_update_display(void *opaque)
>          full_update = 1;
>      }
>  
> +    /* someone else requested a full update */
> +    if (s->do_full_update) {
> +        s->do_full_update = 0;
> +        full_update = 1;
> +    }
> +
>      /* draw each line according to conditions */
>      snap = memory_region_snapshot_and_clear_dirty(&s->local_mem_region,
>                offset, width * height * src_bpp, DIRTY_MEMORY_VGA);
> 

Re: [Qemu-devel] [PATCH 2/5] sm501: Perform a full update after palette change

[Sebastian Bauer]

Am 2018-06-22 03:34, schrieb Philippe Mathieu-Daudé:
> On 06/21/2018 05:08 AM, BALATON Zoltan wrote:
>> From: Sebastian Bauer <mail@sebastianbauer.info>
>> 
>> Changing the palette of a color index has as an immediate effect on
>> all pixels with the corresponding index on real hardware. Performing a
>> full update after a palette change is a simple way to emulate this
>> effect.
>> 
>> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> 
>> Notes:
>>     v4: Updated commit message
>> 
>>  hw/display/sm501.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> 
>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>> index 0625cf5..a2ee6e3 100644
>> --- a/hw/display/sm501.c
>> +++ b/hw/display/sm501.c
>> @@ -479,6 +479,7 @@ typedef struct SM501State {
>>      MemoryRegion twoD_engine_region;
>>      uint32_t last_width;
>>      uint32_t last_height;
>> +    uint32_t do_full_update; /* perform a full update next time */
> 
> Can this be a bool?

Yes, this can be a bool. Let me know if you think that this is required.

Bye
Sebastian

Re: [Qemu-devel] [PATCH 2/5] sm501: Perform a full update after palette change

[BALATON Zoltan]

On Fri, 22 Jun 2018, Sebastian Bauer wrote:
> Am 2018-06-22 03:34, schrieb Philippe Mathieu-Daudé:
>> On 06/21/2018 05:08 AM, BALATON Zoltan wrote:
>>> From: Sebastian Bauer <mail@sebastianbauer.info>
>>> 
>>> Changing the palette of a color index has as an immediate effect on
>>> all pixels with the corresponding index on real hardware. Performing a
>>> full update after a palette change is a simple way to emulate this
>>> effect.
>>> 
>>> Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> 
>>> Notes:
>>>     v4: Updated commit message
>>>
>>>  hw/display/sm501.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>> 
>>> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
>>> index 0625cf5..a2ee6e3 100644
>>> --- a/hw/display/sm501.c
>>> +++ b/hw/display/sm501.c
>>> @@ -479,6 +479,7 @@ typedef struct SM501State {
>>>      MemoryRegion twoD_engine_region;
>>>      uint32_t last_width;
>>>      uint32_t last_height;
>>> +    uint32_t do_full_update; /* perform a full update next time */
>> 
>> Can this be a bool?
>
> Yes, this can be a bool. Let me know if you think that this is required.

I can take care of this in next submission, no need to resend the patch, I 
can change it to bool.

Regards,
BALATON Zoltan

[Qemu-devel] [PATCH 3/5] sm501: Use values from the pitch register for 2d operations

[BALATON Zoltan]

From: Sebastian Bauer <mail@sebastianbauer.info>

Before, crt_h_total was used for src_width and dst_width. This is a
property of the current display setting and not relevant for the 2d
operation that also can be done off-screen. The pitch register's purpose
is to describe line pitch relevant of the 2d operation.

Signed-off-by: Sebastian Bauer <mail@sebastianbauer.info>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index a2ee6e3..a6232ed 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -701,8 +701,8 @@ static void sm501_2d_operation(SM501State *s)
     /* get frame buffer info */
     uint8_t *src = s->local_mem + (s->twoD_source_base & 0x03FFFFFF);
     uint8_t *dst = s->local_mem + (s->twoD_destination_base & 0x03FFFFFF);
-    int src_width = (s->dc_crt_h_total & 0x00000FFF) + 1;
-    int dst_width = (s->dc_crt_h_total & 0x00000FFF) + 1;
+    int src_width = s->twoD_pitch & 0x1FFF;
+    int dst_width = (s->twoD_pitch >> 16) & 0x1FFF;
 
     if (addressing != 0x0) {
         printf("%s: only XY addressing is supported.\n", __func__);
-- 
2.7.6

[Qemu-devel] [PATCH 4/5] sm501: Set updated region dirty after 2D operation

[BALATON Zoltan]

Set the changed memory region dirty after performed a 2D operation to
ensure that the screen is updated properly.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/sm501.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index a6232ed..e426d2f 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -774,6 +774,11 @@ static void sm501_2d_operation(SM501State *s)
         abort();
         break;
     }
+
+    memory_region_set_dirty(&s->local_mem_region,
+                            s->twoD_destination_base & 0x03FFFFFF,
+                            (dst_y + operation_height) * dst_width +
+                            dst_x + operation_width);
 }
 
 static uint64_t sm501_system_config_read(void *opaque, hwaddr addr,
-- 
2.7.6

Re: [Qemu-devel] [PATCH 0/5] Misc sm501 improvements

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

On Thu, Jun 21, 2018 at 10:08:21AM +0200, BALATON Zoltan wrote:
> These are fixes to sm501 emulation that were found while testing with
> AmigaOS 4.1FE (AmigaOS 4 is known to work on real hardware but we have
> no way to verify it against hardware). While this is mainly to improve
> sam460ex emulation this is now a separate series with the sm501
> specific patches because that's also used on SH emulation. I hope
> someone can review and merge this.

I'm not really familiar with the sm501 hardware, so I can't speak to
these technically, beyond the fact that there's nothing obviously
bogus.

I can say that BALATON Zoltan is the only person who's made
non-trivial changes to sm501 lately, and there don't seem to have been
any screams, so I'm pretty happy to assume he knows what he's doing.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 0/5] Misc sm501 improvements

[Philippe Mathieu-Daudé]

[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]

Cc'ing Gerd who maintains many hw/display/ files.

On 06/21/2018 09:51 PM, David Gibson wrote:
> On Thu, Jun 21, 2018 at 10:08:21AM +0200, BALATON Zoltan wrote:
>> These are fixes to sm501 emulation that were found while testing with
>> AmigaOS 4.1FE (AmigaOS 4 is known to work on real hardware but we have
>> no way to verify it against hardware). While this is mainly to improve
>> sam460ex emulation this is now a separate series with the sm501
>> specific patches because that's also used on SH emulation. I hope
>> someone can review and merge this.
[...]
> hw/display/sm501.c                 | 166
+++++++++++++++++++++++++++++++++++--
> 
> I'm not really familiar with the sm501 hardware, so I can't speak to
> these technically, beyond the fact that there's nothing obviously
> bogus.
> 
> I can say that BALATON Zoltan is the only person who's made
> non-trivial changes to sm501 lately, and there don't seem to have been
> any screams, so I'm pretty happy to assume he knows what he's doing.
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 0/5] Misc sm501 improvements

[Gerd Hoffmann]

On Thu, Jun 21, 2018 at 10:37:10PM -0300, Philippe Mathieu-Daudé wrote:
> Cc'ing Gerd who maintains many hw/display/ files.
> 
> On 06/21/2018 09:51 PM, David Gibson wrote:
> > On Thu, Jun 21, 2018 at 10:08:21AM +0200, BALATON Zoltan wrote:
> >> These are fixes to sm501 emulation that were found while testing with
> >> AmigaOS 4.1FE (AmigaOS 4 is known to work on real hardware but we have
> >> no way to verify it against hardware). While this is mainly to improve
> >> sam460ex emulation this is now a separate series with the sm501
> >> specific patches because that's also used on SH emulation. I hope
> >> someone can review and merge this.
> [...]
> > hw/display/sm501.c                 | 166
> +++++++++++++++++++++++++++++++++++--
> > 
> > I'm not really familiar with the sm501 hardware, so I can't speak to
> > these technically, beyond the fact that there's nothing obviously
> > bogus.
> > 
> > I can say that BALATON Zoltan is the only person who's made
> > non-trivial changes to sm501 lately, and there don't seem to have been
> > any screams, so I'm pretty happy to assume he knows what he's doing.

Changes all look sane to me even without having sm501 specs at hand.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH 0/5] Misc sm501 improvements

[Philippe Mathieu-Daudé]

Hi Zoltan,

On 06/21/2018 05:08 AM, BALATON Zoltan wrote:
> These are fixes to sm501 emulation that were found while testing with
> AmigaOS 4.1FE (AmigaOS 4 is known to work on real hardware but we have
> no way to verify it against hardware). While this is mainly to improve

Is it possible to fetch/use this image? (and add integration tests)

> sam460ex emulation this is now a separate series with the sm501
> specific patches because that's also used on SH emulation. I hope
> someone can review and merge this.
> 
> BALATON Zoltan (3):
>   sm501: Implement i2c part for reading monitor EDID
>   sm501: Set updated region dirty after 2D operation
>   sm501: Fix support for non-zero frame buffer start address
> 
> Sebastian Bauer (2):
>   sm501: Perform a full update after palette change
>   sm501: Use values from the pitch register for 2d operations.
> 
>  default-configs/ppc-softmmu.mak    |   1 +
>  default-configs/ppcemb-softmmu.mak |   1 +
>  default-configs/sh4-softmmu.mak    |   2 +
>  default-configs/sh4eb-softmmu.mak  |   2 +
>  hw/display/sm501.c                 | 166 +++++++++++++++++++++++++++++++++++--
>  5 files changed, 163 insertions(+), 9 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/5] Misc sm501 improvements

[BALATON Zoltan]

On Thu, 21 Jun 2018, Philippe Mathieu-Daudé wrote:
> Hi Zoltan,
>
> On 06/21/2018 05:08 AM, BALATON Zoltan wrote:
>> These are fixes to sm501 emulation that were found while testing with
>> AmigaOS 4.1FE (AmigaOS 4 is known to work on real hardware but we have
>> no way to verify it against hardware). While this is mainly to improve
>
> Is it possible to fetch/use this image? (and add integration tests)

AmigaOS 4 is not freely available, it needs to be purchased so I'm not 
sure it could be used for tests. Also the current version available for 
Sam460 does not work too well with QEMU yet. To be able to use it 
according to Sebastian one needs at least updated ehci driver and maybe 
CLGD graphics driver which are not on this boot CD and editing the CD is 
not convenient or doable by most people. Therefore we are trying to fix 
QEMU with Sebastian to be usable with the current boot CD but maybe an 
updated CD with better drivers would be the way to go, although I don't 
know if the vendor wants to do that or not.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH 0/5] Misc sm501 improvements

[Philippe Mathieu-Daudé]

On 06/22/2018 05:55 AM, BALATON Zoltan wrote:
> On Thu, 21 Jun 2018, Philippe Mathieu-Daudé wrote:
>> Hi Zoltan,
>>
>> On 06/21/2018 05:08 AM, BALATON Zoltan wrote:
>>> These are fixes to sm501 emulation that were found while testing with
>>> AmigaOS 4.1FE (AmigaOS 4 is known to work on real hardware but we have
>>> no way to verify it against hardware). While this is mainly to improve
>>
>> Is it possible to fetch/use this image? (and add integration tests)
> 
> AmigaOS 4 is not freely available, it needs to be purchased so I'm not
> sure it could be used for tests. Also the current version available for
> Sam460 does not work too well with QEMU yet. To be able to use it
> according to Sebastian one needs at least updated ehci driver and maybe
> CLGD graphics driver which are not on this boot CD and editing the CD is
> not convenient or doable by most people. Therefore we are trying to fix
> QEMU with Sebastian to be usable with the current boot CD but maybe an
> updated CD with better drivers would be the way to go, although I don't
> know if the vendor wants to do that or not.

OK thanks for the explanation, I plan to add part of it somewhere in the
wiki.