Re: [hatari-devel] DSP patch suggestion

[Andreas Grabher <andreas_g86@xxxxxxxxxx>]

> Am 03.12.2023 um 22:50 schrieb Nicolas Pomarède <npomarede@xxxxxxxxxxxx>:
> 
> Le 03/12/2023 à 13:51, Andreas Grabher a écrit :
>>> Am 03.12.2023 um 12:32 schrieb 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
>>> 
>>> <dsp_patch_improving.diff><dsp_patch_cleaning.diff><dsp_patch_futureproofing.diff>
>> I just recognised that my patches had the wrong level and different directory names. Appended patches are fixed. They can be applied with patch -p1.
> 
> Thanks for splitting your patches and sharing them (I just had to remove the reference to "dsp_core.dma_address_counter=0", Hatari has no dma_address_counter member)
> 
> I applied to Hatari, for regular Falcon users (Laurent :) ), please check you don't see any regression
> 
> Nicolas
> 
Great! Sorry for the trouble with dma_address_counter. It is part of the DMA interface and i forgot to remove it for Hatari.

Regards,
Andreas