diff mbox series

[3/3] ASoC: fsl_asrc: Fix error with S24_3LE format bitstream in i.MX8

Message ID 2b6e028ca27b8569da4ab7868d7b90ff8c3225d0.1568025083.git.shengjiu.wang@nxp.com
State New
Headers show
Series update supported sample format | expand

Commit Message

Shengjiu Wang Sept. 9, 2019, 10:33 p.m. UTC 
There is error "aplay: pcm_write:2023: write error: Input/output error"
on i.MX8QM/i.MX8QXP platform for S24_3LE format.

In i.MX8QM/i.MX8QXP, the DMA is EDMA, which don't support 24bit
sample, but we didn't add any constraint, that cause issues.

So we need to query the caps of dma, then update the hw parameters
according to the caps.

Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>

---
 sound/soc/fsl/fsl_asrc.c     |  4 +-
 sound/soc/fsl/fsl_asrc.h     |  3 ++
 sound/soc/fsl/fsl_asrc_dma.c | 93 +++++++++++++++++++++++++++++++++---
 3 files changed, 92 insertions(+), 8 deletions(-)

-- 
2.21.0

Comments

Nicolin Chen Sept. 10, 2019, 1:52 a.m. UTC | #1 
On Mon, Sep 09, 2019 at 06:33:21PM -0400, Shengjiu Wang wrote:
> There is error "aplay: pcm_write:2023: write error: Input/output error"

> on i.MX8QM/i.MX8QXP platform for S24_3LE format.

> 

> In i.MX8QM/i.MX8QXP, the DMA is EDMA, which don't support 24bit

> sample, but we didn't add any constraint, that cause issues.

> 

> So we need to query the caps of dma, then update the hw parameters

> according to the caps.


> @@ -285,8 +293,81 @@ static int fsl_asrc_dma_startup(struct snd_pcm_substream *substream)

>  

>  	runtime->private_data = pair;

>  

> -	snd_pcm_hw_constraint_integer(substream->runtime,

> -				      SNDRV_PCM_HW_PARAM_PERIODS);

> +	ret = snd_pcm_hw_constraint_integer(substream->runtime,

> +					    SNDRV_PCM_HW_PARAM_PERIODS);

> +	if (ret < 0) {

> +		dev_err(dev, "failed to set pcm hw params periods\n");

> +		return ret;

> +	}

> +

> +	dma_data = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);

> +

> +	/* Request a temp pair, which is release in the end */

> +	fsl_asrc_request_pair(1, pair);


Not sure if it'd be practical, but a pair request could fail. Will
probably need to check return value.

And a quick feeling is that below code is mostly identical to what
is in the soc-generic-dmaengine-pcm.c file. So I'm wondering if we
could abstract a helper function somewhere in the ASoC core: Mark?

Thanks
Nicolin

> +	tmp_chan = fsl_asrc_get_dma_channel(pair, dir);

> +	if (!tmp_chan) {

> +		dev_err(dev, "can't get dma channel\n");

> +		return -EINVAL;

> +	}

> +

> +	ret = dma_get_slave_caps(tmp_chan, &dma_caps);

> +	if (ret == 0) {

> +		if (dma_caps.cmd_pause)

> +			snd_imx_hardware.info |= SNDRV_PCM_INFO_PAUSE |

> +						 SNDRV_PCM_INFO_RESUME;

> +		if (dma_caps.residue_granularity <=

> +			DMA_RESIDUE_GRANULARITY_SEGMENT)

> +			snd_imx_hardware.info |= SNDRV_PCM_INFO_BATCH;

> +

> +		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)

> +			addr_widths = dma_caps.dst_addr_widths;

> +		else

> +			addr_widths = dma_caps.src_addr_widths;

> +	}

> +

> +	/*

> +	 * If SND_DMAENGINE_PCM_DAI_FLAG_PACK is set keep

> +	 * hw.formats set to 0, meaning no restrictions are in place.

> +	 * In this case it's the responsibility of the DAI driver to

> +	 * provide the supported format information.

> +	 */

> +	if (!(dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK))

> +		/*

> +		 * Prepare formats mask for valid/allowed sample types. If the

> +		 * dma does not have support for the given physical word size,

> +		 * it needs to be masked out so user space can not use the

> +		 * format which produces corrupted audio.

> +		 * In case the dma driver does not implement the slave_caps the

> +		 * default assumption is that it supports 1, 2 and 4 bytes

> +		 * widths.

> +		 */

> +		for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {

> +			int bits = snd_pcm_format_physical_width(i);

> +

> +			/*

> +			 * Enable only samples with DMA supported physical

> +			 * widths

> +			 */

> +			switch (bits) {

> +			case 8:

> +			case 16:

> +			case 24:

> +			case 32:

> +			case 64:

> +				if (addr_widths & (1 << (bits / 8)))

> +					snd_imx_hardware.formats |= (1LL << i);

> +				break;

> +			default:

> +				/* Unsupported types */

> +				break;

> +			}

> +		}

> +

> +	if (tmp_chan)

> +		dma_release_channel(tmp_chan);

> +	fsl_asrc_release_pair(pair);

> +

>  	snd_soc_set_runtime_hwparams(substream, &snd_imx_hardware);

>  

>  	return 0;

> -- 

> 2.21.0

>
Shengjiu Wang Sept. 10, 2019, 2:24 a.m. UTC | #2 
Hi

> 

> On Mon, Sep 09, 2019 at 06:33:21PM -0400, Shengjiu Wang wrote:

> > There is error "aplay: pcm_write:2023: write error: Input/output error"

> > on i.MX8QM/i.MX8QXP platform for S24_3LE format.

> >

> > In i.MX8QM/i.MX8QXP, the DMA is EDMA, which don't support 24bit

> > sample, but we didn't add any constraint, that cause issues.

> >

> > So we need to query the caps of dma, then update the hw parameters

> > according to the caps.

> 

> > @@ -285,8 +293,81 @@ static int fsl_asrc_dma_startup(struct

> > snd_pcm_substream *substream)

> >

> >       runtime->private_data = pair;

> >

> > -     snd_pcm_hw_constraint_integer(substream->runtime,

> > -                                   SNDRV_PCM_HW_PARAM_PERIODS);

> > +     ret = snd_pcm_hw_constraint_integer(substream->runtime,

> > +                                         SNDRV_PCM_HW_PARAM_PERIODS);

> > +     if (ret < 0) {

> > +             dev_err(dev, "failed to set pcm hw params periods\n");

> > +             return ret;

> > +     }

> > +

> > +     dma_data = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);

> > +

> > +     /* Request a temp pair, which is release in the end */

> > +     fsl_asrc_request_pair(1, pair);

> 

> Not sure if it'd be practical, but a pair request could fail. Will probably need

> to check return value.

> 

> And a quick feeling is that below code is mostly identical to what is in the

> soc-generic-dmaengine-pcm.c file. So I'm wondering if we could abstract a

> helper function somewhere in the ASoC core: Mark?

> 

> Thanks

> Nicolin

> 

Yes, it refers to the code in soc-generic-dmaengine-pcm.c, if there is a common
API, this is helpful.

Best regards
Wang shengjiu
Mark Brown Sept. 11, 2019, 11:08 a.m. UTC | #3 
On Mon, Sep 09, 2019 at 06:52:13PM -0700, Nicolin Chen wrote:

> And a quick feeling is that below code is mostly identical to what

> is in the soc-generic-dmaengine-pcm.c file. So I'm wondering if we

> could abstract a helper function somewhere in the ASoC core: Mark?


That's roughly what sound/core/pcm_dmaengine.c is doing -
possibly we should move more stuff into there.
Nicolin Chen Sept. 12, 2019, 11:53 p.m. UTC | #4 
On Wed, Sep 11, 2019 at 12:08:07PM +0100, Mark Brown wrote:
> On Mon, Sep 09, 2019 at 06:52:13PM -0700, Nicolin Chen wrote:

> 

> > And a quick feeling is that below code is mostly identical to what

> > is in the soc-generic-dmaengine-pcm.c file. So I'm wondering if we

> > could abstract a helper function somewhere in the ASoC core: Mark?

> 

> That's roughly what sound/core/pcm_dmaengine.c is doing -

> possibly we should move more stuff into there.


It looks like a right place to me. Thank you!
diff mbox series

Patch

diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
index 584badf956d2..0bf91a6f54b9 100644
--- a/sound/soc/fsl/fsl_asrc.c
+++ b/sound/soc/fsl/fsl_asrc.c
@@ -115,7 +115,7 @@  static void fsl_asrc_sel_proc(int inrate, int outrate,
  * within range [ANCA, ANCA+ANCB-1], depends on the channels of pair A
  * while pair A and pair C are comparatively independent.
  */
-static int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
+int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
 {
 	enum asrc_pair_index index = ASRC_INVALID_PAIR;
 	struct fsl_asrc *asrc_priv = pair->asrc_priv;
@@ -158,7 +158,7 @@  static int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
  *
  * It clears the resource from asrc_priv and releases the occupied channels.
  */
-static void fsl_asrc_release_pair(struct fsl_asrc_pair *pair)
+void fsl_asrc_release_pair(struct fsl_asrc_pair *pair)
 {
 	struct fsl_asrc *asrc_priv = pair->asrc_priv;
 	enum asrc_pair_index index = pair->index;
diff --git a/sound/soc/fsl/fsl_asrc.h b/sound/soc/fsl/fsl_asrc.h
index 38af485bdd22..2b57e8c53728 100644
--- a/sound/soc/fsl/fsl_asrc.h
+++ b/sound/soc/fsl/fsl_asrc.h
@@ -462,4 +462,7 @@  struct fsl_asrc {
 #define DRV_NAME "fsl-asrc-dai"
 extern struct snd_soc_component_driver fsl_asrc_component;
 struct dma_chan *fsl_asrc_get_dma_channel(struct fsl_asrc_pair *pair, bool dir);
+int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair);
+void fsl_asrc_release_pair(struct fsl_asrc_pair *pair);
+
 #endif /* _FSL_ASRC_H */
diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
index 01052a0808b0..30e27917016e 100644
--- a/sound/soc/fsl/fsl_asrc_dma.c
+++ b/sound/soc/fsl/fsl_asrc_dma.c
@@ -16,13 +16,11 @@ 
 
 #define FSL_ASRC_DMABUF_SIZE	(256 * 1024)
 
-static const struct snd_pcm_hardware snd_imx_hardware = {
+static struct snd_pcm_hardware snd_imx_hardware = {
 	.info = SNDRV_PCM_INFO_INTERLEAVED |
 		SNDRV_PCM_INFO_BLOCK_TRANSFER |
 		SNDRV_PCM_INFO_MMAP |
-		SNDRV_PCM_INFO_MMAP_VALID |
-		SNDRV_PCM_INFO_PAUSE |
-		SNDRV_PCM_INFO_RESUME,
+		SNDRV_PCM_INFO_MMAP_VALID,
 	.buffer_bytes_max = FSL_ASRC_DMABUF_SIZE,
 	.period_bytes_min = 128,
 	.period_bytes_max = 65535, /* Limited by SDMA engine */
@@ -276,6 +274,16 @@  static int fsl_asrc_dma_startup(struct snd_pcm_substream *substream)
 	struct device *dev = component->dev;
 	struct fsl_asrc *asrc_priv = dev_get_drvdata(dev);
 	struct fsl_asrc_pair *pair;
+	bool tx = substream->stream == SNDRV_PCM_STREAM_PLAYBACK;
+	u8 dir = tx ? OUT : IN;
+	struct dma_slave_caps dma_caps;
+	struct dma_chan *tmp_chan;
+	struct snd_dmaengine_dai_dma_data *dma_data;
+	u32 addr_widths = BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
+			  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
+			  BIT(DMA_SLAVE_BUSWIDTH_4_BYTES);
+	int ret;
+	int i;
 
 	pair = kzalloc(sizeof(struct fsl_asrc_pair), GFP_KERNEL);
 	if (!pair)
@@ -285,8 +293,81 @@  static int fsl_asrc_dma_startup(struct snd_pcm_substream *substream)
 
 	runtime->private_data = pair;
 
-	snd_pcm_hw_constraint_integer(substream->runtime,
-				      SNDRV_PCM_HW_PARAM_PERIODS);
+	ret = snd_pcm_hw_constraint_integer(substream->runtime,
+					    SNDRV_PCM_HW_PARAM_PERIODS);
+	if (ret < 0) {
+		dev_err(dev, "failed to set pcm hw params periods\n");
+		return ret;
+	}
+
+	dma_data = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
+
+	/* Request a temp pair, which is release in the end */
+	fsl_asrc_request_pair(1, pair);
+
+	tmp_chan = fsl_asrc_get_dma_channel(pair, dir);
+	if (!tmp_chan) {
+		dev_err(dev, "can't get dma channel\n");
+		return -EINVAL;
+	}
+
+	ret = dma_get_slave_caps(tmp_chan, &dma_caps);
+	if (ret == 0) {
+		if (dma_caps.cmd_pause)
+			snd_imx_hardware.info |= SNDRV_PCM_INFO_PAUSE |
+						 SNDRV_PCM_INFO_RESUME;
+		if (dma_caps.residue_granularity <=
+			DMA_RESIDUE_GRANULARITY_SEGMENT)
+			snd_imx_hardware.info |= SNDRV_PCM_INFO_BATCH;
+
+		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+			addr_widths = dma_caps.dst_addr_widths;
+		else
+			addr_widths = dma_caps.src_addr_widths;
+	}
+
+	/*
+	 * If SND_DMAENGINE_PCM_DAI_FLAG_PACK is set keep
+	 * hw.formats set to 0, meaning no restrictions are in place.
+	 * In this case it's the responsibility of the DAI driver to
+	 * provide the supported format information.
+	 */
+	if (!(dma_data->flags & SND_DMAENGINE_PCM_DAI_FLAG_PACK))
+		/*
+		 * Prepare formats mask for valid/allowed sample types. If the
+		 * dma does not have support for the given physical word size,
+		 * it needs to be masked out so user space can not use the
+		 * format which produces corrupted audio.
+		 * In case the dma driver does not implement the slave_caps the
+		 * default assumption is that it supports 1, 2 and 4 bytes
+		 * widths.
+		 */
+		for (i = 0; i <= SNDRV_PCM_FORMAT_LAST; i++) {
+			int bits = snd_pcm_format_physical_width(i);
+
+			/*
+			 * Enable only samples with DMA supported physical
+			 * widths
+			 */
+			switch (bits) {
+			case 8:
+			case 16:
+			case 24:
+			case 32:
+			case 64:
+				if (addr_widths & (1 << (bits / 8)))
+					snd_imx_hardware.formats |= (1LL << i);
+				break;
+			default:
+				/* Unsupported types */
+				break;
+			}
+		}
+
+	if (tmp_chan)
+		dma_release_channel(tmp_chan);
+	fsl_asrc_release_pair(pair);
+
 	snd_soc_set_runtime_hwparams(substream, &snd_imx_hardware);
 
 	return 0;