Re: [PATCH] ASoC: imx-ssi: Fix mono playback

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Mark Brown]

On Mon, Aug 06, 2012 at 04:36:25PM -0300, Fabio Estevam wrote:
> Currently mono playback is played on a faster rate.
> 
> Adjust channels_min to 2 channels in order to play it back correctly.

This doesn't actually fix mono playback so much as remove support for
it...

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Fabio Estevam]

On Mon, Aug 6, 2012 at 4:54 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Aug 06, 2012 at 04:36:25PM -0300, Fabio Estevam wrote:
>> Currently mono playback is played on a faster rate.
>>
>> Adjust channels_min to 2 channels in order to play it back correctly.
>
> This doesn't actually fix mono playback so much as remove support for
> it...

Ok, understood.

Sascha,

Do you know if mono playback used to work previously in imx-ssi?

Regards,

Fabio Estevam

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Fabio Estevam]

On Mon, Aug 6, 2012 at 4:58 PM, Fabio Estevam <festevam@gmail.com> wrote:

> Ok, understood.
>
> Sascha,
>
> Do you know if mono playback used to work previously in imx-ssi?

Ok, just tested a mono track on a mx6qsabrelite and it played well.

So it seems the issue is mc13783 related then.

Regards,

Fabio Estevam

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Mark Brown]

On Mon, Aug 06, 2012 at 06:49:04PM -0300, Fabio Estevam wrote:

> So it seems the issue is mc13783 related then.

Is the device using I2S?  I2S doesn't really support stero properly...

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Fabio Estevam]

On Mon, Aug 6, 2012 at 7:01 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Aug 06, 2012 at 06:49:04PM -0300, Fabio Estevam wrote:
>
>> So it seems the issue is mc13783 related then.
>
> Is the device using I2S?  I2S doesn't really support stero properly...

The format is:

#define FMT_SSI (SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_NB_NF | \
		SND_SOC_DAIFMT_CBM_CFM)

In wm1133-ev1.c you did:

	/* TODO: The SSI driver should figure this out for us */
	switch (channels) {
	case 2:
		snd_soc_dai_set_tdm_slot(cpu_dai, 0xffffffc, 0xffffffc, 2, 0);
		break;
	case 1:
		snd_soc_dai_set_tdm_slot(cpu_dai, 0xffffffe, 0xffffffe, 1, 0);
		break;
	default:
		return -EINVAL;
	}

Maybe I should do something similar in imx-mc13783?

Regards,

Fabio Estevam

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Mark Brown]

On Mon, Aug 06, 2012 at 07:10:07PM -0300, Fabio Estevam wrote:
> On Mon, Aug 6, 2012 at 7:01 PM, Mark Brown

> > Is the device using I2S?  I2S doesn't really support stero properly...

> The format is:

> #define FMT_SSI (SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_NB_NF | \
> 		SND_SOC_DAIFMT_CBM_CFM)

No, then.

> Maybe I should do something similar in imx-mc13783?

That's cut'n'paste since before my time, and the comment about it being
something the SSI driver ought to do does apply.  But it's worth trying.

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Tabi Timur-B04825]

On Mon, Aug 6, 2012 at 5:01 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
>
> Is the device using I2S?  I2S doesn't really support stero properly...

Every use case of I2S I've seen has been stereo only.  What do you
mean that it doesn't support stereo properly?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Tabi Timur-B04825]

On Mon, Aug 6, 2012 at 2:58 PM, Fabio Estevam <festevam@gmail.com> wrote:

> Do you know if mono playback used to work previously in imx-ssi?

I doubt it.  I don't think the I2S standard allows for mono playback,
so the SSI would need to send the same sample twice, and I don't see
any support for that feature in the RM.

When I wrote fsl_ssi, I couldn't find any way to get mono playback to work.

BTW, I can't find the original patch in my inbox or the mailing list
archives.  When was it sent?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Fabio Estevam]

Hi Timur,

On Mon, Aug 6, 2012 at 8:33 PM, Tabi Timur-B04825 <B04825@freescale.com> wrote:
> On Mon, Aug 6, 2012 at 2:58 PM, Fabio Estevam <festevam@gmail.com> wrote:
>
>> Do you know if mono playback used to work previously in imx-ssi?
>
> I doubt it.  I don't think the I2S standard allows for mono playback,
> so the SSI would need to send the same sample twice, and I don't see
> any support for that feature in the RM.
>
> When I wrote fsl_ssi, I couldn't find any way to get mono playback to work.
>
> BTW, I can't find the original patch in my inbox or the mailing list
> archives.  When was it sent?

I sent the patch from FSL and it was blocked because I am not
subscribed in alsa-devel list from my FSL account.

The original patch I sent was the following and it allowed me to play
a mono track:

From bc46087110bd48a41f1f7dcb826853349662c766 Mon Sep 17 00:00:00 2001
From: Fabio Estevam <fabio.estevam@freescale.com>
Date: Mon, 6 Aug 2012 16:30:51 -0300
Subject: [PATCH] ASoC: imx-ssi: Fix mono playback

Currently mono playback is played on a faster rate.

Adjust channels_min to 2 channels in order to play it back correctly.

Reported-by: Gaëtan Carlier <gcembed@gmail.com>
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 sound/soc/fsl/imx-ssi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/sound/soc/fsl/imx-ssi.c b/sound/soc/fsl/imx-ssi.c
index 28dd76c..c42df31 100644
--- a/sound/soc/fsl/imx-ssi.c
+++ b/sound/soc/fsl/imx-ssi.c
@@ -380,7 +380,7 @@ static int imx_ssi_dai_probe(struct snd_soc_dai *dai)
 static struct snd_soc_dai_driver imx_ssi_dai = {
 	.probe = imx_ssi_dai_probe,
 	.playback = {
-		.channels_min = 1,
+		.channels_min = 2,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_8000_96000,
 		.formats = SNDRV_PCM_FMTBIT_S16_LE,
-- 
1.7.1
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Fabio Estevam]

On Mon, Aug 6, 2012 at 6:49 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Mon, Aug 6, 2012 at 4:58 PM, Fabio Estevam <festevam@gmail.com> wrote:
>
>> Ok, understood.
>>
>> Sascha,
>>
>> Do you know if mono playback used to work previously in imx-ssi?
>
> Ok, just tested a mono track on a mx6qsabrelite and it played well.

Ops, I was making a confusion:

mx6sabrelite runs a dt kernel and uses fsl_ssi.c driver.

mx31pdk runs a non-dt kernel and uses imx-ssi.c driver.

The fsl_ssi.c code has:

	.playback = {
		/* The SSI does not support monaural audio. */
		.channels_min = 2,
		.channels_max = 2,
		.rates = FSLSSI_I2S_RATES,
		.formats = FSLSSI_I2S_FORMATS,
	},

and if I do the same on imx-ssi.c, ie, change 'channels_min' to 2,
then it also plays correctly.

So is my original patch correct? Maybe I should change the wording of
the commit log?

Thanks,

Fabio Estevam

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Trent Piepho]

It depends on the codec connected to the I2S.  We have SSI that works
in mono mode, using a modified driver.  Our codec allows mono playback
by only using the samples on the left channel.

There is the race issue in that playback and record must both match
mono and stereo and ALSA doesn't handle substreams with constraints
that depend on other substreams in a race free manner.

On Mon, Aug 6, 2012 at 7:33 PM, Tabi Timur-B04825 <B04825@freescale.com> wrote:
> On Mon, Aug 6, 2012 at 2:58 PM, Fabio Estevam <festevam@gmail.com> wrote:
>
>> Do you know if mono playback used to work previously in imx-ssi?
>
> I doubt it.  I don't think the I2S standard allows for mono playback,
> so the SSI would need to send the same sample twice, and I don't see
> any support for that feature in the RM.
>
> When I wrote fsl_ssi, I couldn't find any way to get mono playback to work.
>
> BTW, I can't find the original patch in my inbox or the mailing list
> archives.  When was it sent?
>
> --
> Timur Tabi
> Linux kernel developer at Freescale
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Sascha Hauer]

On Mon, Aug 06, 2012 at 04:58:38PM -0300, Fabio Estevam wrote:
> On Mon, Aug 6, 2012 at 4:54 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> > On Mon, Aug 06, 2012 at 04:36:25PM -0300, Fabio Estevam wrote:
> >> Currently mono playback is played on a faster rate.
> >>
> >> Adjust channels_min to 2 channels in order to play it back correctly.
> >
> > This doesn't actually fix mono playback so much as remove support for
> > it...
> 
> Ok, understood.
> 
> Sascha,
> 
> Do you know if mono playback used to work previously in imx-ssi?

AFAIK only stereo works with the mc13783.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Fabio Estevam]

Sascha,

On Tue, Aug 7, 2012 at 3:26 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

> AFAIK only stereo works with the mc13783.

Ok, could you please let me know what do you think about my patch? It
does the same thing as in fsl_ssi.c by setting channels_min to 2 and I
am interested to know if this is an acceptable change.

Thanks,

Fabio Estevam

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Tabi Timur-B04825]

Trent Piepho wrote:
> It depends on the codec connected to the I2S.  We have SSI that works
> in mono mode, using a modified driver.  Our codec allows mono playback
> by only using the samples on the left channel.

I would *love* to see that driver.

> There is the race issue in that playback and record must both match
> mono and stereo and ALSA doesn't handle substreams with constraints
> that depend on other substreams in a race free manner.

Isn't that only a problem if both streams are opened literally 
simultaneously?  I would assume that most apps will open one stream at a time.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Mark Brown]

On Tue, Aug 07, 2012 at 01:39:26AM -0400, Trent Piepho wrote:

Don't top post!

> It depends on the codec connected to the I2S.  We have SSI that works
> in mono mode, using a modified driver.  Our codec allows mono playback
> by only using the samples on the left channel.

This is extremely common.

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Timur Tabi]

Mark Brown wrote:
>> > It depends on the codec connected to the I2S.  We have SSI that works
>> > in mono mode, using a modified driver.  Our codec allows mono playback
>> > by only using the samples on the left channel.
> This is extremely common.

Wouldn't this require the I2S controller to inject dummy data into the
right channel?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Mark Brown]

On Tue, Aug 07, 2012 at 09:26:34AM -0500, Timur Tabi wrote:
> Mark Brown wrote:
> >> > It depends on the codec connected to the I2S.  We have SSI that works
> >> > in mono mode, using a modified driver.  Our codec allows mono playback
> >> > by only using the samples on the left channel.

> > This is extremely common.

> Wouldn't this require the I2S controller to inject dummy data into the
> right channel?

Yes, generally the hardware just won't send anything on any empty bit
clocks (or will ignore data that happens to be sent when it's not
interested on the other end).

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Mark Brown]

On Mon, Aug 06, 2012 at 11:28:37PM +0000, Tabi Timur-B04825 wrote:
> On Mon, Aug 6, 2012 at 5:01 PM, Mark Brown

> > Is the device using I2S?  I2S doesn't really support stero properly...

> Every use case of I2S I've seen has been stereo only.  What do you
> mean that it doesn't support stereo properly?

Sorry, I meant mono.

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Mark Brown]

On Tue, Aug 07, 2012 at 12:22:41AM -0300, Fabio Estevam wrote:

> So is my original patch correct? Maybe I should change the wording of
> the commit log?

Yeah, just say something about removing mono support since it doesn't
work.

Re: [PATCH] ASoC: imx-ssi: Fix mono playback

[Trent Piepho]

On Tue, Aug 7, 2012 at 8:00 AM, Tabi Timur-B04825 <B04825@freescale.com> wrote:
>> There is the race issue in that playback and record must both match
>> mono and stereo and ALSA doesn't handle substreams with constraints
>> that depend on other substreams in a race free manner.
>
> Isn't that only a problem if both streams are opened literally
> simultaneously?  I would assume that most apps will open one stream at a time.

The problem comes up if another stream is opened before the hw_params
are fixed on the other stream.  So if the first stream is configured
past the hw_params level before the next is opened there is no
problem.  But if not there is a race.  There could be two different
independent processes using the hardware which might be running at the
same time.

I remember posting something about this race in more detail.  One can
mostly make it work by dynamically adding constraints in the open()
method based on the configuration of the linked substream and then
checking the hw_params again for correctness in the hw_params()
method.

There end up being two problems.  One is on the userspace side.  An
app could get a set constraints, choose hwparams that are valid
according to those constraints, and then get told hwparams are not
valid anyway.  While an app could handle this, it has never been
documented as something an app should handle in a certain way.  So
apps will most likely not handle it well.

The other problem is that if two substreams call hw_params at the same
time there is a race internal to ALSA.  I don't remember what the
problem is exactly, just that there was one.