Re: [hatari-devel] fixed a crash when restoring a snapshot using DSP

[Laurent Sallafranque <laurent.sallafranque@xxxxxxx>]

Hello,

I don't see a problem with your fix except perhaps in dsp_core_init() 
where there's :

    memset(&dsp_core, 0, sizeof(dsp_core_t));

This is here to initialize the dsp memory area with 0 (or NOP 
instructions for the P memory).
The DSP memory is not reseted to zero after a reboot, so we just must be 
careful to keep this working as it should (ie: correctly init the DSP 
memory at first init, but not after a reboot or a continuity in using it).
But we can consider that loading a memory snapshot inits the memory 
first and then the snapshot will rewrite it with the saved datas.


> But I think code could be simplified a bit, there're some calls to 
DSP_Init and DSP_UnInit when nDSPType is changed, but I wonder if it's 
necessary.

I think we must be careful here with this memory reset to 0 if we 
disable / reenable the DSP (it must be considered as a first start to 
have the DSP memory full clean)


I hope this makes sense to you.

Laurent



Le 03/01/2015 13:17, Nicolas Pomarède a écrit :
> Hello
>
> while tracing another problem, I noticed that Hatari segfaulted when I 
> restore a memory snapshot for Falcon+DSP.
>
> The problem is that we have this sequence :
>  1) call DSP_Init() from main
>  2) call DSP_MemorySnapShot_Capture
>
> but in DSP_Init(), we had :
>
> if (bDspEnabled || ConfigureParams.System.nDSPType != DSP_TYPE_EMU)
>   return;
> dsp_core_init(DSP_TriggerHostInterrupt);
>
> The problem is that in my case, DSP defaults to none in my hatari.cfg 
> (my default config is STF), so DSP_Init() will return without calling 
> dsp_core_init() and dsp_host_interrupt function pointer is null -> 
> crash in dsp_core_hostport_update_hreq.
>
> Fix is to call DSP_Init() also after restoring the DSP part in the 
> memory snapshot (which also requires to remove the check for 
> bDspEnabled that was already set to true in main.c)
>
> With this snapshot are correctly restored.
>
> But I think code could be simplified a bit, there're some calls to 
> DSP_Init and DSP_UnInit when nDSPType is changed, but I wonder if it's 
> necessary.
>
> DSP_Init only builds a few tables, and DSP_UnInit only sets a flag, so 
> I wonder if it would not be possible to change DSP_Init by removing 
> the test :
>         if (ConfigureParams.System.nDSPType != DSP_TYPE_EMU)
>                 return;
> And to call it only once in main.c at start ; it shouldn't matter if 
> we call it in STF mode for example, as dsp_core_init only sets a few 
> tables.
> The DSP_UnInit() would only be called when exiting.
>
> Laurent, what do you think ?