vdp: make the line-at-a-time H40 path match the slot-at-a-time one (fixes #50 artifacts) (#54)

* vdp: make the line-at-a-time H40 path match the slot-at-a-time one

vdp_h40 renders a whole line in one go via vdp_h40_line whenever the caller
asks it to advance by at least MCLKS_LINE. That fast path is a hand unrolled
copy of the ~210 slot cases and had drifted away from them in six places:

- advance_output_line() ran before the whole run of sprite slots instead of at
  slot 179 (BG_START_SLOT + LINEBUF_SIZE/2)
- the external slot at 232 was skipped entirely; the loop already accounts for
  it by running 27 times over 28 slots, but the slot itself never happened
- the loop for slots 245-246 ran three times rather than two, so every line got
  one sprite render too many
- column 0 was rendered inside the column loop instead of interleaved with the
  tail of line slots, leaving 249, 251, 252, 253, 255 and 0 without their map
  reads, renders and output
- read_sprite_x() was batched after the column loop instead of running once per
  column between read_map_scroll_b and render_map_3
- the per column external slot and the two at 161-162 were missing

The net effect was that sprite renders ended up attributed to the wrong line, so
a masking sprite could be applied one line off and leave pixels unmasked that
should have been hidden. Sonic 1's title screen shows this as a stray two pixel
speck inside the 'O'.

Only the x86 68000 core reached any of this. The generated core syncs after
every instruction, so target_cycles - cycles never reaches MCLKS_LINE there and
vdp_h40_line is effectively dead code, which is why the artifacts only ever
showed up on x86 builds.

Verified against Sonic 1 (REV00) over 620 frames: the fast path and the
slot-at-a-time path now produce identical output, the x86 and generated 68000
cores produce identical output, and the traced sequence of VDP operations
matches exactly at 252985 operations. The fast path stays enabled.

* genesis: make refresh delay accounting independent of sync granularity

gen_update_refresh() worked out its delay with a single division and left the
delay cycles to be picked up by the next call, while
gen_update_refresh_free_access() added a second order delay to context->cycles
but never fed it back into refresh_counter. Both made the result depend on how
large a chunk of time the caller happened to hand over, and the two 68000 cores
sync at very different granularity, so they drifted apart.

Fold both into a refresh_advance() helper that walks the counter one interval at
a time and feeds each delay back in, which makes the outcome the same however
the elapsed time is split up.

Measured on Sonic 1 over 60 frames: 969 disagreements in refresh state between
the x86 and generated 68000 cores before, none after. Output impact is small,
the generated core renders identically over 619 frames and the x86 core differs
on 9 of them.

* build: let the Z80 core be selected independently of the 68000 one

NEW_CORE picked both the generated 68000 core and the generated Z80 core, so
there was no way to tell which of the two a difference in behaviour came from.
Split the Z80 half out into NEW_Z80, which NEW_CORE still implies, so that
NEW_Z80=1 on its own pairs the x86 68000 JIT with the generated Z80.

Each conditional was classified by which CPU it belongs to. gst.c turned out to
be handling both cores in one file, which was invisible while they shared a
macro. sms.c and coleco.c are Z80 only machines, so all of theirs moved.

No behaviour change for existing configurations: NEW_CORE builds and plain x86
builds select exactly the same objects and take exactly the same code paths as
before.

Makefile

@@ -256,12 +256,15 @@ ifdef NO_FILE_CHOOSER
 CHOOSER:=nuklear_ui/filechooser_nulll.o
 endif
 
+#The generated Z80 core is normally selected together with the generated 68K
+#core, but the two are independent. NEW_Z80 can be set on its own to pair the
+#JIT 68K with the generated Z80, which is useful for narrowing down which core
+#a timing difference comes from.
 ifdef NEW_CORE
-Z80OBJS=z80.o z80inst.o
+NEW_Z80:=1
 M68KOBJS+= m68k.o
 CFLAGS+= -DNEW_CORE
 else
-Z80OBJS=z80inst.o z80_to_x86.o
 ifeq ($(CPU),x86_64)
 M68KOBJS+= m68k_core.o m68k_core_x86.o
 TRANSOBJS+= gen_x86.o backend_x86.o
@@ -272,6 +275,13 @@ TRANSOBJS+= gen_x86.o backend_x86.o
 endif
 endif
 endif
+
+ifdef NEW_Z80
+Z80OBJS=z80.o z80inst.o
+CFLAGS+= -DNEW_Z80
+else
+Z80OBJS=z80inst.o z80_to_x86.o
+endif
 AUDIOOBJS=ym2612.o ymf262.o ym_common.o psg.o wave.o flac.o vgm.o event_log.o render_audio.o rf5c164.o
 CONFIGOBJS=config.o tern.o util.o paths.o
 NUKLEAROBJS=$(FONT) $(CHOOSER) nuklear_ui/blastem_nuklear.o nuklear_ui/sfnt.o nuklear_ui/debug_ui.o

coleco.c

@@ -10,7 +10,7 @@
 #include "bindings.h"
 #include "saves.h"
 
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 #define Z80_CYCLE cycles
 #define Z80_OPTS opts
 #define z80_handle_code_write(...)
@@ -42,7 +42,7 @@ static uint8_t coleco_controller_read(uint32_t location, void *vcontext)
 
 static void update_interrupts(coleco_context *coleco)
 {
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 	if (coleco->z80->nmi_cycle == CYCLE_NEVER) {
 #else
 	if (coleco->z80->nmi_start == CYCLE_NEVER) {
@@ -203,7 +203,7 @@ static uint8_t load_state(system_header *system, uint8_t slot)
 	coleco_context *coleco = (coleco_context *)system;
 	char *statepath = get_slot_name(system, slot, "state");
 	uint8_t ret;
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 	if (!coleco->z80->native_pc) {
 		ret = get_modification_time(statepath) != 0;
 		if (ret) {
@@ -349,7 +349,7 @@ static void soft_reset(system_header *system)
 {
 	coleco_context *coleco = (coleco_context *)system;
 	z80_assert_reset(coleco->z80, coleco->z80->Z80_CYCLE);
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 	coleco->z80->target_cycle = coleco->z80->sync_cycle = coleco->z80->Z80_CYCLE;
 #endif
 }
@@ -373,7 +373,7 @@ static void request_exit(system_header *system)
 {
 	coleco_context *coleco = (coleco_context *)system;
 	coleco->should_return = 1;
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 	coleco->z80->target_cycle = coleco->z80->sync_cycle = coleco->z80->Z80_CYCLE;
 #endif
 }

coleco.h

@@ -4,7 +4,7 @@
 #include "system.h"
 #include "vdp.h"
 #include "psg.h"
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 #include "z80.h"
 #else
 #include "z80_to_x86.h"

debug.h

@@ -5,9 +5,12 @@
 #include "tern.h"
 #ifdef NEW_CORE
 #include "m68k.h"
-#include "z80.h"
 #else
 #include "m68k_core.h"
+#endif
+#ifdef NEW_Z80
+#include "z80.h"
+#else
 #include "z80_to_x86.h"
 #endif
 #include "upd78k2.h"

genesis.c

@@ -43,15 +43,17 @@ uint32_t MCLKS_PER_68K;
 #define MAX_SOUND_CYCLES 100000
 #endif
 
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 #define Z80_CYCLE cycles
 #define Z80_OPTS opts
 #define z80_handle_code_write(...)
-#define int_num int_priority
 #else
 #define Z80_CYCLE current_cycle
 #define Z80_OPTS options
 #endif
+#ifdef NEW_CORE
+#define int_num int_priority
+#endif
 
 void genesis_serialize(genesis_context *gen, serialize_buffer *buf, uint32_t m68k_pc, uint8_t all)
 {
@@ -451,7 +453,7 @@ static void adjust_int_cycle(m68k_context * context, vdp_context * v_context)
 static void z80_next_int_pulse(z80_context * z_context)
 {
 	genesis_context * gen = z_context->system;
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 	z_context->int_cycle = vdp_next_vint_z80(gen->vdp);
 	z_context->int_end_cycle = z_context->int_cycle + Z80_INT_PULSE_MCLKS;
 	z_context->int_value = 0xFF;
@@ -468,7 +470,7 @@ static void sync_z80(genesis_context *gen, uint32_t mclks)
 	z80_context *z_context = gen->z80;
 #ifndef NO_Z80
 	if (z80_enabled) {
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 		if (z_context->int_cycle == 0xFFFFFFFFU) {
 			z80_next_int_pulse(z_context);
 		}
@@ -515,14 +517,31 @@ static void sync_sound(genesis_context * gen, uint32_t target)
 #define REFRESH_INTERVAL 128
 #define REFRESH_DELAY 2
 
-void gen_update_refresh(m68k_context *context)
+//Advances the refresh counter by elapsed master cycles and returns the refresh
+//delay incurred along the way. The delay cycles are themselves time in which the
+//counter keeps running, so they have to be fed back into it. Doing that in a loop
+//rather than in a single division is what makes the result independent of how
+//large the chunks are that the caller happens to hand us; the interpreter and the
+//JIT sync at different granularity, so anything chunk-dependent makes the two
+//cores drift apart.
+static uint32_t refresh_advance(genesis_context *gen, uint32_t elapsed)
 {
 	uint32_t interval = MCLKS_PER_68K * REFRESH_INTERVAL;
+	uint32_t delay = 0;
+	gen->refresh_counter += elapsed;
+	while (gen->refresh_counter >= interval) {
+		gen->refresh_counter -= interval;
+		gen->refresh_counter += REFRESH_DELAY * MCLKS_PER_68K;
+		delay += REFRESH_DELAY * MCLKS_PER_68K;
+	}
+	return delay;
+}
+
+void gen_update_refresh(m68k_context *context)
+{
 	genesis_context *gen = context->system;
-	gen->refresh_counter += context->cycles - gen->last_sync_cycle;
+	context->cycles += refresh_advance(gen, context->cycles - gen->last_sync_cycle);
 	gen->last_sync_cycle = context->cycles;
-	context->cycles += REFRESH_DELAY * MCLKS_PER_68K * (gen->refresh_counter / interval);
-	gen->refresh_counter = gen->refresh_counter % interval;
 }
 
 void gen_update_refresh_free_access(m68k_context *context)
@@ -532,21 +551,12 @@ void gen_update_refresh_free_access(m68k_context *context)
 	if (before < gen->last_sync_cycle) {
 		return;
 	}
-	//Add refresh delays for any accesses that happened beofre the current one
-	gen->refresh_counter += before - gen->last_sync_cycle;
-	uint32_t interval = MCLKS_PER_68K * REFRESH_INTERVAL;
-	uint32_t delay = REFRESH_DELAY * MCLKS_PER_68K * (gen->refresh_counter / interval);
-	if (delay) {
-		//To avoid the extra cycles being absorbed in the refresh free update below, we need to update again
-		gen->refresh_counter = gen->refresh_counter % interval;
-		gen->refresh_counter += delay;
-		delay += REFRESH_DELAY * MCLKS_PER_68K * (gen->refresh_counter / interval);
-		context->cycles += delay;
-	}
+	//Add refresh delays for any accesses that happened before the current one
+	context->cycles += refresh_advance(gen, before - gen->last_sync_cycle);
 	gen->last_sync_cycle = context->cycles;
 	//advance refresh counter for the current access, but don't generate delays
 	gen->refresh_counter += 4*MCLKS_PER_68K;
-	gen->refresh_counter = gen->refresh_counter % interval;
+	gen->refresh_counter = gen->refresh_counter % (MCLKS_PER_68K * REFRESH_INTERVAL);
 }
 
 void gen_update_refresh_no_wait(m68k_context *context)
@@ -678,14 +688,14 @@ static m68k_context *sync_components(m68k_context * context, uint32_t address)
 			}
 #endif
 		}
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 		if (gen->header.save_state) {
 #else
 		if (gen->header.save_state && (z_context->pc || !z_context->native_pc || z_context->reset || !z_context->busreq)) {
 #endif
 			uint8_t slot = gen->header.save_state - 1;
 			gen->header.save_state = 0;
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 			if (z_context->native_pc && !z_context->reset) {
 				//advance Z80 core to the start of an instruction
 				while (!z_context->pc)
@@ -1023,7 +1033,7 @@ static void enter_z80_debugger_immediately(void *context)
 	if (gen->z80->sync_cycle > gen->z80->Z80_CYCLE + 1) {
 		gen->z80->sync_cycle = gen->z80->Z80_CYCLE + 1;
 	}
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 	if (gen->z80->target_cycle > gen->z80->sync_cycle) {
 		gen->z80->target_cycle = gen->z80->sync_cycle;
 	}
@@ -2907,7 +2917,7 @@ static genesis_context *shared_init(uint32_t system_opts, rom_info *rom, uint8_t
 	z80_options *z_opts = malloc(sizeof(z80_options));
 	init_z80_opts(z_opts, z80_map, 5, NULL, 0, MCLKS_PER_Z80, 0xFFFF);
 	gen->z80 = init_z80_context(z_opts);
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 	gen->z80->next_int_pulse = z80_next_int_pulse;
 #endif
 	z80_assert_reset(gen->z80, 0);

genesis.h

@@ -10,9 +10,12 @@
 #include "system.h"
 #ifdef NEW_CORE
 #include "m68k.h"
-#include "z80.h"
 #else
 #include "m68k_core.h"
+#endif
+#ifdef NEW_Z80
+#include "z80.h"
+#else
 #include "z80_to_x86.h"
 #endif
 #include "ym2612.h"

gst.c

@@ -183,7 +183,7 @@ uint8_t z80_load_gst(z80_context * context, FILE * gstfile)
 	}
 	uint8_t * curpos = regdata;
 	uint8_t f = *(curpos++);
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 	context->main[6] = context->last_flag_result = f;
 	context->chflags = ((f & 1) ? 0x80 : 0) | ((f & 0x10) ? 8 : 0);
 	context->nflag = f & 2;
@@ -226,7 +226,7 @@ uint8_t z80_load_gst(z80_context * context, FILE * gstfile)
 	context->sp = read_le_16(curpos);
 	curpos += 4;
 	f = *(curpos++);
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 	context->alt[6] = f;
 	context->alt[7] = *curpos;
 	curpos += 3;
@@ -281,12 +281,12 @@ uint8_t z80_load_gst(z80_context * context, FILE * gstfile)
 	{
 		if (context->mem_pointers[0][i] != buffer[i]) {
 			context->mem_pointers[0][i] = buffer[i];
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 			z80_handle_code_write(i, context);
 #endif
 		}
 	}
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 	context->native_pc = NULL;
 	context->extra_pc = NULL;
 #endif
@@ -371,7 +371,7 @@ uint8_t z80_save_gst(z80_context * context, FILE * gstfile)
 	uint8_t regdata[GST_Z80_REG_SIZE];
 	uint8_t * curpos = regdata;
 	memset(regdata, 0, sizeof(regdata));
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 	uint8_t f = context->last_flag_result;
 	if (context->zflag) { f |= 0x40; }
 	if (context->chflags & 8) { f |= 0x10; }
@@ -417,7 +417,7 @@ uint8_t z80_save_gst(z80_context * context, FILE * gstfile)
 	curpos += 4;
 	write_le_16(curpos, context->sp);
 	curpos += 4;
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 	*(curpos++) = context->alt[6];
 	*curpos = context->alt[7];
 	

sms.c

@@ -11,7 +11,7 @@
 #include "bindings.h"
 #include "korean_sms_multi.h"
 
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 #define Z80_CYCLE cycles
 #define Z80_OPTS opts
 #define z80_handle_code_write(...)
@@ -81,7 +81,7 @@ static void update_interrupts(sms_context *sms)
 {
 	uint32_t vint = vdp_next_vint(sms->vdp);
 	uint32_t hint = vdp_next_hint(sms->vdp);
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 	sms->z80->int_cycle = vint < hint ? vint : hint;
 	z80_sync_cycle(sms->z80, sms->z80->sync_cycle);
 #else
@@ -1081,7 +1081,7 @@ static uint8_t load_state(system_header *system, uint8_t slot)
 	sms_context *sms = (sms_context *)system;
 	char *statepath = get_slot_name(system, slot, "state");
 	uint8_t ret;
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 	if (!sms->z80->native_pc) {
 		ret = get_modification_time(statepath) != 0;
 		if (ret) {
@@ -1133,7 +1133,7 @@ static void run_sms(system_header *system)
 				}
 			}
 		}
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 		if ((system->enter_debugger || sms->z80->wp_hit) && sms->z80->pc) {
 			if (!sms->z80->wp_hit) {
 				system->enter_debugger = 0;
@@ -1143,7 +1143,7 @@ static void run_sms(system_header *system)
 #endif
 		}
 #endif
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 		if (sms->z80->nmi_cycle == CYCLE_NEVER) {
 #else
 		if (sms->z80->nmi_start == CYCLE_NEVER) {
@@ -1154,7 +1154,7 @@ static void run_sms(system_header *system)
 			}
 		}
 
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 		if (system->enter_debugger || sms->z80->wp_hit) {
 			target_cycle = sms->z80->Z80_CYCLE + 1;
 		}
@@ -1254,7 +1254,7 @@ static void soft_reset(system_header *system)
 {
 	sms_context *sms = (sms_context *)system;
 	z80_assert_reset(sms->z80, sms->z80->Z80_CYCLE);
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 	sms->z80->target_cycle = sms->z80->sync_cycle = sms->z80->Z80_CYCLE;
 #endif
 }
@@ -1279,7 +1279,7 @@ static void request_exit(system_header *system)
 {
 	sms_context *sms = (sms_context *)system;
 	sms->should_return = 1;
-#ifndef NEW_CORE
+#ifndef NEW_Z80
 	sms->z80->target_cycle = sms->z80->sync_cycle = sms->z80->Z80_CYCLE;
 #endif
 }

sms.h

@@ -4,7 +4,7 @@
 #include "system.h"
 #include "vdp.h"
 #include "psg.h"
-#ifdef NEW_CORE
+#ifdef NEW_Z80
 #include "z80.h"
 #else
 #include "z80_to_x86.h"

vdp.c

@@ -3661,12 +3661,26 @@ static void vdp_h40_line(vdp_context * context)
 			}
 		}
 	}
-	advance_output_line(context);
 	//169-242 (inclusive)
 	for (int i = 0; i < 27; i++)
 	{
+		if (169 + i == BG_START_SLOT + LINEBUF_SIZE/2) {
+			//the slot-at-a-time path advances the output line here, not before
+			//the whole run of sprite slots
+			advance_output_line(context);
+			if (!context->output) {
+				context->output = dummy_buffer;
+			}
+		}
 		render_sprite_cells(context);
 		scan_sprite_table(context->vcounter, context);
+		if (i == 16) {
+			//the loop walks slots 169-182 (i 0-13) and then 229-242 (i 14-26),
+			//so i 16 is slot 231. Slot 232 is an external slot rather than a
+			//sprite render slot, which is why this runs 27 times over 28 slots,
+			//but the external slot itself still has to happen.
+			external_slot(context);
+		}
 	}
 	//243
 	render_sprite_cells(context);
@@ -3694,8 +3708,8 @@ static void vdp_h40_line(vdp_context * context)
 	context->hscroll_b = context->vdpmem[address+2] << 8 | context->vdpmem[address+3];
 	context->hscroll_b_fine = context->hscroll_b & 0xF;
 	//printf("%d: HScroll A: %d, HScroll B: %d\n", context->vcounter, context->hscroll_a, context->hscroll_b);
-	//243-246 inclusive
-	for (int i = 0; i < 3; i++)
+	//245-246 inclusive
+	for (int i = 0; i < 2; i++)
 	{
 		render_sprite_cells(context);
 		scan_sprite_table(context->vcounter, context);
@@ -3723,19 +3737,27 @@ static void vdp_h40_line(vdp_context * context)
 	scan_sprite_table(context->vcounter, context);
 	context->buf_a_off = (context->buf_a_off + SCROLL_BUFFER_DRAW) & SCROLL_BUFFER_MASK;
 	context->buf_b_off = (context->buf_b_off + SCROLL_BUFFER_DRAW) & SCROLL_BUFFER_MASK;
+	//249
+	read_map_scroll_a(0, context->vcounter, context);
 	//250
 	render_sprite_cells(context);
 	scan_sprite_table(context->vcounter, context);
 	//251
+	render_map_1(context);
 	scan_sprite_table(context->vcounter, context);//Just a guess
 	//252
+	render_map_2(context);
 	scan_sprite_table(context->vcounter, context);//Just a guess
+	//253
+	read_map_scroll_b(0, context->vcounter, context);
 	//254
 	render_sprite_cells(context);
 	scan_sprite_table(context->vcounter, context);
 	//255
+	render_map_3(context);
 	scan_sprite_table(context->vcounter, context);
 	//0
+	render_map_output(context->vcounter, 0, context);
 	scan_sprite_table(context->vcounter, context);//Just a guess
 	//seems like the sprite table scan fills a shift register
 	//values are FIFO, but unused slots precede used slots
@@ -3744,21 +3766,29 @@ static void vdp_h40_line(vdp_context * context)
 	context->cur_slot = context->slot_counter;
 	context->sprite_x_offset = 0;
 	context->sprite_draws = MAX_SPRITES_LINE;
-	//background planes and layer compositing
-	for (int col = 0; col < 42; col+=2)
+	//background planes and layer compositing, with sprite rendering phase 2
+	//interleaved exactly as COLUMN_RENDER_BLOCK does it: one read_sprite_x per
+	//column between read_map_scroll_b and render_map_3. Batching them after the
+	//loop instead changes which line a sprite ends up attributed to.
+	//Column 0 is rendered by the tail-of-line slots above, so this starts at 2.
+	for (int col = 2; col < 42; col+=2)
 	{
 		read_map_scroll_a(col, context->vcounter, context);
+		if (col & 7) {
+			//COLUMN_RENDER_BLOCK has an external slot here; the refresh variant
+			//used for every fourth column does not
+			external_slot(context);
+		}
 		render_map_1(context);
 		render_map_2(context);
 		read_map_scroll_b(col, context->vcounter, context);
+		read_sprite_x(context->vcounter, context);
 		render_map_3(context);
 		render_map_output(context->vcounter, col, context);
 	}
-	//sprite rendering phase 2
-	for (int i = 0; i < MAX_SPRITES_LINE; i++)
-	{
-		read_sprite_x(context->vcounter, context);
-	}
+	//161-162
+	external_slot(context);
+	external_slot(context);
 	//163
 	context->cur_slot = MAX_SPRITES_LINE-1;
 	memset(context->linebuf, 0, LINEBUF_SIZE);