From 830704b4a958ae93db531310c3a7687954de0b02 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 19 Jul 2022 15:39:57 -0400 Subject: [PATCH 1/6] Clarify and slightly improve state machine. No more using the visible flag to permit a DMA control fetch. --- Machines/Amiga/Chipset.cpp | 2 +- Machines/Amiga/Sprites.cpp | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/Machines/Amiga/Chipset.cpp b/Machines/Amiga/Chipset.cpp index ca4b78e87..2372f7264 100644 --- a/Machines/Amiga/Chipset.cpp +++ b/Machines/Amiga/Chipset.cpp @@ -611,7 +611,7 @@ template bool Chipset::perform_cycle() { constexpr auto sprite_id = (cycle - 0x16) >> 2; static_assert(sprite_id >= 0 && sprite_id < std::tuple_size::value); - if(sprites_[sprite_id].advance_dma(!(cycle&2))) { + if(sprites_[sprite_id].advance_dma((~cycle&2) >> 1)) { return false; } } diff --git a/Machines/Amiga/Sprites.cpp b/Machines/Amiga/Sprites.cpp index c886f4f97..b17e3a635 100644 --- a/Machines/Amiga/Sprites.cpp +++ b/Machines/Amiga/Sprites.cpp @@ -59,35 +59,35 @@ void Sprite::advance_line(int y, bool is_end_of_blank) { } if(is_end_of_blank || y == v_stop_) { dma_state_ = DMAState::FetchControl; - visible = true; } } bool Sprite::advance_dma(int offset) { - if(!visible) return false; + assert(offset == 0 || offset == 1); - // Fetch another word. + // Determine which word would be fetched, if DMA occurs. + // A bit of a cheat. const uint16_t next_word = ram_[pointer_[0] & ram_mask_]; - ++pointer_[0]; // Put the fetched word somewhere appropriate and update the DMA state. switch(dma_state_) { - // i.e. stopped. - default: return false; - case DMAState::FetchControl: if(offset) { set_stop_and_control(next_word); } else { set_start_position(next_word); } - return true; + break; case DMAState::FetchImage: - set_image_data(1 - bool(offset), next_word); - return true; + if(!visible) return false; + set_image_data(1 - offset, next_word); + break; } - return false; + + // Acknowledge the fetch. + ++pointer_[0]; + return true; } template void TwoSpriteShifter::load( From cb42ee3ade854c6af52de2c31b06ef85f8f6c781 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 19 Jul 2022 16:11:29 -0400 Subject: [PATCH 2/6] Eliminate `DMAState`; it sounds like VSTOP solves this problem. --- Machines/Amiga/Chipset.cpp | 31 +++++++++++++-------------- Machines/Amiga/Sprites.cpp | 43 +++++++++++++++++--------------------- Machines/Amiga/Sprites.hpp | 8 +------ 3 files changed, 35 insertions(+), 47 deletions(-) diff --git a/Machines/Amiga/Chipset.cpp b/Machines/Amiga/Chipset.cpp index 2372f7264..6592469b1 100644 --- a/Machines/Amiga/Chipset.cpp +++ b/Machines/Amiga/Chipset.cpp @@ -21,6 +21,8 @@ using namespace Amiga; +// TODO: I don't think the nonsense below, which was intended to allow a typed enum but also +// clean combination, really works. Rethink. namespace { template struct Mask { @@ -34,6 +36,18 @@ template struct Mask { template struct InterruptMask: Mask {}; template struct DMAMask: Mask {}; +constexpr uint16_t AudioFlags[] = { + DMAMask::value, + DMAMask::value, + DMAMask::value, + DMAMask::value, +}; +constexpr auto BlitterFlag = DMAMask::value; +constexpr auto BitplaneFlag = DMAMask::value; +constexpr auto CopperFlag = DMAMask::value; +constexpr auto DiskFlag = DMAMask::value; +constexpr auto SpritesFlag = DMAMask::value; + } #define DMA_CONSTRUCT *this, reinterpret_cast(map.chip_ram.data()), map.chip_ram.size() >> 1 @@ -488,17 +502,6 @@ void Chipset::flush_output() { /// @returns @c true if this was a CPU slot; @c false otherwise. template bool Chipset::perform_cycle() { - constexpr uint16_t AudioFlags[] = { - DMAMask::value, - DMAMask::value, - DMAMask::value, - DMAMask::value, - }; - constexpr auto BlitterFlag = DMAMask::value; - constexpr auto BitplaneFlag = DMAMask::value; - constexpr auto CopperFlag = DMAMask::value; - constexpr auto DiskFlag = DMAMask::value; - constexpr auto SpritesFlag = DMAMask::value; // Update state as to whether bitplane fetching should happen now. // @@ -611,7 +614,7 @@ template bool Chipset::perform_cycle() { constexpr auto sprite_id = (cycle - 0x16) >> 2; static_assert(sprite_id >= 0 && sprite_id < std::tuple_size::value); - if(sprites_[sprite_id].advance_dma((~cycle&2) >> 1)) { + if(sprites_[sprite_id].advance_dma((~cycle&2) >> 1, y_)) { return false; } } @@ -741,10 +744,6 @@ template Chipset::Changes Chipset::run(HalfCycles length) { is_long_field_ ^= interlace_; } - for(auto &sprite: sprites_) { - sprite.advance_line(y_, y_ == vertical_blank_height_); - } - fetch_vertical_ |= y_ == display_window_start_[1]; fetch_vertical_ &= y_ != display_window_stop_[1]; } diff --git a/Machines/Amiga/Sprites.cpp b/Machines/Amiga/Sprites.cpp index b17e3a635..49f53aa8b 100644 --- a/Machines/Amiga/Sprites.cpp +++ b/Machines/Amiga/Sprites.cpp @@ -45,7 +45,6 @@ void Sprite::set_stop_and_control(uint16_t value) { // Disarm the sprite, but expect graphics next from DMA. visible = false; - dma_state_ = DMAState::FetchImage; } void Sprite::set_image_data(int slot, uint16_t value) { @@ -53,36 +52,32 @@ void Sprite::set_image_data(int slot, uint16_t value) { visible |= slot == 0; } -void Sprite::advance_line(int y, bool is_end_of_blank) { - if(dma_state_ == DMAState::FetchImage && y == v_start_) { - visible = true; - } - if(is_end_of_blank || y == v_stop_) { - dma_state_ = DMAState::FetchControl; - } -} - -bool Sprite::advance_dma(int offset) { +bool Sprite::advance_dma(int offset, int y) { assert(offset == 0 || offset == 1); // Determine which word would be fetched, if DMA occurs. // A bit of a cheat. const uint16_t next_word = ram_[pointer_[0] & ram_mask_]; - // Put the fetched word somewhere appropriate and update the DMA state. - switch(dma_state_) { - case DMAState::FetchControl: - if(offset) { - set_stop_and_control(next_word); - } else { - set_start_position(next_word); - } - break; + // "When the vertical position of the beam counter is equal to the VSTOP + // value in the sprite control words, the next two words fetched from the + // sprite data structure are written into the sprite control registers + // instead of being sent to the color registers" + if(y == v_stop_) { + if(offset) { + // Second control word: stop position (mostly). + set_stop_and_control(next_word); + } else { + // First control word: start position. + set_start_position(next_word); + } + } else { + visible |= y == v_start_; + if(!visible) return false; - case DMAState::FetchImage: - if(!visible) return false; - set_image_data(1 - offset, next_word); - break; + // Write colour word 1, then colour word 0; 0 is the word that 'arms' + // the sprite (i.e. makes it visible). + set_image_data(1 - offset, next_word); } // Acknowledge the fetch. diff --git a/Machines/Amiga/Sprites.hpp b/Machines/Amiga/Sprites.hpp index 947b72633..f15f97ad8 100644 --- a/Machines/Amiga/Sprites.hpp +++ b/Machines/Amiga/Sprites.hpp @@ -23,8 +23,7 @@ class Sprite: public DMADevice<1> { void set_stop_and_control(uint16_t value); void set_image_data(int slot, uint16_t value); - void advance_line(int y, bool is_end_of_blank); - bool advance_dma(int offset); + bool advance_dma(int offset, int y); uint16_t data[2]{}; bool attached = false; @@ -33,11 +32,6 @@ class Sprite: public DMADevice<1> { private: uint16_t v_start_ = 0, v_stop_ = 0; - - enum class DMAState { - FetchControl, - FetchImage - } dma_state_ = DMAState::FetchControl; }; class TwoSpriteShifter { From feee6afe0f3740816ff272a124915ee61a1e64f6 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 19 Jul 2022 16:19:19 -0400 Subject: [PATCH 3/6] Improve documentation. --- Machines/Amiga/Sprites.cpp | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/Machines/Amiga/Sprites.cpp b/Machines/Amiga/Sprites.cpp index 49f53aa8b..d908c6b6f 100644 --- a/Machines/Amiga/Sprites.cpp +++ b/Machines/Amiga/Sprites.cpp @@ -33,21 +33,31 @@ static_assert(expand_sprite_word(0x0000) == 0x00'00'00'00'00'00'00'00); // MARK: - Sprites. void Sprite::set_start_position(uint16_t value) { + // b8–b15: low 8 bits of VSTART; + // b0–b7: high 8 bits of HSTART. v_start_ = (v_start_ & 0xff00) | (value >> 8); h_start = uint16_t((h_start & 0x0001) | ((value & 0xff) << 1)); } void Sprite::set_stop_and_control(uint16_t value) { + // b8–b15: low 8 bits of VSTOP; + // b7: attachment flag; + // b3–b6: unused; + // b2: VSTART high bit; + // b1: VSTOP high bit; + // b0: HSTART low bit. h_start = uint16_t((h_start & 0x01fe) | (value & 0x01)); v_stop_ = uint16_t((value >> 8) | ((value & 0x02) << 7)); v_start_ = uint16_t((v_start_ & 0x00ff) | ((value & 0x04) << 6)); attached = value & 0x80; - // Disarm the sprite, but expect graphics next from DMA. + // Disarm the sprite. visible = false; } void Sprite::set_image_data(int slot, uint16_t value) { + // Store data; also mark sprite as visible (i.e. 'arm' it) + // if data is being stored to slot 0. data[slot] = value; visible |= slot == 0; } From 57186c3c14664c133d8a9f86e9a852729451e59f Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 19 Jul 2022 16:37:13 -0400 Subject: [PATCH 4/6] Don't limit sprite fetch area; add further commentary. --- Machines/Amiga/Chipset.cpp | 2 +- Machines/Amiga/Sprites.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Machines/Amiga/Chipset.cpp b/Machines/Amiga/Chipset.cpp index 6592469b1..46abacfab 100644 --- a/Machines/Amiga/Chipset.cpp +++ b/Machines/Amiga/Chipset.cpp @@ -610,7 +610,7 @@ template bool Chipset::perform_cycle() { } if constexpr (cycle >= 0x16 && cycle < 0x36) { - if((dma_control_ & SpritesFlag) == SpritesFlag && y_ >= vertical_blank_height_) { + if((dma_control_ & SpritesFlag) == SpritesFlag) { constexpr auto sprite_id = (cycle - 0x16) >> 2; static_assert(sprite_id >= 0 && sprite_id < std::tuple_size::value); diff --git a/Machines/Amiga/Sprites.cpp b/Machines/Amiga/Sprites.cpp index d908c6b6f..8d3a16afc 100644 --- a/Machines/Amiga/Sprites.cpp +++ b/Machines/Amiga/Sprites.cpp @@ -83,7 +83,7 @@ bool Sprite::advance_dma(int offset, int y) { } } else { visible |= y == v_start_; - if(!visible) return false; + if(!visible) return false; // Act as if there wasn't a fetch. // Write colour word 1, then colour word 0; 0 is the word that 'arms' // the sprite (i.e. makes it visible). From 89abf7faeb88d952c63a54690f6087a64bf9c523 Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 19 Jul 2022 21:25:34 -0400 Subject: [PATCH 5/6] Take a guess at reintroducing a special case for end-of-blank. --- Machines/Amiga/Chipset.cpp | 2 +- Machines/Amiga/Sprites.cpp | 7 +++++-- Machines/Amiga/Sprites.hpp | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Machines/Amiga/Chipset.cpp b/Machines/Amiga/Chipset.cpp index 46abacfab..e9ed2fd77 100644 --- a/Machines/Amiga/Chipset.cpp +++ b/Machines/Amiga/Chipset.cpp @@ -614,7 +614,7 @@ template bool Chipset::perform_cycle() { constexpr auto sprite_id = (cycle - 0x16) >> 2; static_assert(sprite_id >= 0 && sprite_id < std::tuple_size::value); - if(sprites_[sprite_id].advance_dma((~cycle&2) >> 1, y_)) { + if(sprites_[sprite_id].advance_dma((~cycle&2) >> 1, y_, y_ == vertical_blank_height_)) { return false; } } diff --git a/Machines/Amiga/Sprites.cpp b/Machines/Amiga/Sprites.cpp index 8d3a16afc..d29679a3e 100644 --- a/Machines/Amiga/Sprites.cpp +++ b/Machines/Amiga/Sprites.cpp @@ -62,7 +62,7 @@ void Sprite::set_image_data(int slot, uint16_t value) { visible |= slot == 0; } -bool Sprite::advance_dma(int offset, int y) { +bool Sprite::advance_dma(int offset, int y, bool is_first_line) { assert(offset == 0 || offset == 1); // Determine which word would be fetched, if DMA occurs. @@ -73,7 +73,10 @@ bool Sprite::advance_dma(int offset, int y) { // value in the sprite control words, the next two words fetched from the // sprite data structure are written into the sprite control registers // instead of being sent to the color registers" - if(y == v_stop_) { + // + // Guesswork, primarily from observing Spindizzy Worlds: the first line after + // vertical blank also triggers a control reload. Seek to verify. + if(y == v_stop_ || is_first_line) { if(offset) { // Second control word: stop position (mostly). set_stop_and_control(next_word); diff --git a/Machines/Amiga/Sprites.hpp b/Machines/Amiga/Sprites.hpp index f15f97ad8..a2c05da4e 100644 --- a/Machines/Amiga/Sprites.hpp +++ b/Machines/Amiga/Sprites.hpp @@ -23,7 +23,7 @@ class Sprite: public DMADevice<1> { void set_stop_and_control(uint16_t value); void set_image_data(int slot, uint16_t value); - bool advance_dma(int offset, int y); + bool advance_dma(int offset, int y, bool is_first_line); uint16_t data[2]{}; bool attached = false; From f29d3055973992ec436cae5ef86bdbfcf93166ba Mon Sep 17 00:00:00 2001 From: Thomas Harte Date: Tue, 19 Jul 2022 21:40:16 -0400 Subject: [PATCH 6/6] Add missing #include. --- Machines/Amiga/Sprites.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Machines/Amiga/Sprites.cpp b/Machines/Amiga/Sprites.cpp index d29679a3e..1c400fac6 100644 --- a/Machines/Amiga/Sprites.cpp +++ b/Machines/Amiga/Sprites.cpp @@ -8,6 +8,8 @@ #include "Sprites.hpp" +#include + using namespace Amiga; namespace {