#189 ROMCS handling broken with more than one romcs peripherals

open
nobody
None
5
2013-01-01
2009-10-16
Gergely Szasz
No

If more than one peripherale share the ROMCS line than they clash with the "last" because there is only one romcs memory_map (memory_map_romcs[]).

Here is two patch:
- first change the common memory_map_romcs[] to local memory_map_romcs[]s for every peripherials
- the second change memory_map_read[] and memory_map_write[] to pointers

Discussion

  • Gergely Szasz
    Gergely Szasz
    2009-10-16

     
    Attachments
  • Gergely Szasz
    Gergely Szasz
    2009-10-16

     
    Attachments
  • I think that the "romcs" memory pages should be dynamically allocated as required from the memory pool (e.g. see if2_from_snapshot() ) rather than being statically allocated in each periph file.

     
  • Gergely Szasz
    Gergely Szasz
    2009-10-19

    Hmm... the data itself dynamically allocated of course (the patch does not change this area at all), only the administration area (the memory_page struct) 'multiplicated' in order to each periph have their own.

     
  • I see, you are correct and I was confused.

    The only additional point I have is that it would be nice if each "periph" ROM block was named for the peripheral (e.g. in if2.c memory_map_romcs -> if2_memory_map_rom or similar) - I'll have a pass at doing so.

     
  • Committed a slightly amended fuse.romcs_a00.diff in revision 4099.

    I have looked at fuse.romcs_b00.diff and it does exactly as you say, changes memory_map_read and memory_map_write to pointers, but I wasn't sure what the benefit was? I don't see any particular downside either, but I wondered what motivated the change?

     
  • "what the benefit was?"
    only, we do not copy structs (needlessly), just pointers. (4 byte instead of 40)

     
  • In this case I think we are likely swapping an uncommon 40 byte copy when changing paging layout for a pointer indirection every time we check for contention on memory reads and writes - have you done any profiling to check whether this is a performance win?

     
  • "have you done any profiling..." didn't ... :(
    but we already use indirection when we check contention on memory read/write:

    libspectrum_byte
    readbyte( libspectrum_word address )
    {
    libspectrum_word bank;
    memory_page *mapping;

    bank = address >> 13;
    mapping = &memory_map_read[ bank ];

    if( debugger_mode != DEBUGGER_MODE_INACTIVE )
    debugger_check( DEBUGGER_BREAKPOINT_TYPE_READ, address );

    if( mapping->contended ) tstates += ula_contention[ tstates ];
    tstates += 3;

    if( opus_active && address >= 0x2800 && address < 0x3800 )
    return opus_read( address );

    return mapping->page[ address & 0x1fff ];
    }

    ... the writebyte() is quite identical too...

     
  • All the following routines are macros as they are performance-critical, and we would be adding extra indirection here. I'd be happy to apply if we can demonstrate that we get a performance benefit:

    /* Get the appropriate contended memory delay. Use a macro for performance
    reasons in the main core, but a function for flexibility when building
    the core tester */

    #ifndef CORETEST

    #define contend_read(address,time) \ if( memory_map_read[ (address) >> 13 ].contended ) \ tstates += ula_contention[ tstates ]; \ tstates += (time);

    #define contend_read_no_mreq(address,time) \ if( memory_map_read[ (address) >> 13 ].contended ) \ tstates += ula_contention_no_mreq[ tstates ]; \ tstates += (time);

    #define contend_write_no_mreq(address,time) \ if( memory_map_write[ (address) >> 13 ].contended ) \ tstates += ula_contention_no_mreq[ tstates ]; \ tstates += (time);