diff mbox series

[v5,2/2] ati-vga: Implement DDC and EDID info from monitor

Message ID 046ddebb7ec8db48c4e877ee444ec1c41e385a74.1561028123.git.balaton@eik.bme.hu (mailing list archive)
State New, archived
Headers show
Series ati-vga: Implement DDC and EDID info from monitor | expand

Commit Message

BALATON Zoltan June 20, 2019, 10:55 a.m. UTC
This adds DDC support to ati-vga and connects i2c-ddc to it. This
allows at least MacOS with an ATI ndrv, Linux radeonfb and MorphOS to
get monitor EDID info (although MorphOS splash screen is not displayed
and radeonfb needs additional tables from vgabios-rv100). Xorg needs
additional support from VESA vgabios, it's missing INT10 0x4F15
function (see
https://gitlab.freedesktop.org/xorg/xserver/blob/master/hw/xfree86/vbe/vbe.c)
without which no DDC is available that also prevents loading the
accelerated X driver.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/display/Kconfig    |  2 ++
 hw/display/ati.c      | 60 +++++++++++++++++++++++++++++++++++++++++++++++++--
 hw/display/ati_int.h  |  5 +++++
 hw/display/ati_regs.h |  2 ++
 4 files changed, 67 insertions(+), 2 deletions(-)

Comments

Gerd Hoffmann June 20, 2019, 3:09 p.m. UTC | #1
On Thu, Jun 20, 2019 at 12:55:23PM +0200, BALATON Zoltan wrote:
> This adds DDC support to ati-vga and connects i2c-ddc to it. This
> allows at least MacOS with an ATI ndrv, Linux radeonfb and MorphOS to

linux radeonfb is rv100 only, and aty128fb has no i2c support.
Do MacOS and MorphOS have working edid with both card variants?

> +    case GPIO_MONID ... GPIO_MONID + 3:
> +        /* FIXME What does Radeon have here? */
> +        if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
> +            /* Rage128p accesses DDC used to get EDID on these pins */
> +            ati_reg_write_offs(&s->regs.gpio_monid,
> +                               addr - GPIO_MONID, data, size);
> +            if ((s->regs.gpio_monid & BIT(25)) &&

Extra enable bit, ok.

> +                addr <= GPIO_MONID + 2 && addr + size > GPIO_MONID + 2) {

Hmm, isn't this just "addr == GPIO_MONID + 2" ?

> +                s->regs.gpio_monid = ati_i2c(s->bbi2c, s->regs.gpio_monid, 1);

So all i2c bits are shifted by one compared to rv100, correct?

cheers,
  Gerd
BALATON Zoltan June 20, 2019, 3:40 p.m. UTC | #2
On Thu, 20 Jun 2019, Gerd Hoffmann wrote:
> On Thu, Jun 20, 2019 at 12:55:23PM +0200, BALATON Zoltan wrote:
>> This adds DDC support to ati-vga and connects i2c-ddc to it. This
>> allows at least MacOS with an ATI ndrv, Linux radeonfb and MorphOS to
>
> linux radeonfb is rv100 only, and aty128fb has no i2c support.
> Do MacOS and MorphOS have working edid with both card variants?

I've only tested EDID with MacOS with an NDRV from an ATI card ROM and 
MorphOS on mac99. These could read EDID with this patch. Haven't tried 
RV100 as that's known to be very incomplete to work yet (probably needs at 
least command FIFO to do something). The rage128 Xorg driver might load 
but that wants to use VESA BIOS function mentioned in the commit message 
to read EDID so it does not work yet. If you can add that function to 
vesabios it might get further.

>> +    case GPIO_MONID ... GPIO_MONID + 3:
>> +        /* FIXME What does Radeon have here? */
>> +        if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
>> +            /* Rage128p accesses DDC used to get EDID on these pins */
>> +            ati_reg_write_offs(&s->regs.gpio_monid,
>> +                               addr - GPIO_MONID, data, size);
>> +            if ((s->regs.gpio_monid & BIT(25)) &&
>
> Extra enable bit, ok.

This bit is listed as mask bit in docs and clients set this to enable 
other bits. It could probably safely be ignored (does not seem to be 
present on RV100 only on older card) but checking it does not hurt either.

>> +                addr <= GPIO_MONID + 2 && addr + size > GPIO_MONID + 2) {
>
> Hmm, isn't this just "addr == GPIO_MONID + 2" ?

No because there could be all kinds of unalligned or multibyte access and 
we only want to trigger this when the byte with the enable bits are 
touched. (The MacOS NDRV accesses this 1 byte at a time so this is needed 
to avoid spurious i2c bit banging but other drivers write 4 bytes so then 
addr is not equal but covering above byte which is what this test allows.

>> +                s->regs.gpio_monid = ati_i2c(s->bbi2c, s->regs.gpio_monid, 1);
>
> So all i2c bits are shifted by one compared to rv100, correct?

They are in a different register and drivers I've tried poke bits shifted 
by one on R128P.

Regards,
BALATON Zoltan
Gerd Hoffmann June 20, 2019, 3:52 p.m. UTC | #3
Hi,

> > > +                addr <= GPIO_MONID + 2 && addr + size > GPIO_MONID + 2) {
> > 
> > Hmm, isn't this just "addr == GPIO_MONID + 2" ?
> 
> No because there could be all kinds of unalligned or multibyte access and we
> only want to trigger this when the byte with the enable bits are touched.
> (The MacOS NDRV accesses this 1 byte at a time so this is needed to avoid
> spurious i2c bit banging but other drivers write 4 bytes so then addr is not
> equal but covering above byte which is what this test allows.

Can you add a comment explaining this (no need to respin, incremental patch is fine)?

thanks,
  Gerd
Corey Minyard June 25, 2019, 12:56 p.m. UTC | #4
On Thu, Jun 20, 2019 at 12:55:23PM +0200, BALATON Zoltan wrote:
> This adds DDC support to ati-vga and connects i2c-ddc to it. This
> allows at least MacOS with an ATI ndrv, Linux radeonfb and MorphOS to
> get monitor EDID info (although MorphOS splash screen is not displayed
> and radeonfb needs additional tables from vgabios-rv100). Xorg needs
> additional support from VESA vgabios, it's missing INT10 0x4F15
> function (see
> https://gitlab.freedesktop.org/xorg/xserver/blob/master/hw/xfree86/vbe/vbe.c)
> without which no DDC is available that also prevents loading the
> accelerated X driver.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

This patch also looks good, and thanks to Gerd for reviewing.

I didn't see the followup documentation patch that Gerd suggested, did
I miss that?

Also, how would you like to handle these?

Thanks,

-corey

> ---
>  hw/display/Kconfig    |  2 ++
>  hw/display/ati.c      | 60 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  hw/display/ati_int.h  |  5 +++++
>  hw/display/ati_regs.h |  2 ++
>  4 files changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/Kconfig b/hw/display/Kconfig
> index 910dccb2f7..cbdf7b1a67 100644
> --- a/hw/display/Kconfig
> +++ b/hw/display/Kconfig
> @@ -130,3 +130,5 @@ config ATI_VGA
>      default y if PCI_DEVICES
>      depends on PCI
>      select VGA
> +    select BITBANG_I2C
> +    select DDC
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index 76595d9511..61e351a024 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -26,6 +26,7 @@
>  #include "qapi/error.h"
>  #include "hw/hw.h"
>  #include "ui/console.h"
> +#include "hw/display/i2c-ddc.h"
>  #include "trace.h"
>  
>  #define ATI_DEBUG_HW_CURSOR 0
> @@ -215,6 +216,24 @@ static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int scr_y)
>      }
>  }
>  
> +static uint64_t ati_i2c(bitbang_i2c_interface *i2c, uint64_t data, int base)
> +{
> +    bool c = (data & BIT(base + 17) ? !!(data & BIT(base + 1)) : 1);
> +    bool d = (data & BIT(base + 16) ? !!(data & BIT(base)) : 1);
> +
> +    bitbang_i2c_set(i2c, BITBANG_I2C_SCL, c);
> +    d = bitbang_i2c_set(i2c, BITBANG_I2C_SDA, d);
> +
> +    data &= ~0xf00ULL;
> +    if (c) {
> +        data |= BIT(base + 9);
> +    }
> +    if (d) {
> +        data |= BIT(base + 8);
> +    }
> +    return data;
> +}
> +
>  static inline uint64_t ati_reg_read_offs(uint32_t reg, int offs,
>                                           unsigned int size)
>  {
> @@ -266,7 +285,16 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
>      case DAC_CNTL:
>          val = s->regs.dac_cntl;
>          break;
> -/*    case GPIO_MONID: FIXME hook up DDC I2C here */
> +    case GPIO_VGA_DDC:
> +        val = s->regs.gpio_vga_ddc;
> +        break;
> +    case GPIO_DVI_DDC:
> +        val = s->regs.gpio_dvi_ddc;
> +        break;
> +    case GPIO_MONID ... GPIO_MONID + 3:
> +        val = ati_reg_read_offs(s->regs.gpio_monid,
> +                                addr - GPIO_MONID, size);
> +        break;
>      case PALETTE_INDEX:
>          /* FIXME unaligned access */
>          val = vga_ioport_read(&s->vga, VGA_PEL_IR) << 16;
> @@ -497,7 +525,28 @@ static void ati_mm_write(void *opaque, hwaddr addr,
>          s->regs.dac_cntl = data & 0xffffe3ff;
>          s->vga.dac_8bit = !!(data & DAC_8BIT_EN);
>          break;
> -/*    case GPIO_MONID: FIXME hook up DDC I2C here */
> +    case GPIO_VGA_DDC:
> +        if (s->dev_id != PCI_DEVICE_ID_ATI_RAGE128_PF) {
> +            /* FIXME: Maybe add a property to select VGA or DVI port? */
> +        }
> +        break;
> +    case GPIO_DVI_DDC:
> +        if (s->dev_id != PCI_DEVICE_ID_ATI_RAGE128_PF) {
> +            s->regs.gpio_dvi_ddc = ati_i2c(s->bbi2c, data, 0);
> +        }
> +        break;
> +    case GPIO_MONID ... GPIO_MONID + 3:
> +        /* FIXME What does Radeon have here? */
> +        if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
> +            /* Rage128p accesses DDC used to get EDID on these pins */
> +            ati_reg_write_offs(&s->regs.gpio_monid,
> +                               addr - GPIO_MONID, data, size);
> +            if ((s->regs.gpio_monid & BIT(25)) &&
> +                addr <= GPIO_MONID + 2 && addr + size > GPIO_MONID + 2) {
> +                s->regs.gpio_monid = ati_i2c(s->bbi2c, s->regs.gpio_monid, 1);
> +            }
> +        }
> +        break;
>      case PALETTE_INDEX ... PALETTE_INDEX + 3:
>          if (size == 4) {
>              vga_ioport_write(&s->vga, VGA_PEL_IR, (data >> 16) & 0xff);
> @@ -788,6 +837,12 @@ static void ati_vga_realize(PCIDevice *dev, Error **errp)
>          vga->cursor_draw_line = ati_cursor_draw_line;
>      }
>  
> +    /* ddc, edid */
> +    I2CBus *i2cbus = i2c_init_bus(DEVICE(s), "ati-vga.ddc");
> +    s->bbi2c = bitbang_i2c_init(i2cbus);
> +    I2CSlave *i2cddc = I2C_SLAVE(qdev_create(BUS(i2cbus), TYPE_I2CDDC));
> +    i2c_set_slave_address(i2cddc, 0x50);
> +
>      /* mmio register space */
>      memory_region_init_io(&s->mm, OBJECT(s), &ati_mm_ops, s,
>                            "ati.mmregs", 0x4000);
> @@ -813,6 +868,7 @@ static void ati_vga_exit(PCIDevice *dev)
>      ATIVGAState *s = ATI_VGA(dev);
>  
>      graphic_console_close(s->vga.con);
> +    g_free(s->bbi2c);
>  }
>  
>  static Property ati_vga_properties[] = {
> diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
> index 2f426064cf..51465f5630 100644
> --- a/hw/display/ati_int.h
> +++ b/hw/display/ati_int.h
> @@ -10,6 +10,7 @@
>  #define ATI_INT_H
>  
>  #include "hw/pci/pci.h"
> +#include "hw/i2c/bitbang_i2c.h"
>  #include "vga_int.h"
>  
>  /*#define DEBUG_ATI*/
> @@ -35,6 +36,9 @@ typedef struct ATIVGARegs {
>      uint32_t crtc_gen_cntl;
>      uint32_t crtc_ext_cntl;
>      uint32_t dac_cntl;
> +    uint32_t gpio_vga_ddc;
> +    uint32_t gpio_dvi_ddc;
> +    uint32_t gpio_monid;
>      uint32_t crtc_h_total_disp;
>      uint32_t crtc_h_sync_strt_wid;
>      uint32_t crtc_v_total_disp;
> @@ -83,6 +87,7 @@ typedef struct ATIVGAState {
>      uint16_t cursor_size;
>      uint32_t cursor_offset;
>      QEMUCursor *cursor;
> +    bitbang_i2c_interface *bbi2c;
>      MemoryRegion io;
>      MemoryRegion mm;
>      ATIVGARegs regs;
> diff --git a/hw/display/ati_regs.h b/hw/display/ati_regs.h
> index 923bfd33ce..1ec3498b73 100644
> --- a/hw/display/ati_regs.h
> +++ b/hw/display/ati_regs.h
> @@ -37,6 +37,8 @@
>  #define CRTC_GEN_CNTL                           0x0050
>  #define CRTC_EXT_CNTL                           0x0054
>  #define DAC_CNTL                                0x0058
> +#define GPIO_VGA_DDC                            0x0060
> +#define GPIO_DVI_DDC                            0x0064
>  #define GPIO_MONID                              0x0068
>  #define I2C_CNTL_1                              0x0094
>  #define PALETTE_INDEX                           0x00b0
> -- 
> 2.13.7
>
BALATON Zoltan June 25, 2019, 1:20 p.m. UTC | #5
On Tue, 25 Jun 2019, Corey Minyard wrote:
> On Thu, Jun 20, 2019 at 12:55:23PM +0200, BALATON Zoltan wrote:
>> This adds DDC support to ati-vga and connects i2c-ddc to it. This
>> allows at least MacOS with an ATI ndrv, Linux radeonfb and MorphOS to
>> get monitor EDID info (although MorphOS splash screen is not displayed
>> and radeonfb needs additional tables from vgabios-rv100). Xorg needs
>> additional support from VESA vgabios, it's missing INT10 0x4F15
>> function (see
>> https://gitlab.freedesktop.org/xorg/xserver/blob/master/hw/xfree86/vbe/vbe.c)
>> without which no DDC is available that also prevents loading the
>> accelerated X driver.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> This patch also looks good, and thanks to Gerd for reviewing.
>
> I didn't see the followup documentation patch that Gerd suggested, did
> I miss that?
>
> Also, how would you like to handle these?

Gerd has already queued them (including the requested follow up to update 
comment which I think only cc'd to Gerd that's why you may have missed it, 
but another fix up I've sent adding reg names to ati_dbg.h may still be 
missing from Gerd's tree). I think Gerd can send these via his tree with 
your Acked-by on the i2c patch which should be OK if nobody disagrees.

There's still the issue with cleaning up bitbang_i2c that results in an 
ASan warning which we have discussed before:
https://lists.nongnu.org/archive/html/qemu-devel/2019-03/msg05966.html
but we could not find an easy fix so since it's preexisting problem it can 
be fixed later. Although, I don't have plans to fix that, only mentioning 
it here as a reminder.

Thank you,
BALATON Zoltan
diff mbox series

Patch

diff --git a/hw/display/Kconfig b/hw/display/Kconfig
index 910dccb2f7..cbdf7b1a67 100644
--- a/hw/display/Kconfig
+++ b/hw/display/Kconfig
@@ -130,3 +130,5 @@  config ATI_VGA
     default y if PCI_DEVICES
     depends on PCI
     select VGA
+    select BITBANG_I2C
+    select DDC
diff --git a/hw/display/ati.c b/hw/display/ati.c
index 76595d9511..61e351a024 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -26,6 +26,7 @@ 
 #include "qapi/error.h"
 #include "hw/hw.h"
 #include "ui/console.h"
+#include "hw/display/i2c-ddc.h"
 #include "trace.h"
 
 #define ATI_DEBUG_HW_CURSOR 0
@@ -215,6 +216,24 @@  static void ati_cursor_draw_line(VGACommonState *vga, uint8_t *d, int scr_y)
     }
 }
 
+static uint64_t ati_i2c(bitbang_i2c_interface *i2c, uint64_t data, int base)
+{
+    bool c = (data & BIT(base + 17) ? !!(data & BIT(base + 1)) : 1);
+    bool d = (data & BIT(base + 16) ? !!(data & BIT(base)) : 1);
+
+    bitbang_i2c_set(i2c, BITBANG_I2C_SCL, c);
+    d = bitbang_i2c_set(i2c, BITBANG_I2C_SDA, d);
+
+    data &= ~0xf00ULL;
+    if (c) {
+        data |= BIT(base + 9);
+    }
+    if (d) {
+        data |= BIT(base + 8);
+    }
+    return data;
+}
+
 static inline uint64_t ati_reg_read_offs(uint32_t reg, int offs,
                                          unsigned int size)
 {
@@ -266,7 +285,16 @@  static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
     case DAC_CNTL:
         val = s->regs.dac_cntl;
         break;
-/*    case GPIO_MONID: FIXME hook up DDC I2C here */
+    case GPIO_VGA_DDC:
+        val = s->regs.gpio_vga_ddc;
+        break;
+    case GPIO_DVI_DDC:
+        val = s->regs.gpio_dvi_ddc;
+        break;
+    case GPIO_MONID ... GPIO_MONID + 3:
+        val = ati_reg_read_offs(s->regs.gpio_monid,
+                                addr - GPIO_MONID, size);
+        break;
     case PALETTE_INDEX:
         /* FIXME unaligned access */
         val = vga_ioport_read(&s->vga, VGA_PEL_IR) << 16;
@@ -497,7 +525,28 @@  static void ati_mm_write(void *opaque, hwaddr addr,
         s->regs.dac_cntl = data & 0xffffe3ff;
         s->vga.dac_8bit = !!(data & DAC_8BIT_EN);
         break;
-/*    case GPIO_MONID: FIXME hook up DDC I2C here */
+    case GPIO_VGA_DDC:
+        if (s->dev_id != PCI_DEVICE_ID_ATI_RAGE128_PF) {
+            /* FIXME: Maybe add a property to select VGA or DVI port? */
+        }
+        break;
+    case GPIO_DVI_DDC:
+        if (s->dev_id != PCI_DEVICE_ID_ATI_RAGE128_PF) {
+            s->regs.gpio_dvi_ddc = ati_i2c(s->bbi2c, data, 0);
+        }
+        break;
+    case GPIO_MONID ... GPIO_MONID + 3:
+        /* FIXME What does Radeon have here? */
+        if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
+            /* Rage128p accesses DDC used to get EDID on these pins */
+            ati_reg_write_offs(&s->regs.gpio_monid,
+                               addr - GPIO_MONID, data, size);
+            if ((s->regs.gpio_monid & BIT(25)) &&
+                addr <= GPIO_MONID + 2 && addr + size > GPIO_MONID + 2) {
+                s->regs.gpio_monid = ati_i2c(s->bbi2c, s->regs.gpio_monid, 1);
+            }
+        }
+        break;
     case PALETTE_INDEX ... PALETTE_INDEX + 3:
         if (size == 4) {
             vga_ioport_write(&s->vga, VGA_PEL_IR, (data >> 16) & 0xff);
@@ -788,6 +837,12 @@  static void ati_vga_realize(PCIDevice *dev, Error **errp)
         vga->cursor_draw_line = ati_cursor_draw_line;
     }
 
+    /* ddc, edid */
+    I2CBus *i2cbus = i2c_init_bus(DEVICE(s), "ati-vga.ddc");
+    s->bbi2c = bitbang_i2c_init(i2cbus);
+    I2CSlave *i2cddc = I2C_SLAVE(qdev_create(BUS(i2cbus), TYPE_I2CDDC));
+    i2c_set_slave_address(i2cddc, 0x50);
+
     /* mmio register space */
     memory_region_init_io(&s->mm, OBJECT(s), &ati_mm_ops, s,
                           "ati.mmregs", 0x4000);
@@ -813,6 +868,7 @@  static void ati_vga_exit(PCIDevice *dev)
     ATIVGAState *s = ATI_VGA(dev);
 
     graphic_console_close(s->vga.con);
+    g_free(s->bbi2c);
 }
 
 static Property ati_vga_properties[] = {
diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
index 2f426064cf..51465f5630 100644
--- a/hw/display/ati_int.h
+++ b/hw/display/ati_int.h
@@ -10,6 +10,7 @@ 
 #define ATI_INT_H
 
 #include "hw/pci/pci.h"
+#include "hw/i2c/bitbang_i2c.h"
 #include "vga_int.h"
 
 /*#define DEBUG_ATI*/
@@ -35,6 +36,9 @@  typedef struct ATIVGARegs {
     uint32_t crtc_gen_cntl;
     uint32_t crtc_ext_cntl;
     uint32_t dac_cntl;
+    uint32_t gpio_vga_ddc;
+    uint32_t gpio_dvi_ddc;
+    uint32_t gpio_monid;
     uint32_t crtc_h_total_disp;
     uint32_t crtc_h_sync_strt_wid;
     uint32_t crtc_v_total_disp;
@@ -83,6 +87,7 @@  typedef struct ATIVGAState {
     uint16_t cursor_size;
     uint32_t cursor_offset;
     QEMUCursor *cursor;
+    bitbang_i2c_interface *bbi2c;
     MemoryRegion io;
     MemoryRegion mm;
     ATIVGARegs regs;
diff --git a/hw/display/ati_regs.h b/hw/display/ati_regs.h
index 923bfd33ce..1ec3498b73 100644
--- a/hw/display/ati_regs.h
+++ b/hw/display/ati_regs.h
@@ -37,6 +37,8 @@ 
 #define CRTC_GEN_CNTL                           0x0050
 #define CRTC_EXT_CNTL                           0x0054
 #define DAC_CNTL                                0x0058
+#define GPIO_VGA_DDC                            0x0060
+#define GPIO_DVI_DDC                            0x0064
 #define GPIO_MONID                              0x0068
 #define I2C_CNTL_1                              0x0094
 #define PALETTE_INDEX                           0x00b0