[PATCH v3 0/2] ALSA controls management using index/device/sub-devices fields

[PATCH v3 0/2] ALSA controls management using index/device/sub-devices fields

[Arnaud Pouliquen]

V3:
I changed "RFC" prefix to "PATCH", hoping  that patchset is enough mature in term 
of code and discussions, to be considered as a potential patch... 

Previous discussions summary:
 - topic of  the patchset: 
 	Patch concerns ASoc DAI drivers.
        Aim is to be able to instantiate PCM control using device field according 
        to the associated PCM char device.  For this, a relationship between DAIs PCM 
        control and PCM char device needs to be establish.
        => Proposal is to add field in DAI driver struct to declare PCM controls that
	need to be linked to PCM character device on DAI link probing.
        
  - Limitation of the patchset:
  	This patchset seems only a first step as 2 concerns are been highlighted:
         - Patch is limited to "static" DAI-link. This not responds to backend (no_pcm dai links) and 
         some none-DAI codecs, that could register PCm controls. 
         In theses cases it is not possible to establish relationship during probe.
         	=> No solution proposed for time beeing.
           
         - This patchset does not fix conflict of 2 "identical" PCM controls declared by 2 drivers. 
          For instance, a conflict can exist between a  PCM control created in DAI driver 
          and the same control declared in a codec driver.  As index field is forced to 0, prefix
          should be used in codec.
        	=> To discuss solution in a separate thread as issue already present without this patchset 
                	several approaches seem possible like using index field ( control auto indexation in
                        ASoC) or prefix in Naming. 
 
 Changes:
 - [PATCH v3 1/2] ASoC: core: allow DAI PCM controls bound to PCM device
 	Code optimization based on Takashi Sakamoto suggestion
 - [RFC V2 3/3]   ASoC: hdmi-codec: Example of PCM control bound to PCM device for multi
	patch suppressed as it was send as example in previous version. Patch is valid but
        can have impact on existing drivers as control device field value is updated 
  - [PATCH v3 2/2] ASoC: sti: bind pcm controls to pcm device. 
 	No change vs V2
        
V2:
http://www.spinics.net/lists/alsa-devel/msg57045.html

Aim of this version is to continue discussion on DAI PCM control focused on ASoC drivers.
In this V2  implementation in Soc-core is simplified to limit impact on existing code.

 Update of the RFC V1 based on discussions:
 - [RFC 4/4] iecset: allow to select control with device and sub-device numbers
 	no more part of the RFC V2, will be discussed in a separate thread
 - [RFC 2/4] ALSA: control: increment index field for duplicated control.
 	no more part of the RFC V2, no more need as RFC subject is PCM controls
   
- [RFC V2 1/3] ASoC: core: allow DAI PCM controls bound to PCM device
	Patch reworked from V1 to simplify implementation
        - Binding is not done for Dai links tagged with no_pcm (DPCM).
        - no more possibility to add the controls after the DAI link probing.

- [RFC V2 2/3]   ASoC: sti: bind PCM controls to PCM device.
- [RFC V2 3/3]   ASoC: hdmi-codec: Example of PCM control bound to PCM device for multi
        Example of implementation in STI DAI driver and HDMI-codec drivers
        
V1: 
 http://www.spinics.net/lists/alsa-devel/msg56479.html

 1) Alsa-utils patch

- iecset: allow to select control with device and sub-device numbers
  This patch allows to access to 2 iec controls differentiated by
  device/sub-devices numbers
=> For me, this patch is mandatory to be able to address the ASoC IEC
   controls, in case of no fix is implemented to allows index field
   update in ASoC.

2) Alsa driver patches
  - ASoC: core: allow PCM control binding to PCM device
  	Add relationship between DAIs PCM controls and PCM device.

  - ALSA: control: increment index field for duplicated control.
   	Generic implementation of the patch proposed in HDA driver
        (http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add)

  - ASoC: sti: use bind_pcm_ctl
  	implementation of bind_pcm_ctl for sti driver.

Arnaud Pouliquen (2):
  ASoC: core: allow DAI PCM controls bound to PCM device
  ASoC: sti: bind pcm controls to pcm device.

 include/sound/soc-dai.h      |  4 ++++
 sound/soc/soc-core.c         | 28 ++++++++++++++++++++++++++++
 sound/soc/sti/sti_uniperif.c | 33 ++++-----------------------------
 3 files changed, 36 insertions(+), 29 deletions(-)

-- 
1.9.1

[PATCH v3 1/2] ASoC: core: allow DAI PCM controls bound to PCM device

[Arnaud Pouliquen]

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;
+			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);
-- 
1.9.1

[PATCH v3 2/2] ASoC: sti: bind pcm controls to pcm device.

[Arnaud Pouliquen]

Move PCM control list in DAI driver struct, to
bind PCM control to PCM device created during DAI linking.

Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@st.com>
---
 sound/soc/sti/sti_uniperif.c | 33 ++++-----------------------------
 1 file changed, 4 insertions(+), 29 deletions(-)

diff --git a/sound/soc/sti/sti_uniperif.c b/sound/soc/sti/sti_uniperif.c
index 549fac3..db8bab5 100644
--- a/sound/soc/sti/sti_uniperif.c
+++ b/sound/soc/sti/sti_uniperif.c
@@ -225,34 +225,6 @@ int sti_uniperiph_get_tdm_word_pos(struct uniperif *uni,
 }
 
 /*
- * sti_uniperiph_dai_create_ctrl
- * This function is used to create Ctrl associated to DAI but also pcm device.
- * Request is done by front end to associate ctrl with pcm device id
- */
-static int sti_uniperiph_dai_create_ctrl(struct snd_soc_dai *dai)
-{
-	struct sti_uniperiph_data *priv = snd_soc_dai_get_drvdata(dai);
-	struct uniperif *uni = priv->dai_data.uni;
-	struct snd_kcontrol_new *ctrl;
-	int i;
-
-	if (!uni->num_ctrls)
-		return 0;
-
-	for (i = 0; i < uni->num_ctrls; i++) {
-		/*
-		 * Several Control can have same name. Controls are indexed on
-		 * Uniperipheral instance ID
-		 */
-		ctrl = &uni->snd_ctrls[i];
-		ctrl->index = uni->id;
-		ctrl->device = uni->id;
-	}
-
-	return snd_soc_add_dai_controls(dai, uni->snd_ctrls, uni->num_ctrls);
-}
-
-/*
  * DAI
  */
 int sti_uniperiph_dai_hw_params(struct snd_pcm_substream *substream,
@@ -342,7 +314,7 @@ static int sti_uniperiph_dai_probe(struct snd_soc_dai *dai)
 	dai_data->dma_data.addr = dai_data->uni->fifo_phys_address;
 	dai_data->dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
 
-	return sti_uniperiph_dai_create_ctrl(dai);
+	return 0;
 }
 
 static const struct snd_soc_dai_driver sti_uniperiph_dai_template = {
@@ -460,6 +432,9 @@ static int sti_uniperiph_probe(struct platform_device *pdev)
 
 	ret = sti_uniperiph_cpu_dai_of(node, priv);
 
+	priv->dai->pcm_controls = priv->dai_data.uni->snd_ctrls;
+	priv->dai->num_pcm_controls = priv->dai_data.uni->num_ctrls;
+
 	dev_set_drvdata(&pdev->dev, priv);
 
 	ret = devm_snd_soc_register_component(&pdev->dev,
-- 
1.9.1

Re: [PATCH v3 0/2] ALSA controls management using index/device/sub-devices fields

[Takashi Sakamoto]

On Nov 28 2016 18:33, Arnaud Pouliquen wrote:
> V3:
> I changed "RFC" prefix to "PATCH", hoping  that patchset is enough mature in term
> of code and discussions, to be considered as a potential patch...
>
> Previous discussions summary:
>  - topic of  the patchset:
>  	Patch concerns ASoc DAI drivers.
>         Aim is to be able to instantiate PCM control using device field according
>         to the associated PCM char device.  For this, a relationship between DAIs PCM
>         control and PCM char device needs to be establish.
>         => Proposal is to add field in DAI driver struct to declare PCM controls that
> 	need to be linked to PCM character device on DAI link probing.
>
>   - Limitation of the patchset:
>   	This patchset seems only a first step as 2 concerns are been highlighted:
>          - Patch is limited to "static" DAI-link. This not responds to backend (no_pcm dai links) and
>          some none-DAI codecs, that could register PCm controls.
>          In theses cases it is not possible to establish relationship during probe.
>          	=> No solution proposed for time beeing.
>
>          - This patchset does not fix conflict of 2 "identical" PCM controls declared by 2 drivers.
>           For instance, a conflict can exist between a  PCM control created in DAI driver
>           and the same control declared in a codec driver.  As index field is forced to 0, prefix
>           should be used in codec.
>         	=> To discuss solution in a separate thread as issue already present without this patchset
>                 	several approaches seem possible like using index field ( control auto indexation in
>                         ASoC) or prefix in Naming.

Enough information for reviewers, very nice ;)

>  Changes:
>  - [PATCH v3 1/2] ASoC: core: allow DAI PCM controls bound to PCM device
>  	Code optimization based on Takashi Sakamoto suggestion
>  - [RFC V2 3/3]   ASoC: hdmi-codec: Example of PCM control bound to PCM device for multi
> 	patch suppressed as it was send as example in previous version. Patch is valid but
>         can have impact on existing drivers as control device field value is updated
>   - [PATCH v3 2/2] ASoC: sti: bind pcm controls to pcm device.
>  	No change vs V2
>
> V2:
> http://www.spinics.net/lists/alsa-devel/msg57045.html
>
> Aim of this version is to continue discussion on DAI PCM control focused on ASoC drivers.
> In this V2  implementation in Soc-core is simplified to limit impact on existing code.
>
>  Update of the RFC V1 based on discussions:
>  - [RFC 4/4] iecset: allow to select control with device and sub-device numbers
>  	no more part of the RFC V2, will be discussed in a separate thread
>  - [RFC 2/4] ALSA: control: increment index field for duplicated control.
>  	no more part of the RFC V2, no more need as RFC subject is PCM controls
>
> - [RFC V2 1/3] ASoC: core: allow DAI PCM controls bound to PCM device
> 	Patch reworked from V1 to simplify implementation
>         - Binding is not done for Dai links tagged with no_pcm (DPCM).
>         - no more possibility to add the controls after the DAI link probing.
>
> - [RFC V2 2/3]   ASoC: sti: bind PCM controls to PCM device.
> - [RFC V2 3/3]   ASoC: hdmi-codec: Example of PCM control bound to PCM device for multi
>         Example of implementation in STI DAI driver and HDMI-codec drivers
>
> V1:
>  http://www.spinics.net/lists/alsa-devel/msg56479.html
>
>  1) Alsa-utils patch
>
> - iecset: allow to select control with device and sub-device numbers
>   This patch allows to access to 2 iec controls differentiated by
>   device/sub-devices numbers
> => For me, this patch is mandatory to be able to address the ASoC IEC
>    controls, in case of no fix is implemented to allows index field
>    update in ASoC.
>
> 2) Alsa driver patches
>   - ASoC: core: allow PCM control binding to PCM device
>   	Add relationship between DAIs PCM controls and PCM device.
>
>   - ALSA: control: increment index field for duplicated control.
>    	Generic implementation of the patch proposed in HDA driver
>         (http://git.kernel.org/cgit/linux/kernel/git/tiwai/sound.git/commit/?id=ea9b43add)
>
>   - ASoC: sti: use bind_pcm_ctl
>   	implementation of bind_pcm_ctl for sti driver.
>
> Arnaud Pouliquen (2):
>   ASoC: core: allow DAI PCM controls bound to PCM device
>   ASoC: sti: bind pcm controls to pcm device.
>
>  include/sound/soc-dai.h      |  4 ++++
>  sound/soc/soc-core.c         | 28 ++++++++++++++++++++++++++++
>  sound/soc/sti/sti_uniperif.c | 33 ++++-----------------------------
>  3 files changed, 36 insertions(+), 29 deletions(-)

Regards

Takashi Sakamoto

Re: [PATCH v3 1/2] ASoC: core: allow DAI PCM controls bound to PCM device

[Takashi Sakamoto]

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

Re: [PATCH v3 1/2] ASoC: core: allow DAI PCM controls bound to PCM device

[Arnaud Pouliquen]

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

Re: [PATCH v3 1/2] ASoC: core: allow DAI PCM controls bound to PCM device

[Takashi Sakamoto]

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

[RFC] ASOC: HDMI audio info frame speaker allocation

[Arnaud Pouliquen]

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

Re: [RFC] ASOC: HDMI audio info frame speaker allocation

[Pierre-Louis Bossart]

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
>

Re: [RFC] ASOC: HDMI audio info frame speaker allocation

[Takashi Sakamoto]

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

Re: [RFC] ASOC: HDMI audio info frame speaker allocation

[Takashi Iwai]

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

Re: [RFC] ASOC: HDMI audio info frame speaker allocation

[Arnaud Pouliquen]

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