[Sound-open-firmware] [PATCH] apl-ssp: fix capture double speed issue
Previously TFT and RFT are set to 0s which means we are using threshold level 1, where interrupt asserts for every valid bits of each slot(e.g. for s16_le capture, interrupts assert for each 16 bits), but DMAC will treat it as notification of one full sample(e.g. 32 bits for s16_le stereo) and read those all bits, this will lead to capture with double speed at format s16_le stereo.
Here add handle for FIFO trigger Threshold level, set them to bytes for each full sample(active_slots * valid_bits / 8), and the issue is fixed.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com --- src/drivers/apl-ssp.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index 1ee107c..ecf70fb 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -43,6 +43,19 @@ #define trace_ssp_error(__e) trace_error(TRACE_CLASS_SSP, __e) #define tracev_ssp(__e) tracev_event(TRACE_CLASS_SSP, __e)
+/* FIXME: move this to a helper and optimize */ +static int hweight_32(uint32_t mask) +{ + int i; + int count = 0; + + for (i = 0; i < 32; i++) { + count += mask & 1; + mask >>= 1; + } + return count; +} + /* save SSP context prior to entering D3 */ static int ssp_context_store(struct dai *dai) { @@ -96,6 +109,9 @@ static inline int ssp_set_config(struct dai *dai, uint32_t dummy_stop; uint32_t frame_len = 0; uint32_t bdiv_min; + uint32_t active_tx_slots = 2; + uint32_t active_rx_slots = 2; + bool inverted_frame = false; int ret = 0;
@@ -353,6 +369,9 @@ static inline int ssp_set_config(struct dai *dai, sspsp |= SSPSP_SFRMP(!inverted_frame); sspsp |= SSPSP_FSRT;
+ active_tx_slots = hweight_32(config->tx_slot_mask); + active_rx_slots = hweight_32(config->rx_slot_mask); + break; case SOF_DAI_FMT_DSP_B:
@@ -371,6 +390,9 @@ static inline int ssp_set_config(struct dai *dai, */ sspsp |= SSPSP_SFRMP(!inverted_frame);
+ active_tx_slots = hweight_32(config->tx_slot_mask); + active_rx_slots = hweight_32(config->rx_slot_mask); + break; default: trace_ssp_error("eca"); @@ -408,6 +430,9 @@ static inline int ssp_set_config(struct dai *dai, /* bypass divider for MCLK */ mdivr = 0x00000fff;
+ sscr3 |= SSCR3_TX(active_tx_slots * config->sample_valid_bits / 8) | + SSCR3_RX(active_rx_slots * config->sample_valid_bits / 8); + trace_ssp("coe"); ssp_write(dai, SSCR0, sscr0); ssp_write(dai, SSCR1, sscr1);
On Wed, 2018-03-14 at 23:12 +0800, Keyon Jie wrote:
Previously TFT and RFT are set to 0s which means we are using threshold level 1, where interrupt asserts for every valid bits of each slot(e.g. for s16_le capture, interrupts assert for each 16 bits), but DMAC will treat it as notification of one full sample(e.g. 32 bits for s16_le stereo) and read those all bits, this will lead to capture with double speed at format s16_le stereo.
Here add handle for FIFO trigger Threshold level, set them to bytes for each full sample(active_slots * valid_bits / 8), and the issue is fixed.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com
src/drivers/apl-ssp.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index 1ee107c..ecf70fb 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -43,6 +43,19 @@ #define trace_ssp_error(__e) trace_error(TRACE_CLASS_SSP, __e) #define tracev_ssp(__e) tracev_event(TRACE_CLASS_SSP, __e)
+/* FIXME: move this to a helper and optimize */ +static int hweight_32(uint32_t mask)
count_set_bits32() and can you move to lib/lib.c probably separate patch too.
+{
- int i;
- int count = 0;
- for (i = 0; i < 32; i++) {
count += mask & 1;mask >>= 1;- }
- return count;
+}
/* save SSP context prior to entering D3 */ static int ssp_context_store(struct dai *dai) { @@ -96,6 +109,9 @@ static inline int ssp_set_config(struct dai *dai, uint32_t dummy_stop; uint32_t frame_len = 0; uint32_t bdiv_min;
- uint32_t active_tx_slots = 2;
- uint32_t active_rx_slots = 2;
why do we use 2 as default value, for stereo. ?
This value should always be set based on channel mask so I would set to 0 here and check that it's not 0 later on (return error and produce trace if 0).
- bool inverted_frame = false; int ret = 0;
@@ -353,6 +369,9 @@ static inline int ssp_set_config(struct dai *dai, sspsp |= SSPSP_SFRMP(!inverted_frame); sspsp |= SSPSP_FSRT;
active_tx_slots = hweight_32(config->tx_slot_mask);active_rx_slots = hweight_32(config->rx_slot_mask);- break; case SOF_DAI_FMT_DSP_B:
@@ -371,6 +390,9 @@ static inline int ssp_set_config(struct dai *dai, */ sspsp |= SSPSP_SFRMP(!inverted_frame);
active_tx_slots = hweight_32(config->tx_slot_mask);active_rx_slots = hweight_32(config->rx_slot_mask);- break; default: trace_ssp_error("eca");
@@ -408,6 +430,9 @@ static inline int ssp_set_config(struct dai *dai, /* bypass divider for MCLK */ mdivr = 0x00000fff;
- sscr3 |= SSCR3_TX(active_tx_slots * config->sample_valid_bits / 8) |
SSCR3_RX(active_rx_slots * config->sample_valid_bits / 8);- trace_ssp("coe"); ssp_write(dai, SSCR0, sscr0); ssp_write(dai, SSCR1, sscr1);
On 03/14/2018 11:04 AM, Liam Girdwood wrote:
On Wed, 2018-03-14 at 23:12 +0800, Keyon Jie wrote:
Previously TFT and RFT are set to 0s which means we are using threshold level 1, where interrupt asserts for every valid bits of each slot(e.g. for s16_le capture, interrupts assert for each 16 bits), but DMAC will treat it as notification of one full sample(e.g. 32 bits for s16_le stereo) and read those all bits, this will lead to capture with double speed at format s16_le stereo.
Here add handle for FIFO trigger Threshold level, set them to bytes for each full sample(active_slots * valid_bits / 8), and the issue is fixed.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com
src/drivers/apl-ssp.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index 1ee107c..ecf70fb 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -43,6 +43,19 @@ #define trace_ssp_error(__e) trace_error(TRACE_CLASS_SSP, __e) #define tracev_ssp(__e) tracev_event(TRACE_CLASS_SSP, __e)
+/* FIXME: move this to a helper and optimize */ +static int hweight_32(uint32_t mask)
count_set_bits32() and can you move to lib/lib.c probably separate patch too.
Came from ssp.c, something I never optimized...
+{
- int i;
- int count = 0;
- for (i = 0; i < 32; i++) {
count += mask & 1;mask >>= 1;- }
- return count;
+}
- /* save SSP context prior to entering D3 */ static int ssp_context_store(struct dai *dai) {
@@ -96,6 +109,9 @@ static inline int ssp_set_config(struct dai *dai, uint32_t dummy_stop; uint32_t frame_len = 0; uint32_t bdiv_min;
- uint32_t active_tx_slots = 2;
- uint32_t active_rx_slots = 2;
why do we use 2 as default value, for stereo. ?
This value should always be set based on channel mask so I would set to 0 here and check that it's not 0 later on (return error and produce trace if 0).
this was an 'optimization' based on the fact that for I2S/LEFT_J we can keep this as stereo. If we don't set those then we have to manually set this to two in the switch case.
- bool inverted_frame = false; int ret = 0;
@@ -353,6 +369,9 @@ static inline int ssp_set_config(struct dai *dai, sspsp |= SSPSP_SFRMP(!inverted_frame); sspsp |= SSPSP_FSRT;
active_tx_slots = hweight_32(config->tx_slot_mask);active_rx_slots = hweight_32(config->rx_slot_mask);- break; case SOF_DAI_FMT_DSP_B:
@@ -371,6 +390,9 @@ static inline int ssp_set_config(struct dai *dai, */ sspsp |= SSPSP_SFRMP(!inverted_frame);
active_tx_slots = hweight_32(config->tx_slot_mask);active_rx_slots = hweight_32(config->rx_slot_mask);- break; default: trace_ssp_error("eca");
@@ -408,6 +430,9 @@ static inline int ssp_set_config(struct dai *dai, /* bypass divider for MCLK */ mdivr = 0x00000fff;
- sscr3 |= SSCR3_TX(active_tx_slots * config->sample_valid_bits / 8) |
SSCR3_RX(active_rx_slots * config->sample_valid_bits / 8);
For Baytrail, we had:
/* FIXME: * watermarks - (RFT + 1) should equal DMA SRC_MSIZE */ sfifott = (SFIFOTT_TX(2*active_tx_slots) | SFIFOTT_RX(2*active_rx_slots));
Don't we need to align RFT/TFT to the DMA_SRC_MSIZE as well? e.g. for 24 bits can we actually have a threshold that would be a multiple of 3??
trace_ssp("coe"); ssp_write(dai, SSCR0, sscr0); ssp_write(dai, SSCR1, sscr1);
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Wed, 2018-03-14 at 11:37 -0500, Pierre-Louis Bossart wrote:
On 03/14/2018 11:04 AM, Liam Girdwood wrote:
On Wed, 2018-03-14 at 23:12 +0800, Keyon Jie wrote:
Previously TFT and RFT are set to 0s which means we are using threshold level 1, where interrupt asserts for every valid bits of each slot(e.g. for s16_le capture, interrupts assert for each 16 bits), but DMAC will treat it as notification of one full sample(e.g. 32 bits for s16_le stereo) and read those all bits, this will lead to capture with double speed at format s16_le stereo.
Here add handle for FIFO trigger Threshold level, set them to bytes for each full sample(active_slots * valid_bits / 8), and the issue is fixed.
Signed-off-by: Keyon Jie yang.jie@linux.intel.com
src/drivers/apl-ssp.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)
diff --git a/src/drivers/apl-ssp.c b/src/drivers/apl-ssp.c index 1ee107c..ecf70fb 100644 --- a/src/drivers/apl-ssp.c +++ b/src/drivers/apl-ssp.c @@ -43,6 +43,19 @@ #define trace_ssp_error(__e) trace_error(TRACE_CLASS_SSP, __e) #define tracev_ssp(__e) tracev_event(TRACE_CLASS_SSP, __e)
+/* FIXME: move this to a helper and optimize */ +static int hweight_32(uint32_t mask)
count_set_bits32() and can you move to lib/lib.c probably separate patch too.
Came from ssp.c, something I never optimized...
+{
- int i;
- int count = 0;
- for (i = 0; i < 32; i++) {
count += mask & 1;mask >>= 1;- }
- return count;
+}
- /* save SSP context prior to entering D3 */ static int ssp_context_store(struct dai *dai) {
@@ -96,6 +109,9 @@ static inline int ssp_set_config(struct dai *dai, uint32_t dummy_stop; uint32_t frame_len = 0; uint32_t bdiv_min;
- uint32_t active_tx_slots = 2;
- uint32_t active_rx_slots = 2;
why do we use 2 as default value, for stereo. ?
This value should always be set based on channel mask so I would set to 0 here and check that it's not 0 later on (return error and produce trace if 0).
this was an 'optimization' based on the fact that for I2S/LEFT_J we can keep this as stereo. If we don't set those then we have to manually set this to two in the switch case.
Some of the data used comes from topology so we would need to make sure that topology tx/rx slot mask would be non zero as they are used to multiply this value;.
- bool inverted_frame = false; int ret = 0;
@@ -353,6 +369,9 @@ static inline int ssp_set_config(struct dai *dai, sspsp |= SSPSP_SFRMP(!inverted_frame); sspsp |= SSPSP_FSRT;
active_tx_slots = hweight_32(config->tx_slot_mask);active_rx_slots = hweight_32(config->rx_slot_mask);- break; case SOF_DAI_FMT_DSP_B:
@@ -371,6 +390,9 @@ static inline int ssp_set_config(struct dai *dai, */ sspsp |= SSPSP_SFRMP(!inverted_frame);
active_tx_slots = hweight_32(config->tx_slot_mask);active_rx_slots = hweight_32(config->rx_slot_mask);- break; default: trace_ssp_error("eca");
@@ -408,6 +430,9 @@ static inline int ssp_set_config(struct dai *dai, /* bypass divider for MCLK */ mdivr = 0x00000fff;
- sscr3 |= SSCR3_TX(active_tx_slots * config->sample_valid_bits /
- |
SSCR3_RX(active_rx_slots * config->sample_valid_bits /8);
For Baytrail, we had:
/* FIXME: * watermarks - (RFT + 1) should equal DMA SRC_MSIZE */ sfifott = (SFIFOTT_TX(2*active_tx_slots) | SFIFOTT_RX(2*active_rx_slots));Don't we need to align RFT/TFT to the DMA_SRC_MSIZE as well? e.g. for 24 bits can we actually have a threshold that would be a multiple of 3??
Not sure, iirc one of the watermark registers is deprecated.
Liam
trace_ssp("coe"); ssp_write(dai, SSCR0, sscr0); ssp_write(dai, SSCR1, sscr1);
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
/* save SSP context prior to entering D3 */ static int ssp_context_store(struct dai *dai) { @@ -96,6 +109,9 @@ static inline int ssp_set_config(struct dai *dai, uint32_t dummy_stop; uint32_t frame_len = 0; uint32_t bdiv_min;
- uint32_t active_tx_slots = 2;
- uint32_t active_rx_slots = 2;
why do we use 2 as default value, for stereo. ?
This value should always be set based on channel mask so I would set to 0 here and check that it's not 0 later on (return error and produce trace if 0).
this was an 'optimization' based on the fact that for I2S/LEFT_J we can keep this as stereo. If we don't set those then we have to manually set this to two in the switch case.
Some of the data used comes from topology so we would need to make sure that topology tx/rx slot mask would be non zero as they are used to multiply this value;.
For I2S and LeftJ I don't think we can have a mono solution or more than 2 channels, we might as well ignore the channel mask to use 2 slots, always.
- bool inverted_frame = false; int ret = 0;
@@ -353,6 +369,9 @@ static inline int ssp_set_config(struct dai *dai, sspsp |= SSPSP_SFRMP(!inverted_frame); sspsp |= SSPSP_FSRT;
active_tx_slots = hweight_32(config->tx_slot_mask);active_rx_slots = hweight_32(config->rx_slot_mask);break;@@ -371,6 +390,9 @@ static inline int ssp_set_config(struct dai *dai, */ sspsp |= SSPSP_SFRMP(!inverted_frame);
active_tx_slots = hweight_32(config->tx_slot_mask);active_rx_slots = hweight_32(config->rx_slot_mask);break;@@ -408,6 +430,9 @@ static inline int ssp_set_config(struct dai *dai, /* bypass divider for MCLK */ mdivr = 0x00000fff;
- sscr3 |= SSCR3_TX(active_tx_slots * config->sample_valid_bits /
- |
SSCR3_RX(active_rx_slots * config->sample_valid_bits /8);
For Baytrail, we had:
/* FIXME: * watermarks - (RFT + 1) should equal DMA SRC_MSIZE */ sfifott = (SFIFOTT_TX(2*active_tx_slots) | SFIFOTT_RX(2*active_rx_slots));Don't we need to align RFT/TFT to the DMA_SRC_MSIZE as well? e.g. for 24 bits can we actually have a threshold that would be a multiple of 3??
Not sure, iirc one of the watermark registers is deprecated.
Let's ask a different question: does this work with 24 bits with a DMA handling powers of two?
On 2018年03月15日 02:08, Pierre-Louis Bossart wrote:
/* save SSP context prior to entering D3 */ static int ssp_context_store(struct dai *dai) { @@ -96,6 +109,9 @@ static inline int ssp_set_config(struct dai *dai, uint32_t dummy_stop; uint32_t frame_len = 0; uint32_t bdiv_min; + uint32_t active_tx_slots = 2; + uint32_t active_rx_slots = 2;
why do we use 2 as default value, for stereo. ?
This value should always be set based on channel mask so I would set to 0 here and check that it's not 0 later on (return error and produce trace if 0).
this was an 'optimization' based on the fact that for I2S/LEFT_J we can keep this as stereo. If we don't set those then we have to manually set this to two in the switch case.
Some of the data used comes from topology so we would need to make sure that topology tx/rx slot mask would be non zero as they are used to multiply this value;.
yes, that's true, as Pierre said, we copy this from ssp.c, shall we optimize that later, together with ssp.c?
For I2S and LeftJ I don't think we can have a mono solution or more than 2 channels, we might as well ignore the channel mask to use 2 slots, always.
bool inverted_frame = false; int ret = 0; @@ -353,6 +369,9 @@ static inline int ssp_set_config(struct dai *dai, sspsp |= SSPSP_SFRMP(!inverted_frame); sspsp |= SSPSP_FSRT; + active_tx_slots = hweight_32(config->tx_slot_mask); + active_rx_slots = hweight_32(config->rx_slot_mask);
break; case SOF_DAI_FMT_DSP_B: @@ -371,6 +390,9 @@ static inline int ssp_set_config(struct dai *dai, */ sspsp |= SSPSP_SFRMP(!inverted_frame); + active_tx_slots = hweight_32(config->tx_slot_mask); + active_rx_slots = hweight_32(config->rx_slot_mask);
break; default: trace_ssp_error("eca"); @@ -408,6 +430,9 @@ static inline int ssp_set_config(struct dai *dai, /* bypass divider for MCLK */ mdivr = 0x00000fff; + sscr3 |= SSCR3_TX(active_tx_slots * config->sample_valid_bits / 8) | + SSCR3_RX(active_rx_slots * config->sample_valid_bits / 8);
For Baytrail, we had:
/* FIXME: * watermarks - (RFT + 1) should equal DMA SRC_MSIZE */ sfifott = (SFIFOTT_TX(2*active_tx_slots) | SFIFOTT_RX(2*active_rx_slots));
Don't we need to align RFT/TFT to the DMA_SRC_MSIZE as well? e.g. for 24 bits can we actually have a threshold that would be a multiple of 3??
Not sure, iirc one of the watermark registers is deprecated.
Let's ask a different question: does this work with 24 bits with a DMA handling powers of two?
that's actually a concern from me also after sent this out, maybe I need rework to treat 24bits as 32 bits for this case?
Thanks, ~Keyon
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Thu, 2018-03-15 at 14:36 +0800, Keyon Jie wrote:
Some of the data used comes from topology so we would need to make sure that topology tx/rx slot mask would be non zero as they are used to multiply this value;.
yes, that's true, as Pierre said, we copy this from ssp.c, shall we optimize that later, together with ssp.c?
Yes, we can do that later for 1.2
Liam
participants (3)
-
Keyon Jie -
Liam Girdwood -
Pierre-Louis Bossart