| Message ID | 20210310080908.11861-7-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | mac_via: fixes and improvements | expand |
Le 10/03/2021 à 09:09, Mark Cave-Ayland a écrit : > 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; > > if (s->ier & VIA1_IRQ_60HZ) { > timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz); > Reviewed-by: Laurent Vivier <laurent@vivier.Eu>
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? Regards, BALATON Zoltan > > if (s->ier & VIA1_IRQ_60HZ) { > timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz); >
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... Thanks, Laurent
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 Thanks, Laurent
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
On 3/10/21 2:10 PM, Laurent Vivier wrote: > 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... Good candidate to switch to the Clock API, see docs/devel/clocks.rst once this gets merged: https://www.mail-archive.com/qemu-devel@nongnu.org/msg788864.html > 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 > > Thanks, > Laurent > >
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.
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-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? 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
On 11/03/2021 00:15, BALATON Zoltan wrote: > 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-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? > > 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. Yes indeed, depending upon the documentation it is referred to as either the 60Hz or the 60.15Hz timer. Certainly that's enough information for anyone familiar with Mac internals to understand exactly what you are referring to. There are also plenty of examples of this elsewhere e.g. for graphics cards the high level claim will be that it supports over 16 million colours whereas the engineers know specifically that the exact number is 16777216. I'll write up a suitable comment with a #define and send through a v2, perhaps altering the comment on the timer itself to read 60.15Hz as that's what the reference cited by Laurent also uses. ATB, Mark.
Le 11/03/2021 à 10:04, Mark Cave-Ayland a écrit : > On 11/03/2021 00:15, BALATON Zoltan wrote: > >> 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-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? >> >> 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. > > Yes indeed, depending upon the documentation it is referred to as either the 60Hz or the 60.15Hz > timer. Certainly that's enough information for anyone familiar with Mac internals to understand > exactly what you are referring to. There are also plenty of examples of this elsewhere e.g. for > graphics cards the high level claim will be that it supports over 16 million colours whereas the > engineers know specifically that the exact number is 16777216. > > I'll write up a suitable comment with a #define and send through a v2, perhaps altering the comment > on the timer itself to read 60.15Hz as that's what the reference cited by Laurent also uses. If you want more details, this is also in "Apple Guide to the Macintosh Family Hardware", 2nd edition, Chapter 12, "Displays", p. 401 (p.440 of the following PDF): https://archive.org/details/apple-guide-macintosh-family-hardware Thanks, Laurent
On 11/03/2021 09:44, Laurent Vivier wrote: >> Yes indeed, depending upon the documentation it is referred to as either the 60Hz or the 60.15Hz >> timer. Certainly that's enough information for anyone familiar with Mac internals to understand >> exactly what you are referring to. There are also plenty of examples of this elsewhere e.g. for >> graphics cards the high level claim will be that it supports over 16 million colours whereas the >> engineers know specifically that the exact number is 16777216. >> >> I'll write up a suitable comment with a #define and send through a v2, perhaps altering the comment >> on the timer itself to read 60.15Hz as that's what the reference cited by Laurent also uses. > > If you want more details, this is also in "Apple Guide to the Macintosh Family Hardware", 2nd > edition, Chapter 12, "Displays", p. 401 (p.440 of the following PDF): > > https://archive.org/details/apple-guide-macintosh-family-hardware Ah yes, that's a better reference as I don't see online versions of "Macintosh Family Hardware Reference" anywhere. I'll post the v2 shortly. ATB, Mark.
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; if (s->ier & VIA1_IRQ_60HZ) { timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz);
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(-)