dirkwhoffmann / vAmiga

vAmiga is a user-friendly Amiga 500, 1000, 2000 emulator for macOS
https://dirkwhoffmann.github.io/vAmiga
Other
293 stars 24 forks source link

Majic 12 - Ray of hope 2 of hope glitches #799

Closed mras0 closed 5 months ago

mras0 commented 11 months ago

Pouet link, Direct download.

Noticed this one was mentioned in response to a recent update in the EAB Undocumented Amiga hardware stuff thread (see also following posts), and tested it in vAmigaWeb/net/own fork.

At least first interference part seems to glitch (black vertical blocks visible). Maybe more issues later on as well as I didn't do full comparison.

image

Config tested: OCS (not old)/KS 1.3/plain 68000/Mem: 512+512. No glitches in WinUAE 5.0.0 w/ most common A500 setup.

dirkwhoffmann commented 11 months ago

Very cool. Thanks for reporting! I've set up a new test case exploiting the undocumented ECS feature (more precisely: the ECS Denise feature. The Agnus type doesn't matter, because Agnus only handles the vertical DIW).

A500 (OCS Denise):

minmax3_A500_ECS

A500+ (ECS Denise):

minmax3_A500+

The difference in both images results from this code fragment which will trigger the DIW stop logic on machines with a SuperDenise or Lisa chip:

        dc.w    $5681,$FFFE  ; WAIT 
    dc.w    DIWSTOP,$2c02
    dc.w    DIWHIGH,$0000
    dc.w    $5701,$FFFE  ; WAIT 
    dc.w    DIWHIGH,$2100
    dc.w    DIWSTOP,_DIWSTOP

    dc.w    $5881,$FFFE  ; WAIT 
    dc.w    DIWSTOP,$2c03
    dc.w    DIWHIGH,$0000
    dc.w    $5901,$FFFE  ; WAIT 
    dc.w    DIWHIGH,$2100
    dc.w    DIWSTOP,_DIWSTOP 
dirkwhoffmann commented 6 months ago

Just did a little code review. Before this bug can be properly fixed, the following function needs to be made timing-sensitive (like setDIWSTRT and setDIWSTOP already are):

void
Denise::setDIWHIGH(u16 value)
{
    trace(DIW_DEBUG, "setDIWHIGH(%x)\n", value);

    if (!isECS()) return;

    // 15 14 13 12 11 10  9  8  7  6  5  4  3  2  1  0
    // -- -- H8 -- -- -- -- -- -- -- H8 -- -- -- -- --
    //     (stop)                  (strt)

    diwhigh = value;

    hstrt = LO_BYTE(diwstrt) | (GET_BIT(diwhigh,  5) ? 0x100 : 0x000);
    hstop = LO_BYTE(diwstop) | (GET_BIT(diwhigh, 13) ? 0x100 : 0x000);

    if (hstrt > 0x1C7) hstrt = INT16_MAX;
    if (hstop > 0x1C7) hstop = INT16_MAX;

    // Inform the debugger about the changed display window
    debugger.updateDiwH(hstrt, hstop);
}

TODO:

  1. Write some timing tests for register DIWHIGH.
  2. Factor out the timing-related functionality from setDIWSTRT and setDIWSTOP to separate functions.
  3. Call the new function from setDIWHIGH.
dirkwhoffmann commented 6 months ago

One issue of setDIWHIGH was that it didn't check for the invalid trigger positions 0 and 1. In fact, this shielded an OCS bug, namely this one, mentioned in the EAB thread earlier.

After fixing this, things got worse from the outside because the demos's splash screens are now gone. To get the DIW logic 100% correct, it should probably be handled event-driven (via the event scheduler). I need to consider whether it's worth doing, as it would slow down emulation a bit.

dirkwhoffmann commented 6 months ago

Summing up:

  1. DIW emulation is more accurate now. vAmiga now passes minmax3 in ECS mode:
Bildschirmfoto 2024-01-03 um 13 30 45
  1. The graphics bug reported by @mras0 (flickering black stripes) has a different cause. It has nothing to do with DIW logic. Interestingly, there is nothing special going on during the faulty demo scene. Just bitplane DMA inside the main display area and some Blitter DMA in the lower part:
Bildschirmfoto 2024-01-03 um 13 24 59
emoon commented 6 months ago

Without knowing anything about what is going on in the demo (but knowing Amiga in general) my guess would be that it's a "stray" sprite showing up for whatever reason.

dirkwhoffmann commented 6 months ago

my guess would be that it's a "stray" sprite showing up for whatever reason.

Yes, indeed! After disabling sprite drawing with the following hack, it looks perfect.

template <Resolution R> void
Denise::drawSprites()
{
    // REMOVE ASAP
    wasArmed = false;
emoon commented 6 months ago

Cool. Thanks for confirming :)

So it might be that the sprite DMA has been left on by accident then and when horizontal scrolling a screen on Amiga (at least on OCS) the last sprite (and half of the second last) can't be used because the extra DMA needing for the offscreen fetching. So it might be that sprite data that shows up for some reason.

If that isn't the case it might be that the sprite that shows up ends up pointing on stale data somewhere and depending on how the memory is initialized it may end up showing some data or not showing it.

dirkwhoffmann commented 6 months ago

More findings: Culprit is indeed sprite 7. With an additional trace statement in here...

template <isize nr> void
Agnus::executeFirstSpriteCycle()
{
    if (pos.v == sprVStop[nr]) {

        sprDmaState[nr] = SPR_DMA_IDLE;

        if (busOwner[pos.h] == BUS_NONE) {

            // Read in the next control word (POS part)
            if (sprdma()) {

                auto value = doSpriteDmaRead<nr>();
                trace(true, "executeFirstSpriteCycle %ld %u\n", nr, value);
                agnus.pokeSPRxPOS<nr>(value);
                denise.pokeSPRxPOS<nr>(value);

            } else {

                busOwner[pos.h] = BUS_BLOCKED;
            }
        }

... the following output is produced:

[2564] ( 25, 21) 05209A  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 0 0
[2564] ( 25, 25) 052092  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 1 0
[2564] ( 25, 29) 052092  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 2 0
[2564] ( 25, 33) 052092  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 3 0
[2564] ( 25, 37) 05209A  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 4 0
[2564] ( 25, 41) 052092  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 5 0
[2564] ( 25, 45) 052092  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 6 0
[2564] ( 25, 49) 052092  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 7 3587
[2565] ( 25, 21) 052092  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 0 0
[2565] ( 25, 25) 052092  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 1 0
[2565] ( 25, 29) 05209A  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 2 0
[2565] ( 25, 33) 05209A  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 3 0
[2565] ( 25, 37) 052092  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 4 0
[2565] ( 25, 41) 052092  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 5 0
[2565] ( 25, 45) 05209A  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 6 0
[2565] ( 25, 49) 05209A  0 BCBSDA 4020 37C0 Agnus:508 executeFirstSpriteCycle 7 54280

Bogus values are written into SPR7POS. Interestingly, the DMA slots for sprite 7 are not blocked by bitplane DMA. Needs further investigation...

emoon commented 6 months ago

Interestingly, the DMA slots for sprite 7 are not blocked by bitplane DMA.

I can't remember what actually happens on the original hardware in this case. It might be that fetched bitplane graphics ends up as a sprite.

dirkwhoffmann commented 6 months ago

More findings:

The demo starts DDF very early at $18, thus blocking DMA for all sprites except for sprite 0 (which cannot be blocked). 2 bitplanes are enabled which blocks the red cycle for sprite 7:

Bildschirmfoto 2024-01-04 um 13 24 07

For sprites 1 to 6, the corresponding cycles are also blocked. However, this does not cause any harm because their SPRxPTR is correctly set. For Sprite 7, initialization is missing.

In case a sprite's DMA cycle is blocked, the current code does nothing, i.e., the old sprite register value is kept.

        if (busOwner[pos.h] == BUS_NONE) {

            if (sprdma()) {

                // Read in the next data word (part B)
                auto value = doSpriteDmaRead<nr>();
                denise.pokeSPRxDATB<nr>(value);

            } else {

                busOwner[pos.h] = BUS_BLOCKED;
            }
        }

TODO: Write a test case to check if bitplane data is copied into the register rather than keeping the old value.

dirkwhoffmann commented 6 months ago

Simple test case running in vAmiga 2.4:

Bildschirmfoto 2024-01-04 um 15 56 24

Sprite DMA is blocked by expanding the DDF window temporarily. As vAmiga works right now, the sprite data registers keep their old value during the blocking period.

Tomorrow, I'll run the test case on a real machine. I expect it to behave differently based on a hardware-design point of view. Keeping the old value would likely require some extra logic.

dirkwhoffmann commented 6 months ago

Just run the test on both an A500 and an A500+. Unexpectedly, vAmiga does the right thing:

blocked1_A500

dirkwhoffmann commented 6 months ago

At least I have gotten a better understanding of what's going on. The faulty scene has a Copper list that resets SPRxPTR for all sprites, but sprite 7:

Bildschirmfoto 2024-01-05 um 10 38 38

SPR7PTR still contains the value used in the previous scene. From now on, SPR7PTR is freely running, thus using everything as position and data that is found in memory.

Bildschirmfoto 2024-01-05 um 10 39 10

It doesn't crawl through uninitialized memory, though. Thus, it is still unclear why the stray sprite does not show up on a real machine.

emoon commented 6 months ago

You don't happen to have a Action replay for you Amiga? That way you could investigate the value of the data on the original hw.

mras0 commented 6 months ago

If DMA isn't enabled for sprite 7 then the pointer value won't increase. In WinUAE I can see SPR7PT stays $33004.

dirkwhoffmann commented 6 months ago

I've modified my test case above to only enable two bitplanes. This steals the second sprite cycle and leaves the first one enabled. Because only SPR7DATA is written now (and SPR7DATB is skipped), the sprite data for data register B should end up in SPT7DATA in every other line. This should visually stretch the sprite vertically, which is precisely what happens in vAmiga:

Bildschirmfoto 2024-01-05 um 16 37 38

On a real machine, however, the result is identical to what we see for four bitplanes enabled (which steals all sprite cycles).

Therefore, I think I know what's going wrong in vAmiga:

dirkwhoffmann commented 6 months ago

Part of the sprite logic:

Bildschirmfoto 2024-01-05 um 17 29 18

Not 100% sure, but the BPRUN signal could be the one that decides whether a sprite DMA cycle is skipped or not.

dirkwhoffmann commented 6 months ago

Blocking sprite DMA slots w.r.t. BPRUN fixes the issue. Stray sprites are gone.

mras0 commented 6 months ago

Yeah, I'm not sure how the logic works, but yes Agnus blocks sprite fetches if bitplanes need them (or is supposed to). Apparently there's an OCS bug related to that though:

"- sprite DMA vs bitplane DMA off-by-one fix. Second sprite control was stolen by bitplane DMA even if slot was free but next slot was used by bitplane. (ECS Agnus)" (example: https://eab.abime.net/showthread.php?t=75094)

dirkwhoffmann commented 6 months ago

The eab article talks about an undocumented OCS hardware bug, but the bug is not explained? At least I didn't find any infos.

In the meantime, I've also improved my test case: blocking4. It enables all 7 sprites and shifts the DDF window in small steps across the sprite DMA area:

A500 ECS:

blocked4_A500_ECS

A500 OCS:

blocked4_A500_OCS

Latest vAmiga (dev branch) passes the ECS test:

Bildschirmfoto 2024-01-06 um 10 44 39

It fails the OCS test though:

Bildschirmfoto 2024-01-06 um 10 44 22

Interestingly, vAmiga shows a DDF window error, independent of the sprite mismatches The first three bars in the extended areas to the left are too short.

dirkwhoffmann commented 6 months ago

Update:

Summing up:

blocked4_A500_OCS_Kopie

Sprite 0 exhibits "zebra stripes", indicating that the second sprite DMA cycle is stolen. In theory, this should not happen because sprite 0 was meant to be unblockable by the hardware designers. In ECS Agnus, they have fixed the timing of the BPRUN line, which fixes the bug.

As always in science (OK, this is not really science, but anyway...), these are only hypotheses waiting to be refuted.