Hello,
On Tue, 12 Feb 2019, Philippe Mathieu-Daudé wrote:
Hi Zoltan,
Thanks for the quick review and testing. I'll use your suggestions for the
other (mips) patches in a v2. For this one I'm not convinced.
On 2/11/19 4:19 AM, BALATON Zoltan wrote:
[...]
+
+static void ati_reg_write_offs(uint32_t *reg, int offs,
+ uint64_t data, unsigned int size)
+{
+ int shift, i;
+ uint32_t mask;
+
+ for (i = 0; i < size; i++) {
+ shift = (offs + i) * 8;
+ mask = 0xffUL << shift;
+ *reg &= ~mask;
+ *reg |= (data & 0xff) << shift;
+ data >>= 8;
I'd have use a pair of extract32/deposit32 but this is probably easier
to singlestep.
You've told me that before but I have concerns about the asserts in those
functions which to me seem like unnecessary overhead in such low level
functions so unless these are removed or *_noassert versions introduced
I'll stay away from them.
But I'm also not too happy about these *_offs functions but some registers
support 8/16/32 bit access and guest code seems to actually do this to
update bits in the middle of the register at an odd address. Best would be
if I could just set .impl.min = 4, .impl.max = 4 and .valid.min = 1
.valid.max = 4 for the mem region ops but I'm not sure that would work or
would it? If that's working maybe I should just go with that instead.
[...]
diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
new file mode 100644
index 0000000000..85d045517c
--- /dev/null
+++ b/hw/display/ati_int.h
@@ -0,0 +1,67 @@
+/*
+ * QEMU ATI SVGA emulation
+ *
+ * Copyright (c) 2019 BALATON Zoltan
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/pci/pci.h"
+#include "vga_int.h"
+
+#undef DEBUG_ATI
+
+#ifdef DEBUG_ATI
+#define DPRINTF(fmt, ...) printf("%s: " fmt, __func__, ## __VA_ARGS__)
+#else
+#define DPRINTF(fmt, ...) do {} while (0)
Please use tracepoints (you already add some!).
I won't and here's why: This is not a finished device model and I expect
to need to add debug logs and change them frequently during further
development and for such ad-hoc debugging DPRINF is still easier to use
because I don't have to define the format string at one file and use them
somewhere else. With DPRINTF I can just add a debug log at one place and
change it easily without editing it at two unrelated places so it's easier
to work with. Once development is finished those that we intend to leave
in for later tracing can be converted to trace points (for which trace
point is better) and at that point remove the DPRINTF macro. We still have
enough DPRINTFs in QEMU so this should be OK. I've already added trace
points to two such places but even for those I almost considered ditching
them when checkpatch insisted I have to add 0x prefix to hex numbers (I
don't like this because I know these are hex and printing e.g. 0x8 instead
of 8 is just distracting from the actual important value which is what
counts when I'm looking at a lot of these during debugging. Anything that
distracts from actual values and makes it harder to read (such as
timestamps and pids added by trace) is bad so I've considered going back
to DPRINTF even for those trace points but will see if I can live with
these for now.) But those that are still DPRINTFs won't be converted to
trace but supposed to be removed when no longer needed.
[...]
I don't understand well the display code, but the result works very
well, nice work :)
Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com>
Thanks, it's a start and currently only targeting Linux console with a lot
more to do for it to be more useful. But I have limited time for this so
since it's already useful to get mips_fulong2e working I thought that
justifies including it now so others have a chance to look at it and maybe
even help to improve it which can't happen if it's only sitting on my
machine.
Regards,
BALATON Zoltan