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
