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

[BALATON Zoltan]

On Mon, 11 Mar 2019, BALATON Zoltan wrote:
> On Mon, 11 Mar 2019, Gerd Hoffmann wrote:
>> On Sun, Mar 10, 2019 at 12:22:17AM +0100, BALATON Zoltan wrote:
>>> This adds DDC support to ati-vga and connects i2c-ddc to provide EDID
>>> info that is read by guests to find available screen modes. Not sure
>>> if this is 100% correct yet but at least MorphOS is happy with it and
>>> starts in a high resolution mode instead of 640x480 (although its
>>> splash screen is still not correct).
>> 
>>> Linux needs support from VESA
>>> vgabios, it seems to be 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.
>> 
>> radeonfb doesn't pick up the edid either.
>> There seem to be four i2c busses though:
>> 
>> void radeon_create_i2c_busses(struct radeonfb_info *rinfo)
>> {
>>      rinfo->i2c[0].rinfo     = rinfo;
>>      rinfo->i2c[0].ddc_reg   = GPIO_MONID;
>> #ifndef CONFIG_PPC
>>      rinfo->i2c[0].adapter.class = I2C_CLASS_HWMON;
>> #endif
>>      radeon_setup_i2c_bus(&rinfo->i2c[0], "monid");
>>
>>      rinfo->i2c[1].rinfo     = rinfo;
>>      rinfo->i2c[1].ddc_reg   = GPIO_DVI_DDC;
>>      radeon_setup_i2c_bus(&rinfo->i2c[1], "dvi");
>>
>>      rinfo->i2c[2].rinfo     = rinfo;
>>      rinfo->i2c[2].ddc_reg   = GPIO_VGA_DDC;
>>      radeon_setup_i2c_bus(&rinfo->i2c[2], "vga");
>>
>>      rinfo->i2c[3].rinfo     = rinfo;
>>      rinfo->i2c[3].ddc_reg   = GPIO_CRT2_DDC;
>>      radeon_setup_i2c_bus(&rinfo->i2c[3], "crt2");
>> }
>> 
>> aty128fb has no i2c support.
>
> On rage128p I think there's only GPIO_MONID where there are 4 pins that can 
> have 2 i2c busses. I think they are DDC for CRT and flat-panel ports but not 
> sure about that. So what it's missing is i2c for hw monitoring which the 
> GPIO_MONID may be used for on radeon according to the above.
>
> The r128 xorg driver complains about missing support in VESA VBE as quoted 
> above and unloads itself. I think it may work if that support is implemented 
> in vgabios which I hope you can look at as I have no experience with vgabios.
>
> For radeon the DDC may need to be added on the GPIO_VGA_PORT as well based on 
> the above code excerpt but I haven't checked that.

I was thinking about something like the patch below but it does not seem 
to change anything for me. Does it work for you with radeonfb? Maybe it 
does not get to reading the EDID as it's missing something before that?

Regards,
BALATON Zoltan

-- >8 --

diff --git a/hw/display/ati.c b/hw/display/ati.c
index 260cd803d8..e2efc6f222 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -269,6 +269,9 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, 
unsigned int size)
     case DAC_CNTL:
         val = s->regs.dac_cntl;
         break;
+    case GPIO_VGA_DDC:
+        val = s->regs.gpio_vga_ddc;
+        break;
     case GPIO_MONID:
         val = s->regs.gpio_monid;
         break;
@@ -505,7 +508,24 @@ 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_VGA_DDC:
+        if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
+            break;
+        }
+        s->regs.gpio_vga_ddc = data & 0xf000f;
+        if (data & BIT(17)) {
+            s->regs.gpio_monid |= !!(data & BIT(1)) << 9;
+            bitbang_i2c_set(s->bbi2c, BITBANG_I2C_SCL, (data & BIT(1)) != 0);
+        }
+        if (data & BIT(16)) {
+            s->regs.gpio_monid |= bitbang_i2c_set(s->bbi2c, BITBANG_I2C_SDA,
+                                                  data & BIT(0)) << 8;
+        }
+        break;
     case GPIO_MONID:
+        if (s->dev_id != PCI_DEVICE_ID_ATI_RAGE128_PF) {
+            break; /* FIXME What does Radeon have here? */
+        }
         s->regs.gpio_monid = data & 0x0f0f000f;
         if (data & BIT(2) << 24) {
             s->regs.gpio_monid |= !!(data & BIT(2)) << 10;
diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
index 8df00efd93..5cf1f3269e 100644
--- a/hw/display/ati_int.h
+++ b/hw/display/ati_int.h
@@ -37,6 +37,7 @@ 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_monid;
     uint32_t crtc_h_total_disp;
     uint32_t crtc_h_sync_strt_wid;
diff --git a/hw/display/ati_regs.h b/hw/display/ati_regs.h
index 923bfd33ce..90384c886e 100644
--- a/hw/display/ati_regs.h
+++ b/hw/display/ati_regs.h
@@ -37,6 +37,7 @@
 #define CRTC_GEN_CNTL                           0x0050
 #define CRTC_EXT_CNTL                           0x0054
 #define DAC_CNTL                                0x0058
+#define GPIO_VGA_DDC                            0x0060
 #define GPIO_MONID                              0x0068
 #define I2C_CNTL_1                              0x0094
 #define PALETTE_INDEX                           0x00b0