[Qemu-devel] [RFC PATCH] ati-vga: Implement dummy VBlank IRQ

BALATON Zoltan posted 1 patch 4 years, 8 months ago
Test FreeBSD passed
Test docker-mingw@fedora passed
Test asan passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Test s390x failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190815002652.79FDE7456E2@zero.eik.bme.hu
hw/display/ati.c      | 41 +++++++++++++++++++++++++++++++++++++++++
hw/display/ati_dbg.c  |  1 +
hw/display/ati_int.h  |  4 ++++
hw/display/ati_regs.h |  1 +
4 files changed, 47 insertions(+)
[Qemu-devel] [RFC PATCH] ati-vga: Implement dummy VBlank IRQ
Posted by BALATON Zoltan 4 years, 8 months ago
The MacOS driver exits if the card does not have an interrupt. If we
set PCI_INTERRUPT_PIN to 1 then it enables VBlank interrupts and it
boots but the mouse poniter can not be moved. This patch implements a
dummy VBlank interrupt by a timer triggered at 60 Hz to test if it
helps. Unfortunately it doesn't: MacOS with this patch hangs during
boot just polling interrupts and acknowledging them so maybe it needs
something else or there may be some other problem with this
implementation.

This is posted for comments and to let others experiment with it but
probably should not be committed upstream yet.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/ati.c      | 41 +++++++++++++++++++++++++++++++++++++++++
 hw/display/ati_dbg.c  |  1 +
 hw/display/ati_int.h  |  4 ++++
 hw/display/ati_regs.h |  1 +
 4 files changed, 47 insertions(+)

diff --git a/hw/display/ati.c b/hw/display/ati.c
index a365e2455d..e06cbf3e91 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -243,6 +243,21 @@ static uint64_t ati_i2c(bitbang_i2c_interface *i2c, uint64_t data, int base)
     return data;
 }
 
+static void ati_vga_update_irq(ATIVGAState *s)
+{
+    pci_set_irq(&s->dev, s->regs.gen_int_status & 1);
+}
+
+static void ati_vga_vblank_irq(void *opaque)
+{
+    ATIVGAState *s = opaque;
+
+    timer_mod(&s->vblank_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
+              NANOSECONDS_PER_SECOND / 60);
+    s->regs.gen_int_status |= 1;
+    ati_vga_update_irq(s);
+}
+
 static inline uint64_t ati_reg_read_offs(uint32_t reg, int offs,
                                          unsigned int size)
 {
@@ -283,6 +298,12 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
                                 addr - (BIOS_0_SCRATCH + i * 4), size);
         break;
     }
+    case GEN_INT_CNTL:
+        val = s->regs.gen_int_cntl;
+        break;
+    case GEN_INT_STATUS:
+        val = s->regs.gen_int_status;
+        break;
     case CRTC_GEN_CNTL ... CRTC_GEN_CNTL + 3:
         val = ati_reg_read_offs(s->regs.crtc_gen_cntl,
                                 addr - CRTC_GEN_CNTL, size);
@@ -512,6 +533,19 @@ static void ati_mm_write(void *opaque, hwaddr addr,
                            addr - (BIOS_0_SCRATCH + i * 4), data, size);
         break;
     }
+    case GEN_INT_CNTL:
+        s->regs.gen_int_cntl = data;
+        if (data & 1) {
+            ati_vga_vblank_irq(s);
+        } else {
+            timer_del(&s->vblank_timer);
+        }
+        break;
+    case GEN_INT_STATUS:
+        data &= (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF ?
+                 0x000f040fUL : 0xfc080effUL);
+        s->regs.gen_int_status &= ~data;
+        break;
     case CRTC_GEN_CNTL ... CRTC_GEN_CNTL + 3:
     {
         uint32_t val = s->regs.crtc_gen_cntl;
@@ -902,12 +936,18 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_MEM_PREFETCH, &vga->vram);
     pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &s->io);
     pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mm);
+
+    /* most interrupts are not yet emulated but MacOS needs at least VBlank */
+    dev->config[PCI_INTERRUPT_PIN] = 1;
+    timer_init_ns(&s->vblank_timer, QEMU_CLOCK_VIRTUAL, ati_vga_vblank_irq, s);
 }
 
 static void ati_vga_reset(DeviceState *dev)
 {
     ATIVGAState *s = ATI_VGA(dev);
 
+    timer_del(&s->vblank_timer);
+
     /* reset vga */
     vga_common_reset(&s->vga);
     s->mode = VGA_MODE;
@@ -917,6 +957,7 @@ static void ati_vga_exit(PCIDevice *dev)
 {
     ATIVGAState *s = ATI_VGA(dev);
 
+    timer_del(&s->vblank_timer);
     graphic_console_close(s->vga.con);
 }
 
diff --git a/hw/display/ati_dbg.c b/hw/display/ati_dbg.c
index 7e59c41ac2..0ebbd36f14 100644
--- a/hw/display/ati_dbg.c
+++ b/hw/display/ati_dbg.c
@@ -16,6 +16,7 @@ static struct ati_regdesc ati_reg_names[] = {
     {"BUS_CNTL", 0x0030},
     {"BUS_CNTL1", 0x0034},
     {"GEN_INT_CNTL", 0x0040},
+    {"GEN_INT_STATUS", 0x0044},
     {"CRTC_GEN_CNTL", 0x0050},
     {"CRTC_EXT_CNTL", 0x0054},
     {"DAC_CNTL", 0x0058},
diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
index 5b4d3be1e6..2a16708e4f 100644
--- a/hw/display/ati_int.h
+++ b/hw/display/ati_int.h
@@ -9,6 +9,7 @@
 #ifndef ATI_INT_H
 #define ATI_INT_H
 
+#include "qemu/timer.h"
 #include "hw/pci/pci.h"
 #include "hw/i2c/bitbang_i2c.h"
 #include "vga_int.h"
@@ -33,6 +34,8 @@
 typedef struct ATIVGARegs {
     uint32_t mm_index;
     uint32_t bios_scratch[8];
+    uint32_t gen_int_cntl;
+    uint32_t gen_int_status;
     uint32_t crtc_gen_cntl;
     uint32_t crtc_ext_cntl;
     uint32_t dac_cntl;
@@ -89,6 +92,7 @@ typedef struct ATIVGAState {
     uint16_t cursor_size;
     uint32_t cursor_offset;
     QEMUCursor *cursor;
+    QEMUTimer vblank_timer;
     bitbang_i2c_interface bbi2c;
     MemoryRegion io;
     MemoryRegion mm;
diff --git a/hw/display/ati_regs.h b/hw/display/ati_regs.h
index 02046e97c2..2b6f949bd4 100644
--- a/hw/display/ati_regs.h
+++ b/hw/display/ati_regs.h
@@ -34,6 +34,7 @@
 #define BUS_CNTL                                0x0030
 #define BUS_CNTL1                               0x0034
 #define GEN_INT_CNTL                            0x0040
+#define GEN_INT_STATUS                          0x0044
 #define CRTC_GEN_CNTL                           0x0050
 #define CRTC_EXT_CNTL                           0x0054
 #define DAC_CNTL                                0x0058
-- 
2.13.7


Re: [Qemu-devel] [RFC PATCH] ati-vga: Implement dummy VBlank IRQ
Posted by Gerd Hoffmann 4 years, 8 months ago
On Thu, Aug 15, 2019 at 02:25:07AM +0200, BALATON Zoltan wrote:
> The MacOS driver exits if the card does not have an interrupt. If we
> set PCI_INTERRUPT_PIN to 1 then it enables VBlank interrupts and it
> boots but the mouse poniter can not be moved. This patch implements a
> dummy VBlank interrupt by a timer triggered at 60 Hz to test if it
> helps. Unfortunately it doesn't: MacOS with this patch hangs during
> boot just polling interrupts and acknowledging them so maybe it needs
> something else or there may be some other problem with this
> implementation.
> 
> This is posted for comments and to let others experiment with it but
> probably should not be committed upstream yet.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/display/ati.c      | 41 +++++++++++++++++++++++++++++++++++++++++
>  hw/display/ati_dbg.c  |  1 +
>  hw/display/ati_int.h  |  4 ++++
>  hw/display/ati_regs.h |  1 +
>  4 files changed, 47 insertions(+)
> 
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index a365e2455d..e06cbf3e91 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -243,6 +243,21 @@ static uint64_t ati_i2c(bitbang_i2c_interface *i2c, uint64_t data, int base)
>      return data;
>  }
>  
> +static void ati_vga_update_irq(ATIVGAState *s)
> +{
> +    pci_set_irq(&s->dev, s->regs.gen_int_status & 1);

This should be "s->regs.gen_int_status & s->regs.gen_int_cntl" I guess?

> +static void ati_vga_vblank_irq(void *opaque)
> +{
> +    ATIVGAState *s = opaque;
> +
> +    timer_mod(&s->vblank_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +              NANOSECONDS_PER_SECOND / 60);
> +    s->regs.gen_int_status |= 1;

#defines for the irq status bits would be nice.

> +    case GEN_INT_CNTL:
> +        s->regs.gen_int_cntl = data;
> +        if (data & 1) {
> +            ati_vga_vblank_irq(s);
> +        } else {
> +            timer_del(&s->vblank_timer);
> +        }

ati_vga_update_irq() needed here.

> +        break;
> +    case GEN_INT_STATUS:
> +        data &= (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF ?
> +                 0x000f040fUL : 0xfc080effUL);

Add IRQ_MASK #define ?

> +        s->regs.gen_int_status &= ~data;

ati_vga_update_irq() needed here too.

cheers,
  Gerd


Re: [Qemu-devel] [RFC PATCH] ati-vga: Implement dummy VBlank IRQ
Posted by BALATON Zoltan 4 years, 8 months ago
On Thu, 15 Aug 2019, Gerd Hoffmann wrote:
> On Thu, Aug 15, 2019 at 02:25:07AM +0200, BALATON Zoltan wrote:
>> The MacOS driver exits if the card does not have an interrupt. If we
>> set PCI_INTERRUPT_PIN to 1 then it enables VBlank interrupts and it
>> boots but the mouse poniter can not be moved. This patch implements a
>> dummy VBlank interrupt by a timer triggered at 60 Hz to test if it
>> helps. Unfortunately it doesn't: MacOS with this patch hangs during
>> boot just polling interrupts and acknowledging them so maybe it needs
>> something else or there may be some other problem with this
>> implementation.
>>
>> This is posted for comments and to let others experiment with it but
>> probably should not be committed upstream yet.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/display/ati.c      | 41 +++++++++++++++++++++++++++++++++++++++++
>>  hw/display/ati_dbg.c  |  1 +
>>  hw/display/ati_int.h  |  4 ++++
>>  hw/display/ati_regs.h |  1 +
>>  4 files changed, 47 insertions(+)
>>
>> diff --git a/hw/display/ati.c b/hw/display/ati.c
>> index a365e2455d..e06cbf3e91 100644
>> --- a/hw/display/ati.c
>> +++ b/hw/display/ati.c
>> @@ -243,6 +243,21 @@ static uint64_t ati_i2c(bitbang_i2c_interface *i2c, uint64_t data, int base)
>>      return data;
>>  }
>>
>> +static void ati_vga_update_irq(ATIVGAState *s)
>> +{
>> +    pci_set_irq(&s->dev, s->regs.gen_int_status & 1);
>
> This should be "s->regs.gen_int_status & s->regs.gen_int_cntl" I guess?

Probably, but we only try to emulate VBlank yet so to avoid any problems 
due to raising irq for unknown bits I restricted it for that now.

>> +static void ati_vga_vblank_irq(void *opaque)
>> +{
>> +    ATIVGAState *s = opaque;
>> +
>> +    timer_mod(&s->vblank_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>> +              NANOSECONDS_PER_SECOND / 60);
>> +    s->regs.gen_int_status |= 1;
>
> #defines for the irq status bits would be nice.

Yes, I thought about that but this was only for quick testing. I'll add 
constant for this in next version.

>> +    case GEN_INT_CNTL:
>> +        s->regs.gen_int_cntl = data;
>> +        if (data & 1) {
>> +            ati_vga_vblank_irq(s);
>> +        } else {
>> +            timer_del(&s->vblank_timer);
>> +        }
>
> ati_vga_update_irq() needed here.
>
>> +        break;
>> +    case GEN_INT_STATUS:
>> +        data &= (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF ?
>> +                 0x000f040fUL : 0xfc080effUL);
>
> Add IRQ_MASK #define ?

I'd leave these as constants because there are many of them (basically 
reserved bit mask for regs where we care or in this case writable bits) 
and one has to check docs to verify them and also in some cases we combine 
rage128p and rv100 so hiding them behind a constant would just make code 
less readable in my opinion. (This would become 3 lines for example with 
defines you'd have to look up in a different header so it's easier to see 
this way.)

>> +        s->regs.gen_int_status &= ~data;
>
> ati_vga_update_irq() needed here too.

Thanks. Indeed I forgot this. With that it works a bit better, mouse now 
can be moved but only vertically... No idea why, I'll have to check,

Regards,
BALATON Zoltan

Re: [Qemu-devel] [RFC PATCH] ati-vga: Implement dummy VBlank IRQ
Posted by Gerd Hoffmann 4 years, 8 months ago
  Hi,

> > > +static void ati_vga_update_irq(ATIVGAState *s)
> > > +{
> > > +    pci_set_irq(&s->dev, s->regs.gen_int_status & 1);
> > 
> > This should be "s->regs.gen_int_status & s->regs.gen_int_cntl" I guess?
> 
> Probably, but we only try to emulate VBlank yet so to avoid any problems due
> to raising irq for unknown bits I restricted it for that now.

Well, qemu doesn't set unknown status bits, only vblank.  The guest
can't set them either due to status register having write-one-to-clear
semantics.  So, that should not happen.  If you want an extra check to
catch programming errors I'd suggest to add an assert() for that.

> > > +        s->regs.gen_int_status &= ~data;
> > 
> > ati_vga_update_irq() needed here too.
> 
> Thanks. Indeed I forgot this. With that it works a bit better, mouse now can
> be moved but only vertically... No idea why, I'll have to check,

Still progress.  One step at a time ;)

cheers,
  Gerd


Re: [Qemu-devel] [RFC PATCH] ati-vga: Implement dummy VBlank IRQ
Posted by BALATON Zoltan 4 years, 8 months ago
On Thu, 15 Aug 2019, Gerd Hoffmann wrote:
>>>> +static void ati_vga_update_irq(ATIVGAState *s)
>>>> +{
>>>> +    pci_set_irq(&s->dev, s->regs.gen_int_status & 1);
>>>
>>> This should be "s->regs.gen_int_status & s->regs.gen_int_cntl" I guess?
>>
>> Probably, but we only try to emulate VBlank yet so to avoid any problems due
>> to raising irq for unknown bits I restricted it for that now.
>
> Well, qemu doesn't set unknown status bits, only vblank.  The guest
> can't set them either due to status register having write-one-to-clear
> semantics.  So, that should not happen.  If you want an extra check to
> catch programming errors I'd suggest to add an assert() for that.

OK I'll change that then.

>>>> +        s->regs.gen_int_status &= ~data;
>>>
>>> ati_vga_update_irq() needed here too.
>>
>> Thanks. Indeed I forgot this. With that it works a bit better, mouse now can
>> be moved but only vertically... No idea why, I'll have to check,
>
> Still progress.  One step at a time ;)

Got this too (and cursor color as well). These are becuase MacOS accesses 
these regs with less than 4 size so we need to support unaligned access 
for them. It's a bit tricky because we could get reg content in pieces and 
we need to decide when it's complete to act on it. I'll try to fix that 
and then it hopefully will work (well, at least boot to see it fail 
whenever tries to use any unimplemented acceleration but at least we can 
test emulation with it when further pieces are implemented in the future).

Regards,
BALATON Zoltan