[Qemu-devel] [PATCH] ati-vga: i2c fix

Gerd Hoffmann posted 1 patch 9 weeks ago
Failed in applying to current master (apply log)
hw/display/ati_int.h  |  1 +
hw/display/ati_regs.h |  1 +
hw/display/ati.c      | 35 +++++++++++++++++++++++++++--------
3 files changed, 29 insertions(+), 8 deletions(-)

[Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by Gerd Hoffmann 9 weeks ago
gets radeonfb going for me, on top of your i2c patches.
---
 hw/display/ati_int.h  |  1 +
 hw/display/ati_regs.h |  1 +
 hw/display/ati.c      | 35 +++++++++++++++++++++++++++--------
 3 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
index 3f4a06f1e1ed..7289db206cd2 100644
--- a/hw/display/ati_int.h
+++ b/hw/display/ati_int.h
@@ -38,6 +38,7 @@ typedef struct ATIVGARegs {
     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;
diff --git a/hw/display/ati_regs.h b/hw/display/ati_regs.h
index 90384c886ecb..1ec3498b731c 100644
--- a/hw/display/ati_regs.h
+++ b/hw/display/ati_regs.h
@@ -38,6 +38,7 @@
 #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
diff --git a/hw/display/ati.c b/hw/display/ati.c
index e2efc6f2225e..ffced39aad9c 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -272,6 +272,9 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
     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:
         val = s->regs.gpio_monid;
         break;
@@ -426,6 +429,22 @@ static inline void ati_reg_write_offs(uint32_t *reg, int offs,
     }
 }
 
+static uint64_t ati_i2c(bitbang_i2c_interface *i2c,
+                        uint64_t data)
+{
+    bool clk = !(data & BIT(17));
+    bool out = !(data & BIT(16));
+    bool in;
+
+    bitbang_i2c_set(i2c, BITBANG_I2C_SCL, clk);
+    in = bitbang_i2c_set(i2c, BITBANG_I2C_SDA, out);
+
+    data &= 0xf000f;
+    data |= clk ? BIT(9) : 0;
+    data |= in  ? BIT(8) : 0;
+    return data;
+}
+
 static void ati_mm_write(void *opaque, hwaddr addr,
                            uint64_t data, unsigned int size)
 {
@@ -512,15 +531,15 @@ static void ati_mm_write(void *opaque, hwaddr addr,
         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;
+#if 0
+        s->regs.gpio_vga_ddc = ati_i2c(s->bbi2c, data);
+#endif
+        break;
+    case GPIO_DVI_DDC:
+        if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
+            break;
         }
+        s->regs.gpio_dvi_ddc = ati_i2c(s->bbi2c, data);
         break;
     case GPIO_MONID:
         if (s->dev_id != PCI_DEVICE_ID_ATI_RAGE128_PF) {
-- 
2.18.1


Re: [Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by BALATON Zoltan 9 weeks ago
On Thu, 14 Mar 2019, Gerd Hoffmann wrote:
> gets radeonfb going for me, on top of your i2c patches.
> ---
> hw/display/ati_int.h  |  1 +
> hw/display/ati_regs.h |  1 +
> hw/display/ati.c      | 35 +++++++++++++++++++++++++++--------
> 3 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
> index 3f4a06f1e1ed..7289db206cd2 100644
> --- a/hw/display/ati_int.h
> +++ b/hw/display/ati_int.h
> @@ -38,6 +38,7 @@ typedef struct ATIVGARegs {
>     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;
> diff --git a/hw/display/ati_regs.h b/hw/display/ati_regs.h
> index 90384c886ecb..1ec3498b731c 100644
> --- a/hw/display/ati_regs.h
> +++ b/hw/display/ati_regs.h
> @@ -38,6 +38,7 @@
> #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
> diff --git a/hw/display/ati.c b/hw/display/ati.c
> index e2efc6f2225e..ffced39aad9c 100644
> --- a/hw/display/ati.c
> +++ b/hw/display/ati.c
> @@ -272,6 +272,9 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
>     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:
>         val = s->regs.gpio_monid;
>         break;
> @@ -426,6 +429,22 @@ static inline void ati_reg_write_offs(uint32_t *reg, int offs,
>     }
> }
>
> +static uint64_t ati_i2c(bitbang_i2c_interface *i2c,
> +                        uint64_t data)
> +{
> +    bool clk = !(data & BIT(17));
> +    bool out = !(data & BIT(16));
> +    bool in;
> +
> +    bitbang_i2c_set(i2c, BITBANG_I2C_SCL, clk);
> +    in = bitbang_i2c_set(i2c, BITBANG_I2C_SDA, out);
> +
> +    data &= 0xf000f;
> +    data |= clk ? BIT(9) : 0;
> +    data |= in  ? BIT(8) : 0;
> +    return data;
> +}
> +
> static void ati_mm_write(void *opaque, hwaddr addr,
>                            uint64_t data, unsigned int size)
> {
> @@ -512,15 +531,15 @@ static void ati_mm_write(void *opaque, hwaddr addr,
>         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;
> +#if 0
> +        s->regs.gpio_vga_ddc = ati_i2c(s->bbi2c, data);
> +#endif

Thanks, I'll try and merge this. What's this #if 0 line?

Regards,
BALATON Zoltan

> +        break;
> +    case GPIO_DVI_DDC:
> +        if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
> +            break;
>         }
> +        s->regs.gpio_dvi_ddc = ati_i2c(s->bbi2c, data);
>         break;
>     case GPIO_MONID:
>         if (s->dev_id != PCI_DEVICE_ID_ATI_RAGE128_PF) {
>

Re: [Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by Gerd Hoffmann 9 weeks ago
On Thu, Mar 14, 2019 at 08:11:21PM +0100, BALATON Zoltan wrote:
> On Thu, 14 Mar 2019, Gerd Hoffmann wrote:
> > gets radeonfb going for me, on top of your i2c patches.
> > ---
> > hw/display/ati_int.h  |  1 +
> > hw/display/ati_regs.h |  1 +
> > hw/display/ati.c      | 35 +++++++++++++++++++++++++++--------
> > 3 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
> > index 3f4a06f1e1ed..7289db206cd2 100644
> > --- a/hw/display/ati_int.h
> > +++ b/hw/display/ati_int.h
> > @@ -38,6 +38,7 @@ typedef struct ATIVGARegs {
> >     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;
> > diff --git a/hw/display/ati_regs.h b/hw/display/ati_regs.h
> > index 90384c886ecb..1ec3498b731c 100644
> > --- a/hw/display/ati_regs.h
> > +++ b/hw/display/ati_regs.h
> > @@ -38,6 +38,7 @@
> > #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
> > diff --git a/hw/display/ati.c b/hw/display/ati.c
> > index e2efc6f2225e..ffced39aad9c 100644
> > --- a/hw/display/ati.c
> > +++ b/hw/display/ati.c
> > @@ -272,6 +272,9 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, unsigned int size)
> >     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:
> >         val = s->regs.gpio_monid;
> >         break;
> > @@ -426,6 +429,22 @@ static inline void ati_reg_write_offs(uint32_t *reg, int offs,
> >     }
> > }
> > 
> > +static uint64_t ati_i2c(bitbang_i2c_interface *i2c,
> > +                        uint64_t data)
> > +{
> > +    bool clk = !(data & BIT(17));
> > +    bool out = !(data & BIT(16));
> > +    bool in;
> > +
> > +    bitbang_i2c_set(i2c, BITBANG_I2C_SCL, clk);
> > +    in = bitbang_i2c_set(i2c, BITBANG_I2C_SDA, out);
> > +
> > +    data &= 0xf000f;
> > +    data |= clk ? BIT(9) : 0;
> > +    data |= in  ? BIT(8) : 0;
> > +    return data;
> > +}
> > +
> > static void ati_mm_write(void *opaque, hwaddr addr,
> >                            uint64_t data, unsigned int size)
> > {
> > @@ -512,15 +531,15 @@ static void ati_mm_write(void *opaque, hwaddr addr,
> >         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;
> > +#if 0
> > +        s->regs.gpio_vga_ddc = ati_i2c(s->bbi2c, data);
> > +#endif
> 
> Thanks, I'll try and merge this. What's this #if 0 line?

Avoid the monitor show up on both vga ...

> 
> Regards,
> BALATON Zoltan
> 
> > +        break;
> > +    case GPIO_DVI_DDC:
> > +        if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
> > +            break;
> >         }
> > +        s->regs.gpio_dvi_ddc = ati_i2c(s->bbi2c, data);

... and dvi.

A more correct model would probably be to create two i2c busses for
that, then hook up the ddc to one of them (possibly depending on a
config option).

cheers,
  Gerd


Re: [Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by BALATON Zoltan 9 weeks ago
On Fri, 15 Mar 2019, Gerd Hoffmann wrote:
> On Thu, Mar 14, 2019 at 08:11:21PM +0100, BALATON Zoltan wrote:
>> On Thu, 14 Mar 2019, Gerd Hoffmann wrote:
>>> gets radeonfb going for me, on top of your i2c patches.
>>> ---
>>> @@ -512,15 +531,15 @@ static void ati_mm_write(void *opaque, hwaddr addr,
>>>         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;
>>> +#if 0
>>> +        s->regs.gpio_vga_ddc = ati_i2c(s->bbi2c, data);
>>> +#endif
>>
>> Thanks, I'll try and merge this. What's this #if 0 line?
>
> Avoid the monitor show up on both vga ...
>
>>
>> Regards,
>> BALATON Zoltan
>>
>>> +        break;
>>> +    case GPIO_DVI_DDC:
>>> +        if (s->dev_id == PCI_DEVICE_ID_ATI_RAGE128_PF) {
>>> +            break;
>>>         }
>>> +        s->regs.gpio_dvi_ddc = ati_i2c(s->bbi2c, data);
>
> ... and dvi.
>
> A more correct model would probably be to create two i2c busses for
> that, then hook up the ddc to one of them (possibly depending on a
> config option).

Isn't it enough to only emulate the DVI port DDC then? I've sent an 
updated patch as v2 that also cleans up the bitbang_i2c.h header 
inclusion. (I've checked that Linux first checks DVI then VGA so my 
original patch may have also worked if the copy paste error is fixed and 
updated the right reg bits instead of gpio_monid. But let's go with the 
default and use a DVI port, then we likely not need VGA as we don't have 
a mointor there.)

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by Gerd Hoffmann 9 weeks ago
  Hi,

> > A more correct model would probably be to create two i2c busses for
> > that, then hook up the ddc to one of them (possibly depending on a
> > config option).
> 
> Isn't it enough to only emulate the DVI port DDC then?

Well, strictly speaking the radion has 4 i2c busses and the most correct
way to emulate them is hooking up 4 busses.  But in practice it probably
doesn't matter much for the guest whenever we don't emulate the unused
busses or whenever we emulate a i2c bus with no devices connected ...

cheers,
  Gerd


Re: [Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by BALATON Zoltan 9 weeks ago
Hello,

On Mon, 18 Mar 2019, Gerd Hoffmann wrote:
>>> A more correct model would probably be to create two i2c busses for
>>> that, then hook up the ddc to one of them (possibly depending on a
>>> config option).
>>
>> Isn't it enough to only emulate the DVI port DDC then?
>
> Well, strictly speaking the radion has 4 i2c busses and the most correct
> way to emulate them is hooking up 4 busses.  But in practice it probably
> doesn't matter much for the guest whenever we don't emulate the unused
> busses or whenever we emulate a i2c bus with no devices connected ...

Right, emulating the ports and corresponding DDC as present but no monitor 
connected would be more correct. However we don't emulate the second 
display controller or flat panel and there may be cards without additional 
connectors so guests should cope with those not present. So unless this 
found to cause a problem for some guests I'd keep the code simple for now 
and not emulate empty ports. It's a complex enough device without that.

Does it work with the latest patch for you? I could not make Linux driver 
recognise the card yet so if you have some settings that's needed for this 
you could share those. It may also need changes to vgabios but I'm not 
familiar with that so I hope you can help with that. I've found the r128 X 
driver needs a VBE BIOS function to access DDC as mentioned in the commit 
message.

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by Gerd Hoffmann 9 weeks ago
> Does it work with the latest patch for you? I could not make Linux driver
> recognise the card yet so if you have some settings that's needed for this
> you could share those. It may also need changes to vgabios but I'm not
> familiar with that so I hope you can help with that. I've found the r128 X
> driver needs a VBE BIOS function to access DDC as mentioned in the commit
> message.

vgabios bits pushed to https://git.kraxel.org/cgit/qemu/log/?h=sirius/ati-vga

bios tables with pll info are there, so radeonfb works with that.

No edid/ddc support in vgabios yet,
radeonfb manages to read edid though.

HTH,
  Gerd


Re: [Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by Gerd Hoffmann 9 weeks ago
  Hi,

> Does it work with the latest patch for you?

No (testing with radeonfb.ko).

> you could share those. It may also need changes to vgabios but I'm not
> familiar with that so I hope you can help with that. I've found the r128 X
> driver needs a VBE BIOS function to access DDC as mentioned in the commit
> message.

Can look into that but it's not top priority.

cheers,
  Gerd


Re: [Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by BALATON Zoltan 8 weeks ago
On Mon, 18 Mar 2019, Gerd Hoffmann wrote:
>> Does it work with the latest patch for you?
>
> No (testing with radeonfb.ko).

I'm confused. You said here that it works with radeonfb with your patch:
http://lists.nongnu.org/archive/html/qemu-devel/2019-03/msg04745.html
and my patch should be the same but now you say it does not work. So can 
it access EDID or not?

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by Gerd Hoffmann 8 weeks ago
On Wed, Mar 20, 2019 at 12:32:42AM +0100, BALATON Zoltan wrote:
> On Mon, 18 Mar 2019, Gerd Hoffmann wrote:
> > > Does it work with the latest patch for you?
> > 
> > No (testing with radeonfb.ko).
> 
> I'm confused. You said here that it works with radeonfb with your patch:
> http://lists.nongnu.org/archive/html/qemu-devel/2019-03/msg04745.html
> and my patch should be the same but now you say it does not work. So can it
> access EDID or not?

It works with my patch but doesn't with yours.
Didn't debug why, but apparently the logic is not the same ...

cheers,
  Gerd


Re: [Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by BALATON Zoltan 8 weeks ago
On Wed, 20 Mar 2019, Gerd Hoffmann wrote:
> On Wed, Mar 20, 2019 at 12:32:42AM +0100, BALATON Zoltan wrote:
>> On Mon, 18 Mar 2019, Gerd Hoffmann wrote:
>>>> Does it work with the latest patch for you?
>>>
>>> No (testing with radeonfb.ko).
>>
>> I'm confused. You said here that it works with radeonfb with your patch:
>> http://lists.nongnu.org/archive/html/qemu-devel/2019-03/msg04745.html
>> and my patch should be the same but now you say it does not work. So can it
>> access EDID or not?
>
> It works with my patch but doesn't with yours.
> Didn't debug why, but apparently the logic is not the same ...

OK thanks, I'll check. What does it look like when you say it works? I've 
found radeonfb loaded with my patch too and it mentioned monitors 
connected but maybe you get something more?

Regards,
BALATON Zoltan

Re: [Qemu-devel] [PATCH] ati-vga: i2c fix

Posted by Gerd Hoffmann 8 weeks ago
On Wed, Mar 20, 2019 at 12:41:30PM +0100, BALATON Zoltan wrote:
> On Wed, 20 Mar 2019, Gerd Hoffmann wrote:
> > On Wed, Mar 20, 2019 at 12:32:42AM +0100, BALATON Zoltan wrote:
> > > On Mon, 18 Mar 2019, Gerd Hoffmann wrote:
> > > > > Does it work with the latest patch for you?
> > > > 
> > > > No (testing with radeonfb.ko).
> > > 
> > > I'm confused. You said here that it works with radeonfb with your patch:
> > > http://lists.nongnu.org/archive/html/qemu-devel/2019-03/msg04745.html
> > > and my patch should be the same but now you say it does not work. So can it
> > > access EDID or not?
> > 
> > It works with my patch but doesn't with yours.
> > Didn't debug why, but apparently the logic is not the same ...
> 
> OK thanks, I'll check. What does it look like when you say it works? I've
> found radeonfb loaded with my patch too and it mentioned monitors connected
> but maybe you get something more?

If it works it comes up with 1024x768 (taken from edid blob),
if it doesn't work it comes up with 640x480.

There is also a pr_debug() in the radeonfb source code which
you can enable (or change to pr_info()).

HTH,
  Gerd