| Message ID | 20170513221327.10114-3-alexander.sverdlin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Sun, May 14, 2017 at 12:13:27AM +0200, Alexander Sverdlin wrote: > As dmaengine.h suggests, device_terminate_all() "Aborts all transfers on a > channel". Seems that no other driver tried to busy-wait and actually drain > the data. Moreover, this happens with IRQs disabled, therefore induces huge > latency: Perhaps you should move current code to device_synchronize callback to allow people to do so, if desired. > > irqsoff latency trace v1.1.5 on 4.11.0 > -------------------------------------------------------------------- > latency: 39770 us, #57/57, CPU#0 | (M:preempt VP:0, KP:0, SP:0 HP:0) > ----------------- > | task: process-129 (uid:0 nice:0 policy:2 rt_prio:50) > ----------------- > => started at: _snd_pcm_stream_lock_irqsave > => ended at: snd_pcm_stream_unlock_irqrestore > > _------=> CPU# > / _-----=> irqs-off > | / _----=> need-resched > || / _---=> hardirq/softirq > ||| / _--=> preempt-depth > |||| / delay > cmd pid ||||| time | caller > \ / ||||| \ | / > process-129 0d.s. 3us : _snd_pcm_stream_lock_irqsave > process-129 0d.s1 9us : snd_pcm_stream_lock <-_snd_pcm_stream_lock_irqsave > process-129 0d.s1 15us : preempt_count_add <-snd_pcm_stream_lock > process-129 0d.s2 22us : preempt_count_add <-snd_pcm_stream_lock > process-129 0d.s3 32us : snd_pcm_update_hw_ptr0 <-snd_pcm_period_elapsed > process-129 0d.s3 41us : soc_pcm_pointer <-snd_pcm_update_hw_ptr0 > process-129 0d.s3 50us : dmaengine_pcm_pointer <-soc_pcm_pointer > process-129 0d.s3 58us+: snd_dmaengine_pcm_pointer_no_residue <-dmaengine_pcm_pointer > process-129 0d.s3 96us : update_audio_tstamp <-snd_pcm_update_hw_ptr0 > process-129 0d.s3 103us : snd_pcm_update_state <-snd_pcm_update_hw_ptr0 > process-129 0d.s3 112us : xrun <-snd_pcm_update_state > process-129 0d.s3 119us : snd_pcm_stop <-xrun > process-129 0d.s3 126us : snd_pcm_action <-snd_pcm_stop > process-129 0d.s3 134us : snd_pcm_action_single <-snd_pcm_action > process-129 0d.s3 141us : snd_pcm_pre_stop <-snd_pcm_action_single > process-129 0d.s3 150us : snd_pcm_do_stop <-snd_pcm_action_single > process-129 0d.s3 157us : soc_pcm_trigger <-snd_pcm_do_stop > process-129 0d.s3 166us : snd_dmaengine_pcm_trigger <-soc_pcm_trigger > process-129 0d.s3 175us : ep93xx_dma_terminate_all <-snd_dmaengine_pcm_trigger > process-129 0d.s3 182us : preempt_count_add <-ep93xx_dma_terminate_all > process-129 0d.s4 189us*: m2p_hw_shutdown <-ep93xx_dma_terminate_all > process-129 0d.s4 39472us : m2p_hw_setup <-ep93xx_dma_terminate_all > > ... rest skipped... > > process-129 0d.s. 40080us : <stack trace> > => ep93xx_dma_tasklet > => tasklet_action > => __do_softirq > => irq_exit > => __handle_domain_irq > => vic_handle_irq > => __irq_usr > => 0xb66c6668 > > Just abort the transfers and warn if the HW state is not what we expect. > > Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> > Cc: stable@vger.kernel.org > --- > drivers/dma/ep93xx_dma.c | 13 ++----------- > 1 file changed, 2 insertions(+), 11 deletions(-) > > diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c > index a3d946d2a8e1..e78d1fbf2179 100644 > --- a/drivers/dma/ep93xx_dma.c > +++ b/drivers/dma/ep93xx_dma.c > @@ -345,19 +345,10 @@ static inline u32 m2p_channel_state(struct ep93xx_dma_chan *edmac) > > static void m2p_hw_shutdown(struct ep93xx_dma_chan *edmac) > { > - u32 control; > - > - control = readl(edmac->regs + M2P_CONTROL); > - control &= ~(M2P_CONTROL_STALLINT | M2P_CONTROL_NFBINT); > - m2p_set_control(edmac, control); > - > - while (m2p_channel_state(edmac) >= M2P_STATE_ON) > - cpu_relax(); > - > m2p_set_control(edmac, 0); > > - while (m2p_channel_state(edmac) == M2P_STATE_STALL) > - cpu_relax(); > + while (m2p_channel_state(edmac) != M2P_STATE_IDLE) > + dev_warn(chan2dev(edmac), "M2P: Not yet IDLE\n"); > } > > static void m2p_fill_desc(struct ep93xx_dma_chan *edmac) > -- > 2.12.2 >
Hello Vinod, On 14/05/17 15:16, Vinod Koul wrote: >> As dmaengine.h suggests, device_terminate_all() "Aborts all transfers on a >> channel". Seems that no other driver tried to busy-wait and actually drain >> the data. Moreover, this happens with IRQs disabled, therefore induces huge >> latency: > Perhaps you should move current code to device_synchronize callback to allow > people to do so, if desired. you are right, otherwise device_free_chan_resources() can be called on running channel... Will do! >> irqsoff latency trace v1.1.5 on 4.11.0 >> -------------------------------------------------------------------- >> latency: 39770 us, #57/57, CPU#0 | (M:preempt VP:0, KP:0, SP:0 HP:0) >> ----------------- >> | task: process-129 (uid:0 nice:0 policy:2 rt_prio:50) >> ----------------- >> => started at: _snd_pcm_stream_lock_irqsave >> => ended at: snd_pcm_stream_unlock_irqrestore >> >> _------=> CPU# >> / _-----=> irqs-off >> | / _----=> need-resched >> || / _---=> hardirq/softirq >> ||| / _--=> preempt-depth >> |||| / delay >> cmd pid ||||| time | caller >> \ / ||||| \ | / >> process-129 0d.s. 3us : _snd_pcm_stream_lock_irqsave >> process-129 0d.s1 9us : snd_pcm_stream_lock <-_snd_pcm_stream_lock_irqsave >> process-129 0d.s1 15us : preempt_count_add <-snd_pcm_stream_lock >> process-129 0d.s2 22us : preempt_count_add <-snd_pcm_stream_lock >> process-129 0d.s3 32us : snd_pcm_update_hw_ptr0 <-snd_pcm_period_elapsed >> process-129 0d.s3 41us : soc_pcm_pointer <-snd_pcm_update_hw_ptr0 >> process-129 0d.s3 50us : dmaengine_pcm_pointer <-soc_pcm_pointer >> process-129 0d.s3 58us+: snd_dmaengine_pcm_pointer_no_residue <-dmaengine_pcm_pointer >> process-129 0d.s3 96us : update_audio_tstamp <-snd_pcm_update_hw_ptr0 >> process-129 0d.s3 103us : snd_pcm_update_state <-snd_pcm_update_hw_ptr0 >> process-129 0d.s3 112us : xrun <-snd_pcm_update_state >> process-129 0d.s3 119us : snd_pcm_stop <-xrun >> process-129 0d.s3 126us : snd_pcm_action <-snd_pcm_stop >> process-129 0d.s3 134us : snd_pcm_action_single <-snd_pcm_action >> process-129 0d.s3 141us : snd_pcm_pre_stop <-snd_pcm_action_single >> process-129 0d.s3 150us : snd_pcm_do_stop <-snd_pcm_action_single >> process-129 0d.s3 157us : soc_pcm_trigger <-snd_pcm_do_stop >> process-129 0d.s3 166us : snd_dmaengine_pcm_trigger <-soc_pcm_trigger >> process-129 0d.s3 175us : ep93xx_dma_terminate_all <-snd_dmaengine_pcm_trigger >> process-129 0d.s3 182us : preempt_count_add <-ep93xx_dma_terminate_all >> process-129 0d.s4 189us*: m2p_hw_shutdown <-ep93xx_dma_terminate_all >> process-129 0d.s4 39472us : m2p_hw_setup <-ep93xx_dma_terminate_all >> >> ... rest skipped... >> >> process-129 0d.s. 40080us : <stack trace> >> => ep93xx_dma_tasklet >> => tasklet_action >> => __do_softirq >> => irq_exit >> => __handle_domain_irq >> => vic_handle_irq >> => __irq_usr >> => 0xb66c6668 >> >> Just abort the transfers and warn if the HW state is not what we expect. >> >> Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> >> Cc: stable@vger.kernel.org >> --- >> drivers/dma/ep93xx_dma.c | 13 ++----------- >> 1 file changed, 2 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c >> index a3d946d2a8e1..e78d1fbf2179 100644 >> --- a/drivers/dma/ep93xx_dma.c >> +++ b/drivers/dma/ep93xx_dma.c >> @@ -345,19 +345,10 @@ static inline u32 m2p_channel_state(struct ep93xx_dma_chan *edmac) >> >> static void m2p_hw_shutdown(struct ep93xx_dma_chan *edmac) >> { >> - u32 control; >> - >> - control = readl(edmac->regs + M2P_CONTROL); >> - control &= ~(M2P_CONTROL_STALLINT | M2P_CONTROL_NFBINT); >> - m2p_set_control(edmac, control); >> - >> - while (m2p_channel_state(edmac) >= M2P_STATE_ON) >> - cpu_relax(); >> - >> m2p_set_control(edmac, 0); >> >> - while (m2p_channel_state(edmac) == M2P_STATE_STALL) >> - cpu_relax(); >> + while (m2p_channel_state(edmac) != M2P_STATE_IDLE) >> + dev_warn(chan2dev(edmac), "M2P: Not yet IDLE\n"); >> } -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/ep93xx_dma.c b/drivers/dma/ep93xx_dma.c index a3d946d2a8e1..e78d1fbf2179 100644 --- a/drivers/dma/ep93xx_dma.c +++ b/drivers/dma/ep93xx_dma.c @@ -345,19 +345,10 @@ static inline u32 m2p_channel_state(struct ep93xx_dma_chan *edmac) static void m2p_hw_shutdown(struct ep93xx_dma_chan *edmac) { - u32 control; - - control = readl(edmac->regs + M2P_CONTROL); - control &= ~(M2P_CONTROL_STALLINT | M2P_CONTROL_NFBINT); - m2p_set_control(edmac, control); - - while (m2p_channel_state(edmac) >= M2P_STATE_ON) - cpu_relax(); - m2p_set_control(edmac, 0); - while (m2p_channel_state(edmac) == M2P_STATE_STALL) - cpu_relax(); + while (m2p_channel_state(edmac) != M2P_STATE_IDLE) + dev_warn(chan2dev(edmac), "M2P: Not yet IDLE\n"); } static void m2p_fill_desc(struct ep93xx_dma_chan *edmac)
As dmaengine.h suggests, device_terminate_all() "Aborts all transfers on a channel". Seems that no other driver tried to busy-wait and actually drain the data. Moreover, this happens with IRQs disabled, therefore induces huge latency: irqsoff latency trace v1.1.5 on 4.11.0 -------------------------------------------------------------------- latency: 39770 us, #57/57, CPU#0 | (M:preempt VP:0, KP:0, SP:0 HP:0) ----------------- | task: process-129 (uid:0 nice:0 policy:2 rt_prio:50) ----------------- => started at: _snd_pcm_stream_lock_irqsave => ended at: snd_pcm_stream_unlock_irqrestore _------=> CPU# / _-----=> irqs-off | / _----=> need-resched || / _---=> hardirq/softirq ||| / _--=> preempt-depth |||| / delay cmd pid ||||| time | caller \ / ||||| \ | / process-129 0d.s. 3us : _snd_pcm_stream_lock_irqsave process-129 0d.s1 9us : snd_pcm_stream_lock <-_snd_pcm_stream_lock_irqsave process-129 0d.s1 15us : preempt_count_add <-snd_pcm_stream_lock process-129 0d.s2 22us : preempt_count_add <-snd_pcm_stream_lock process-129 0d.s3 32us : snd_pcm_update_hw_ptr0 <-snd_pcm_period_elapsed process-129 0d.s3 41us : soc_pcm_pointer <-snd_pcm_update_hw_ptr0 process-129 0d.s3 50us : dmaengine_pcm_pointer <-soc_pcm_pointer process-129 0d.s3 58us+: snd_dmaengine_pcm_pointer_no_residue <-dmaengine_pcm_pointer process-129 0d.s3 96us : update_audio_tstamp <-snd_pcm_update_hw_ptr0 process-129 0d.s3 103us : snd_pcm_update_state <-snd_pcm_update_hw_ptr0 process-129 0d.s3 112us : xrun <-snd_pcm_update_state process-129 0d.s3 119us : snd_pcm_stop <-xrun process-129 0d.s3 126us : snd_pcm_action <-snd_pcm_stop process-129 0d.s3 134us : snd_pcm_action_single <-snd_pcm_action process-129 0d.s3 141us : snd_pcm_pre_stop <-snd_pcm_action_single process-129 0d.s3 150us : snd_pcm_do_stop <-snd_pcm_action_single process-129 0d.s3 157us : soc_pcm_trigger <-snd_pcm_do_stop process-129 0d.s3 166us : snd_dmaengine_pcm_trigger <-soc_pcm_trigger process-129 0d.s3 175us : ep93xx_dma_terminate_all <-snd_dmaengine_pcm_trigger process-129 0d.s3 182us : preempt_count_add <-ep93xx_dma_terminate_all process-129 0d.s4 189us*: m2p_hw_shutdown <-ep93xx_dma_terminate_all process-129 0d.s4 39472us : m2p_hw_setup <-ep93xx_dma_terminate_all ... rest skipped... process-129 0d.s. 40080us : <stack trace> => ep93xx_dma_tasklet => tasklet_action => __do_softirq => irq_exit => __handle_domain_irq => vic_handle_irq => __irq_usr => 0xb66c6668 Just abort the transfers and warn if the HW state is not what we expect. Signed-off-by: Alexander Sverdlin <alexander.sverdlin@gmail.com> Cc: stable@vger.kernel.org --- drivers/dma/ep93xx_dma.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)