Re: [hatari-devel] DSP patch suggestion

[Andreas Grabher <andreas_g86@xxxxxxxxxx>]

> Am 03.12.2023 um 12:09 schrieb Nicolas Pomarède <npomarede@xxxxxxxxxxxx>:
> 
> Le 03/12/2023 à 11:09, Andreas Grabher a écrit :
>> Hello all,
>> I created a new patch that will cleanly merge with Hatari. It contains these changes:
>> Changes without (noticeable) effect:
>>  * dsp.c: Changing "P ram" to "P rom“ in spaces[2][1]. This element of
>>    the array is unused by Hatari and the change therefore has zero
>>    effect but makes things future proof in case one wants to add the
>>    bootstrap ROM later.
>>  * dsp.c: Moving the check for program space above the check for
>>    internal RAM. This change has no functional effect but makes things
>>    future proof in case one wants to add the bootstrap ROM later. Very
>>    minor side effect is that one additional condition is checked in
>>    case of X and Y internal RAM access. Effect on performance is not
>>    noticeable (if one wants to optimise for performance many other
>>    things could be improved). No other accesses are affected.
>>  * dsp_core.c: Removing duplicate HSR initialisation. This has zero effect.
>>  * dsp_core.c: Moving dsp_host_interrupt initialisation to a different
>>    place inside the function. This is just cleaning and has zero effect.
>>  * dsp_core.h: Adding rom[2] (P ROM). This change has no functional
>>    effect but makes things future proof in case one wants to add the
>>    bootstrap ROM later.
>>  * dsp_cpu.c: Moving check for program space above check for internal
>>    RAM. See comments in point 2.
>> Changes with effect:
>>  * dsp_core.c: Adding host interface initialisation. This function of
>>    the DSP was not simulated previously but is required for some DSP
>>    code on the NeXT. I don’t know if any Hatari program makes use of
>>    this function but I think it should be included anyway to match real
>>    DSP more closely. I highly recommend this addition.
>>  * dsp_core.c: Adding early abort of the hacked bootstrap loader. The
>>    real bootstrap loader permanently checks HF0. If HF0 is set, it
>>    exits early even if a limited amount of bytes have been transferred.
>>    NeXT makes heavy use of this feature. This is required to accurately
>>    simulate the function of the bootstrap loader programmed to the real
>>    DSP. This addition is highly recommended and has zero negative side
>>    effects.
>>  * dsp_core.c: The real bootstrap loader sets R0 to the amount of bytes
>>    copied and OMR to 2. This needs to be added to accurately simulate
>>    the bootstrap program and match real DSP. This change is highly
>>    recommended and has zero negative side effects.
>>  * dsp_core.c: Adding host received data interrupt. This is missing
>>    from the current Hatari code. It must be a mistake. This is highly
>>    recommended to match real DSP.
>> Changes not included:
>>  * DMA interface: Atari obviously does not use the DMA interface of the
>>    DSP host port. Therefore I didn’t include my implementation of it.
>>  * Bootstrap ROM: This feature is not necessary. It is more a question
>>    of wanting to avoid using hacks (dsp_core.running et al). I did not
>>    include this in my patch because it requires extra CPU power without
>>    functional benefits.
>>  * Starting with different modes: The DSP has 4 different modes. Hatari
>>    is hard coded to always use mode 2 (normal expanded mode). Real
>>    simulation of the modes would be required for using bootstrap ROM.
>>    Although in theory there could be other programs making use of this,
>>    there seem to be none on the NeXT and probably also none on Atari.
>>    Therefore I did not include this.
>> There seem to be a few white space issues, because tabs are used between code and comments and because changing from SDL types to normal fixed length types messed things a bit. If you want me to fix this, I can provide a patch (replacing tabs with spaces).
>> Feel free to ask any questions.
> 
> Hi
> 
> thanks for your patch.
> since it implies several changes, could it be possible for you to split it at least in two ? one patch for not noticeable effect and one for possible effects ?
> 
> I understand these work under Previous, but just in case it breaks sthg in Hatari for some games/demos, it would make bisescting easier if we don't commit one single whole patch.
> 
> If not possible, the single patch can be applied anyway as there's no one really working on dsp in hatari at the moment, any improvement/fix should be merged.
> 
> Nicolas
> 
Yes, of course! I have split it into three patches. And yes, the functional changes (patch „improving“) have been part of Previous for a long time and have not caused any regressions. On the contrary, they were necessary to solve problems. Before making changes I always checked the data sheet of the 56001 to confirm that my changes match the specifications.

Regards
Andreas

Attachment: dsp_patch_improving.diff
Description: Binary data

Attachment: dsp_patch_cleaning.diff
Description: Binary data

Attachment: dsp_patch_futureproofing.diff
Description: Binary data