| Message ID | 1453484912-24547-6-git-send-email-arnaud.pouliquen@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/22/16 19:48, Arnaud Pouliquen wrote: > Create 'IEC958 Playback Default' controls to support IEC61937 formats. > the use of the alsa control is optional, using 'iec_ctl' flag. > I applied the patches "ALSA: pcm: add IEC958 channel status control helper", "ASoC: core: add code to complete dai init after pcm creation", and "ASoc: hdmi-codec: add IEC control" to my BBB HDMI audio branch. I needed to do some trivial conflict solving, but after that everything compiled fine. However, when I tried to read the iec mixer with: # amixer -c0 cget name='IEC958 Playback Default',device=0 amixer: Cannot find the given element from control hw:0 The same command worked just fine on my intel based laptop: # amixer -c0 cget iface=MIXER,name='IEC958 Playback Default',device=0 numid=31,iface=MIXER,name='IEC958 Playback Default' ; type=IEC958,access=rw------,values=1 : values=[AES0=0x04 AES1=0x00 AES2=0x00 AES3=0x00] How did you test the mixer yourself? Best regards, Jyri > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> > --- > include/sound/hdmi-codec.h | 1 + > sound/soc/codecs/hdmi-codec.c | 59 +++++++++++++++++++++++++++++++++---------- > 2 files changed, 46 insertions(+), 14 deletions(-) > > diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h > index ed780b2..58e8eae 100644 > --- a/include/sound/hdmi-codec.h > +++ b/include/sound/hdmi-codec.h > @@ -96,6 +96,7 @@ struct hdmi_codec_pdata { > const struct hdmi_codec_ops *ops; > uint i2s:1; > uint spdif:1; > + uint iec_ctl:1; > int max_i2s_channels; > }; > > diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c > index 94349b3..1b6cb5a 100644 > --- a/sound/soc/codecs/hdmi-codec.c > +++ b/sound/soc/codecs/hdmi-codec.c > @@ -32,6 +32,7 @@ struct hdmi_codec_priv { > struct snd_pcm_substream *current_stream; > struct snd_pcm_hw_constraint_list ratec; > uint8_t eld[MAX_ELD_BYTES]; > + struct snd_aes_iec958 iec; > }; > > static const struct snd_soc_dapm_widget hdmi_widgets[] = { > @@ -140,27 +141,30 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, > struct snd_soc_dai *dai) > { > struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); > - struct hdmi_codec_params hp = { > - .iec = { > - .status = { 0 }, > - .subcode = { 0 }, > - .pad = 0, > - .dig_subframe = { 0 }, > - } > - }; > + struct hdmi_codec_params hp; > int ret; > > dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__, > params_width(params), params_rate(params), > params_channels(params)); > > - ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status, > - sizeof(hp.iec.status)); > - if (ret < 0) { > - dev_err(dai->dev, "Creating IEC958 channel status failed %d\n", > - ret); > - return ret; > + mutex_lock(&hcp->current_stream_lock); > + hp.iec = hcp->iec; > + > + if (!hcp->hcd.iec_ctl) { > + /* > + * only PCM format supported > + *channel status set according to runtime parameters > + */ > + ret = snd_pcm_create_iec958_consumer_hw_params(params, > + hp.iec.status, sizeof(hp.iec.status)); > + if (ret < 0) { > + dev_err(dai->dev, "Creating IEC958 status failed %d\n", > + ret); > + return ret; > + } > } > + mutex_unlock(&hcp->current_stream_lock); > > ret = hdmi_codec_new_stream(substream, dai); > if (ret) > @@ -266,12 +270,37 @@ static int hdmi_codec_digital_mute(struct snd_soc_dai *dai, int mute) > return 0; > } > > +static int hdmi_codec_pcm_new(struct snd_soc_pcm_runtime *rtd, > + struct snd_soc_dai *dai) > +{ > + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); > + struct snd_pcm_iec958_params *iec958_params; > + > + dev_dbg(dai->dev, "%s()\n", __func__); > + > + if (!hcp->hcd.iec_ctl) > + return 0; > + > + iec958_params = devm_kzalloc(dai->dev, sizeof(*iec958_params), > + GFP_KERNEL); > + if (!iec958_params) > + return -ENOMEM; > + > + iec958_params->iec = &hcp->iec; > + iec958_params->pdata = hcp; > + iec958_params->mutex = &hcp->current_stream_lock; > + > + return snd_pcm_create_iec958_ctl(rtd->pcm, iec958_params, > + SNDRV_PCM_STREAM_PLAYBACK); > +} > + > static const struct snd_soc_dai_ops hdmi_dai_ops = { > .startup = hdmi_codec_startup, > .shutdown = hdmi_codec_shutdown, > .hw_params = hdmi_codec_hw_params, > .set_fmt = hdmi_codec_set_fmt, > .digital_mute = hdmi_codec_digital_mute, > + .pcm_new = hdmi_codec_pcm_new, > }; > > > @@ -352,6 +381,8 @@ static int hdmi_codec_probe(struct platform_device *pdev) > if (!hcp) > return -ENOMEM; > > + memset(&hcp->iec, 0, sizeof(hcp->iec)); > + > hcp->hcd = *hcd; > mutex_init(&hcp->current_stream_lock); > >
On 02/06/2016 08:29 PM, Jyri Sarha wrote: > On 01/22/16 19:48, Arnaud Pouliquen wrote: >> Create 'IEC958 Playback Default' controls to support IEC61937 formats. >> the use of the alsa control is optional, using 'iec_ctl' flag. >> > > I applied the patches "ALSA: pcm: add IEC958 channel status control > helper", "ASoC: core: add code to complete dai init after pcm creation", > and "ASoc: hdmi-codec: add IEC control" to my BBB HDMI audio branch. I > needed to do some trivial conflict solving, but after that everything > compiled fine. However, when I tried to read the iec mixer with: > > # amixer -c0 cget name='IEC958 Playback Default',device=0 > amixer: Cannot find the given element from control hw:0 > > The same command worked just fine on my intel based laptop: > # amixer -c0 cget iface=MIXER,name='IEC958 Playback Default',device=0 > numid=31,iface=MIXER,name='IEC958 Playback Default' > ; type=IEC958,access=rw------,values=1 > : values=[AES0=0x04 AES1=0x00 AES2=0x00 AES3=0x00] Do you enable "iec_ctl" field in hdmi_codec_pdata structure? i add this field because control can be declared and used by CPU DAI or codec, depending on hardware. > > How did you test the mixer yourself? To test on my platform i hacked my code because control is handled by CPU_DAI... But just by disabling control creation in CPU DAI for HDMI and set "iec_ctl" filed to 1, i can see and use the control. **** List of PLAYBACK Hardware Devices **** card 0: Default [sti audio card], device 0: Uni Player #0 (HDMI)-i2s-hifi i2s-hifi-0 [] card 0: Default [sti audio card], device 1: Uni Player #1 (DAC)-sas-dai-dac sas-dai-dac-1 [] card 0: Default [sti audio card], device 2: Uni Player #1 (PIO)-sas-dai-spdif-out sas-dai-spdif-out-2 [] # amixer -c0 controls | grep IEC958 numid=5,iface=PCM,name='IEC958 Playback Default' numid=3,iface=PCM,name='IEC958 Playback Default',device=3 BR, Arnaud > > Best regards, > Jyri > >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> >> --- >> include/sound/hdmi-codec.h | 1 + >> sound/soc/codecs/hdmi-codec.c | 59 +++++++++++++++++++++++++++++++++---------- >> 2 files changed, 46 insertions(+), 14 deletions(-) >> >> diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h >> index ed780b2..58e8eae 100644 >> --- a/include/sound/hdmi-codec.h >> +++ b/include/sound/hdmi-codec.h >> @@ -96,6 +96,7 @@ struct hdmi_codec_pdata { >> const struct hdmi_codec_ops *ops; >> uint i2s:1; >> uint spdif:1; >> + uint iec_ctl:1; >> int max_i2s_channels; >> }; >> >> diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c >> index 94349b3..1b6cb5a 100644 >> --- a/sound/soc/codecs/hdmi-codec.c >> +++ b/sound/soc/codecs/hdmi-codec.c >> @@ -32,6 +32,7 @@ struct hdmi_codec_priv { >> struct snd_pcm_substream *current_stream; >> struct snd_pcm_hw_constraint_list ratec; >> uint8_t eld[MAX_ELD_BYTES]; >> + struct snd_aes_iec958 iec; >> }; >> >> static const struct snd_soc_dapm_widget hdmi_widgets[] = { >> @@ -140,27 +141,30 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, >> struct snd_soc_dai *dai) >> { >> struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); >> - struct hdmi_codec_params hp = { >> - .iec = { >> - .status = { 0 }, >> - .subcode = { 0 }, >> - .pad = 0, >> - .dig_subframe = { 0 }, >> - } >> - }; >> + struct hdmi_codec_params hp; >> int ret; >> >> dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__, >> params_width(params), params_rate(params), >> params_channels(params)); >> >> - ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status, >> - sizeof(hp.iec.status)); >> - if (ret < 0) { >> - dev_err(dai->dev, "Creating IEC958 channel status failed %d\n", >> - ret); >> - return ret; >> + mutex_lock(&hcp->current_stream_lock); >> + hp.iec = hcp->iec; >> + >> + if (!hcp->hcd.iec_ctl) { >> + /* >> + * only PCM format supported >> + *channel status set according to runtime parameters >> + */ >> + ret = snd_pcm_create_iec958_consumer_hw_params(params, >> + hp.iec.status, sizeof(hp.iec.status)); >> + if (ret < 0) { >> + dev_err(dai->dev, "Creating IEC958 status failed %d\n", >> + ret); >> + return ret; >> + } >> } >> + mutex_unlock(&hcp->current_stream_lock); >> >> ret = hdmi_codec_new_stream(substream, dai); >> if (ret) >> @@ -266,12 +270,37 @@ static int hdmi_codec_digital_mute(struct snd_soc_dai *dai, int mute) >> return 0; >> } >> >> +static int hdmi_codec_pcm_new(struct snd_soc_pcm_runtime *rtd, >> + struct snd_soc_dai *dai) >> +{ >> + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); >> + struct snd_pcm_iec958_params *iec958_params; >> + >> + dev_dbg(dai->dev, "%s()\n", __func__); >> + >> + if (!hcp->hcd.iec_ctl) >> + return 0; >> + >> + iec958_params = devm_kzalloc(dai->dev, sizeof(*iec958_params), >> + GFP_KERNEL); >> + if (!iec958_params) >> + return -ENOMEM; >> + >> + iec958_params->iec = &hcp->iec; >> + iec958_params->pdata = hcp; >> + iec958_params->mutex = &hcp->current_stream_lock; >> + >> + return snd_pcm_create_iec958_ctl(rtd->pcm, iec958_params, >> + SNDRV_PCM_STREAM_PLAYBACK); >> +} >> + >> static const struct snd_soc_dai_ops hdmi_dai_ops = { >> .startup = hdmi_codec_startup, >> .shutdown = hdmi_codec_shutdown, >> .hw_params = hdmi_codec_hw_params, >> .set_fmt = hdmi_codec_set_fmt, >> .digital_mute = hdmi_codec_digital_mute, >> + .pcm_new = hdmi_codec_pcm_new, >> }; >> >> >> @@ -352,6 +381,8 @@ static int hdmi_codec_probe(struct platform_device *pdev) >> if (!hcp) >> return -ENOMEM; >> >> + memset(&hcp->iec, 0, sizeof(hcp->iec)); >> + >> hcp->hcd = *hcd; >> mutex_init(&hcp->current_stream_lock); >> >> >
On 02/15/16 12:51, Arnaud Pouliquen wrote: > > > On 02/06/2016 08:29 PM, Jyri Sarha wrote: >> On 01/22/16 19:48, Arnaud Pouliquen wrote: >>> Create 'IEC958 Playback Default' controls to support IEC61937 formats. >>> the use of the alsa control is optional, using 'iec_ctl' flag. >>> >> >> I applied the patches "ALSA: pcm: add IEC958 channel status control >> helper", "ASoC: core: add code to complete dai init after pcm creation", >> and "ASoc: hdmi-codec: add IEC control" to my BBB HDMI audio branch. I >> needed to do some trivial conflict solving, but after that everything >> compiled fine. However, when I tried to read the iec mixer with: >> >> # amixer -c0 cget name='IEC958 Playback Default',device=0 >> amixer: Cannot find the given element from control hw:0 >> >> The same command worked just fine on my intel based laptop: >> # amixer -c0 cget iface=MIXER,name='IEC958 Playback Default',device=0 >> numid=31,iface=MIXER,name='IEC958 Playback Default' >> ; type=IEC958,access=rw------,values=1 >> : values=[AES0=0x04 AES1=0x00 AES2=0x00 AES3=0x00] > Do you enable "iec_ctl" field in hdmi_codec_pdata structure? > i add this field because control can be declared and used by CPU DAI or > codec, depending on hardware. > >> >> How did you test the mixer yourself? > To test on my platform i hacked my code because control is handled by > CPU_DAI... > But just by disabling control creation in CPU DAI for HDMI and set > "iec_ctl" filed to 1, i can see and use the control. > Oh, my mistake. Simply overlooked the iec_ctl. However, when testing the mixer element again, I found another problem. When getting the IEC958 value, I got following dump: [ 690.127298] [ 690.128957] ===================================== [ 690.133956] [ BUG: bad unlock balance detected! ] [ 690.138965] 4.4.0-rc6-01061-g2b96fb3-dirty #12 Not tainted [ 690.144785] ------------------------------------- [ 690.149785] amixer/1409 is trying to release lock (&hcp->current_stream_lock) at: [ 690.157992] [<bf08a09c>] snd_pcm_iec958_get+0x1c/0x70 [snd_pcm] [ 690.164277] but there are no more locks to release! [ 690.169454] [ 690.169454] other info that might help us debug this: [ 690.176388] 2 locks held by amixer/1409: [ 690.180554] #0: (&card->power_lock){+.+...}, at: [<bf050f08>] snd_ctl_ioctl+0x514/0xcfc [snd] [ 690.190010] #1: (&card->controls_rwsem){++++..}, at: [<bf050f28>] snd_ctl_ioctl+0x534/0xcfc [snd] [ 690.199760] [ 690.199760] stack backtrace: [ 690.204409] CPU: 0 PID: 1409 Comm: amixer Not tainted 4.4.0-rc6-01061-g2b96fb3-dirty #12 [ 690.212990] Hardware name: Generic AM33XX (Flattened Device Tree) [ 690.219516] [<c0017b84>] (unwind_backtrace) from [<c0013ee8>] (show_stack+0x10/0x14) [ 690.227754] [<c0013ee8>] (show_stack) from [<c03480a0>] (dump_stack+0x84/0x9c) [ 690.235443] [<c03480a0>] (dump_stack) from [<c008ce8c>] (print_unlock_imbalance_bug+0xac/0xdc) [ 690.244598] [<c008ce8c>] (print_unlock_imbalance_bug) from [<c0091aa8>] (lock_release+0x268/0x3c0) [ 690.254125] [<c0091aa8>] (lock_release) from [<c0650c7c>] (__mutex_unlock_slowpath+0xb4/0x1a4) [ 690.263401] [<c0650c7c>] (__mutex_unlock_slowpath) from [<bf08a09c>] (snd_pcm_iec958_get+0x1c/0x70 [snd_pcm]) [ 690.274104] [<bf08a09c>] (snd_pcm_iec958_get [snd_pcm]) from [<bf050fc4>] (snd_ctl_ioctl+0x5d0/0xcfc [snd]) [ 690.284521] [<bf050fc4>] (snd_ctl_ioctl [snd]) from [<c01840bc>] (do_vfs_ioctl+0x4c0/0x7e4) [ 690.293405] [<c01840bc>] (do_vfs_ioctl) from [<c018444c>] (SyS_ioctl+0x6c/0x7c) [ 690.301191] [<c018444c>] (SyS_ioctl) from [<c000f6e0>] (ret_fast_syscall+0x0/0x1c) It seems you have a bug at sound/core/pcm_iec958.c:49 (I'll comment that separately). BR, Jyri
diff --git a/include/sound/hdmi-codec.h b/include/sound/hdmi-codec.h index ed780b2..58e8eae 100644 --- a/include/sound/hdmi-codec.h +++ b/include/sound/hdmi-codec.h @@ -96,6 +96,7 @@ struct hdmi_codec_pdata { const struct hdmi_codec_ops *ops; uint i2s:1; uint spdif:1; + uint iec_ctl:1; int max_i2s_channels; }; diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c index 94349b3..1b6cb5a 100644 --- a/sound/soc/codecs/hdmi-codec.c +++ b/sound/soc/codecs/hdmi-codec.c @@ -32,6 +32,7 @@ struct hdmi_codec_priv { struct snd_pcm_substream *current_stream; struct snd_pcm_hw_constraint_list ratec; uint8_t eld[MAX_ELD_BYTES]; + struct snd_aes_iec958 iec; }; static const struct snd_soc_dapm_widget hdmi_widgets[] = { @@ -140,27 +141,30 @@ static int hdmi_codec_hw_params(struct snd_pcm_substream *substream, struct snd_soc_dai *dai) { struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); - struct hdmi_codec_params hp = { - .iec = { - .status = { 0 }, - .subcode = { 0 }, - .pad = 0, - .dig_subframe = { 0 }, - } - }; + struct hdmi_codec_params hp; int ret; dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__, params_width(params), params_rate(params), params_channels(params)); - ret = snd_pcm_create_iec958_consumer_hw_params(params, hp.iec.status, - sizeof(hp.iec.status)); - if (ret < 0) { - dev_err(dai->dev, "Creating IEC958 channel status failed %d\n", - ret); - return ret; + mutex_lock(&hcp->current_stream_lock); + hp.iec = hcp->iec; + + if (!hcp->hcd.iec_ctl) { + /* + * only PCM format supported + *channel status set according to runtime parameters + */ + ret = snd_pcm_create_iec958_consumer_hw_params(params, + hp.iec.status, sizeof(hp.iec.status)); + if (ret < 0) { + dev_err(dai->dev, "Creating IEC958 status failed %d\n", + ret); + return ret; + } } + mutex_unlock(&hcp->current_stream_lock); ret = hdmi_codec_new_stream(substream, dai); if (ret) @@ -266,12 +270,37 @@ static int hdmi_codec_digital_mute(struct snd_soc_dai *dai, int mute) return 0; } +static int hdmi_codec_pcm_new(struct snd_soc_pcm_runtime *rtd, + struct snd_soc_dai *dai) +{ + struct hdmi_codec_priv *hcp = snd_soc_dai_get_drvdata(dai); + struct snd_pcm_iec958_params *iec958_params; + + dev_dbg(dai->dev, "%s()\n", __func__); + + if (!hcp->hcd.iec_ctl) + return 0; + + iec958_params = devm_kzalloc(dai->dev, sizeof(*iec958_params), + GFP_KERNEL); + if (!iec958_params) + return -ENOMEM; + + iec958_params->iec = &hcp->iec; + iec958_params->pdata = hcp; + iec958_params->mutex = &hcp->current_stream_lock; + + return snd_pcm_create_iec958_ctl(rtd->pcm, iec958_params, + SNDRV_PCM_STREAM_PLAYBACK); +} + static const struct snd_soc_dai_ops hdmi_dai_ops = { .startup = hdmi_codec_startup, .shutdown = hdmi_codec_shutdown, .hw_params = hdmi_codec_hw_params, .set_fmt = hdmi_codec_set_fmt, .digital_mute = hdmi_codec_digital_mute, + .pcm_new = hdmi_codec_pcm_new, }; @@ -352,6 +381,8 @@ static int hdmi_codec_probe(struct platform_device *pdev) if (!hcp) return -ENOMEM; + memset(&hcp->iec, 0, sizeof(hcp->iec)); + hcp->hcd = *hcd; mutex_init(&hcp->current_stream_lock);
Create 'IEC958 Playback Default' controls to support IEC61937 formats. the use of the alsa control is optional, using 'iec_ctl' flag. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> --- include/sound/hdmi-codec.h | 1 + sound/soc/codecs/hdmi-codec.c | 59 +++++++++++++++++++++++++++++++++---------- 2 files changed, 46 insertions(+), 14 deletions(-)