| Message ID | 20190314084924.8854-1-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ati-vga: i2c fix | expand |
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) { >
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
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
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
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
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
> 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
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
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
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
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
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) {