| Message ID | 1619195089-29710-1-git-send-email-Vijendar.Mukunda@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] ASoC: dwc: add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN to dwc driver | expand |
On 4/23/21 7:46 PM, Mark Brown wrote: > On Fri, Apr 23, 2021 at 09:54:38PM +0530, Vijendar Mukunda wrote: > >> For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and >> I2S FIFO should be stopped before stopping I2S Controller DMA. > >> When DMA is progressing and stop request received, while DMA transfer >> ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA >> stop resulting DMA Channel stop failure. > > This again... copying in Peter for the sequencing discussion. If we > need to do this I'm not convinced that bodging it in the driver is a > good idea, and especially not deferring it outside of the trigger > operation - for example on a suspend or pause we won't actually do a > shutdown() so the trigger will end up not happening which seems like it > may cause problems. It will certainly leave the i2s running and can lead to hard to explain issues > We'd probably be better off with the core knowing > what's going on and being able to reorder the callbacks although > designing an interface for that seems a bit annoying. I agree, it would be better to have some sort of flag which tells the core that there is an integration issue between the DMA and peripheral. I believe this is only affecting playback? >> This issue can't be fixed in ACP DMA driver due to design constraint. > > What is the design constraint here - can't we fix the design? Or is it > a hardware design constraint (presumably broken signalling between the > I2S and DMA blocks)?
On 4/26/21 11:49 AM, Péter Ujfalusi wrote: > > > On 4/23/21 7:46 PM, Mark Brown wrote: >> On Fri, Apr 23, 2021 at 09:54:38PM +0530, Vijendar Mukunda wrote: >> >>> For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and >>> I2S FIFO should be stopped before stopping I2S Controller DMA. >> >>> When DMA is progressing and stop request received, while DMA transfer >>> ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA >>> stop resulting DMA Channel stop failure. >> >> This again... copying in Peter for the sequencing discussion. If we >> need to do this I'm not convinced that bodging it in the driver is a >> good idea, and especially not deferring it outside of the trigger >> operation - for example on a suspend or pause we won't actually do a >> shutdown() so the trigger will end up not happening which seems like it >> may cause problems. > > It will certainly leave the i2s running and can lead to hard to explain > issues Yes I do agree. Moving code from trigger callback to some other place is not a good idea. > >> We'd probably be better off with the core knowing >> what's going on and being able to reorder the callbacks although >> designing an interface for that seems a bit annoying. > > I agree, it would be better to have some sort of flag which tells the > core that there is an integration issue between the DMA and peripheral. > I believe this is only affecting playback? No its affecting both playback and capture use cases. > >>> This issue can't be fixed in ACP DMA driver due to design constraint. >> >> What is the design constraint here - can't we fix the design? Or is it >> a hardware design constraint (presumably broken signalling between the >> I2S and DMA blocks)? Its a hardware design constraint. I2S driver is not directly exposing DMA interface to host. ACP 2.x has unique design where ACP driver controls data flow between host and I2S as mentioned above. ACP IP has different IP blocks within it which includes I2S controller and DMA controller. ACP DMA Driver is responsible for DMA transactions between system memory and I2S controller.It uses two step DMA mechanism to copy data between system memory <-> ACP SRAM and ACP SRAM <-> I2S FIFO for playback/capture use cases. ACP driver program two DMA channels for DMA transfers between System memory & I2S FIFO. ACP DMA driver isn't general purpose DMA controller driver where we can implement terminate_all() API. I2S controller DMA transactions are tightly coupled with ACP DMA controller. while DMA transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA stop resulting DMA Channel stop failure. Its not related to I2S FIFO flushing related handling. Once the DMA channel failure observed during the closure of the stream, when again new stream opened, DMA won't progress at all. Need find a right place to implement a work around only for AMD stoneyridge platform. > > From the description my guess is that stop on the DMA want to flush it's > FIFO (complete the in progress packet, segment). Since the peripheral is > stopped it will not pull in more data -> the DMA will time out internally. > > The question: how the ACP DMA driver's terminate_all is implemented? It > can not really wait for the DMA to stop, we can not use > terminate_all_sync() in trigger, it must just set a stop bit and make > sure at synchronize() time that it has stopped, right? > > What happens if the time between the DMA stop and the DAI stop is less > then it would take to flush the DMA FIFO? You would have the same issue, > but in a rather hard to reproducible way? > > As sidenote: TI's k3-udma initially had similar issue at the design > phase on the playback side which got solved by a flush bit on the > channel to detach it from the peripheral and set it to free run to drain > w/o peripheral. > On capture however I need to push a dummy 'drain' packet to flush out > the data from the DMA (if the stop happens when we did not have active > descriptor on the channel). > > With a flag to reorder the DMA/DAI stop sequence it might work most of > the time, but imho it is going to introduce a nasty time-bomb of failure. > Also your DAI will underflow instead of the DMA error. >
On 4/26/21 3:21 PM, Mukunda,Vijendar wrote: >>> What is the design constraint here - can't we fix the design? Or is it >>> a hardware design constraint (presumably broken signalling between the >>> I2S and DMA blocks)? > > Its a hardware design constraint. > > > I2S driver is not directly exposing DMA interface to host. > ACP 2.x has unique design where ACP driver controls data flow between > host and I2S as mentioned above. > ACP IP has different IP blocks within it which includes I2S controller > and DMA controller. > > ACP DMA Driver is responsible for DMA transactions between system memory > and I2S controller.It uses two step DMA mechanism to copy data between > system memory <-> ACP SRAM and ACP SRAM <-> I2S FIFO for > playback/capture use cases. > ACP driver program two DMA channels for DMA transfers between System > memory & I2S FIFO. > > ACP DMA driver isn't general purpose DMA controller driver where we can > implement terminate_all() API. > > I2S controller DMA transactions are tightly coupled with ACP DMA > controller. > while DMA transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S > DMA prior to ACP DMA stop resulting DMA Channel stop failure. > Its not related to I2S FIFO flushing related handling. > Once the DMA channel failure observed during the closure of the stream, > when again new stream opened, DMA won't progress at all. Thanks for the explanation. This is not upstream, right? What is still not clear to me is which channel fails? A) the DMA between ACP FIFO and the I2S B) the DMA between ACP FIFO and system memory in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to check if the channel is in fact stopped in response to the cleared run, IOCEn bits and the set Rst bit. Channel closer to the destination is stopped first which sounds reasonable, but on playback you ignore timeout from A, on capture you ignore the timeout from B. Still the issue sounds like exactly what I have described. One of the DMA is failing to drain because the IP is stopped? > Need find a right place to implement a work around only for AMD > stoneyridge platform. Is this really only affecting stoneyridge platform? Are there other platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ?
>> ACP DMA Driver is responsible for DMA transactions between system memory >> and I2S controller.It uses two step DMA mechanism to copy data between >> system memory <-> ACP SRAM and ACP SRAM <-> I2S FIFO for >> playback/capture use cases. >> ACP driver program two DMA channels for DMA transfers between System >> memory & I2S FIFO. >> >> ACP DMA driver isn't general purpose DMA controller driver where we can >> implement terminate_all() API. >> >> I2S controller DMA transactions are tightly coupled with ACP DMA >> controller. >> while DMA transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S >> DMA prior to ACP DMA stop resulting DMA Channel stop failure. >> Its not related to I2S FIFO flushing related handling. >> Once the DMA channel failure observed during the closure of the stream, >> when again new stream opened, DMA won't progress at all. > > Thanks for the explanation. > This is not upstream, right? Driver is already upstreamed. Stoneyridge platform based products already into market and working fine with 4.14 kernel version. Currently Kernel migration from v4.14 to v5.10 is in progress for Stoneyridge platform and release got blocked due to Audio use cases failures. In v5.10 kernel base, re-ordering of stop trigger sequence is causing DMA channel stop failure for both playback & capture use cases. > > What is still not clear to me is which channel fails? > A) the DMA between ACP FIFO and the I2S > B) the DMA between ACP FIFO and system memory There is difference for playback and Capture use cases. Playback: channel 1 : DMA transfer from System memory -> ACP memory channel 2 : DMA transfer from ACP memory -> I2S memory Capture: channel 1: DMA transfer from I2S memory to ACP memory channel 2: DMA transfer from ACP memory to System memory In case of playback, Channel 2 is failing where as in case of capture channel 1 is failing. > > in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to > check if the channel is in fact stopped in response to the cleared run, > IOCEn bits and the set Rst bit. DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in case of playback . After that as part of DMA stop sequence, DMA channel reset is applied. When DMA channel status is polled for stop, its failed to stop. > > Channel closer to the destination is stopped first which sounds > reasonable, but on playback you ignore timeout from A, on capture you > ignore the timeout from B. Please refer above explanation. > Still the issue sounds like exactly what I have described. One of the > DMA is failing to drain because the IP is stopped? As per our understanding, failing to stop the DMA by hardware is causing the issue. > >> Need find a right place to implement a work around only for AMD >> stoneyridge platform. > > Is this really only affecting stoneyridge platform? Are there other > platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ? > This design is being used only in stoneyridge and Carrizo platforms. But only stoneyridge platform is productized. New design is implemented for later generations of APU series.
On 28.4.2021 18.35, Mukunda,Vijendar wrote: >> Thanks for the explanation. >> This is not upstream, right? > > Driver is already upstreamed. > Stoneyridge platform based products already into market and working fine > with 4.14 kernel version. > Currently Kernel migration from v4.14 to v5.10 is in progress for > Stoneyridge platform and release got blocked due to Audio use cases > failures. > In v5.10 kernel base, re-ordering of stop trigger sequence is causing > DMA channel stop failure for both playback & capture use cases. The dai - pcm start/stop ordering has changed in v5.5, more than a year ago. If the support is upstream it should have been noticed by users. >> What is still not clear to me is which channel fails? >> A) the DMA between ACP FIFO and the I2S >> B) the DMA between ACP FIFO and system memory > > There is difference for playback and Capture use cases. > > Playback: > > channel 1 : DMA transfer from System memory -> ACP memory > channel 2 : DMA transfer from ACP memory -> I2S memory > > Capture: > > channel 1: DMA transfer from I2S memory to ACP memory > channel 2: DMA transfer from ACP memory to System memory Yes, this is why I used A and B to refer to the DMA channels. > In case of playback, Channel 2 is failing where as in case of > capture channel 1 is failing. So channel A is failing. >> in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to >> check if the channel is in fact stopped in response to the cleared run, >> IOCEn bits and the set Rst bit. > > DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in > case of playback . > After that as part of DMA stop sequence, DMA channel reset is applied. > When DMA channel status is polled for stop, its failed to stop. >> >> Channel closer to the destination is stopped first which sounds >> reasonable, but on playback you ignore timeout from A, on capture you >> ignore the timeout from B. > > Please refer above explanation. > >> Still the issue sounds like exactly what I have described. One of the >> DMA is failing to drain because the IP is stopped? > > As per our understanding, failing to stop the DMA by hardware is causing > the issue. > >> >>> Need find a right place to implement a work around only for AMD >>> stoneyridge platform. >> >> Is this really only affecting stoneyridge platform? Are there other >> platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ? >> > This design is being used only in stoneyridge and Carrizo platforms. > But only stoneyridge platform is productized. > New design is implemented for later generations of APU series. Right. This version of ACP is only used with the designware I2S IP? I would try to find a way to force stop the DMA in case the DAI has been already stopped. If this is not possible than the only solution is to do this in core, imho. For that you would need a flag to say that the platform (DMA) needs the DAI to be active when stopping it. If the same ACP have problems with different DAIs, then it is a property of the platform driver. If the ACP only have problem against the designware I2S then it is a link property. If this ACP only used with the designware I2S then it is again, most likely the property of the platform driver. It is surely not a designware IP issue, trying to solve it there is wrong.
On 4/30/21 11:12 AM, Péter Ujfalusi wrote: > > > On 28.4.2021 18.35, Mukunda,Vijendar wrote: >>> Thanks for the explanation. >>> This is not upstream, right? >> >> Driver is already upstreamed. >> Stoneyridge platform based products already into market and working fine >> with 4.14 kernel version. >> Currently Kernel migration from v4.14 to v5.10 is in progress for >> Stoneyridge platform and release got blocked due to Audio use cases >> failures. >> In v5.10 kernel base, re-ordering of stop trigger sequence is causing >> DMA channel stop failure for both playback & capture use cases. > > The dai - pcm start/stop ordering has changed in v5.5, more than a year > ago. If the support is upstream it should have been noticed by users. AMD partner using their own repositories where kernel is not migrated to 5.10. Still products are running with v4.14 kernel only. That's why users hasn't faced this issue. > >>> What is still not clear to me is which channel fails? >>> A) the DMA between ACP FIFO and the I2S >>> B) the DMA between ACP FIFO and system memory >> >> There is difference for playback and Capture use cases. >> >> Playback: >> >> channel 1 : DMA transfer from System memory -> ACP memory >> channel 2 : DMA transfer from ACP memory -> I2S memory >> >> Capture: >> >> channel 1: DMA transfer from I2S memory to ACP memory >> channel 2: DMA transfer from ACP memory to System memory > > Yes, this is why I used A and B to refer to the DMA channels. > >> In case of playback, Channel 2 is failing where as in case of >> capture channel 1 is failing. > > So channel A is failing. > >>> in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to >>> check if the channel is in fact stopped in response to the cleared run, >>> IOCEn bits and the set Rst bit. >> >> DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in >> case of playback . >> After that as part of DMA stop sequence, DMA channel reset is applied. >> When DMA channel status is polled for stop, its failed to stop. >>> >>> Channel closer to the destination is stopped first which sounds >>> reasonable, but on playback you ignore timeout from A, on capture you >>> ignore the timeout from B. >> >> Please refer above explanation. >> >>> Still the issue sounds like exactly what I have described. One of the >>> DMA is failing to drain because the IP is stopped? >> >> As per our understanding, failing to stop the DMA by hardware is causing >> the issue. >> >>> >>>> Need find a right place to implement a work around only for AMD >>>> stoneyridge platform. >>> >>> Is this really only affecting stoneyridge platform? Are there other >>> platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ? >>> >> This design is being used only in stoneyridge and Carrizo platforms. >> But only stoneyridge platform is productized. >> New design is implemented for later generations of APU series. > > Right. > > This version of ACP is only used with the designware I2S IP? Yes this version of ACP only uses designware I2S controller. > > I would try to find a way to force stop the DMA in case the DAI has been > already stopped. > > If this is not possible than the only solution is to do this in core, imho. > > For that you would need a flag to say that the platform (DMA) needs the > DAI to be active when stopping it. > > If the same ACP have problems with different DAIs, then it is a property > of the platform driver. > If the ACP only have problem against the designware I2S then it is a > link property. > If this ACP only used with the designware I2S then it is again, most > likely the property of the platform driver. > Hardware signal broken between ACP and Designware I2S controller with re-ordering the sequence. > It is surely not a designware IP issue, trying to solve it there is wrong. > As it's not a designware IP issue, initially we started idea with introducing quirk that applies for this ACP version based AMD platforms.
On 5/1/21 12:05 AM, Mukunda,Vijendar wrote: > > > On 4/30/21 11:12 AM, Péter Ujfalusi wrote: >> >> >> On 28.4.2021 18.35, Mukunda,Vijendar wrote: >>>> Thanks for the explanation. >>>> This is not upstream, right? >>> >>> Driver is already upstreamed. >>> Stoneyridge platform based products already into market and working fine >>> with 4.14 kernel version. >>> Currently Kernel migration from v4.14 to v5.10 is in progress for >>> Stoneyridge platform and release got blocked due to Audio use cases >>> failures. >>> In v5.10 kernel base, re-ordering of stop trigger sequence is causing >>> DMA channel stop failure for both playback & capture use cases. >> >> The dai - pcm start/stop ordering has changed in v5.5, more than a year >> ago. If the support is upstream it should have been noticed by users. > > AMD partner using their own repositories where kernel is not migrated to > 5.10. Still products are running with v4.14 kernel only. > That's why users hasn't faced this issue. >> >>>> What is still not clear to me is which channel fails? >>>> A) the DMA between ACP FIFO and the I2S >>>> B) the DMA between ACP FIFO and system memory >>> >>> There is difference for playback and Capture use cases. >>> >>> Playback: >>> >>> channel 1 : DMA transfer from System memory -> ACP memory >>> channel 2 : DMA transfer from ACP memory -> I2S memory >>> >>> Capture: >>> >>> channel 1: DMA transfer from I2S memory to ACP memory >>> channel 2: DMA transfer from ACP memory to System memory >> >> Yes, this is why I used A and B to refer to the DMA channels. >> >>> In case of playback, Channel 2 is failing where as in case of >>> capture channel 1 is failing. >> >> So channel A is failing. >> >>>> in acp-pcm-dma.c on stop you have a busy loop (10000 iterations) to >>>> check if the channel is in fact stopped in response to the cleared run, >>>> IOCEn bits and the set Rst bit. >>> >>> DMA channel run bit is cleared and Ioc bit also cleared for channel 2 in >>> case of playback . >>> After that as part of DMA stop sequence, DMA channel reset is applied. >>> When DMA channel status is polled for stop, its failed to stop. >>>> >>>> Channel closer to the destination is stopped first which sounds >>>> reasonable, but on playback you ignore timeout from A, on capture you >>>> ignore the timeout from B. >>> >>> Please refer above explanation. >>> >>>> Still the issue sounds like exactly what I have described. One of the >>>> DMA is failing to drain because the IP is stopped? >>> >>> As per our understanding, failing to stop the DMA by hardware is causing >>> the issue. >>> >>>> >>>>> Need find a right place to implement a work around only for AMD >>>>> stoneyridge platform. >>>> >>>> Is this really only affecting stoneyridge platform? Are there other >>>> platforms using drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c ? >>>> >>> This design is being used only in stoneyridge and Carrizo platforms. >>> But only stoneyridge platform is productized. >>> New design is implemented for later generations of APU series. >> >> Right. >> >> This version of ACP is only used with the designware I2S IP? > Yes this version of ACP only uses designware I2S controller. >> >> I would try to find a way to force stop the DMA in case the DAI has been >> already stopped. >> >> If this is not possible than the only solution is to do this in core, >> imho. >> >> For that you would need a flag to say that the platform (DMA) needs the >> DAI to be active when stopping it. >> >> If the same ACP have problems with different DAIs, then it is a property >> of the platform driver. >> If the ACP only have problem against the designware I2S then it is a >> link property. >> If this ACP only used with the designware I2S then it is again, most >> likely the property of the platform driver. >> > Hardware signal broken between ACP and Designware I2S controller with > re-ordering the sequence. > > It is surely not a designware IP issue, trying to solve it there is > wrong. >> > As it's not a designware IP issue, initially we started idea with > introducing quirk that applies for this ACP version based AMD platforms. > Hi Peter, Any suggestion on the work around for this issue? How about declaring a flag in sound card structure and this flag will be set in stoneyridge machine driver. Based on flag check trigger stop sequence will be re-ordered. Thanks, Vijendar
On Mon, May 10, 2021 at 10:57:25PM +0530, Mukunda,Vijendar wrote: > How about declaring a flag in sound card structure and this flag will be set > in stoneyridge machine driver. > Based on flag check trigger stop sequence will be re-ordered. A couple of people suggested that already, making sure the core knows what's going on is probably the best way forwards here.
On 10/05/2021 20:27, Mukunda,Vijendar wrote: >> Hardware signal broken between ACP and Designware I2S controller with >> re-ordering the sequence. >> > It is surely not a designware IP issue, trying to solve it there >> is wrong. >>> >> As it's not a designware IP issue, initially we started idea with >> introducing quirk that applies for this ACP version based AMD platforms. >> > > Hi Peter, > > Any suggestion on the work around for this issue? > How about declaring a flag in sound card structure and this flag will be > set in stoneyridge machine driver. This can only be solved in the core, that's clear. If this issue only affects this version of ACP with dw I2S (the same ACP version works fine with other audio IP), then it is more like machine driver level of quirk. If this ACP have the same issue with other audio IPs as well then it is platform quirk. If the this ACP is only used in this setup then I would consider machine level quirk as it might be simpler to implement. Other thing to consider is how other setups with similar issues can use the new quirk/flag... Some might need to make sure that a component is first, not last for example. > Based on flag check trigger stop sequence will be re-ordered. > > Thanks, > Vijendar
diff --git a/include/sound/designware_i2s.h b/include/sound/designware_i2s.h index 80d275b..a700d20 100644 --- a/include/sound/designware_i2s.h +++ b/include/sound/designware_i2s.h @@ -34,6 +34,7 @@ struct i2s_platform_data { #define DW_I2S_QUIRK_COMP_REG_OFFSET (1 << 0) #define DW_I2S_QUIRK_COMP_PARAM1 (1 << 1) #define DW_I2S_QUIRK_16BIT_IDX_OVERRIDE (1 << 2) + #define DW_I2S_QUIRK_STOP_ON_SHUTDOWN (1 << 3) unsigned int quirks; unsigned int i2s_reg_comp1; unsigned int i2s_reg_comp2; diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c index fd41602..f3a681f 100644 --- a/sound/soc/dwc/dwc-i2s.c +++ b/sound/soc/dwc/dwc-i2s.c @@ -308,6 +308,10 @@ static int dw_i2s_hw_params(struct snd_pcm_substream *substream, static void dw_i2s_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { + struct dw_i2s_dev *dev = snd_soc_dai_get_drvdata(dai); + + if (dev->quirks & DW_I2S_QUIRK_STOP_ON_SHUTDOWN) + i2s_stop(dev, substream); snd_soc_dai_set_dma_data(dai, substream, NULL); } @@ -342,7 +346,8 @@ static int dw_i2s_trigger(struct snd_pcm_substream *substream, case SNDRV_PCM_TRIGGER_SUSPEND: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: dev->active--; - i2s_stop(dev, substream); + if (!(dev->quirks & DW_I2S_QUIRK_STOP_ON_SHUTDOWN)) + i2s_stop(dev, substream); break; default: ret = -EINVAL;
For CZ/StoneyRidge platforms, ACP DMA between ACP SRAM and I2S FIFO should be stopped before stopping I2S Controller DMA. When DMA is progressing and stop request received, while DMA transfer ongoing between ACP SRAM and I2S FIFO, Stopping I2S DMA prior to ACP DMA stop resulting DMA Channel stop failure. This issue can't be fixed in ACP DMA driver due to design constraint. Add a quirk DW_I2S_QUIRK_STOP_ON_SHUTDOWN in dwc driver to fix DMA stop failure by invoking i2s_stop() sequence in shutdown() callback. Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> --- include/sound/designware_i2s.h | 1 + sound/soc/dwc/dwc-i2s.c | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-)