Re: [hatari-devel] DSP bug found (and maybe solved)

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

On 16.4.2024 0.28, Laurent Sallafranque wrote:
> I've tested many programs tonight and didn't noticed any regression.
>
> The fix seems OK to me.
>
> Great. I love Audio Fun Machine, and I'm happy to have it working 
> correctly now.

I updated the compat doc a bit further (menu works if one is a bit careful).


> And also, thank you Eero for the moongame button C tips to launch the game.

Btw. of the things not yet checked for release, "L'Abbaye Des Morts" 
remains.

Its current (v2.3 based) info states:

"Needs FPU. Slower than it should, because Hatari doesn't support Videl 
60Hz Falcon emulation yet, only 50Hz. DMA sounds effects are too low 
volume compared to YM"

I don't have real HW i.e. no comparison point on the speed or volumes...


	- Eero


> Le 15/04/2024 à 22:10, Laurent Sallafranque a écrit :
> > Hi,
> >
> >
> > Thanks Andreas. I prefer this 2nd patch.
> >
> > I've commited it and changed the compatibiliy file too (but it didn't 
> > change the main hatari site compatibility list).
> >
> > I'll do more tests tonight with programs I know that use modulo buffers.
> >
> >
> > Regards
> >
> > Laurent
> >
> >
> > Le 15/04/2024 à 21:58, Andreas Grabher a écrit :
> > > > Am 15.04.2024 um 11:50 schrieb Andreas Grabher 
> > > > <andreas_g86@xxxxxxxxxx>:
> > > >
> > > >
> > > > > Am 14.04.2024 um 23:48 schrieb Laurent Sallafranque 
> > > > > <laurent.sallafranque@xxxxxxx>:
> > > > >
> > > > > Hi all,
> > > > >
> > > > >
> > > > > I've spent the last 2 weeks tracing Audio Fun Machine to find the 
> > > > > bug when the equalizer is ON (sound becomes noisy).
> > > > >
> > > > >
> > > > > I've isolated the bug : it's about Rn + Nn + Mn updating.
> > > > > An example :
> > > > >        move    #>0,r0                     ; R0 = 0
> > > > >        movec    #$01,m0                ; modulo 2
> > > > >        move    #$02,n0                   ; incrementor = 2
> > > > >
> > > > >        move    #>1,a
> > > > >        move    #>1,b
> > > > >        move    a,x:(r0)+                 ; r0 = 1
> > > > >        nop
> > > > >        nop
> > > > >        move    b,x:(r0)+                 ; r0 = 0  (because of the 
> > > > > modulo)
> > > > >        nop
> > > > >        nop
> > > > >        move    (r0)+n0                   ; R0 = 0 (should be 2)
> > > > >        nop
> > > > >        nop
> > > > >        move    a,x:(r0)+                 ; R0 = 1 (should be 3)
> > > > >
> > > > >
> > > > > I suggest the following fix in the function :
> > > > > static void dsp_update_rn_modulo(uint32_t numreg, int16_t modifier)
> > > > >
> > > > > Line 1694 :
> > > > > replace
> > > > > if (abs_modifier>modulo) {
> > > > > by
> > > > > if (abs_modifier>modulo-1) {
> > > > >
> > > > >
> > > > > In line 1673, modulo is added +1 for buffer boundaries computing.
> > > > > But I think modulo should be -1 when tested with modifier.
> > > > >
> > > > > At least, Audio Fun Machine sound becomes good with this patch, but 
> > > > > DSP programs should be tested for non regression.
> > > > > I've tested 2 or 3 of them, but not all of them.
> > > > >
> > > > > @eero, could you test my patch and tell me if AFM works well for you ?
> > > > >
> > > > >
> > > > > Regards
> > > > >
> > > > > Laurent
> > > > Great discovery! Obviously the special case where abs_modifier 
> > > > equals modulo was not handled. I think the more logical solution 
> > > > would be
> > > >
> > > > if (abs_modifier>=modulo)
> > > I was wrong with my first patch and I think the solution from Laurent 
> > > is also wrong, because it does not correctly handle the case where M 
> > > equals |Nn| but is not equal to 2^k.
> > >
> > > I created a patch (appended). I think this is the cleanest way: It 
> > > first checks for the case where |Nn| is not a multiple of 2^k. It 
> > > then checks if Nn is greater than M which means unpredictable result 
> > > else it does normal modulo operation. In the special case where |Nn| 
> > > is a multiple of 2^k it jumps to the next buffer (this includes the 
> > > case where P is 0 and makes this case a bit faster).
> > >
> > > Special case is described in the data sheet of the 56001 DSP:
> > > "If an offset, Nn, is used in the address calculations, the 16-bit 
> > > absolute value, INnl, must be less than or equal to M for proper 
> > > modulo addressing. If Nn>M, the result is data dependent and 
> > > unpredictable, except for the special case where Nn = P x 2^k, a 
> > > multiple of the block size where P is a positive integer. For this 
> > > special case, when using the (Rn) + Nn addressing mode, the pointer, 
> > > Rn, will jump linearly to the same relative address in a new buffer, 
> > > which is P blocks forward in memory (see Figure 5-12). Similarly, for 
> > > (Rn) - Nn, the pointer will jump P blocks backward in memory.“
> > >
> > > I think the data sheet is not very clear because if |Nn| is equal to 
> > > M there are two possible cases but only one is described.
> > >
> > > Please verify my thoughts. I hope there is no mistake.