[BUG][CI] dsp panic on BDW and BYT #3146

Activity

1. aiChaoSONG

   added

   bugSomething isn't working as expectedSomething isn't working as expected

   on Jul 8, 2020
2. lgirdwood commented on Jul 8, 2020

   

   lgirdwood

   on Jul 8, 2020

   Member

   More actions

   Thanks @aiChaoSONG I see this too now that the Jenkins CI is back, it was off for the last few days... I want to make Jenkins mandatory again. Can you bisect - it should be a recent commot from this week or last.
3. plbossart commented on Jul 8, 2020

   

   plbossart

   on Jul 8, 2020

   Member

   More actions

   FWIW, Baytrail/Cherrytrail worked fine with the topology changes on top of commit 5441b13 ('ipc: component free fix')

   That may help with the bisect?
4. aiChaoSONG commented on Jul 9, 2020

   

   aiChaoSONG

   on Jul 9, 2020

   CollaboratorAuthor

   More actions

   @lgirdwood @plbossart @ktrzcinx Bisect result: 1f092df

   dmesg already tells us where error occured.

   [  119.636706] sof-audio-acpi INT3438:00: error: ipc timed out for 0x60010000 size 108
   [  119.636715] sof-audio-acpi INT3438:00: info: preventing DSP entering D3 state to preserve context
   [  119.636792] sof-audio-acpi INT3438:00: error: assertion failed
   [  119.636796] sof-audio-acpi INT3438:00: error: trace point 80000000
   [  119.636800] sof-audio-acpi INT3438:00: error: panic at ...pcm_converter/pcm_converter.c�:31
   
5. lgirdwood commented on Jul 9, 2020

   

   lgirdwood

   on Jul 9, 2020

   Member

   More actions

   @ktrzcinx should not enough free/avail really be fatal here ? It's probably best if we trace_err() here instead and return.

   void pcm_convert_as_linear(const struct audio_stream *source, uint32_t ioffset,
   			   struct audio_stream *sink, uint32_t ooffset,
   			   uint32_t samples, pcm_converter_lin_func converter)
   {
   	const int s_size_in = audio_stream_sample_bytes(source);
   	const int s_size_out = audio_stream_sample_bytes(sink);
   	char *r_ptr = audio_stream_get_frag(source, source->r_ptr, ioffset,
   					    s_size_in);
   	char *w_ptr = audio_stream_get_frag(sink, sink->w_ptr, ooffset,
   					    s_size_out);
   	int i = 0;
   	int chunk;
   	int N1, N2;
   
   	assert(audio_stream_get_avail_samples(source) >= samples + ioffset);
   	assert(audio_stream_get_free_samples(sink) >= samples + ooffset);
6. ktrzcinx commented on Jul 9, 2020

   

   ktrzcinx

   on Jul 9, 2020

   Last edited by ktrzcinx

   Member

   More actions

   @lgirdwood it can be easily change to tr_err() and return, but I believe that runtime validation with if should be done in higher layers of FW, where some action may be taken, assertion is here only for debug purposes. Otherwise we end up with "noised output" and don't really know where problem comes from.
7. lgirdwood commented on Jul 9, 2020

   

   lgirdwood

   on Jul 9, 2020

   Member

   More actions

   @ktrzcinx ok - I'm fine if the check is done by the caller (as it can give us more context), but using an assert for debug is probably a bit of overkill. Can you send a PR for the tr_err() and remove the assert() Thanks !
8. plbossart commented on Jul 9, 2020

   

   plbossart

   on Jul 9, 2020

   Member

   More actions

   Didn't we decide already that assert() is not permitted in SOF firmware code?
9. lgirdwood commented on Jul 9, 2020

   

   lgirdwood

   on Jul 9, 2020

   Member

   More actions

   Didn't we decide already that assert() is not permitted in SOF firmware code?

   Good point - although I may have missed that conversation but I agree, if it's not fatal then we should just report and fail.
10. plbossart commented on Jul 9, 2020

    

    plbossart

    on Jul 9, 2020

    Member

    More actions

    IIRC it was a thread with @dbaluta and/or @paulstelian97 on removing asserts - maybe 4-6 months ago?
11. paulstelian97 commented on Jul 9, 2020

    

    paulstelian97

    on Jul 9, 2020

    Collaborator

    More actions

    My statement back then was not completely removing asserts, but considering them unreliable as once the Zephyr migration is complete asserts can be disabled. Asserts also should ONLY be used to catch programming errors that are caused internally in the firmware itself, not programming errors of the kernel or topologies with an invalid configuration; for those regular error reporting (return a negative error code, send an IPC with said negative error code and so on) is better.

    In here, it is not clear to me why it would be a firmware programming error the situation where those buffers are too full. So I'd vote for an error return (-EINVAL, -ENOSPC or whichever fits this one most -- do you have better ideas?) coupled with an error trace (do check if they cause spam in the traces though, they probably shouldn't).

    Asserts should be used, though only sparingly, given the above in mind, at least in my opinion.

    👍React with 👍1Reacted by Marc Herbert
12. plbossart commented on Jul 9, 2020

    

    plbossart

    on Jul 9, 2020

    Member

    More actions

    @paulstelian97 given that 90% of the flows happen due to configurations sent by kernel/topology I am not sure how we might identify internal programming errors...
13. paulstelian97 commented on Jul 9, 2020

    

    paulstelian97

    on Jul 9, 2020

    Collaborator

    More actions

    @paulstelian97 given that 90% of the flows happen due to configurations sent by kernel/topology I am not sure how we might identify internal programming errors...

    As such asserts should be a rarity. You could assert certain things about the validity of the output data. I can assert in my DMA driver that I get the proper form of descriptors from SOF or that the hardware is in a certain state. Those aren't directly controllable by the kernel so they should always be in a reasonable state, unless firmware-only programming errors occurred.

    But I do agree with the sentiment that this is a rarity and asserts shouldn't be out there too much.
14. slawblauciak

    assigned

    ktrzcinx

    on Jul 14, 2020
15. ktrzcinx commented on Jul 14, 2020

    

    ktrzcinx

    on Jul 14, 2020

    Member

    More actions

    @aiChaoSONG I supposed this issue should be closed because of #3159 merge
16. aiChaoSONG commented on Jul 14, 2020

    

    aiChaoSONG

    on Jul 14, 2020

    CollaboratorAuthor

    More actions

    close for #3159 merged
17. aiChaoSONG

    closed this as completedon Jul 14, 2020