Misc sm501 improvements

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 <[email protected]>

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

default-configs/ppc-softmmu.mak +1 -0

index b8b0526..e131e24 100644
@@ -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

default-configs/ppcemb-softmmu.mak +1 -0

index 37af193..ac44f15 100644
@@ -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

default-configs/sh4-softmmu.mak +2 -0

index 546d855..caeccd5 100644
@@ -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

default-configs/sh4eb-softmmu.mak +2 -0

index 2d3fd49..53b9cd7 100644
@@ -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

hw/display/sm501.c +5 -0

index ca0840f..0625cf5 100644
@@ -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;

hw/display/sm501.c +15 -0

+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;

hw/display/sm501.c +6 -0

+    return ret;
+}
+
+static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
+                            unsigned size)
+{

hw/display/sm501.c +12 -0

+    },
+    .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;

hw/display/sm501.c +8 -2

+    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),

Quoted 16 lines · 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 <[email protected]>

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

hw/display/sm501.c typedef struct SM501State { · line 498

Quoted 8 lines

hw/display/sm501.c +4 -0

@@ -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.

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 909

Quoted 8 lines

hw/display/sm501.c +1 -0

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

Quoted 19 lines

hw/display/sm501.c +19 -0

+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:

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 934

Quoted 6 lines

hw/display/sm501.c +6 -0

+        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.

Quoted 6 lines

hw/display/sm501.c +6 -0

+    return ret;
+}
+
+static void sm501_i2c_write(void *opaque, hwaddr addr, uint64_t value,
+                            unsigned size)
+{

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 950

Quoted 10 lines

hw/display/sm501.c +10 -0

+    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

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 951

Quoted 1 line

hw/display/sm501.c +1 -0

+            if (value & 4) {

value & R_CTRL_START_MASK

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 955

Quoted 4 lines

hw/display/sm501.c +4 -0

+                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);

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 966

Quoted 11 lines

hw/display/sm501.c +11 -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);

Ditto.

This could be a goto transfer_error at end of func.

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 972

Quoted 7 lines

hw/display/sm501.c +6 -0

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

                          s->i2c_status = R_STATUS_COMPLETE_MASK;

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 978

Quoted 6 lines

hw/display/sm501.c +6 -0

+                    }
+                }
+            } 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;

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 983

Quoted 5 lines

hw/display/sm501.c +5 -0

+            }
+        }
+        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);
              }

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 986

Quoted 3 lines

hw/display/sm501.c +3 -0

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

Implicit mask, fine.

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 989

Quoted 3 lines

hw/display/sm501.c +3 -0

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

Ditto.

hw/display/sm501.c static const MemoryRegionOps sm501_system_config_ops = { · line 1002

Quoted 13 lines

hw/display/sm501.c +13 -0

+        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 ...
      }

Quoted 23 lines

hw/display/sm501.c +12 -0

+    },
+    .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;

hw/display/sm501.c static void sm501_init(SM501State *s, DeviceState *dev, · line 1734

Quoted 5 lines

hw/display/sm501.c +2 -0

@@ -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 */

hw/display/sm501.c static void sm501_init(SM501State *s, DeviceState *dev, · line 1736

Quoted 2 lines

hw/display/sm501.c +2 -0

+    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.

hw/display/sm501.c static void sm501_init(SM501State *s, DeviceState *dev, · line 1746

Quoted 10 lines

hw/display/sm501.c +3 -0

+
     /* 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.

Quoted 29 lines

hw/display/sm501.c +8 -2

+    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),

hw/display/sm501.c static void sm501_reset_pci(DeviceState *dev) · line 1978

Quoted 10 lines

hw/display/sm501.c +2 -2

@@ -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é <[email protected]>

Regards,

Phil.

From: Sebastian Bauer <[email protected]>

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 <[email protected]>

Signed-off-by BALATON Zoltan <[email protected]>

---

Notes:
v4: Updated commit message

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

hw/display/sm501.c +0 -0

index 0625cf5..a2ee6e3 100644

hw/display/sm501.c +7 -0

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

Quoted 17 lines

From: Sebastian Bauer <[email protected]>

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 <[email protected]>

Signed-off-by BALATON Zoltan <[email protected]>

---

Notes:
v4: Updated commit message

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

hw/display/sm501.c typedef struct SM501State { · line 482

Quoted 9 lines

hw/display/sm501.c +1 -0

index 0625cf5..a2ee6e3 100644
@@ -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?

Am 2018-06-22 03:34, schrieb Philippe Mathieu-Daudé:

Quoted 18 lines · BALATON Zoltan

Quoted 17 lines

From: Sebastian Bauer <[email protected]>

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 <[email protected]>

Signed-off-by BALATON Zoltan <[email protected]>

---

Notes:
v4: Updated commit message

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

Quoted 11 lines

hw/display/sm501.c typedef struct SM501State { · line 482

Quoted 9 lines

hw/display/sm501.c +1 -0

index 0625cf5..a2ee6e3 100644
@@ -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.

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 <[email protected]>

Signed-off-by BALATON Zoltan <[email protected]>

---
hw/display/sm501.c | 13 ++++++++++---

1 file changed, 10 insertions(+), 3 deletions(-)

hw/display/sm501.c +10 -3

index e426d2f..acc26f6 100644
@@ -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 */

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.

Quoted 6 lines · 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.

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.

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

Quoted 7 lines · David Gibson

Quoted 6 lines · 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.

[...]

Quoted 1 line

hw/display/sm501.c | 166

+++++++++++++++++++++++++++++++++++--

Quoted 8 lines

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.

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

Hi Zoltan,

Quoted 3 lines · 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

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

Quoted 8 lines · Philippe Mathieu-Daudé

Hi Zoltan,

Quoted 3 lines · 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

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.