[Sound-open-firmware] about multiple BE dai_links support
Hi all,
As we are using link->id for BE dai_link match in snd_soc_find_dai_link(), so the link->id define in machine driver is crucial.
Previously we set all link->id to be 0s in machine drivers(e.g. nocodec.c, bytcr_rt5651.c, cnl_rt274.c, bxt_pcm512x.c...), but in many other cAVS machine drivers(skl_rt286.c, kbl_rt5663_max98927.c, bxt_tdf8532.c, ...), they are using different link->id for different BE dai_links.
So, to reuse those machine driver and BE dai_links for SOF, we have 2 solution to fix this:
1. remove the hardcode(to 0) of link->id in topology and add a param for that. I provide a RFC patch below.
2. change snd_soc_find_dai_link(), remove the link->id comparison.
Personally I don't like the #2 solution, it will lead to backward compatibility issue for those exist drivers, and also is mismatch with the comment of snd_soc_find_dai_link(): * @id: DAI link ID to match * @name: DAI link name to match, optional * @stream_name: DAI link stream name to match, optional
Hi Pierre/Liam, any comments on this? I hope we can align this soon then I can refine and send machine driver for GPMRB.
Thanks, ~Keyon
diff --git a/topology/m4/dai.m4 b/topology/m4/dai.m4 index 0b030af..0a79364 100644 --- a/topology/m4/dai.m4 +++ b/topology/m4/dai.m4 @@ -140,17 +140,17 @@ define(`DAI_TDM', dnl DAI Config) define(`N_DAI_CONFIG', `DAICONFIG.'$1)
-dnl DAI_CONFIG(type, idx, name, format, valid bits, mclk, bclk, fsync, tdm) +dnl DAI_CONFIG(type, idx, link_id, name, format, valid bits, mclk, bclk, fsync, tdm) define(`DAI_CONFIG', `SectionHWConfig."'$1$2`" {' `' ` id "'$2`"' ` format "'$4`"' `' -` '$6 ` '$7 ` '$8 ` '$9 +` '$10 `}' `SectionVendorTuples."'N_DAI_CONFIG($1$2)`_tuples_str" {' ` tokens "sof_dai_tokens"' @@ -164,15 +164,15 @@ define(`DAI_CONFIG', `SectionVendorTuples."'N_DAI_CONFIG($1$2)`_tuples" {' ` tokens "sof_dai_tokens"' ` tuples."word" {' -` SOF_TKN_DAI_SAMPLE_BITS' STR($5) +` SOF_TKN_DAI_SAMPLE_BITS' STR($6) ` }' `}' `SectionData."'N_DAI_CONFIG($1$2)`_data" {' ` tuples "'N_DAI_CONFIG($1$2)`_tuples"' `}' `' -`SectionBE."'$3`" {' -` index "0"' +`SectionBE."'$4`" {' +` index "'$3`"' ` default_hw_conf_id "'$2`"' `' ` hw_configs ['
On Tue, 2018-05-22 at 12:50 +0800, Keyon Jie wrote:
Hi all,
As we are using link->id for BE dai_link match in snd_soc_find_dai_link(), so the link->id define in machine driver is crucial.
Previously we set all link->id to be 0s in machine drivers(e.g. nocodec.c, bytcr_rt5651.c, cnl_rt274.c, bxt_pcm512x.c...), but in many other cAVS machine drivers(skl_rt286.c, kbl_rt5663_max98927.c, bxt_tdf8532.c, ...), they are using different link->id for different BE dai_links.
So, to reuse those machine driver and BE dai_links for SOF, we have 2 solution to fix this:
- remove the hardcode(to 0) of link->id in topology and add a param
for that. I provide a RFC patch below.
- change snd_soc_find_dai_link(), remove the link->id comparison.
Personally I don't like the #2 solution, it will lead to backward compatibility issue for those exist drivers, and also is mismatch with the comment of snd_soc_find_dai_link():
- @id: DAI link ID to match
- @name: DAI link name to match, optional
- @stream_name: DAI link stream name to match, optional
Hi Pierre/Liam, any comments on this? I hope we can align this soon then I can refine and send machine driver for GPMRB.
Thanks, ~Keyon
diff --git a/topology/m4/dai.m4 b/topology/m4/dai.m4 index 0b030af..0a79364 100644 --- a/topology/m4/dai.m4 +++ b/topology/m4/dai.m4 @@ -140,17 +140,17 @@ define(`DAI_TDM', dnl DAI Config) define(`N_DAI_CONFIG', `DAICONFIG.'$1)
-dnl DAI_CONFIG(type, idx, name, format, valid bits, mclk, bclk, fsync, tdm) +dnl DAI_CONFIG(type, idx, link_id, name, format, valid bits, mclk, bclk, fsync, tdm) define(`DAI_CONFIG', `SectionHWConfig."'$1$2`" {' `' ` id "'$2`"' ` format "'$4`"' `' -` '$6 ` '$7 ` '$8 ` '$9 +` '$10 `}' `SectionVendorTuples."'N_DAI_CONFIG($1$2)`_tuples_str" {' ` tokens "sof_dai_tokens"' @@ -164,15 +164,15 @@ define(`DAI_CONFIG', `SectionVendorTuples."'N_DAI_CONFIG($1$2)`_tuples" {' ` tokens "sof_dai_tokens"' ` tuples."word" {' -` SOF_TKN_DAI_SAMPLE_BITS' STR($5) +` SOF_TKN_DAI_SAMPLE_BITS' STR($6) ` }' `}' `SectionData."'N_DAI_CONFIG($1$2)`_data" {' ` tuples "'N_DAI_CONFIG($1$2)`_tuples"' `}' `' -`SectionBE."'$3`" {' -` index "0"' +`SectionBE."'$4`" {' +` index "'$3`"'
Keyon, I may be incorrect but I have always associated index with Pipeline_ID. So I am not sure this is the right way to assign the id.
Why the choice of index instead of the ID in SectionHWConfig?
` default_hw_conf_id "'$2`"' `' ` hw_configs [' _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On 2018年05月22日 14:26, Ranjani Sridharan wrote:
On Tue, 2018-05-22 at 12:50 +0800, Keyon Jie wrote:
Hi all,
As we are using link->id for BE dai_link match in snd_soc_find_dai_link(), so the link->id define in machine driver is crucial.
Previously we set all link->id to be 0s in machine drivers(e.g. nocodec.c, bytcr_rt5651.c, cnl_rt274.c, bxt_pcm512x.c...), but in many other cAVS machine drivers(skl_rt286.c, kbl_rt5663_max98927.c, bxt_tdf8532.c, ...), they are using different link->id for different BE dai_links.
So, to reuse those machine driver and BE dai_links for SOF, we have 2 solution to fix this:
- remove the hardcode(to 0) of link->id in topology and add a param
for that. I provide a RFC patch below.
- change snd_soc_find_dai_link(), remove the link->id comparison.
Personally I don't like the #2 solution, it will lead to backward compatibility issue for those exist drivers, and also is mismatch with the comment of snd_soc_find_dai_link():
- @id: DAI link ID to match
- @name: DAI link name to match, optional
- @stream_name: DAI link stream name to match, optional
Hi Pierre/Liam, any comments on this? I hope we can align this soon then I can refine and send machine driver for GPMRB.
Thanks, ~Keyon
diff --git a/topology/m4/dai.m4 b/topology/m4/dai.m4 index 0b030af..0a79364 100644 --- a/topology/m4/dai.m4 +++ b/topology/m4/dai.m4 @@ -140,17 +140,17 @@ define(`DAI_TDM', dnl DAI Config) define(`N_DAI_CONFIG', `DAICONFIG.'$1)
-dnl DAI_CONFIG(type, idx, name, format, valid bits, mclk, bclk, fsync, tdm) +dnl DAI_CONFIG(type, idx, link_id, name, format, valid bits, mclk, bclk, fsync, tdm) define(`DAI_CONFIG', `SectionHWConfig."'$1$2`" {' `' ` id "'$2`"' ` format "'$4`"' `' -` '$6 ` '$7 ` '$8 ` '$9 +` '$10 `}' `SectionVendorTuples."'N_DAI_CONFIG($1$2)`_tuples_str" {' ` tokens "sof_dai_tokens"' @@ -164,15 +164,15 @@ define(`DAI_CONFIG', `SectionVendorTuples."'N_DAI_CONFIG($1$2)`_tuples" {' ` tokens "sof_dai_tokens"' ` tuples."word" {' -` SOF_TKN_DAI_SAMPLE_BITS' STR($5) +` SOF_TKN_DAI_SAMPLE_BITS' STR($6) ` }' `}' `SectionData."'N_DAI_CONFIG($1$2)`_data" {' ` tuples "'N_DAI_CONFIG($1$2)`_tuples"' `}' `' -`SectionBE."'$3`" {' -` index "0"' +`SectionBE."'$4`" {' +` index "'$3`"'
Keyon, I may be incorrect but I have always associated index with Pipeline_ID. So I am not sure this is the right way to assign the id.
I also found this issue just now when I am debugging it, then I refer back to the patch I made before 1.1, it actually should be id, not index here.
Thanks for the correction, will send patch with this once everyone aligned to use this solution.
Thanks, ~Keyon
Why the choice of index instead of the ID in SectionHWConfig?
` default_hw_conf_id "'$2`"' `' ` hw_configs [' _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Tue, 2018-05-22 at 14:33 +0800, Keyon Jie wrote:
On 2018年05月22日 14:26, Ranjani Sridharan wrote:
On Tue, 2018-05-22 at 12:50 +0800, Keyon Jie wrote:
Hi all,
As we are using link->id for BE dai_link match in snd_soc_find_dai_link(), so the link->id define in machine driver is crucial.
Previously we set all link->id to be 0s in machine drivers(e.g. nocodec.c, bytcr_rt5651.c, cnl_rt274.c, bxt_pcm512x.c...), but in many other cAVS machine drivers(skl_rt286.c, kbl_rt5663_max98927.c, bxt_tdf8532.c, ...), they are using different link->id for different BE dai_links.
So, to reuse those machine driver and BE dai_links for SOF, we have 2 solution to fix this:
- remove the hardcode(to 0) of link->id in topology and add a
param for that. I provide a RFC patch below.
- change snd_soc_find_dai_link(), remove the link->id
comparison.
Personally I don't like the #2 solution, it will lead to backward compatibility issue for those exist drivers, and also is mismatch with the comment of snd_soc_find_dai_link():
- @id: DAI link ID to match
- @name: DAI link name to match, optional
- @stream_name: DAI link stream name to match, optional
Hi Pierre/Liam, any comments on this? I hope we can align this soon then I can refine and send machine driver for GPMRB.
Thanks, ~Keyon
diff --git a/topology/m4/dai.m4 b/topology/m4/dai.m4 index 0b030af..0a79364 100644 --- a/topology/m4/dai.m4 +++ b/topology/m4/dai.m4 @@ -140,17 +140,17 @@ define(`DAI_TDM', dnl DAI Config) define(`N_DAI_CONFIG', `DAICONFIG.'$1)
-dnl DAI_CONFIG(type, idx, name, format, valid bits, mclk, bclk, fsync, tdm) +dnl DAI_CONFIG(type, idx, link_id, name, format, valid bits, mclk, bclk, fsync, tdm) define(`DAI_CONFIG', `SectionHWConfig."'$1$2`" {' `' ` id "'$2`"' ` format "'$4`"' `' -` '$6 ` '$7 ` '$8 ` '$9 +` '$10 `}' `SectionVendorTuples."'N_DAI_CONFIG($1$2)`_tuples_str" {' ` tokens "sof_dai_tokens"' @@ -164,15 +164,15 @@ define(`DAI_CONFIG', `SectionVendorTuples."'N_DAI_CONFIG($1$2)`_tuples" {' ` tokens "sof_dai_tokens"' ` tuples."word" {' -` SOF_TKN_DAI_SAMPLE_BITS' STR($5) +` SOF_TKN_DAI_SAMPLE_BITS' STR($6) ` }' `}' `SectionData."'N_DAI_CONFIG($1$2)`_data" {' ` tuples "'N_DAI_CONFIG($1$2)`_tuples"' `}' `' -`SectionBE."'$3`" {' -` index "0"' +`SectionBE."'$4`" {' +` index "'$3`"'
Keyon, I may be incorrect but I have always associated index with Pipeline_ID. So I am not sure this is the right way to assign the id.
I also found this issue just now when I am debugging it, then I refer back to the patch I made before 1.1, it actually should be id, not index here.
Thanks for the correction, will send patch with this once everyone aligned to use this solution.
Keyon I've been thinking about this a little bit more and I have a couple of questions:
1. What will the ID be? Could it be the same as the port number? 2. Pierre brought this up in another email, will the ID's be sequential across different DAI types? Or will they start at 0?
Thanks, ~Keyon
Why the choice of index instead of the ID in SectionHWConfig?
` default_hw_conf_id "'$2`"' `' ` hw_configs [' _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmw are
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmwar e
On Wed, 2018-05-23 at 10:48 -0700, Ranjani Sridharan wrote:
Thanks for the correction, will send patch with this once everyone aligned to use this solution.
Keyon I've been thinking about this a little bit more and I have a couple of questions:
- What will the ID be? Could it be the same as the port number?
- Pierre brought this up in another email, will the ID's be sequential
across different DAI types? Or will they start at 0?
Shouldn't ID be the BE DAI link ID ?
Liam
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Liam Girdwood Sent: Thursday, May 24, 2018 6:17 PM To: Ranjani Sridharan ranjani.sridharan@linux.intel.com; Keyon Jie yang.jie@linux.intel.com; sound-open-firmware@alsa-project.org Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Subject: Re: [Sound-open-firmware] about multiple BE dai_links support
On Wed, 2018-05-23 at 10:48 -0700, Ranjani Sridharan wrote:
Thanks for the correction, will send patch with this once everyone aligned to use this solution.
Keyon I've been thinking about this a little bit more and I have a couple of questions:
- What will the ID be? Could it be the same as the port number?
Yes, it could be same, and could be different also.
- Pierre brought this up in another email, will the ID's be
sequential across different DAI types? Or will they start at 0?
In my opinion, it is more important that this ID should be unique, usually we will make them sequential and start at 0, but theoretically, it is not mandatory, nothing can prevent you using ids like 1, 3, 5, 6, 8,...
Shouldn't ID be the BE DAI link ID ?
Yep.
Thanks, ~Keyon
Liam _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On Thu, 2018-05-24 at 13:54 +0000, Jie, Yang wrote:
- What will the ID be? Could it be the same as the port number?
Yes, it could be same, and could be different also.
This breaks when we have SSP0 and DMIC0
- Pierre brought this up in another email, will the ID's be
sequential across different DAI types? Or will they start at 0?
In my opinion, it is more important that this ID should be unique, usually we will make them sequential and start at 0, but theoretically, it is not mandatory, nothing can prevent you using ids like 1, 3, 5, 6, 8,...
IDs are aligned 1:1 to BE DAI links so must be unique. They should start at 0 and I would not expect to see gaps.
Liam
-----Original Message----- From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] Sent: Friday, May 25, 2018 4:54 PM To: Jie, Yang yang.jie@intel.com; Ranjani Sridharan ranjani.sridharan@linux.intel.com; Keyon Jie yang.jie@linux.intel.com; sound-open-firmware@alsa-project.org Cc: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Subject: Re: [Sound-open-firmware] about multiple BE dai_links support
On Thu, 2018-05-24 at 13:54 +0000, Jie, Yang wrote:
- What will the ID be? Could it be the same as the port number?
Yes, it could be same, and could be different also.
This breaks when we have SSP0 and DMIC0
In this case, the ID is different with port number.
- Pierre brought this up in another email, will the ID's be
sequential across different DAI types? Or will they start at 0?
In my opinion, it is more important that this ID should be unique, usually we will make them sequential and start at 0, but theoretically, it is not mandatory, nothing can prevent you using ids like 1, 3, 5,
6, 8,...
IDs are aligned 1:1 to BE DAI links so must be unique. They should start at 0 and I
Agree.
would not expect to see gaps.
That's OK, let's note about this and don't allocate them with gaps in machine driver then.
Thanks, ~Keyon
Liam
- Pierre brought this up in another email, will the ID's be
sequential across different DAI types? Or will they start at 0?
In my opinion, it is more important that this ID should be unique, usually we will make them sequential and start at 0, but theoretically, it is not mandatory, nothing can prevent you using ids like 1, 3, 5,
6, 8,...
IDs are aligned 1:1 to BE DAI links so must be unique. They should start at 0 and I
Agree.
would not expect to see gaps.
That's OK, let's note about this and don't allocate them with gaps in machine driver then.
That's a bad assumption. For development or quirks, you may want to e.g. comment out a back-end on a specific machine driver (such as DMIC or SSP if there is no codec connected) and still expect HDMI to work - in that case the BE ID would not start at zero and might not be sequential. Why do we care about the BE ID value anyways? they need to be unique, period. that's the only thing we should assume.
-----Original Message----- From: sound-open-firmware-bounces@alsa-project.org [mailto:sound-open- firmware-bounces@alsa-project.org] On Behalf Of Pierre-Louis Bossart Sent: Friday, May 25, 2018 10:11 PM To: sound-open-firmware-bounces@alsa-project.org; Liam Girdwood liam.r.girdwood@linux.intel.com; Ranjani Sridharan ranjani.sridharan@linux.intel.com; Keyon Jie yang.jie@linux.intel.com; sound-open-firmware@alsa-project.org Subject: Re: [Sound-open-firmware] about multiple BE dai_links support
- Pierre brought this up in another email, will the ID's be
sequential across different DAI types? Or will they start at 0?
In my opinion, it is more important that this ID should be unique, usually we will make them sequential and start at 0, but theoretically, it is not mandatory, nothing can prevent you using ids like 1, 3, 5,
6, 8,...
IDs are aligned 1:1 to BE DAI links so must be unique. They should start at 0 and I
Agree.
would not expect to see gaps.
That's OK, let's note about this and don't allocate them with gaps in machine
driver then.
That's a bad assumption. For development or quirks, you may want to e.g. comment out a back-end on a specific machine driver (such as DMIC or SSP if there is no codec connected) and still expect HDMI to work - in that case the BE ID would not start at zero and might not be sequential. Why do we care about the BE ID value anyways? they need to be unique, period. that's the only thing we should assume.
Yes, that is exactly what I supposed we should do and care, thank you Pierre.
Thanks, ~Keyon
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
On 5/21/18 11:50 PM, Keyon Jie wrote:
Hi all,
As we are using link->id for BE dai_link match in snd_soc_find_dai_link(), so the link->id define in machine driver is crucial.
Previously we set all link->id to be 0s in machine drivers(e.g. nocodec.c, bytcr_rt5651.c, cnl_rt274.c, bxt_pcm512x.c...), but in many other cAVS machine drivers(skl_rt286.c, kbl_rt5663_max98927.c, bxt_tdf8532.c, ...), they are using different link->id for different BE dai_links.
So, to reuse those machine driver and BE dai_links for SOF, we have 2 solution to fix this:
- remove the hardcode(to 0) of link->id in topology and add a param for
that. I provide a RFC patch below.
- change snd_soc_find_dai_link(), remove the link->id comparison.
Personally I don't like the #2 solution, it will lead to backward compatibility issue for those exist drivers, and also is mismatch with the comment of snd_soc_find_dai_link(): * @id: DAI link ID to match * @name: DAI link name to match, optional * @stream_name: DAI link stream name to match, optional
Hi Pierre/Liam, any comments on this? I hope we can align this soon then I can refine and send machine driver for GPMRB.
I also prefer a link->id set in the topology. There is no rationale or requirement for ordering BE DAIs in the machine driver, so we need to adjust and avoid making assumptions. For the DA7219 work, I had to change code in the machine driver to force the ID to be zero, if there is an easier topology-based solution it's better.
Thanks, ~Keyon
diff --git a/topology/m4/dai.m4 b/topology/m4/dai.m4 index 0b030af..0a79364 100644 --- a/topology/m4/dai.m4 +++ b/topology/m4/dai.m4 @@ -140,17 +140,17 @@ define(`DAI_TDM', dnl DAI Config) define(`N_DAI_CONFIG', `DAICONFIG.'$1)
-dnl DAI_CONFIG(type, idx, name, format, valid bits, mclk, bclk, fsync, tdm) +dnl DAI_CONFIG(type, idx, link_id, name, format, valid bits, mclk, bclk, fsync, tdm) define(`DAI_CONFIG', `SectionHWConfig."'$1$2`" {' `' ` id "'$2`"' ` format "'$4`"' `' -` '$6 ` '$7 ` '$8 ` '$9 +` '$10 `}' `SectionVendorTuples."'N_DAI_CONFIG($1$2)`_tuples_str" {' ` tokens "sof_dai_tokens"' @@ -164,15 +164,15 @@ define(`DAI_CONFIG', `SectionVendorTuples."'N_DAI_CONFIG($1$2)`_tuples" {' ` tokens "sof_dai_tokens"' ` tuples."word" {' -` SOF_TKN_DAI_SAMPLE_BITS' STR($5) +` SOF_TKN_DAI_SAMPLE_BITS' STR($6) ` }' `}' `SectionData."'N_DAI_CONFIG($1$2)`_data" {' ` tuples "'N_DAI_CONFIG($1$2)`_tuples"' `}' `' -`SectionBE."'$3`" {' -` index "0"' +`SectionBE."'$4`" {' +` index "'$3`"' ` default_hw_conf_id "'$2`"' `' ` hw_configs [' _______________________________________________ Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (5)
-
Jie, Yang -
Keyon Jie -
Liam Girdwood -
Pierre-Louis Bossart -
Ranjani Sridharan