| Message ID | 1480325634-23320-2-git-send-email-arnaud.pouliquen@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Nov 28 2016 18:33, Arnaud Pouliquen wrote: > In case of several instances of the same PCM control (e.g IEC controls). > Application should be able to address the control using the > device field number, according to the PCM character device. > This patch allows to link DAI PCM controls to the PCM device. > During DAI_link probe, PCM controls are added after device field is forced > to the PCM device number. > > Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> > --- > include/sound/soc-dai.h | 4 ++++ > sound/soc/soc-core.c | 28 ++++++++++++++++++++++++++++ > 2 files changed, 32 insertions(+) > > diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h > index 964b7de..93624c9 100644 > --- a/include/sound/soc-dai.h > +++ b/include/sound/soc-dai.h > @@ -247,6 +247,10 @@ struct snd_soc_dai_driver { > /* probe ordering - for components with runtime dependencies */ > int probe_order; > int remove_order; > + > + /* Optional PCM controls to bind to PCM device on DAIs link*/ > + const struct snd_kcontrol_new *pcm_controls; > + int num_pcm_controls; > }; > > /* > diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c > index 4afa8db..ace83c9 100644 > --- a/sound/soc/soc-core.c > +++ b/sound/soc/soc-core.c > @@ -1552,6 +1552,25 @@ static int soc_probe_dai(struct snd_soc_dai *dai, int order) > return 0; > } > > +static int soc_link_dai_pcm_controls(struct snd_soc_dai **dais, int num_dais, > + struct snd_soc_pcm_runtime *rtd) > +{ > + struct snd_kcontrol_new kctl; > + int i, j, err; > + > + for (i = 0; i < num_dais; ++i) { > + for (j = 0; j < dais[i]->driver->num_pcm_controls; j++) { > + kctl = dais[i]->driver->pcm_controls[j]; > + if (!rtd->dai_link->no_pcm) > + kctl.device = rtd->pcm->device; For the above codes, please request some comment to the other developers working for ALSA SoC part. I'm not so experienced developer for this part, sorry. > + if (snd_soc_add_dai_controls(dais[i], &kctl, 1)) > + return err; Return value from snd_soc_add_dai_controls() should be assigned to the 'err' variable, I think. > + } > + } > + > + return 0; > +} > + > static int soc_link_dai_widgets(struct snd_soc_card *card, > struct snd_soc_dai_link *dai_link, > struct snd_soc_pcm_runtime *rtd) > @@ -1663,6 +1682,15 @@ static int soc_probe_link_dais(struct snd_soc_card *card, > dai_link->stream_name, ret); > return ret; > } > + > + /* Bind DAIs pcm controls to the PCM device */ > + ret = soc_link_dai_pcm_controls(&cpu_dai, 1, rtd); > + if (ret < 0) > + return ret; > + ret = soc_link_dai_pcm_controls(rtd->codec_dais, > + rtd->num_codecs, rtd); > + if (ret < 0) > + return ret; > } else { > INIT_DELAYED_WORK(&rtd->delayed_work, > codec2codec_close_delayed_work); In the other part of this subsystem, the first parameter of helper functions typically represents a 'subject'. In this context, the subject is PCM runtime specialized for ALSA SoC part, which get some new control element sets for the PCM runtime. Therefore, it's better to move the 'rtd' parameter to the first argument. (This is not so strong demand, and somewhat depends on developers' taste. Furthermore, I have no self-confidence to tell my intension to you correctly in English...) Regards Takashi Sakamoto
On 11/28/2016 02:18 PM, Takashi Sakamoto wrote: > On Nov 28 2016 18:33, Arnaud Pouliquen wrote: >> In case of several instances of the same PCM control (e.g IEC controls). >> Application should be able to address the control using the >> device field number, according to the PCM character device. >> This patch allows to link DAI PCM controls to the PCM device. >> During DAI_link probe, PCM controls are added after device field is forced >> to the PCM device number. >> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> >> --- >> include/sound/soc-dai.h | 4 ++++ >> sound/soc/soc-core.c | 28 ++++++++++++++++++++++++++++ >> 2 files changed, 32 insertions(+) >> >> diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h >> index 964b7de..93624c9 100644 >> --- a/include/sound/soc-dai.h >> +++ b/include/sound/soc-dai.h >> @@ -247,6 +247,10 @@ struct snd_soc_dai_driver { >> /* probe ordering - for components with runtime dependencies */ >> int probe_order; >> int remove_order; >> + >> + /* Optional PCM controls to bind to PCM device on DAIs link*/ >> + const struct snd_kcontrol_new *pcm_controls; >> + int num_pcm_controls; >> }; >> >> /* >> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c >> index 4afa8db..ace83c9 100644 >> --- a/sound/soc/soc-core.c >> +++ b/sound/soc/soc-core.c >> @@ -1552,6 +1552,25 @@ static int soc_probe_dai(struct snd_soc_dai *dai, int order) >> return 0; >> } >> >> +static int soc_link_dai_pcm_controls(struct snd_soc_dai **dais, int num_dais, >> + struct snd_soc_pcm_runtime *rtd) >> +{ >> + struct snd_kcontrol_new kctl; >> + int i, j, err; >> + >> + for (i = 0; i < num_dais; ++i) { >> + for (j = 0; j < dais[i]->driver->num_pcm_controls; j++) { >> + kctl = dais[i]->driver->pcm_controls[j]; >> + if (!rtd->dai_link->no_pcm) >> + kctl.device = rtd->pcm->device; > > For the above codes, please request some comment to the other developers > working for ALSA SoC part. I'm not so experienced developer for this > part, sorry. I think something is missing here, to return an error if following condition is not respected: iface == SNDRV_CTL_ELEM_IFACE_PCM I'm going to add a test. > >> + if (snd_soc_add_dai_controls(dais[i], &kctl, 1)) >> + return err; > > Return value from snd_soc_add_dai_controls() should be assigned to the > 'err' variable, I think. Oops, stupid mistake... thanks! i will correct it in my next version > >> + } >> + } >> + >> + return 0; >> +} >> + >> static int soc_link_dai_widgets(struct snd_soc_card *card, >> struct snd_soc_dai_link *dai_link, >> struct snd_soc_pcm_runtime *rtd) >> @@ -1663,6 +1682,15 @@ static int soc_probe_link_dais(struct snd_soc_card *card, >> dai_link->stream_name, ret); >> return ret; >> } >> + >> + /* Bind DAIs pcm controls to the PCM device */ >> + ret = soc_link_dai_pcm_controls(&cpu_dai, 1, rtd); >> + if (ret < 0) >> + return ret; >> + ret = soc_link_dai_pcm_controls(rtd->codec_dais, >> + rtd->num_codecs, rtd); >> + if (ret < 0) >> + return ret; >> } else { >> INIT_DELAYED_WORK(&rtd->delayed_work, >> codec2codec_close_delayed_work); > > In the other part of this subsystem, the first parameter of helper > functions typically represents a 'subject'. In this context, the subject > is PCM runtime specialized for ALSA SoC part, which get some new control > element sets for the PCM runtime. Therefore, it's better to move the > 'rtd' parameter to the first argument. (This is not so strong demand, > and somewhat depends on developers' taste. Furthermore, I have no > self-confidence to tell my intension to you correctly in English...) I understand your point. Nevertheless, i would not see the PCM runtime as subject, but the DAI. In this way, I was inspired by the soc_link_dai_widgets helper that is also in the subsytem... if rtd is the subject, i think soc_link_dai_pcm_controls should also be renamed snd_soc_runtime_link_dai_ctls (or something like that) But i'm not expert in ASoC semantic, so based on my answer, please confirm me you point of view, i will integrate it in my V4 with other fixes. Regards Arnaud
Hi, On Nov 29 2016 00:18, Arnaud Pouliquen wrote: >>> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c >>> index 4afa8db..ace83c9 100644 >>> --- a/sound/soc/soc-core.c >>> +++ b/sound/soc/soc-core.c >>> @@ -1552,6 +1552,25 @@ static int soc_probe_dai(struct snd_soc_dai *dai, int order) >>> return 0; >>> } >>> >>> +static int soc_link_dai_pcm_controls(struct snd_soc_dai **dais, int num_dais, >>> + struct snd_soc_pcm_runtime *rtd) >>> +{ >>> + struct snd_kcontrol_new kctl; >>> + int i, j, err; >>> + >>> + for (i = 0; i < num_dais; ++i) { >>> + for (j = 0; j < dais[i]->driver->num_pcm_controls; j++) { >>> + kctl = dais[i]->driver->pcm_controls[j]; >>> + if (!rtd->dai_link->no_pcm) >>> + kctl.device = rtd->pcm->device; >> >> For the above codes, please request some comment to the other developers >> working for ALSA SoC part. I'm not so experienced developer for this >> part, sorry. > I think something is missing here, to return an error if following > condition is not respected: iface == SNDRV_CTL_ELEM_IFACE_PCM > I'm going to add a test. When you have unclear points, it's better to ask them to the person who knows it. Furthermore, you attempt to change core codes of ALSA SoC part. Enough deliberation is preferable. Liam Girdwood committed to the 'no_pcm' feature in below patch, and he is still active in this subsystem (I met him this month). You could request him to review. https://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=01d7584cd2e5a93a2b959c9dddaa0d93ec205404 >>> + if (snd_soc_add_dai_controls(dais[i], &kctl, 1)) >>> + return err; >> >> Return value from snd_soc_add_dai_controls() should be assigned to the >> 'err' variable, I think. > > Oops, stupid mistake... thanks! i will correct it in my next version OK. >> In the other part of this subsystem, the first parameter of helper >> functions typically represents a 'subject'. In this context, the subject >> is PCM runtime specialized for ALSA SoC part, which get some new control >> element sets for the PCM runtime. Therefore, it's better to move the >> 'rtd' parameter to the first argument. (This is not so strong demand, >> and somewhat depends on developers' taste. Furthermore, I have no >> self-confidence to tell my intension to you correctly in English...) > > I understand your point. Nevertheless, i would not see the PCM runtime > as subject, but the DAI. In this way, I was inspired by the > soc_link_dai_widgets helper that is also in the subsytem... > if rtd is the subject, i think soc_link_dai_pcm_controls should > also be renamed snd_soc_runtime_link_dai_ctls (or something like that) Fair enough. I might had a confusion due to complexity of ALSA SoC part against ALSA control core, sorry. > But i'm not expert in ASoC semantic, so based on my answer, please > confirm me you point of view, i will integrate it in my V4 with other fixes. Go a head. Regards Takashi Sakamoto
Hello, I'm trying to understand how to implement the speaker allocation for HDMI output. input: - HDMI sink provides the speaker presence in ELD structure - audio source can also contain channels position. output: Audio info frame (IF) should contain audio channel allocation ( hdmi_audio_infoframe structure). So if i well understand the concept, application should be in charge of aligning channels with HDMI sink speakers... Regarding HDA driver, audio IF channel allocation is computing based on "Playback Channel Map"control . Application set the speaker information of the audio IF through this control. On ASOC side, in hdmi-codec, -'ELD" control exists to export speaker information to application - info frame channel allocation is hardware coded in hdmi_codec_hw_params. The configuration is set by default depending on the number of channels. Drawback is that application is not aware of the configuration selected by the driver. So it seems that something is missing to do the link between EDID information and audio source channel mapping on one side and audio IF configuration on other side. What should be the best strategy to support feature in ASoC? - No update in hdmi-codec, consider that application should know the configuration available for 2,4,6 and 8 channels => seems not very flexible... - Update hdmi-codec to implement "Playback Channel Map" in read-only, to provide channel mapping information to application. => impact is light, but application can get the correct mapping only after the update of the number of channels (so after hw_params ops call) - Update hdmi-codec to implement "Playback Channel Map" in read and write access. => HDA implementation. Seems the more flexible solution from my windows... - Implement control in drm drivers => seems not reasonable. - Other? Thanks in advance for your answers. Regards Arnaud
On 11/30/16 11:54 AM, Arnaud Pouliquen wrote: > Hello, > > I'm trying to understand how to implement the speaker allocation for > HDMI output. > > input: > - HDMI sink provides the speaker presence in ELD structure > - audio source can also contain channels position. > > output: > Audio info frame (IF) should contain audio channel allocation ( > hdmi_audio_infoframe structure). > > So if i well understand the concept, application should be in charge of > aligning channels with HDMI sink speakers... Yes. However some HDMI IPs provide hardware means to swap the channels - mainly because the order of channels varies between OSes and encoding schemes. The notion of 'aligning' might be limited in those cases to setting the relevant ALSA controls, not necessarily an active software channel swap and change of the channel map handled by the application. > > Regarding HDA driver, audio IF channel allocation is computing based on > "Playback Channel Map"control . Application set the speaker information > of the audio IF through this control. > > On ASOC side, in hdmi-codec, > -'ELD" control exists to export speaker information to application > - info frame channel allocation is hardware coded in > hdmi_codec_hw_params. The configuration is set by default depending on > the number of channels. Drawback is that application is not aware of the > configuration selected by the driver. > > So it seems that something is missing to do the link between EDID > information and audio source channel mapping on one side and audio IF > configuration on other side. > > What should be the best strategy to support feature in ASoC? > - No update in hdmi-codec, consider that application should know > the configuration available for 2,4,6 and 8 channels > => seems not very flexible... > > - Update hdmi-codec to implement "Playback Channel Map" in read-only, to > provide channel mapping information to application. > => impact is light, but application can get the correct mapping only > after the update of the number of channels (so after hw_params ops call) > > - Update hdmi-codec to implement "Playback Channel Map" in read and > write access. > => HDA implementation. Seems the more flexible solution from my windows... > > - Implement control in drm drivers > => seems not reasonable. > > - Other? > > Thanks in advance for your answers. > > Regards > Arnaud > > > > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
Hi,
This is a nitpicking.
>
In-Reply-To: <b6cf1216-7408-584f-6966-2d2a7cb20df3@sakamocchi.jp>
It's preferable to start one mail thread for one patchset and one issue.
It's better to add URL references into your message instead of adding
'in-reply-to' header, when you post new patchset.
Regards
Takashi Sakamoto
On Wed, 30 Nov 2016 23:27:22 +0100, Pierre-Louis Bossart wrote: > > On 11/30/16 11:54 AM, Arnaud Pouliquen wrote: > > Hello, > > > > I'm trying to understand how to implement the speaker allocation for > > HDMI output. > > > > input: > > - HDMI sink provides the speaker presence in ELD structure > > - audio source can also contain channels position. > > > > output: > > Audio info frame (IF) should contain audio channel allocation ( > > hdmi_audio_infoframe structure). > > > > So if i well understand the concept, application should be in charge of > > aligning channels with HDMI sink speakers... > > Yes. However some HDMI IPs provide hardware means to swap the channels > - > mainly because the order of channels varies between OSes and encoding > schemes. The notion of 'aligning' might be limited in those cases to > setting the relevant ALSA controls, not necessarily an active software > channel swap and change of the channel map handled by the application. The application may just leave the channel mapping as the driver default, too. It's a freedom for user-space whether to accept the default channel mapping as is, or choose the own preferred one. For some IPs that can't handle the channel maps fully, the limitation can be implemented in the driver side, I suppose. It just needs to pass the limited set of channel-map array the hardware may accept, instead of the arrays that are created by parsing the ELD. > > Regarding HDA driver, audio IF channel allocation is computing based on > > "Playback Channel Map"control . Application set the speaker information > > of the audio IF through this control. > > > > On ASOC side, in hdmi-codec, > > -'ELD" control exists to export speaker information to application > > - info frame channel allocation is hardware coded in > > hdmi_codec_hw_params. The configuration is set by default depending on > > the number of channels. Drawback is that application is not aware of the > > configuration selected by the driver. > > > > So it seems that something is missing to do the link between EDID > > information and audio source channel mapping on one side and audio IF > > configuration on other side. > > > > What should be the best strategy to support feature in ASoC? > > - No update in hdmi-codec, consider that application should know > > the configuration available for 2,4,6 and 8 channels > > => seems not very flexible... > > > > - Update hdmi-codec to implement "Playback Channel Map" in read-only, to > > provide channel mapping information to application. > > => impact is light, but application can get the correct mapping only > > after the update of the number of channels (so after hw_params ops call) > > > > - Update hdmi-codec to implement "Playback Channel Map" in read and > > write access. > > => HDA implementation. Seems the more flexible solution from my windows... > > > > - Implement control in drm drivers > > => seems not reasonable. > > > > - Other? We can start from the read-only chmap ctls, with a plan to enhance to the read/write later on. I guess only few applications require the channel map change, and the hardest part is the proper integration in ASoC, not the write support itself. thanks, Takashi
On 12/01/2016 11:07 AM, Takashi Iwai wrote: > On Wed, 30 Nov 2016 23:27:22 +0100, > Pierre-Louis Bossart wrote: >> >> On 11/30/16 11:54 AM, Arnaud Pouliquen wrote: >>> Hello, >>> >>> I'm trying to understand how to implement the speaker allocation for >>> HDMI output. >>> >>> input: >>> - HDMI sink provides the speaker presence in ELD structure >>> - audio source can also contain channels position. >>> >>> output: >>> Audio info frame (IF) should contain audio channel allocation ( >>> hdmi_audio_infoframe structure). >>> >>> So if i well understand the concept, application should be in charge of >>> aligning channels with HDMI sink speakers... >> >> Yes. However some HDMI IPs provide hardware means to swap the channels >> - >> mainly because the order of channels varies between OSes and encoding >> schemes. The notion of 'aligning' might be limited in those cases to >> setting the relevant ALSA controls, not necessarily an active software >> channel swap and change of the channel map handled by the application. > > The application may just leave the channel mapping as the driver > default, too. It's a freedom for user-space whether to accept the > default channel mapping as is, or choose the own preferred one. > > For some IPs that can't handle the channel maps fully, the limitation > can be implemented in the driver side, I suppose. It just needs to > pass the limited set of channel-map array the hardware may accept, > instead of the arrays that are created by parsing the ELD. > >>> Regarding HDA driver, audio IF channel allocation is computing based on >>> "Playback Channel Map"control . Application set the speaker information >>> of the audio IF through this control. >>> >>> On ASOC side, in hdmi-codec, >>> -'ELD" control exists to export speaker information to application >>> - info frame channel allocation is hardware coded in >>> hdmi_codec_hw_params. The configuration is set by default depending on >>> the number of channels. Drawback is that application is not aware of the >>> configuration selected by the driver. Erratum: Channel configuration is not updated but always set to default value 0: stereo FL+FR, even for more than 2 channels playback. >>> >>> So it seems that something is missing to do the link between EDID >>> information and audio source channel mapping on one side and audio IF >>> configuration on other side. >>> >>> What should be the best strategy to support feature in ASoC? >>> - No update in hdmi-codec, consider that application should know >>> the configuration available for 2,4,6 and 8 channels >>> => seems not very flexible... >>> >>> - Update hdmi-codec to implement "Playback Channel Map" in read-only, to >>> provide channel mapping information to application. >>> => impact is light, but application can get the correct mapping only >>> after the update of the number of channels (so after hw_params ops call) >>> >>> - Update hdmi-codec to implement "Playback Channel Map" in read and >>> write access. >>> => HDA implementation. Seems the more flexible solution from my windows... >>> >>> - Implement control in drm drivers >>> => seems not reasonable. >>> >>> - Other? > > We can start from the read-only chmap ctls, with a plan to enhance > to the read/write later on. I guess only few applications require the > channel map change, and the hardest part is the proper integration in > ASoC, not the write support itself. That seems reasonable. If OK, as a first step I will try to propose chmap control in read-only with some predefined configurations in HDMI-codec.c. Concerning channel allocation in HDMI_CODEC, Proposal is to align channels location according to HDA default ones: (hdac_cea_channel_speaker_allocation struct) static struct hdac_cea_channel_speaker_allocation channel_allocations[] = { /* channel: 7 6 5 4 3 2 1 0 */ /*mono or stereo */ { .ca_index = 0x00, .speakers = { 0, 0, 0, 0, 0, 0, FR, FL } }, /* 2.1 */ { .ca_index = 0x01, .speakers = { 0, 0, 0, 0, 0, LFE, FR, FL } }, /* Dolby Surround */ { .ca_index = 0x02, .speakers = { 0, 0, 0, 0, FC, 0, FR, FL } }, /* surround40 */ { .ca_index = 0x08, .speakers = { 0, 0, RR, RL, 0, 0, FR, FL } }, /* surround41 */ { .ca_index = 0x09, .speakers = { 0, 0, RR, RL, 0, LFE, FR, FL } }, /* surround50 */ { .ca_index = 0x0a, .speakers = { 0, 0, RR, RL, FC, 0, FR, FL } }, /* surround51 */ { .ca_index = 0x0b, .speakers = { 0, 0, RR, RL, FC, LFE, FR, FL } }, /* 6.1 */ { .ca_index = 0x0f, .speakers = { 0, RC, RR, RL, FC, LFE, FR, FL } }, /* surround71 */ { .ca_index = 0x13, .speakers = { RRC, RLC, RR, RL, FC, LFE, FR, FL } }, Thanks and Regards, Arnaud
diff --git a/include/sound/soc-dai.h b/include/sound/soc-dai.h index 964b7de..93624c9 100644 --- a/include/sound/soc-dai.h +++ b/include/sound/soc-dai.h @@ -247,6 +247,10 @@ struct snd_soc_dai_driver { /* probe ordering - for components with runtime dependencies */ int probe_order; int remove_order; + + /* Optional PCM controls to bind to PCM device on DAIs link*/ + const struct snd_kcontrol_new *pcm_controls; + int num_pcm_controls; }; /* diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 4afa8db..ace83c9 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1552,6 +1552,25 @@ static int soc_probe_dai(struct snd_soc_dai *dai, int order) return 0; } +static int soc_link_dai_pcm_controls(struct snd_soc_dai **dais, int num_dais, + struct snd_soc_pcm_runtime *rtd) +{ + struct snd_kcontrol_new kctl; + int i, j, err; + + for (i = 0; i < num_dais; ++i) { + for (j = 0; j < dais[i]->driver->num_pcm_controls; j++) { + kctl = dais[i]->driver->pcm_controls[j]; + if (!rtd->dai_link->no_pcm) + kctl.device = rtd->pcm->device; + if (snd_soc_add_dai_controls(dais[i], &kctl, 1)) + return err; + } + } + + return 0; +} + static int soc_link_dai_widgets(struct snd_soc_card *card, struct snd_soc_dai_link *dai_link, struct snd_soc_pcm_runtime *rtd) @@ -1663,6 +1682,15 @@ static int soc_probe_link_dais(struct snd_soc_card *card, dai_link->stream_name, ret); return ret; } + + /* Bind DAIs pcm controls to the PCM device */ + ret = soc_link_dai_pcm_controls(&cpu_dai, 1, rtd); + if (ret < 0) + return ret; + ret = soc_link_dai_pcm_controls(rtd->codec_dais, + rtd->num_codecs, rtd); + if (ret < 0) + return ret; } else { INIT_DELAYED_WORK(&rtd->delayed_work, codec2codec_close_delayed_work);
In case of several instances of the same PCM control (e.g IEC controls). Application should be able to address the control using the device field number, according to the PCM character device. This patch allows to link DAI PCM controls to the PCM device. During DAI_link probe, PCM controls are added after device field is forced to the PCM device number. Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com> --- include/sound/soc-dai.h | 4 ++++ sound/soc/soc-core.c | 28 ++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)