From a9519ebcd8cbf9cd48454828cac96c3d5b735376 Mon Sep 17 00:00:00 2001 From: dave-br Date: Thu, 19 Oct 2023 17:10:27 -0700 Subject: video/mc6847.cpp: Fix synchronization between MC6847 video display generator and screen device (#11594) --- src/devices/video/mc6847.cpp | 159 +++++++++++++++++++++++++++++++------------ 1 file changed, 116 insertions(+), 43 deletions(-) diff --git a/src/devices/video/mc6847.cpp b/src/devices/video/mc6847.cpp index 57649fe39d2..0d347eb1ed9 100644 --- a/src/devices/video/mc6847.cpp +++ b/src/devices/video/mc6847.cpp @@ -61,7 +61,7 @@ GM2-0 pins. This needs to be confirmed. The MC6847 datasheet states that a scanline is 227.5 clock cycles, - but experimentation suggests that it is 228. The game "Dragon Fire" + but experimentation suggests that it is 228. coco_cart:drgnfire has a fine tuned loop that runs in 57 clock cycles by the CPU's reckoning (228 actual clock cycles) and would not function correctly if skew existed. SockMaster has confirmed that scanlines are in @@ -76,27 +76,45 @@ #include "emu.h" #include "video/mc6847.h" +#define LOG_SCANLINE (1U << 1) +#define LOG_HSYNC (1U << 2) +#define LOG_FSYNC (1U << 3) +#define LOG_FLUSH (1U << 4) +#define LOG_INPUT (1U << 5) +#define LOG_NEXTLINE (1U << 6) +#define VERBOSE (0) +#include "logmacro.h" //************************************************************************** // CONSTANTS //************************************************************************** -#define TOP_BORDER 25 -#define USE_HORIZONTAL_CLIP false +namespace { +constexpr int LINES_TOP_BORDER = 25; +constexpr int LINES_ACTIVE_VIDEO = 192; +constexpr int LINES_BOTTOM_BORDER = 26; +constexpr int LINES_VERTICAL_RETRACE = 6; +constexpr int LINES_UNTIL_RETRACE = LINES_TOP_BORDER + LINES_ACTIVE_VIDEO + LINES_BOTTOM_BORDER; +constexpr int LINES_UNTIL_VBLANK = LINES_UNTIL_RETRACE + LINES_VERTICAL_RETRACE; -#define TIMER_HSYNC_PERIOD (228) -#define TIMER_HSYNC_OFF_TIME (10.0) -#define TIMER_HSYNC_ON_TIME (TIMER_HSYNC_OFF_TIME + 16.5) -#define TIMER_FSYNC_OFF_TIME (TIMER_HSYNC_PERIOD * TOP_BORDER + TIMER_HSYNC_ON_TIME) -#define TIMER_FSYNC_ON_TIME (TIMER_HSYNC_PERIOD * (TOP_BORDER + 192) + TIMER_HSYNC_ON_TIME) +constexpr bool USE_HORIZONTAL_CLIP = false; -#define LOG_SCANLINE (1U << 1) -#define LOG_HSYNC (1U << 2) -#define LOG_FSYNC (1U << 3) -#define LOG_FLUSH (1U << 4) -#define LOG_INPUT (1U << 5) -#define VERBOSE (0) -#include "logmacro.h" +constexpr int CLOCKS_HSYNC_PERIOD = 228; +constexpr int CLOCKS_FRONT_PORCH_DURATION = 7; +constexpr int CLOCKS_ACTIVE_VIDEO = 128; +constexpr int CLOCKS_L_OR_R_BORDER = 29; +constexpr int CLOCKS_ACTIVE_VIDEO_AND_BORDERS = CLOCKS_ACTIVE_VIDEO + (2*CLOCKS_L_OR_R_BORDER); +constexpr int TIMER_HSYNC_OFF_TIME = CLOCKS_ACTIVE_VIDEO_AND_BORDERS + CLOCKS_FRONT_PORCH_DURATION; +constexpr int TIMER_HSYNC_ON_TIME = TIMER_HSYNC_OFF_TIME + 16; + +// FSYNC occurs just after active video, just before right border +constexpr int TIMER_FSYNC_TIME = CLOCKS_ACTIVE_VIDEO + CLOCKS_L_OR_R_BORDER; + +// These units are the screen device's bitmap pixels. Multiplied by 2 because +// the pixel clock is the mc6847's clock * 2 +constexpr int BMP_L_OR_R_BORDER = CLOCKS_L_OR_R_BORDER * 2; +constexpr int BMP_ACTIVE_VIDEO = CLOCKS_ACTIVE_VIDEO * 2; +} // anonymous namespace const uint32_t mc6847_base_device::s_palette[mc6847_base_device::PALETTE_LENGTH] = @@ -161,9 +179,9 @@ void mc6847_friend_device::device_start() m_hsync_off_timer = timer_alloc(FUNC(mc6847_friend_device::change_horizontal_sync), this); m_fsync_timer = timer_alloc(FUNC(mc6847_friend_device::change_field_sync), this); - m_frame_timer->adjust(clocks_to_attotime(0), 0, clocks_to_attotime(m_tpfs * TIMER_HSYNC_PERIOD * m_divider)); - m_hsync_on_timer->adjust(clocks_to_attotime(TIMER_HSYNC_ON_TIME * m_divider), 1, clocks_to_attotime(TIMER_HSYNC_PERIOD * m_divider)); - m_hsync_off_timer->adjust(clocks_to_attotime(TIMER_HSYNC_OFF_TIME * m_divider), 0, clocks_to_attotime(TIMER_HSYNC_PERIOD * m_divider)); + m_frame_timer->adjust(clocks_to_attotime(0), 0, clocks_to_attotime(m_tpfs * CLOCKS_HSYNC_PERIOD * m_divider)); + m_hsync_on_timer->adjust(clocks_to_attotime(TIMER_HSYNC_ON_TIME * m_divider), 1, clocks_to_attotime(CLOCKS_HSYNC_PERIOD * m_divider)); + m_hsync_off_timer->adjust(clocks_to_attotime(TIMER_HSYNC_OFF_TIME * m_divider), 0, clocks_to_attotime(CLOCKS_HSYNC_PERIOD * m_divider)); m_top_border_scanlines = 0; m_body_scanlines = 0; @@ -173,7 +191,7 @@ void mc6847_friend_device::device_start() m_logical_scanline_zone = 0; m_field_sync = false; m_horizontal_sync = false; - set_geometry(25, 192, false); + set_geometry(LINES_TOP_BORDER, LINES_ACTIVE_VIDEO, false); /* save states */ save_item(NAME(m_physical_scanline)); @@ -184,6 +202,8 @@ void mc6847_friend_device::device_start() /* artifacting */ m_artifacter.setup_config(this); + + screen().reset_origin(0, 0); } @@ -214,6 +234,13 @@ void mc6847_friend_device::device_post_load() //------------------------------------------------- // update_field_sync_timer +// +// Unlike hsync, fsync uses a single timer to handle +// both ON (FS high) and OFF (FS low). When hsync-ON +// occurs for a line before we expect FS to change, +// this is called to set up an fsync for the next line. +// Which line of the field that is determines whether +// the fsync will signal an ON or OFF //------------------------------------------------- void mc6847_friend_device::update_field_sync_timer() @@ -226,7 +253,7 @@ void mc6847_friend_device::update_field_sync_timer() if (expected_field_sync != m_field_sync) { // if so, determine the duration - attotime duration = clocks_to_attotime(160 * m_divider); + attotime duration = clocks_to_attotime(TIMER_FSYNC_TIME * m_divider); // and reset the timer m_fsync_timer->adjust(duration, expected_field_sync ? 1 : 0); @@ -283,7 +310,7 @@ TIMER_CALLBACK_MEMBER(mc6847_friend_device::change_horizontal_sync) // are we on a rising edge? if (line && !m_horizontal_sync) { - LOGMASKED(LOG_SCANLINE, "%s: change_horizontal_sync(): Recording scanline\n", describe_context()); + LOGMASKED(LOG_SCANLINE, "%s: change_horizontal_sync(): Recording scanline. line=%d, (vpos,hpos)=(%d,%d), m_physical_scanline=%d\n", describe_context(), line ? 1 : 0, screen().vpos(), screen().hpos(), m_physical_scanline); // first store the scanline { @@ -298,7 +325,7 @@ TIMER_CALLBACK_MEMBER(mc6847_friend_device::change_horizontal_sync) case SCANLINE_ZONE_BODY: m_recording_scanline = true; if (m_partial_scanline_clocks > 0) - record_partial_body_scanline(m_physical_scanline, m_logical_scanline, m_partial_scanline_clocks, 228); + record_partial_body_scanline(m_physical_scanline, m_logical_scanline, m_partial_scanline_clocks, CLOCKS_HSYNC_PERIOD); else record_body_scanline(m_physical_scanline, m_logical_scanline); m_recording_scanline = false; @@ -326,7 +353,7 @@ TIMER_CALLBACK_MEMBER(mc6847_friend_device::change_horizontal_sync) m_horizontal_sync = line; // log if apprpriate - LOGMASKED(LOG_HSYNC, "%s: change_horizontal_sync(): line=%d\n", describe_context(), line ? 1 : 0); + LOGMASKED(LOG_HSYNC, "%s: change_horizontal_sync(): line=%d, (vpos,hpos)=(%d,%d), m_physical_scanline=%d\n", describe_context(), line ? 1 : 0, screen().vpos(), screen().hpos(), m_physical_scanline); // invoke callback m_write_hsync(line); @@ -351,7 +378,7 @@ TIMER_CALLBACK_MEMBER(mc6847_friend_device::change_field_sync) m_field_sync = line; /* log if apprpriate */ - LOGMASKED(LOG_FSYNC, "%s: change_field_sync(): line=%d\n", describe_context(), line ? 1 : 0); + LOGMASKED(LOG_FSYNC, "%s: change_field_sync(): line=%d, (vpos,hpos)=(%d,%d), m_physical_scanline=%d\n", describe_context(), line ? 1 : 0, screen().vpos(), screen().hpos(), m_physical_scanline); /* invoke callback */ m_write_fsync(line); @@ -381,13 +408,13 @@ inline void mc6847_friend_device::next_scanline() m_logical_scanline = 0; m_logical_scanline_zone = SCANLINE_ZONE_TOP_BORDER; } - else if ((m_logical_scanline_zone < SCANLINE_ZONE_VBLANK) && (m_physical_scanline >= 25+192+26+6)) + else if ((m_logical_scanline_zone < SCANLINE_ZONE_VBLANK) && (m_physical_scanline >= LINES_UNTIL_VBLANK)) { /* we're now into vblank */ m_logical_scanline = 0; m_logical_scanline_zone = SCANLINE_ZONE_VBLANK; } - else if ((m_logical_scanline_zone < SCANLINE_ZONE_RETRACE) && (m_physical_scanline >= 25+192+26)) + else if ((m_logical_scanline_zone < SCANLINE_ZONE_RETRACE) && (m_physical_scanline >= LINES_UNTIL_RETRACE)) { /* we're now into retrace */ m_logical_scanline = 0; @@ -406,6 +433,15 @@ inline void mc6847_friend_device::next_scanline() m_logical_scanline_zone = SCANLINE_ZONE_BOTTOM_BORDER; enter_bottom_border(); } + + LOGMASKED(\ + LOG_NEXTLINE, \ + "mc6847_friend_device::next_scanline(): (vpos,hpos)=(%d,%d), m_physical_scanline='%d', m_logical_scanline_zone='%s', m_logical_scanline='%d'\n", \ + screen().vpos(), \ + screen().hpos(), \ + m_physical_scanline, \ + scanline_zone_string((scanline_zone)m_logical_scanline_zone), \ + m_logical_scanline); } @@ -514,7 +550,7 @@ std::string mc6847_friend_device::describe_context() const //------------------------------------------------- mc6847_base_device::mc6847_base_device(const machine_config &mconfig, device_type type, const char *tag, device_t *owner, uint32_t clock, const uint8_t *fontdata, double tpfs) : - mc6847_friend_device(mconfig, type, tag, owner, clock, fontdata, (type == MC6847T1_NTSC) || (type == MC6847T1_PAL), tpfs, 25+191, 1, true), + mc6847_friend_device(mconfig, type, tag, owner, clock, fontdata, (type == MC6847T1_NTSC) || (type == MC6847T1_PAL), tpfs, LINES_TOP_BORDER + LINES_ACTIVE_VIDEO - 1, 1, true), m_input_cb(*this, 0), m_black_and_white(false), m_fixed_mode(0), @@ -559,11 +595,23 @@ void mc6847_base_device::device_config_complete() if (!screen().refresh_attoseconds()) { - // FIXME: use correct raw parameters rather than this nonsense - screen().set_refresh_hz(m_tpfs > 310.0 ? 50 : 60); - screen().set_size(320, 243); - screen().set_visarea(0, 320-1, 1, 241-1); - screen().set_vblank_time(0); + // It is preferred to initialize the screen device using set_raw. + // Setting screen device's raw parameters requires values in terms of pixels. + // Multiplying the 6847's clock by 2 produces a pixel clock that allows + // 256 pixels of active video area per line, with apparently no roundoff + // error or drift. + // + // Note: This math is valid for NTSC. If / when PAL is implemented, m_tpfs + // can be used to determine if this 6847 is PAL, and a different set_raw + // call can be made if helpful. + screen().set_raw( + (uint32_t)(clock() * 2), + 456, // htotal + 0, // hbend + BMP_L_OR_R_BORDER * 2 + BMP_ACTIVE_VIDEO, // hbstart + m_tpfs, // vtotal + 0, // vbend + LINES_UNTIL_RETRACE); // vbstart } if (!screen().has_screen_update()) @@ -646,7 +694,7 @@ void mc6847_base_device::record_scanline_res(int scanline, int32_t start_pos, in { // set address at beginning of line if (pos == 0) - m_video_address = scanline / (192 / yres) * sample_count; + m_video_address = scanline / (LINES_ACTIVE_VIDEO / yres) * sample_count; // are we sampling this position? if ((pos % sample_modulo) == 0) @@ -679,7 +727,7 @@ void mc6847_base_device::record_scanline_res(int scanline, int32_t start_pos, in inline void mc6847_base_device::record_body_scanline(uint16_t physical_scanline, uint16_t scanline, int32_t start_pos, int32_t end_pos) { // sanity checks - assert(scanline < 192); + assert(scanline < LINES_ACTIVE_VIDEO); if (m_mode & MODE_AG) { @@ -752,11 +800,36 @@ void mc6847_base_device::record_partial_body_scanline(uint16_t physical_scanline //------------------------------------------------- // scanline_position_from_clock +// +// The hsync ON interrupt marks the end of the +// previous line and the beginning of the next +// line. Measuring clocks since hsync ON gives +// an offset into the current line. This converts +// the clocks offset into a "position" offset for +// use by the record functions as index into +// the video_scanline arrays //------------------------------------------------- int32_t mc6847_base_device::scanline_position_from_clock(int32_t clocks_since_hsync) { - return (clocks_since_hsync - 20) / 4; + // This value determined by experimentation with coco_cart:drgnfire, + // which uses precise timing measurements to change video modes + // multiple times mid-line. This value seems to minimize video + // garbage, but doesn't eliminate it, so there are likely timing + // issues elsewhere. + // + // Unfortunately, I don't understand why this value isn't + // simply 0. I believe hsync-on appears AFTER the front-porch + + // HS pulse + back-porch combo, which should be the entirety of + // the horizontal blanking. But this value makes coco_cart:drgnfire + // significantly better than 0 does. + const int CLOCKS_OFFSET_TO_FIRST_VISIBLE_PIXEL = 25; + + // Dividing by 4 converts 128 clocks per horizontal active video + // into 32 bytes per horizontal active video. The borders come + // along for the ride. They are 58 clocks total (for L + R), and + // dividing by 4 gives 14.5 bytes + return (clocks_since_hsync - CLOCKS_OFFSET_TO_FIRST_VISIBLE_PIXEL) / 4; } @@ -809,12 +882,12 @@ inline mc6847_base_device::pixel_t mc6847_base_device::border_value(uint8_t mode uint32_t mc6847_base_device::screen_update(screen_device &screen, bitmap_rgb32 &bitmap, const rectangle &cliprect) { - int base_x = 32; - int base_y = 25; + int base_x = BMP_L_OR_R_BORDER; + int base_y = LINES_TOP_BORDER; int x, x2, y, width; bool is_mc6847t1 = (type() == MC6847T1_NTSC) || (type() == MC6847T1_PAL); int min_x = USE_HORIZONTAL_CLIP ? cliprect.min_x : 0; - int max_x = USE_HORIZONTAL_CLIP ? cliprect.max_x : (base_x * 2 + 256 - 1); + int max_x = USE_HORIZONTAL_CLIP ? cliprect.max_x : (base_x * 2 + BMP_ACTIVE_VIDEO - 1); int min_y = cliprect.min_y; int max_y = cliprect.max_y; const pixel_t *palette = m_palette; @@ -832,7 +905,7 @@ uint32_t mc6847_base_device::screen_update(screen_device &screen, bitmap_rgb32 & } } - for (y = std::max(0, min_y - base_y); y < std::min(192, max_y - base_y); y++) + for (y = std::max(0, min_y - base_y); y < std::min(LINES_ACTIVE_VIDEO, max_y - base_y); y++) { /* left border */ for (x = min_x; x < base_x; x++) @@ -867,7 +940,7 @@ uint32_t mc6847_base_device::screen_update(screen_device &screen, bitmap_rgb32 & /* right border */ if (width) - for (x = base_x + 256; x <= max_x; x++) + for (x = base_x + BMP_ACTIVE_VIDEO; x <= max_x; x++) *bitmap_addr(bitmap, y + base_y, x) = border_value(m_data[y].m_mode[width - 1], palette, is_mc6847t1); /* artifacting */ @@ -881,13 +954,13 @@ uint32_t mc6847_base_device::screen_update(screen_device &screen, bitmap_rgb32 & } - width = m_data[191].m_sample_count; + width = m_data[LINES_ACTIVE_VIDEO-1].m_sample_count; /* bottom border */ if (width) - for (y = base_y + 192; y <= max_y; y++) + for (y = base_y + LINES_ACTIVE_VIDEO - 1; y <= max_y; y++) for (x = min_x; x <= max_x; x++) - *bitmap_addr(bitmap, y, x) = border_value(m_data[191].m_mode[width - 1], palette, is_mc6847t1); + *bitmap_addr(bitmap, y, x) = border_value(m_data[LINES_ACTIVE_VIDEO - 1].m_mode[width - 1], palette, is_mc6847t1); return 0; } -- cgit v1.2.3