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

[BALATON Zoltan]

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