Re: [hatari-devel] EKO System crash

[Nicolas Pomarède <npomarede@xxxxxxxxxxxx>]

Le 01/11/2020 à 09:18, Thomas Huth a écrit :

> > Hi
> >
> > for DMA sound in STE mode, mask is computed using
> > DMA_MaskAddressHigh() which will return 0x3f in ST/STE mode because
> > sound (or video counter too) is restricted to 4 MB.
>
> I'm not sure about the Falcon here, whether it caps at 4 MB or 16 ...
> that would need some testing with real hardware, certainly...

Hi
yes this should be confirmed on real HW, my feeling was that it was 
possible to access all RAM for video, fdc or sound, so the mask would be 
0xFFFFFF.

> > dmaSnd.c is also accessing STRam, I know that the demo "amberstar
> > cracktro" is buggy and will set start=end, which will play through
> > the whole memory, but it doesn't crash Hatari, maybe by luck and
> > because the mask 0x3f restrict it to a smaller region.
>
> Have you tried with --enable-small-mem ? ... would be interesting to
> see whether it crashes there.

no, I didn't try. Maybe it would crash, but even if it doesn't crash by 
luck, I'm quite sure that accessing STRam[] directly will not be correct.


> > So, dmaSnd.c should also be modified to use get_word().
> >
> > We could add a common function DMA_ReadWord() in m68000.c to be
> > called by blitter.c / dmaSnd.c / crossbar.c.
> >
> > What do you think ?
>
> Sounds reasonable ... I'm just a little bit afraid that the performance
> of Hatari might suffer quite a bit, since dma samples are processed
> quite frequently, so always going through the complex get_word
> function might slow down things a little bit...?

This will add a small overhead, but if we consider 50KHz 16 bit stereo 
sound, we have 100000 words to read per sec, which gives 2000 words per 
VBL using these new DMA functions.
Which is in a fact a rather low number of words if you compare it to the 
words accesses needed to emulate the CPU or the video.

Also, if we want to handle the cases where DMA accesses bus error 
regions, or even overlaps RAM and non-RAM region, then I think using the 
usual memory access functions is the safest way to go (if not, one would 
need to use something like 'STMemory_SafeCopy()' which adds a lot of 
'if' and doesn't support all possible memory region types)

You can try the attached patch to see if it safely replaces your change 
using "% STRamEnd". It only handles the "play" case, but it's easy to 
use it for "record" too.

Nicolas


diff --git a/src/falcon/crossbar.c b/src/falcon/crossbar.c
index bbceee0d..9fb0733b 100644
--- a/src/falcon/crossbar.c
+++ b/src/falcon/crossbar.c
@@ -1513,25 +1513,25 @@ static void Crossbar_Process_DMAPlay_Transfer(void)
 	if (dmaPlay.isRunning == 0)
 		return;
 
-	nFramePos = (dmaPlay.frameStartAddr + dmaPlay.frameCounter) % STRamEnd;
+	nFramePos = (dmaPlay.frameStartAddr + dmaPlay.frameCounter) & ( ( DMA_MaskAddressHigh() << 16 ) | 0xffff );
 	increment_frame = 0;
 
 	/* 16 bits stereo mode ? */
 	if (crossbar.is16Bits) {
 		eightBits = 1;
-		value = (Sint16)do_get_mem_word(&STRam[nFramePos]);
+		value = (Sint16)STMemory_DMA_ReadWord( nFramePos );
 		increment_frame = 2;
 	}
 	/* 8 bits stereo ? */
 	else if (crossbar.isStereo) {
 		eightBits = 64;
-		value = (Sint8)STRam[nFramePos];
+		value = (Sint8)STMemory_DMA_ReadByte( nFramePos );
 		increment_frame = 1;
 	}
 	/* 8 bits mono */
 	else {
 		eightBits = 64;
-		value = (Sint8)STRam[nFramePos];
+		value = (Sint8)STMemory_DMA_ReadByte( nFramePos );
 		if ((dmaPlay.currentFrame & 1) == 0) {
 			increment_frame = 1;
 		}