Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[Mark Cave-Ayland]

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-ayland@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?


ATB,

Mark.