On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
On 10/03/2021 13:24, Laurent Vivier wrote:
Le 10/03/2021 à 14:10, Laurent Vivier a écrit :
Le 10/03/2021 à 13:56, Laurent Vivier a écrit :
Le 10/03/2021 à 13:32, BALATON Zoltan a écrit :
On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
The 60Hz timer is initialised using timer_new_ns() meaning that the
timer
interval should be measured in ns, and therefore its period is a
thousand
times too short.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
---
hw/misc/mac_via.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index f994fefa7c..c6e1552a59 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -302,8 +302,8 @@ static void
via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
MOS6522State *s = MOS6522(v1s);
/* 60 Hz irq */
- v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
16630) /
- 16630 * 16630;
+ v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
16630000) /
+ 16630000 * 16630000;
Can you put this magic number in a #define maybe also rewriting it in a
way that shows it
corresponds to a 60 Hz interval. (There's NANOSECONDS_PER_SECOND defined
in include/qemu/timer.h
that could be used for that, there's also SCALE_MS that might replace
1000 * 1000 elsewhere in this
file). Also NANOSECONDS_PER_SECOND / 60 is 16666666, should that value
be used here instead?
In fact, the Mac Frequency is not exactly 60 Hz, in docs we can find
60.147 Hz, in kernel 60.15 Hz.
I Think there are several ways to compute it...
In fact, we can read:
"the vertical retrace frequency is approximately 60.15 Hz, resulting in a
period of approximately
16.63 milliseconds"
https://developer.apple.com/library/archive/documentation/mac/pdf/Processes/Vertical_Retrace_Mgr.pdf
The exact value is 16625800 ns
"Macintosh Family Hardware Reference" ISBN 0-201-19255-1
"The video interface"
p. 13-3
"[...] This means the full frame is redisplayed every 370 scan lines, or
once every 166625.8 µs."
Thanks Laurent! Given that the exact precision is 6 digits I don't think it's
possible to make use of conversion macros without either making it harder to
read or reducing the precision.
I think the best solution here would be to #define VIA1_60HZ_TIMER_PERIOD_NS
with a comment containing the above reference, and use that in the period
calculation. Would that be sufficient?
Yes, I think that would make this a lot clearer than having this number
without explanation so that would be sufficient.
Is this referred to as 60Hz timer in the hardware docs? Because that name
is a bit misleading when it's actually not exactly 60Hz. But in the
previous patch VBlank which this was called before was also found
misleading so I can't think of a better name. Not sure if calling it
compat_vblank would be too verbose or any better. Maybe you can just add a
comment explaining what this is somewhere where it's defined or near the
update function and then it does not matter what you call it, the comment
should explain why it's not quite sixty_hz. I'm also OK with calling it
sixty_hz just though if there's a way to give it a less confusing name you
could consider that but I don't have a better name either so feel free to
ignore this.
Regards,
BALATON Zoltan
