Re: [Qemu-devel] [PATCH v4] hw/display: Add basic ATI VGA emulation

[BALATON Zoltan]

On Sun, 3 Mar 2019, Philippe Mathieu-Daudé wrote:
> Hi Zoltan,
>
> On 3/3/19 12:34 AM, BALATON Zoltan wrote:
> > At least two machines, the PPC mac99 and MIPS fulong2e, have an ATI
> > gfx chip by default (Rage 128 Pro and M6/RV100 respectively) and
> > guests running on these and the PMON2000 firmware of the fulong2e
> > expect this to be available. Fortunately these are very similar chips
> > so they can be mostly emulated in the same device model. This patch
> > adds basic emulation of these ATI VGA chips.
> >
> > While this is incomplete and currently only enough to run the MIPS
> > firmware and get framebuffer output with Linux, it allows the fulong2e
> > board to work more like the real hardware and having it in QEMU in
> > this state provides a way to experiment with it and allows others to
> > contribute to improve it. It is compiled for all archs but only the
> > fulong2e (which currently has no display output at all) is set to use
> > it by default (in a patch sent separately).
>
> Patch looks good, trivial comment inlined.

Hello,
Thanks for yhe review. I took what I liked, replies for the rest below.

> >  default-configs/pci.mak  |   1 +
> >  hw/display/Makefile.objs |   2 +
> >  hw/display/ati.c         | 686 +++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/display/ati_2d.c      | 134 +++++++++
> >  hw/display/ati_dbg.c     | 254 ++++++++++++++++++
> >  hw/display/ati_int.h     |  87 ++++++
> >  hw/display/ati_regs.h    | 456 +++++++++++++++++++++++++++++++
>
> Please have a look at scripts/git.orderfile :)

Actually in this case I think that would make it less readable by putting 
headers with a lot of defines before actual code.

> > +                qemu_log_mask(LOG_UNIMP, "Unsupported bpp value");
>
> qemu_log_mask() requires trailing '\n' :(

Yes, it's hard to remember which of these QEMU functions need '\n' and 
which don't. Fixed.

> > +static inline uint64_t ati_reg_read_offs(uint32_t reg, int offs,
> > +                                         unsigned int size)
>
> I doubt inlining help here.

Should not hurt either and it shows this is supposed to be a light helper 
function that's only split off for readability. I could also turn it to a 
macro with a ( ? : ) conditional expression now but it's probably more 
readable this way.

> > +    case RBBM_STATUS:
>
>           /* fall through */
>
> > +    case GUI_STAT:
> > +        val = 64; /* free CMDFIFO entries */
> > +        break;

Obviously fall through because there are no statements between case 
labels. I only use /* fall thorugh */ comment where there are some 
statements and a missing break but not in this case which should be clear 
without comment.

> > +    default:
>
>           qemu_log(UNIMP)?
>
> > +        break;
> > +    }
> > +    trace_ati_mm_read(size, addr, ati_reg_name(addr & ~3ULL), val);

No log yet as most of the registers are currently unimplemented so that 
would generate too much logs. The trace should log all register accesses 
including unimplemented ones, that's enough currently.

> > +    if (s->dev_id == PCI_DEVICE_ID_ATI_RADEON_QY &&
> > +        s->vga.vram_size_mb < 16) {
> > +        warn_report("Too small video memory for device id");
> > +        s->vga.vram_size_mb = 16;
>
> I'd rather use error_setg() and return.

Correcting the error if possible is friendlier IMO but this can be debated 
what's the preferred behaviour. I'll leave it as it is for now.

> > diff --git a/hw/display/ati_dbg.c b/hw/display/ati_dbg.c
> > new file mode 100644
> > index 0000000000..5b6fdb299d
> > --- /dev/null
> > +++ b/hw/display/ati_dbg.c
> > @@ -0,0 +1,254 @@
> > +#include "ati_int.h"
> > +
> > +#ifdef DEBUG_ATI
> > +struct ati_regdesc {
> > +    const char *name;
> > +    int num;
>
> uint16_t?

uint16_t would be enough but int should be simpler for current CPUs and 
won't need possible padding to struct so I did not bother with restricting 
type here.

> I'd have inverted the structure member for nicer align ;)

It was easier to generate it with a simple sed command from ati_regs.h and 
update when new regs are added this way. I guess reversing the values 
could be done the same way. Maybe I should automate this so it will be 
updated automatically when new reg definitions are added but for now I 
left it to do by hand.

> > +};
> > +
> > +static struct ati_regdesc ati_reg_names[] = {
>
> static 'const' struct ...
>
> > +    {"MM_INDEX", 0x0000},
[...]
> > +    {"RAGE128_MPP_TB_CONFIG", 0x01c0},
> > +    {NULL, -1}
> > +};
> > +
> > +const char *ati_reg_name(int num)
>
> I'd use reg_addr instead of num.
>
> > +{
> > +    int i;
> > +
> > +    num &= ~3;
> > +    for (i = 0; ati_reg_names[i].name; i++) {
>
> Well since it is const you can iterate until ARRAY_SIZE(ati_reg_names)
> and drop the {NULL, -1} entry.

I could do any of the above but all this file is temporary for development 
and meant to be removed once enough functions of the device are 
implemented so for now I don't bother unless there's a strong argument 
other than style preference to change it.

> > diff --git a/hw/display/trace-events b/hw/display/trace-events
> > index 37d3264bb2..6aed33eeaa 100644
> > --- a/hw/display/trace-events
> > +++ b/hw/display/trace-events
> > @@ -138,3 +138,7 @@ vga_cirrus_write_blt(uint32_t offset, uint32_t val) "offset 
> > 0x%x, val 0x%x"
> >  sii9022_read_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val 0x%02x"
> >  sii9022_write_reg(uint8_t addr, uint8_t val) "addr 0x%02x, val 0x%02x"
> >  sii9022_switch_mode(const char *mode) "mode: %s"
> > +
> > +# hw/display/ati*.c
> > +ati_mm_read(unsigned int size, uint64_t addr, const char *name, uint64_t val) "%d 
> > 0x%"HWADDR_PRIx " %s -> 0x%"PRIx64
> > +ati_mm_write(unsigned int size, uint64_t addr, const char *name, uint64_t val) "%d 
> > 0x%"HWADDR_PRIx " %s <- 0x%"PRIx64
>
> "%u ...

Should not matter as size that can only be 1 to 4 but %u is more correct 
to print unsigned variable so I'll change this.

Thanks,
BALATON Zoltan