Re: [Qemu-devel] [PATCH v5 2/2] ati-vga: Implement DDC and EDID info from monitor

[Corey Minyard]

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 <address@hidden>

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
>