* [PATCH 1/2] soc: fsl: qmc: Only set completion interrupt when needed @ 2025-05-09 7:48 Christophe Leroy 2025-05-09 7:48 ` [PATCH 2/2] ASoC: fsl: fsl_qmc_audio: Only request completion on last channel Christophe Leroy 2025-05-09 9:15 ` [PATCH 1/2] soc: fsl: qmc: Only set completion interrupt when needed Herve Codina 0 siblings, 2 replies; 6+ messages in thread From: Christophe Leroy @ 2025-05-09 7:48 UTC (permalink / raw) To: Herve Codina, Qiang Zhao, Shengjiu Wang, Xiubo Li, Fabio Estevam, Nicolin Chen, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai Cc: Christophe Leroy, linux-kernel, linuxppc-dev, linux-arm-kernel, linux-sound When no post-completion processing is expected, don't waste time handling useless interrupts. Only set QMC_BD_[R/T]X_I and QMC_BD_[R/T]X_UB when a completion function is passed in. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- drivers/soc/fsl/qe/qmc.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/drivers/soc/fsl/qe/qmc.c b/drivers/soc/fsl/qe/qmc.c index 36c0ccc06151..0a704fd0b1a0 100644 --- a/drivers/soc/fsl/qe/qmc.c +++ b/drivers/soc/fsl/qe/qmc.c @@ -474,7 +474,9 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length, xfer_desc->context = context; /* Activate the descriptor */ - ctrl |= (QMC_BD_TX_R | QMC_BD_TX_UB); + ctrl |= QMC_BD_TX_R; + if (complete) + ctrl |= QMC_BD_TX_I | QMC_BD_TX_UB; wmb(); /* Be sure to flush the descriptor before control update */ qmc_write16(&bd->cbd_sc, ctrl); @@ -586,7 +588,9 @@ int qmc_chan_read_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length, QMC_BD_RX_AB | QMC_BD_RX_CR); /* Activate the descriptor */ - ctrl |= (QMC_BD_RX_E | QMC_BD_RX_UB); + ctrl |= QMC_BD_RX_E; + if (complete) + ctrl |= QMC_BD_RX_I | QMC_BD_RX_UB; wmb(); /* Be sure to flush data before descriptor activation */ qmc_write16(&bd->cbd_sc, ctrl); @@ -1482,19 +1486,19 @@ static int qmc_setup_chan(struct qmc *qmc, struct qmc_chan *chan) /* Init Rx BDs and set Wrap bit on last descriptor */ BUILD_BUG_ON(QMC_NB_RXBDS == 0); - val = QMC_BD_RX_I; for (i = 0; i < QMC_NB_RXBDS; i++) { bd = chan->rxbds + i; - qmc_write16(&bd->cbd_sc, val); + qmc_write16(&bd->cbd_sc, 0); } bd = chan->rxbds + QMC_NB_RXBDS - 1; - qmc_write16(&bd->cbd_sc, val | QMC_BD_RX_W); + qmc_write16(&bd->cbd_sc, QMC_BD_RX_W); /* Init Tx BDs and set Wrap bit on last descriptor */ BUILD_BUG_ON(QMC_NB_TXBDS == 0); - val = QMC_BD_TX_I; if (chan->mode == QMC_HDLC) - val |= QMC_BD_TX_L | QMC_BD_TX_TC; + val = QMC_BD_TX_L | QMC_BD_TX_TC; + else + val = 0; for (i = 0; i < QMC_NB_TXBDS; i++) { bd = chan->txbds + i; qmc_write16(&bd->cbd_sc, val); -- 2.47.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] ASoC: fsl: fsl_qmc_audio: Only request completion on last channel 2025-05-09 7:48 [PATCH 1/2] soc: fsl: qmc: Only set completion interrupt when needed Christophe Leroy @ 2025-05-09 7:48 ` Christophe Leroy 2025-05-09 8:45 ` Herve Codina 2025-05-09 9:15 ` [PATCH 1/2] soc: fsl: qmc: Only set completion interrupt when needed Herve Codina 1 sibling, 1 reply; 6+ messages in thread From: Christophe Leroy @ 2025-05-09 7:48 UTC (permalink / raw) To: Herve Codina, Qiang Zhao, Shengjiu Wang, Xiubo Li, Fabio Estevam, Nicolin Chen, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai Cc: Christophe Leroy, linux-kernel, linuxppc-dev, linux-arm-kernel, linux-sound In non-interleaved mode, several QMC channels are used in sync. More details can be found in commit 188d9cae5438 ("ASoC: fsl: fsl_qmc_audio: Add support for non-interleaved mode.") At the time being, an interrupt is requested on each channel to perform capture/playback completion, allthough the completion is really performed only once all channels have completed their work. This leads to a lot more interrupts than really needed. Looking at /proc/interrupts shows ~3800 interrupts per second when using 4 capture and 4 playback devices with 5ms periods while only 1600 (200 x 4 + 200 x 4) periods are processed during one second. The QMC channels work in sync, the one started first is the one finishing first and the one started last is the one finishing last, so when the last one finishes it is guaranteed that the other ones are finished as well. Therefore, only request completion processing on the last QMC channel. On my board with the above exemple, on a kernel started with 'threadirqs' option, the QMC irq thread uses 16% CPU time with this patch while it uses 26% CPU time without this patch. Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> --- sound/soc/fsl/fsl_qmc_audio.c | 95 +++++++++-------------------------- 1 file changed, 24 insertions(+), 71 deletions(-) diff --git a/sound/soc/fsl/fsl_qmc_audio.c b/sound/soc/fsl/fsl_qmc_audio.c index 5614a8b909ed..ecd614984b90 100644 --- a/sound/soc/fsl/fsl_qmc_audio.c +++ b/sound/soc/fsl/fsl_qmc_audio.c @@ -17,12 +17,6 @@ #include <sound/pcm_params.h> #include <sound/soc.h> -struct qmc_dai_chan { - struct qmc_dai_prtd *prtd_tx; - struct qmc_dai_prtd *prtd_rx; - struct qmc_chan *qmc_chan; -}; - struct qmc_dai { char *name; int id; @@ -33,7 +27,7 @@ struct qmc_dai { unsigned int nb_chans_avail; unsigned int nb_chans_used_tx; unsigned int nb_chans_used_rx; - struct qmc_dai_chan *chans; + struct qmc_chan **qmc_chans; }; struct qmc_audio { @@ -57,7 +51,6 @@ struct qmc_dai_prtd { size_t ch_dma_offset; unsigned int channels; - DECLARE_BITMAP(chans_pending, 64); struct snd_pcm_substream *substream; }; @@ -126,17 +119,14 @@ static int qmc_audio_pcm_write_submit(struct qmc_dai_prtd *prtd) int ret; for (i = 0; i < prtd->channels; i++) { - bitmap_set(prtd->chans_pending, i, 1); - - ret = qmc_chan_write_submit(prtd->qmc_dai->chans[i].qmc_chan, + ret = qmc_chan_write_submit(prtd->qmc_dai->qmc_chans[i], prtd->ch_dma_addr_current + i * prtd->ch_dma_offset, prtd->ch_dma_size, - qmc_audio_pcm_write_complete, - &prtd->qmc_dai->chans[i]); + i == prtd->channels - 1 ? qmc_audio_pcm_write_complete : + NULL, prtd); if (ret) { dev_err(prtd->qmc_dai->dev, "write_submit %u failed %d\n", i, ret); - bitmap_clear(prtd->chans_pending, i, 1); return ret; } } @@ -146,20 +136,7 @@ static int qmc_audio_pcm_write_submit(struct qmc_dai_prtd *prtd) static void qmc_audio_pcm_write_complete(void *context) { - struct qmc_dai_chan *chan = context; - struct qmc_dai_prtd *prtd; - - prtd = chan->prtd_tx; - - /* Mark the current channel as completed */ - bitmap_clear(prtd->chans_pending, chan - prtd->qmc_dai->chans, 1); - - /* - * All QMC channels involved must have completed their transfer before - * submitting a new one. - */ - if (!bitmap_empty(prtd->chans_pending, 64)) - return; + struct qmc_dai_prtd *prtd = context; prtd->buffer_ended += prtd->period_size; if (prtd->buffer_ended >= prtd->buffer_size) @@ -182,17 +159,14 @@ static int qmc_audio_pcm_read_submit(struct qmc_dai_prtd *prtd) int ret; for (i = 0; i < prtd->channels; i++) { - bitmap_set(prtd->chans_pending, i, 1); - - ret = qmc_chan_read_submit(prtd->qmc_dai->chans[i].qmc_chan, + ret = qmc_chan_read_submit(prtd->qmc_dai->qmc_chans[i], prtd->ch_dma_addr_current + i * prtd->ch_dma_offset, prtd->ch_dma_size, - qmc_audio_pcm_read_complete, - &prtd->qmc_dai->chans[i]); + i == prtd->channels - 1 ? qmc_audio_pcm_read_complete : + NULL, prtd); if (ret) { dev_err(prtd->qmc_dai->dev, "read_submit %u failed %d\n", i, ret); - bitmap_clear(prtd->chans_pending, i, 1); return ret; } } @@ -202,26 +176,13 @@ static int qmc_audio_pcm_read_submit(struct qmc_dai_prtd *prtd) static void qmc_audio_pcm_read_complete(void *context, size_t length, unsigned int flags) { - struct qmc_dai_chan *chan = context; - struct qmc_dai_prtd *prtd; - - prtd = chan->prtd_rx; - - /* Mark the current channel as completed */ - bitmap_clear(prtd->chans_pending, chan - prtd->qmc_dai->chans, 1); + struct qmc_dai_prtd *prtd = context; if (length != prtd->ch_dma_size) { dev_err(prtd->qmc_dai->dev, "read complete length = %zu, exp %zu\n", length, prtd->ch_dma_size); } - /* - * All QMC channels involved must have completed their transfer before - * submitting a new one. - */ - if (!bitmap_empty(prtd->chans_pending, 64)) - return; - prtd->buffer_ended += prtd->period_size; if (prtd->buffer_ended >= prtd->buffer_size) prtd->buffer_ended = 0; @@ -239,7 +200,6 @@ static int qmc_audio_pcm_trigger(struct snd_soc_component *component, struct snd_pcm_substream *substream, int cmd) { struct qmc_dai_prtd *prtd = substream->runtime->private_data; - unsigned int i; int ret; if (!prtd->qmc_dai) { @@ -249,14 +209,10 @@ static int qmc_audio_pcm_trigger(struct snd_soc_component *component, switch (cmd) { case SNDRV_PCM_TRIGGER_START: - bitmap_zero(prtd->chans_pending, 64); prtd->buffer_ended = 0; prtd->ch_dma_addr_current = prtd->ch_dma_addr_start; if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { - for (i = 0; i < prtd->channels; i++) - prtd->qmc_dai->chans[i].prtd_tx = prtd; - /* Submit first chunk ... */ ret = qmc_audio_pcm_write_submit(prtd); if (ret) @@ -272,9 +228,6 @@ static int qmc_audio_pcm_trigger(struct snd_soc_component *component, if (ret) return ret; } else { - for (i = 0; i < prtd->channels; i++) - prtd->qmc_dai->chans[i].prtd_rx = prtd; - /* Submit first chunk ... */ ret = qmc_audio_pcm_read_submit(prtd); if (ret) @@ -644,9 +597,9 @@ static int qmc_dai_hw_params(struct snd_pcm_substream *substream, chan_param.mode = QMC_TRANSPARENT; chan_param.transp.max_rx_buf_size = params_period_bytes(params) / nb_chans_used; for (i = 0; i < nb_chans_used; i++) { - ret = qmc_chan_set_param(qmc_dai->chans[i].qmc_chan, &chan_param); + ret = qmc_chan_set_param(qmc_dai->qmc_chans[i], &chan_param); if (ret) { - dev_err(dai->dev, "chans[%u], set param failed %d\n", + dev_err(dai->dev, "qmc_chans[%u], set param failed %d\n", i, ret); return ret; } @@ -688,7 +641,7 @@ static int qmc_dai_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_RESUME: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: for (i = 0; i < nb_chans_used; i++) { - ret = qmc_chan_start(qmc_dai->chans[i].qmc_chan, direction); + ret = qmc_chan_start(qmc_dai->qmc_chans[i], direction); if (ret) goto err_stop; } @@ -697,13 +650,13 @@ static int qmc_dai_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_STOP: /* Stop and reset all QMC channels and return the first error encountered */ for (i = 0; i < nb_chans_used; i++) { - ret_tmp = qmc_chan_stop(qmc_dai->chans[i].qmc_chan, direction); + ret_tmp = qmc_chan_stop(qmc_dai->qmc_chans[i], direction); if (!ret) ret = ret_tmp; if (ret_tmp) continue; - ret_tmp = qmc_chan_reset(qmc_dai->chans[i].qmc_chan, direction); + ret_tmp = qmc_chan_reset(qmc_dai->qmc_chans[i], direction); if (!ret) ret = ret_tmp; } @@ -715,7 +668,7 @@ static int qmc_dai_trigger(struct snd_pcm_substream *substream, int cmd, case SNDRV_PCM_TRIGGER_PAUSE_PUSH: /* Stop all QMC channels and return the first error encountered */ for (i = 0; i < nb_chans_used; i++) { - ret_tmp = qmc_chan_stop(qmc_dai->chans[i].qmc_chan, direction); + ret_tmp = qmc_chan_stop(qmc_dai->qmc_chans[i], direction); if (!ret) ret = ret_tmp; } @@ -731,8 +684,8 @@ static int qmc_dai_trigger(struct snd_pcm_substream *substream, int cmd, err_stop: while (i--) { - qmc_chan_stop(qmc_dai->chans[i].qmc_chan, direction); - qmc_chan_reset(qmc_dai->chans[i].qmc_chan, direction); + qmc_chan_stop(qmc_dai->qmc_chans[i], direction); + qmc_chan_reset(qmc_dai->qmc_chans[i], direction); } return ret; } @@ -823,19 +776,19 @@ static int qmc_audio_dai_parse(struct qmc_audio *qmc_audio, struct device_node * return dev_err_probe(qmc_audio->dev, -EINVAL, "dai %d no QMC channel defined\n", qmc_dai->id); - qmc_dai->chans = devm_kcalloc(qmc_audio->dev, count, sizeof(*qmc_dai->chans), GFP_KERNEL); - if (!qmc_dai->chans) + qmc_dai->qmc_chans = devm_kcalloc(qmc_audio->dev, count, sizeof(*qmc_dai->qmc_chans), GFP_KERNEL); + if (!qmc_dai->qmc_chans) return -ENOMEM; for (i = 0; i < count; i++) { - qmc_dai->chans[i].qmc_chan = devm_qmc_chan_get_byphandles_index(qmc_audio->dev, np, - "fsl,qmc-chan", i); - if (IS_ERR(qmc_dai->chans[i].qmc_chan)) { - return dev_err_probe(qmc_audio->dev, PTR_ERR(qmc_dai->chans[i].qmc_chan), + qmc_dai->qmc_chans[i] = devm_qmc_chan_get_byphandles_index(qmc_audio->dev, np, + "fsl,qmc-chan", i); + if (IS_ERR(qmc_dai->qmc_chans[i])) { + return dev_err_probe(qmc_audio->dev, PTR_ERR(qmc_dai->qmc_chans[i]), "dai %d get QMC channel %d failed\n", qmc_dai->id, i); } - ret = qmc_chan_get_info(qmc_dai->chans[i].qmc_chan, &info); + ret = qmc_chan_get_info(qmc_dai->qmc_chans[i], &info); if (ret) { dev_err(qmc_audio->dev, "dai %d get QMC %d channel info failed %d\n", qmc_dai->id, i, ret); -- 2.47.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ASoC: fsl: fsl_qmc_audio: Only request completion on last channel 2025-05-09 7:48 ` [PATCH 2/2] ASoC: fsl: fsl_qmc_audio: Only request completion on last channel Christophe Leroy @ 2025-05-09 8:45 ` Herve Codina 2025-05-09 9:13 ` Christophe Leroy 0 siblings, 1 reply; 6+ messages in thread From: Herve Codina @ 2025-05-09 8:45 UTC (permalink / raw) To: Christophe Leroy Cc: Qiang Zhao, Shengjiu Wang, Xiubo Li, Fabio Estevam, Nicolin Chen, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, linux-kernel, linuxppc-dev, linux-arm-kernel, linux-sound On Fri, 9 May 2025 09:48:45 +0200 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > In non-interleaved mode, several QMC channels are used in sync. > More details can be found in commit 188d9cae5438 ("ASoC: fsl: > fsl_qmc_audio: Add support for non-interleaved mode.") > At the time being, an interrupt is requested on each channel to > perform capture/playback completion, allthough the completion is > really performed only once all channels have completed their work. > > This leads to a lot more interrupts than really needed. Looking at > /proc/interrupts shows ~3800 interrupts per second when using > 4 capture and 4 playback devices with 5ms periods while > only 1600 (200 x 4 + 200 x 4) periods are processed during one second. > > The QMC channels work in sync, the one started first is the one > finishing first and the one started last is the one finishing last, How can we be sure about that? The mapping on the TDM bus has to be taken into account. chan 0 -> TDM bits 0..8 chan 1 -> TDM bits 16..23 chan 2 -> TDM bits 9..15 In that case chan 1 can finish after chan 2. qmc_chan_get_ts_info() could be used to get struct qmc_chan_ts_info and [rx,tx]_ts_mask field in the struct give the mapping information. The channel that ends last is the one with the highest bit set in the mask (rx_tx_mask for capture and tx_ts_mask for playback). Best regards, Hervé > so when the last one finishes it is guaranteed that the other ones are > finished as well. Therefore, only request completion processing on the > last QMC channel. > > On my board with the above exemple, on a kernel started with > 'threadirqs' option, the QMC irq thread uses 16% CPU time with this > patch while it uses 26% CPU time without this patch. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ASoC: fsl: fsl_qmc_audio: Only request completion on last channel 2025-05-09 8:45 ` Herve Codina @ 2025-05-09 9:13 ` Christophe Leroy 2025-05-09 10:12 ` Herve Codina 0 siblings, 1 reply; 6+ messages in thread From: Christophe Leroy @ 2025-05-09 9:13 UTC (permalink / raw) To: Herve Codina Cc: Qiang Zhao, Shengjiu Wang, Xiubo Li, Fabio Estevam, Nicolin Chen, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, linux-kernel, linuxppc-dev, linux-arm-kernel, linux-sound Hi Hervé, Le 09/05/2025 à 10:45, Herve Codina a écrit : > On Fri, 9 May 2025 09:48:45 +0200 > Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > >> In non-interleaved mode, several QMC channels are used in sync. >> More details can be found in commit 188d9cae5438 ("ASoC: fsl: >> fsl_qmc_audio: Add support for non-interleaved mode.") >> At the time being, an interrupt is requested on each channel to >> perform capture/playback completion, allthough the completion is >> really performed only once all channels have completed their work. >> >> This leads to a lot more interrupts than really needed. Looking at >> /proc/interrupts shows ~3800 interrupts per second when using >> 4 capture and 4 playback devices with 5ms periods while >> only 1600 (200 x 4 + 200 x 4) periods are processed during one second. >> >> The QMC channels work in sync, the one started first is the one >> finishing first and the one started last is the one finishing last, > > How can we be sure about that? > > The mapping on the TDM bus has to be taken into account. > > chan 0 -> TDM bits 0..8 > chan 1 -> TDM bits 16..23 > chan 2 -> TDM bits 9..15 In interleaved mode, the QMC will not allow that. You can have TS0-TS1-TS2 or TS1-TS2-TS0 but you can't have TS0-TS2-TS1. In non-interleaved mode we mimic the interleaved mode so I don't expect it either. > > In that case chan 1 can finish after chan 2. > > qmc_chan_get_ts_info() could be used to get struct qmc_chan_ts_info > and [rx,tx]_ts_mask field in the struct give the mapping information. > > The channel that ends last is the one with the highest bit set in the > mask (rx_tx_mask for capture and tx_ts_mask for playback). That would be right if the channels were starting all at exactely the same time. But qmc_audio_pcm_write_submit() and qmc_audio_pcm_read_submit() are calling resp. qmc_chan_write_submit() and qmc_chan_read_submit() one by one. Even if that happens it shouldn't be a problem on its own as there are only a few microseconds between each Timeslot (a full cycle is 125 µs). And also because calling snd_pcm_period_elapsed() doesn't have any destructive effect on ongoing processing. So I wouldn't make it too complicated. Here the benefit is real and worth it. Thanks, Christophe > > Best regards, > Hervé > >> so when the last one finishes it is guaranteed that the other ones are >> finished as well. Therefore, only request completion processing on the >> last QMC channel. >> >> On my board with the above exemple, on a kernel started with >> 'threadirqs' option, the QMC irq thread uses 16% CPU time with this >> patch while it uses 26% CPU time without this patch. >> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> >> --- ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] ASoC: fsl: fsl_qmc_audio: Only request completion on last channel 2025-05-09 9:13 ` Christophe Leroy @ 2025-05-09 10:12 ` Herve Codina 0 siblings, 0 replies; 6+ messages in thread From: Herve Codina @ 2025-05-09 10:12 UTC (permalink / raw) To: Christophe Leroy Cc: Qiang Zhao, Shengjiu Wang, Xiubo Li, Fabio Estevam, Nicolin Chen, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, linux-kernel, linuxppc-dev, linux-arm-kernel, linux-sound Hi Christophe, On Fri, 9 May 2025 11:13:12 +0200 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > Hi Hervé, > > Le 09/05/2025 à 10:45, Herve Codina a écrit : > > On Fri, 9 May 2025 09:48:45 +0200 > > Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > > > >> In non-interleaved mode, several QMC channels are used in sync. > >> More details can be found in commit 188d9cae5438 ("ASoC: fsl: > >> fsl_qmc_audio: Add support for non-interleaved mode.") > >> At the time being, an interrupt is requested on each channel to > >> perform capture/playback completion, allthough the completion is > >> really performed only once all channels have completed their work. > >> > >> This leads to a lot more interrupts than really needed. Looking at > >> /proc/interrupts shows ~3800 interrupts per second when using > >> 4 capture and 4 playback devices with 5ms periods while > >> only 1600 (200 x 4 + 200 x 4) periods are processed during one second. > >> > >> The QMC channels work in sync, the one started first is the one > >> finishing first and the one started last is the one finishing last, > > > > How can we be sure about that? > > > > The mapping on the TDM bus has to be taken into account. > > > > chan 0 -> TDM bits 0..8 > > chan 1 -> TDM bits 16..23 > > chan 2 -> TDM bits 9..15 > > In interleaved mode, the QMC will not allow that. You can have > TS0-TS1-TS2 or TS1-TS2-TS0 but you can't have TS0-TS2-TS1. > > In non-interleaved mode we mimic the interleaved mode so I don't expect > it either. I am not so sure that the case shouldn't be handled by QMC. Even if it is not possible at QMC level, you can have it at qmc_audio level. The qmc_audio driver depends on the DT binding: dai@18 { reg = <18>; /* Non-interleaved mode */ fsl,qmc-chan = <&qmc 18>, <&qmc 19>; }; but you can have dai@18 { reg = <18>; /* Non-interleaved mode */ fsl,qmc-chan = <&qmc 19>, <&qmc 18>; }; > > > > > In that case chan 1 can finish after chan 2. > > > > qmc_chan_get_ts_info() could be used to get struct qmc_chan_ts_info > > and [rx,tx]_ts_mask field in the struct give the mapping information. > > > > The channel that ends last is the one with the highest bit set in the > > mask (rx_tx_mask for capture and tx_ts_mask for playback). > > That would be right if the channels were starting all at exactely the > same time. But qmc_audio_pcm_write_submit() and > qmc_audio_pcm_read_submit() are calling resp. qmc_chan_write_submit() > and qmc_chan_read_submit() one by one. > > Even if that happens it shouldn't be a problem on its own as there are > only a few microseconds between each Timeslot (a full cycle is 125 µs). > And also because calling snd_pcm_period_elapsed() doesn't have any > destructive effect on ongoing processing. > > So I wouldn't make it too complicated. Here the benefit is real and > worth it. I fully understand the benefit and I am not against the feature. Also, I fully agree that it has to be kept as simple as possible. My point is to avoid some possible regressions. Maybe during probe, when the channels are parsed [0], the code should take of the channel location on TDM to have them in the better order in its table. We can imagine that after filling the array, the driver sorts the array using sort() or sort_r() [1] to ensure that the last item in the array is the last on the TDM bus. [0] https://elixir.bootlin.com/linux/v6.15-rc5/source/sound/soc/fsl/fsl_qmc_audio.c#L833 [1] https://elixir.bootlin.com/linux/v6.15-rc5/source/include/linux/sort.h -- Hervé Codina, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] soc: fsl: qmc: Only set completion interrupt when needed 2025-05-09 7:48 [PATCH 1/2] soc: fsl: qmc: Only set completion interrupt when needed Christophe Leroy 2025-05-09 7:48 ` [PATCH 2/2] ASoC: fsl: fsl_qmc_audio: Only request completion on last channel Christophe Leroy @ 2025-05-09 9:15 ` Herve Codina 1 sibling, 0 replies; 6+ messages in thread From: Herve Codina @ 2025-05-09 9:15 UTC (permalink / raw) To: Christophe Leroy Cc: Qiang Zhao, Shengjiu Wang, Xiubo Li, Fabio Estevam, Nicolin Chen, Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai, linux-kernel, linuxppc-dev, linux-arm-kernel, linux-sound Hi Christophe, On Fri, 9 May 2025 09:48:44 +0200 Christophe Leroy <christophe.leroy@csgroup.eu> wrote: > When no post-completion processing is expected, don't waste time > handling useless interrupts. > > Only set QMC_BD_[R/T]X_I and QMC_BD_[R/T]X_UB when a completion > function is passed in. > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu> > --- > drivers/soc/fsl/qe/qmc.c | 18 +++++++++++------- > 1 file changed, 11 insertions(+), 7 deletions(-) > > diff --git a/drivers/soc/fsl/qe/qmc.c b/drivers/soc/fsl/qe/qmc.c > index 36c0ccc06151..0a704fd0b1a0 100644 > --- a/drivers/soc/fsl/qe/qmc.c > +++ b/drivers/soc/fsl/qe/qmc.c > @@ -474,7 +474,9 @@ int qmc_chan_write_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length, > xfer_desc->context = context; > > /* Activate the descriptor */ > - ctrl |= (QMC_BD_TX_R | QMC_BD_TX_UB); > + ctrl |= QMC_BD_TX_R; > + if (complete) > + ctrl |= QMC_BD_TX_I | QMC_BD_TX_UB; Be careful, you don't set the UB bit for all descriptor anymore. Your goal, is to have interrupts only on some descriptors (those where I bit is set). This can lead to issue in the function handling the interrupt. This function, qmc_chan_write_done(), do the processing according to the following: /* * R bit UB bit * 0 0 : The BD is free * 1 1 : The BD is in used, waiting for transfer * 0 1 : The BD is in used, waiting for completion * 1 0 : Should not append */ https://elixir.bootlin.com/linux/v6.15-rc5/source/drivers/soc/fsl/qe/qmc.c#L507 It considers R=0 / UB=0 as a free BD and R=1 / UB=0 as a case that should not happen. Both cases are no more correct with your modification. Have the feeling that UB bit still has to be set even if I bit is not set in order to have qmc_chan_write_done() looking at all descriptors. Suppose: desc 0, no interrupt desc 1, no interrupt desc 2, interrupt When the interrupt for desc 2 is handled, desc 0 and desc 1 are seen with R=0 and UB=0. As desc 0 is considered as free by qmc_chan_write_done(), it will never look at desc 2. > wmb(); /* Be sure to flush the descriptor before control update */ > qmc_write16(&bd->cbd_sc, ctrl); > > @@ -586,7 +588,9 @@ int qmc_chan_read_submit(struct qmc_chan *chan, dma_addr_t addr, size_t length, > QMC_BD_RX_AB | QMC_BD_RX_CR); > > /* Activate the descriptor */ > - ctrl |= (QMC_BD_RX_E | QMC_BD_RX_UB); > + ctrl |= QMC_BD_RX_E; > + if (complete) > + ctrl |= QMC_BD_RX_I | QMC_BD_RX_UB; Exact same comment. Best regards, Hervé ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-05-09 10:12 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-05-09 7:48 [PATCH 1/2] soc: fsl: qmc: Only set completion interrupt when needed Christophe Leroy 2025-05-09 7:48 ` [PATCH 2/2] ASoC: fsl: fsl_qmc_audio: Only request completion on last channel Christophe Leroy 2025-05-09 8:45 ` Herve Codina 2025-05-09 9:13 ` Christophe Leroy 2025-05-09 10:12 ` Herve Codina 2025-05-09 9:15 ` [PATCH 1/2] soc: fsl: qmc: Only set completion interrupt when needed Herve Codina
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).