hoglet67 / PiTubeDirect

Bare-metal Raspberry Pi project that attaches to the Acorn TUBE interface and emulates many BBC Micro Co Processors
GNU General Public License v3.0
187 stars 23 forks source link

Changing MODE in 6502 with Frame Buffer seems to hang #206

Closed mincebert closed 6 months ago

mincebert commented 6 months ago

This is with indigo-alpha4 on an internal board in a Master with MOS 3.50 and Pi 3A+ (my usual setup).

In 6502 JIT copro 24, I type CALL &300 and output shifts to the Pi framebuffer. Things then seem to work OK (graphics, text, etc.) until I change screen MODE, then the cursor stops flashing and the system seems to be hung. A BREAK brings it back. As an example, if I run Sphere and delete line 20 (MODE 5), no problems; put back in line 20 and it hangs there.

This doesn't happen when using the other 6502 copros 0-3, nor with the ARM Native processor 15 and *PIVDU 2 — that works fine.

I've only just noticed this but I don't think it used to happen! I also haven't gone back, trying older versions yet.

hoglet67 commented 6 months ago

Just to confirm I can reproduce this.

It happens on the Pi Zero 2, Pi Zero 3A+ and a Pi 4.

It happens in:

It doesn't happen on the Pi Zero, nor in Hognose.

In the debug console (Pi Zero 2) I see:

>> Data Abort at 0D40B7CC on core 0
Registers:
  r[00]=0D558044
  r[01]=0D4022F8
  r[02]=0D401F00
  r[03]=00000003
  r[04]=00000001
  r[05]=0C100000
  r[06]=00000000
  r[07]=000001FD
  r[08]=00000000
  r[09]=00000040
  r[10]=00000005
  r[11]=01000000
  r[12]=0D55808C
  r[14]=0D40B7D8
Memory:
  0D40B7BC=E3402D40
  0D40B7C0=E58D1008
  0D40B7C4=EEF1FA10
  0D40B7C8=E58D200C
  0D40B7CC=E280C048 <<<<<< 
  0D40B7D0=F46D0ADF
  0D40B7D4=E302139C
  0D40B7D8=E3401D40
  0D40B7DC=E302241C
Flags: 
  NZCV--------------------IFTMMMMM
  01100000000000000000000100010011 (Supervisor Mode)
Halted waiting for reset

Here's a disassembly:

0d40b6b0 <get_screen_mode>:
 d40b6b0:   e3072c78    movw    r2, #31864  @ 0x7c78
 d40b6b4:   e3402d55    movt    r2, #3413   @ 0xd55
 d40b6b8:   e3011fb4    movw    r1, #8116   @ 0x1fb4
 d40b6bc:   e3401d40    movt    r1, #3392   @ 0xd40
 d40b6c0:   e5923000    ldr r3, [r2]
 d40b6c4:   e3530000    cmp r3, #0
 d40b6c8:   aa000003    bge d40b6dc <get_screen_mode+0x2c>
 d40b6cc:   ea000096    b   d40b92c <get_screen_mode+0x27c>
 d40b6d0:   e5b2306c    ldr r3, [r2, #108]! @ 0x6c
 d40b6d4:   e3530000    cmp r3, #0
 d40b6d8:   ba000093    blt d40b92c <get_screen_mode+0x27c>
 d40b6dc:   e1500003    cmp r0, r3
 d40b6e0:   1afffffa    bne d40b6d0 <get_screen_mode+0x20>
 d40b6e4:   e3520000    cmp r2, #0
 d40b6e8:   0a00008f    beq d40b92c <get_screen_mode+0x27c>
 d40b6ec:   e1a00002    mov r0, r2
 d40b6f0:   e5903034    ldr r3, [r0, #52]   @ 0x34
 d40b6f4:   e24dd030    sub sp, sp, #48 @ 0x30
 d40b6f8:   edd07a09    vldr    s15, [r0, #36]  @ 0x24
 d40b6fc:   e3530000    cmp r3, #0
 d40b700:   030237cc    movweq  r3, #10188  @ 0x27cc
 d40b704:   03403d40    movteq  r3, #3392   @ 0xd40
 d40b708:   05803034    streq   r3, [r0, #52]   @ 0x34
 d40b70c:   e5903038    ldr r3, [r0, #56]   @ 0x38
 d40b710:   e3530000    cmp r3, #0
 d40b714:   03013fb8    movweq  r3, #8120   @ 0x1fb8
 d40b718:   03403d40    movteq  r3, #3392   @ 0xd40
 d40b71c:   05803038    streq   r3, [r0, #56]   @ 0x38
 d40b720:   e590303c    ldr r3, [r0, #60]   @ 0x3c
 d40b724:   e3530000    cmp r3, #0
 d40b728:   030b3374    movweq  r3, #45940  @ 0xb374
 d40b72c:   03403d40    movteq  r3, #3392   @ 0xd40
 d40b730:   0580303c    streq   r3, [r0, #60]   @ 0x3c
 d40b734:   e5903040    ldr r3, [r0, #64]   @ 0x40
 d40b738:   e3530000    cmp r3, #0
 d40b73c:   030b3428    movweq  r3, #46120  @ 0xb428
 d40b740:   03403d40    movteq  r3, #3392   @ 0xd40
 d40b744:   05803040    streq   r3, [r0, #64]   @ 0x40
 d40b748:   e5903060    ldr r3, [r0, #96]   @ 0x60
 d40b74c:   e3530000    cmp r3, #0
 d40b750:   030234a8    movweq  r3, #9384   @ 0x24a8
 d40b754:   03403d40    movteq  r3, #3392   @ 0xd40
 d40b758:   05803060    streq   r3, [r0, #96]   @ 0x60
 d40b75c:   e5903064    ldr r3, [r0, #100]  @ 0x64
 d40b760:   e3530000    cmp r3, #0
 d40b764:   03023520    movweq  r3, #9504   @ 0x2520
 d40b768:   03403d40    movteq  r3, #3392   @ 0xd40
 d40b76c:   05803064    streq   r3, [r0, #100]  @ 0x64
 d40b770:   e5903068    ldr r3, [r0, #104]  @ 0x68
 d40b774:   e3530000    cmp r3, #0
 d40b778:   03023584    movweq  r3, #9604   @ 0x2584
 d40b77c:   03403d40    movteq  r3, #3392   @ 0xd40
 d40b780:   05803068    streq   r3, [r0, #104]  @ 0x68
 d40b784:   e5903018    ldr r3, [r0, #24]
 d40b788:   e3530004    cmp r3, #4
 d40b78c:   0a00004d    beq d40b8c8 <get_screen_mode+0x218>
 d40b790:   e3530005    cmp r3, #5
 d40b794:   0a000032    beq d40b864 <get_screen_mode+0x1b4>
 d40b798:   eef57a40    vcmp.f32    s15, #0.0
 d40b79c:   e3021210    movw    r1, #8720   @ 0x2210
 d40b7a0:   e3401d40    movt    r1, #3392   @ 0xd40
 d40b7a4:   e3022260    movw    r2, #8800   @ 0x2260
 d40b7a8:   e3402d40    movt    r2, #3392   @ 0xd40
 d40b7ac:   e88d0006    stm sp, {r1, r2}
 d40b7b0:   e30212f8    movw    r1, #8952   @ 0x22f8
 d40b7b4:   e3401d40    movt    r1, #3392   @ 0xd40
 d40b7b8:   e3012f00    movw    r2, #7936   @ 0x1f00
 d40b7bc:   e3402d40    movt    r2, #3392   @ 0xd40
 d40b7c0:   e58d1008    str r1, [sp, #8]
 d40b7c4:   eef1fa10    vmrs    APSR_nzcv, fpscr
 d40b7c8:   e58d200c    str r2, [sp, #12]
 d40b7cc:   e280c048    add ip, r0, #72 @ 0x48   ; <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
 d40b7d0:   f46d0adf    vld1.64 {d16-d17}, [sp :64]
 d40b7d4:   e302139c    movw    r1, #9116   @ 0x239c
 d40b7d8:   e3401d40    movt    r1, #3392   @ 0xd40
 d40b7dc:   e302241c    movw    r2, #9244   @ 0x241c
 d40b7e0:   e3402d40    movt    r2, #3392   @ 0xd40
 d40b7e4:   f44c0a8f    vst1.32 {d16-d17}, [ip]
 d40b7e8:   e5801058    str r1, [r0, #88]   @ 0x58
 d40b7ec:   e580205c    str r2, [r0, #92]   @ 0x5c
 d40b7f0:   1a00000a    bne d40b820 <get_screen_mode+0x170>
 d40b7f4:   e5901010    ldr r1, [r0, #16]
 d40b7f8:   e3a02001    mov r2, #1
 d40b7fc:   e590c014    ldr ip, [r0, #20]

Abort address is consistent on both systems.

I'm wondering if the abort is actually the next instruction (vld1.64), and this is a stack (mis) alignment issue. It needs to be 64-bit aligned at this point, ortherwise you'll get an exception.

As an experiment I added an additional 32-bit stack write before the JIT Co Pro calls into the C code:

 iostore:
    strb  reg4,fiqjitflag
    push  {r12,lr}
+   push  {lr}
    bl    tube_parasite_write_banksel // Call up to C code to handle parasite write
+   pop   {lr}
    pop   {r12,lr}
    b     testforirq

This is the path that will be used buy the framebuffer driver.

With this hack in place, the data abort no longer happens.

Dominic, any thoughts?

I think we need to review the JIT code and make sure the stack maintains 64-bit alignment.

Dave

hoglet67 commented 6 months ago

For reference, this is the commit that altered he IOSTORE stack aligment re: Hognose

https://github.com/hoglet67/PiTubeDirect/commit/bce50da913af75d12d8e4054e77bf7e773404f87#diff-7571e214c609a2b0098fc1e47ecbad6c26ee340072bbbc714af54bcbb969af52R2200

The very first thing the JIT Co Pro does is push an odd number of words: https://github.com/hoglet67/PiTubeDirect/blob/indigo-dev/src/jit.S#L1940

So really it only worked by luck in hognose, rather than design.

There's another place stack alignment looks suspect: https://github.com/hoglet67/PiTubeDirect/blob/indigo-dev/src/jit.S#L2085

There are other places as well, but they are either in debug code, or are in code that doesn't go on to call C code.

Probably worth checking every push {}

dp111 commented 6 months ago

I'll look at this. I keep on forgetting 64bit stack alignment when using floating point.

On Thu, 29 Feb 2024 at 14:27, David Banks @.***> wrote:

For reference, this is the commit that altered he IOSTORE stack aligment re: Hognose

bce50da

diff-7571e214c609a2b0098fc1e47ecbad6c26ee340072bbbc714af54bcbb969af52R2200

https://github.com/hoglet67/PiTubeDirect/commit/bce50da913af75d12d8e4054e77bf7e773404f87#diff-7571e214c609a2b0098fc1e47ecbad6c26ee340072bbbc714af54bcbb969af52R2200

The very first thing the JIT Co Pro does is push an odd number of words: https://github.com/hoglet67/PiTubeDirect/blob/indigo-dev/src/jit.S#L1940

So really it only worked by luck in hognose, rather than design.

There's one other place stack alignment looks suspect: https://github.com/hoglet67/PiTubeDirect/blob/indigo-dev/src/jit.S#L2085

There are other places as well, but they are either in debug code, or are in code that doesn't go on to call C code.

Probably worth checking every push {}

— Reply to this email directly, view it on GitHub https://github.com/hoglet67/PiTubeDirect/issues/206#issuecomment-1971262284, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEVVFIRFGLWBY7MB6YSUQCDYV45GBAVCNFSM6AAAAABD64CKK6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZRGI3DEMRYGQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

dp111 commented 6 months ago

I've committed a fix to indigo-dev-dp. I've looked at all the assembly files and even if things looked safe I've aligned the stack.

On Thu, 29 Feb 2024 at 14:58, Dominic Plunkett @.***> wrote:

I'll look at this. I keep on forgetting 64bit stack alignment when using floating point.

On Thu, 29 Feb 2024 at 14:27, David Banks @.***> wrote:

For reference, this is the commit that altered he IOSTORE stack aligment re: Hognose

bce50da

diff-7571e214c609a2b0098fc1e47ecbad6c26ee340072bbbc714af54bcbb969af52R2200

https://github.com/hoglet67/PiTubeDirect/commit/bce50da913af75d12d8e4054e77bf7e773404f87#diff-7571e214c609a2b0098fc1e47ecbad6c26ee340072bbbc714af54bcbb969af52R2200

The very first thing the JIT Co Pro does is push an odd number of words: https://github.com/hoglet67/PiTubeDirect/blob/indigo-dev/src/jit.S#L1940

So really it only worked by luck in hognose, rather than design.

There's one other place stack alignment looks suspect: https://github.com/hoglet67/PiTubeDirect/blob/indigo-dev/src/jit.S#L2085

There are other places as well, but they are either in debug code, or are in code that doesn't go on to call C code.

Probably worth checking every push {}

— Reply to this email directly, view it on GitHub https://github.com/hoglet67/PiTubeDirect/issues/206#issuecomment-1971262284, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEVVFIRFGLWBY7MB6YSUQCDYV45GBAVCNFSM6AAAAABD64CKK6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZRGI3DEMRYGQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

hoglet67 commented 6 months ago

Can you double check this one is correct:

https://github.com/hoglet67/PiTubeDirect/commit/ef2169e7bccfc7524d9b2aa402146736e83c35a3#diff-56d3738b46db35c0b217e6dddf6c7820bde76bf6af5cee39a43c000f4c5b7a6bL349

dp111 commented 6 months ago

Sort of right, because the previous push this was unaligned. The right way to fix it is to fix the previous push which I have now done.

On Thu, 29 Feb 2024 at 20:29, David Banks @.***> wrote:

Can you double check this one is correct:

ef2169e

diff-56d3738b46db35c0b217e6dddf6c7820bde76bf6af5cee39a43c000f4c5b7a6bL349

https://github.com/hoglet67/PiTubeDirect/commit/ef2169e7bccfc7524d9b2aa402146736e83c35a3#diff-56d3738b46db35c0b217e6dddf6c7820bde76bf6af5cee39a43c000f4c5b7a6bL349

— Reply to this email directly, view it on GitHub https://github.com/hoglet67/PiTubeDirect/issues/206#issuecomment-1971905554, or unsubscribe https://github.com/notifications/unsubscribe-auth/AEVVFIXEOJU2OX4GQXEHURLYV6HTVAVCNFSM6AAAAABD64CKK6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZRHEYDKNJVGQ . You are receiving this because you commented.Message ID: @.***>

hoglet67 commented 6 months ago

Thanks Dominic.

This is now merged and included in Indigo Alpha5: https://github.com/hoglet67/PiTubeDirect/releases/tag/indigo-alpha5