| Message ID | 08b095d38f59c43690daaa45e1d201f367812228.1443605139.git.mengdong.lin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 30, 2015 at 05:31:34PM +0800, mengdong.lin@intel.com wrote: > From: Vedang Patel <vedang.patel@intel.com> > > For codec-codec links, this struct will be mapped to the DAI links's > params, which is struct snd_soc_pcm_stream and it needs a stream name. OK, so this means that every DAI link name is going to become an ABI too if there's a topology in there. Not a problem but we'll need to watch out for that on review... I wonder if I can make some automated checker thing.
On Fri, 2015-10-02 at 17:59 +0100, Mark Brown wrote: > On Wed, Sep 30, 2015 at 05:31:34PM +0800, mengdong.lin@intel.com wrote: > > From: Vedang Patel <vedang.patel@intel.com> > > > > For codec-codec links, this struct will be mapped to the DAI links's > > params, which is struct snd_soc_pcm_stream and it needs a stream name. > > OK, so this means that every DAI link name is going to become an ABI > too if there's a topology in there. Not a problem but we'll need to > watch out for that on review... I wonder if I can make some automated > checker thing. Probably good to discuss at ELCE as there may be other things that can be auto-checked too .... Liam
> -----Original Message----- > From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] > Sent: Saturday, October 03, 2015 2:04 AM > To: Mark Brown > Cc: Lin, Mengdong; alsa-devel@alsa-project.org; tiwai@suse.de; Patel, > Vedang; Prusty, Subhransu S; Koul, Vinod; Kp, Jeeja > Subject: Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to > snd_soc_tplg_stream > > On Fri, 2015-10-02 at 17:59 +0100, Mark Brown wrote: > > On Wed, Sep 30, 2015 at 05:31:34PM +0800, mengdong.lin@intel.com > wrote: > > > From: Vedang Patel <vedang.patel@intel.com> > > > > > > For codec-codec links, this struct will be mapped to the DAI links's > > > params, which is struct snd_soc_pcm_stream and it needs a stream > name. > > > > OK, so this means that every DAI link name is going to become an ABI > > too if there's a topology in there. Not a problem but we'll need to > > watch out for that on review... I wonder if I can make some automated > > checker thing. Hi Mark, Do you mean we need to check if a DAI link name is already used, or if it's a valid string? Is there any naming convention for a DAI link? Thanks Mengdong > Probably good to discuss at ELCE as there may be other things that can be > auto-checked too .... > > Liam
On Tue, Oct 06, 2015 at 08:55:58AM +0000, Lin, Mengdong wrote: > > > OK, so this means that every DAI link name is going to become an ABI > > > too if there's a topology in there. Not a problem but we'll need to > > > watch out for that on review... I wonder if I can make some automated > > > checker thing. > Do you mean we need to check if a DAI link name is already used, or if it's a valid string? No, I mean that if someone changes a name it'll invalidate topology files that reference that name. > Is there any naming convention for a DAI link? Not really.
> -----Original Message----- > From: Liam Girdwood [mailto:liam.r.girdwood@linux.intel.com] > Sent: Saturday, October 03, 2015 2:04 AM > To: Mark Brown > Cc: Lin, Mengdong; alsa-devel@alsa-project.org; tiwai@suse.de; Patel, > Vedang; Prusty, Subhransu S; Koul, Vinod; Kp, Jeeja > Subject: Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to > snd_soc_tplg_stream > > On Fri, 2015-10-02 at 17:59 +0100, Mark Brown wrote: > > On Wed, Sep 30, 2015 at 05:31:34PM +0800, mengdong.lin@intel.com > wrote: > > > From: Vedang Patel <vedang.patel@intel.com> > > > > > > For codec-codec links, this struct will be mapped to the DAI links's > > > params, which is struct snd_soc_pcm_stream and it needs a stream > name. > > > > OK, so this means that every DAI link name is going to become an ABI > > too if there's a topology in there. Not a problem but we'll need to > > watch out for that on review... I wonder if I can make some automated > > checker thing. Hi Mark, Do you mean we need to check if a DAI link name is already used, or if it's a valid string? Is there any naming convention for a DAI link? Thanks Mengdong > Probably good to discuss at ELCE as there may be other things that can be > auto-checked too .... > > Liam
On Tue, 2015-10-06 at 11:34 +0100, Mark Brown wrote: > On Tue, Oct 06, 2015 at 08:55:58AM +0000, Lin, Mengdong wrote: > > > > > OK, so this means that every DAI link name is going to become an ABI > > > > too if there's a topology in there. Not a problem but we'll need to > > > > watch out for that on review... I wonder if I can make some automated > > > > checker thing. > > > Do you mean we need to check if a DAI link name is already used, or if it's a > > valid string? > > No, I mean that if someone changes a name it'll invalidate topology > files that reference that name. Shouldn't this sort be checked in the alsa-lib while building the topology. It should validate the names, graph and complete only when stuff is right.
On Tue, Oct 06, 2015 at 03:16:30PM +0000, Koul, Vinod wrote: > On Tue, 2015-10-06 at 11:34 +0100, Mark Brown wrote: > > No, I mean that if someone changes a name it'll invalidate topology > > files that reference that name. > Shouldn't this sort be checked in the alsa-lib while building the topology. It should > validate the names, graph and complete only when stuff is right. The problem isn't detecting the error, the point is that if someone upgrades their kernel and someone changed the DAI link name then their existing userspace will break. We don't want that error to be there to be detected in the first place.
On 10/06/2015 06:25 PM, Mark Brown wrote: > On Tue, Oct 06, 2015 at 03:16:30PM +0000, Koul, Vinod wrote: >> On Tue, 2015-10-06 at 11:34 +0100, Mark Brown wrote: > >>> No, I mean that if someone changes a name it'll invalidate topology >>> files that reference that name. > >> Shouldn't this sort be checked in the alsa-lib while building the topology. It should >> validate the names, graph and complete only when stuff is right. > > The problem isn't detecting the error, the point is that if someone > upgrades their kernel and someone changed the DAI link name then their > existing userspace will break. We don't want that error to be there to > be detected in the first place. Another thing to consider is that the topology firmware might be developed against a non-upstream driver. And in order for the driver to go upstream it needs changes that breaks the ABI interface to the firmware which is already shipped and the vendor might not provide support for updating the firmware file. Having these kind of tightly coupled interdependencies between driver and firmware is quite risky business.
On Tue, Oct 06, 2015 at 06:35:28PM +0200, Lars-Peter Clausen wrote: > On 10/06/2015 06:25 PM, Mark Brown wrote: > > The problem isn't detecting the error, the point is that if someone > > upgrades their kernel and someone changed the DAI link name then their > > existing userspace will break. We don't want that error to be there to > > be detected in the first place. > Another thing to consider is that the topology firmware might be developed > against a non-upstream driver. And in order for the driver to go upstream it > needs changes that breaks the ABI interface to the firmware which is already > shipped and the vendor might not provide support for updating the firmware > file. Having these kind of tightly coupled interdependencies between driver > and firmware is quite risky business. Yeah, I think we're kind of stuck with some degree of fairly tight coupling at some point though - at some point we're going to need to work out which link in the driver corresponds to which link in the firmware file.
> -----Original Message----- > From: Mark Brown [mailto:broonie@kernel.org] > Sent: Wednesday, October 07, 2015 1:37 AM > To: Lars-Peter Clausen > Cc: Koul, Vinod; alsa-devel@alsa-project.org; tiwai@suse.de; Lin, Mengdong; > liam.r.girdwood@linux.intel.com; Kp, Jeeja; Prusty, Subhransu S; Patel, > Vedang > Subject: Re: [alsa-devel] [PATCH v2 4/8] ASoC: topology: ABI - Add name > element to snd_soc_tplg_stream > > On Tue, Oct 06, 2015 at 06:35:28PM +0200, Lars-Peter Clausen wrote: > > On 10/06/2015 06:25 PM, Mark Brown wrote: > > > > The problem isn't detecting the error, the point is that if someone > > > upgrades their kernel and someone changed the DAI link name then > > > their existing userspace will break. We don't want that error to be > > > there to be detected in the first place. > > > Another thing to consider is that the topology firmware might be > > developed against a non-upstream driver. And in order for the driver > > to go upstream it needs changes that breaks the ABI interface to the > > firmware which is already shipped and the vendor might not provide > > support for updating the firmware file. Having these kind of tightly > > coupled interdependencies between driver and firmware is quite risky > business. > > Yeah, I think we're kind of stuck with some degree of fairly tight coupling at > some point though - at some point we're going to need to work out which > link in the driver corresponds to which link in the firmware file. There is a 'version' field in struct snd_soc_tplg_hdr, which is a vendor-specific version number. The driver can check this field of a firmware topology and decide whether to support this firmware or not. Is this okay? Thanks Mengdong
On Wed, 07 Oct 2015 11:48:37 +0200, Lin, Mengdong wrote: > > > -----Original Message----- > > From: Mark Brown [mailto:broonie@kernel.org] > > Sent: Wednesday, October 07, 2015 1:37 AM > > To: Lars-Peter Clausen > > Cc: Koul, Vinod; alsa-devel@alsa-project.org; tiwai@suse.de; Lin, Mengdong; > > liam.r.girdwood@linux.intel.com; Kp, Jeeja; Prusty, Subhransu S; Patel, > > Vedang > > Subject: Re: [alsa-devel] [PATCH v2 4/8] ASoC: topology: ABI - Add name > > element to snd_soc_tplg_stream > > > > On Tue, Oct 06, 2015 at 06:35:28PM +0200, Lars-Peter Clausen wrote: > > > On 10/06/2015 06:25 PM, Mark Brown wrote: > > > > > > The problem isn't detecting the error, the point is that if someone > > > > upgrades their kernel and someone changed the DAI link name then > > > > their existing userspace will break. We don't want that error to be > > > > there to be detected in the first place. > > > > > Another thing to consider is that the topology firmware might be > > > developed against a non-upstream driver. And in order for the driver > > > to go upstream it needs changes that breaks the ABI interface to the > > > firmware which is already shipped and the vendor might not provide > > > support for updating the firmware file. Having these kind of tightly > > > coupled interdependencies between driver and firmware is quite risky > > business. > > > > Yeah, I think we're kind of stuck with some degree of fairly tight coupling at > > some point though - at some point we're going to need to work out which > > link in the driver corresponds to which link in the firmware file. > > There is a 'version' field in struct snd_soc_tplg_hdr, which is a vendor-specific version number. > The driver can check this field of a firmware topology and decide whether to support this firmware or not. > > Is this okay? Well, you can't assure that the third vendor would set that field always right. I won't bet it, as the odds are too bad :) How about to allow leaving this name empty as default? Or better to ask: what's the exact merit of having strict binding to DAI link? thanks, Takashi
On Wed, Oct 07, 2015 at 09:48:37AM +0000, Lin, Mengdong wrote: Please fix your mailer to word wrap somewhere a bit nearer to 80 columns. > > Yeah, I think we're kind of stuck with some degree of fairly tight coupling at > > some point though - at some point we're going to need to work out which > > link in the driver corresponds to which link in the firmware file. > There is a 'version' field in struct snd_soc_tplg_hdr, which is a vendor-specific version number. > The driver can check this field of a firmware topology and decide whether to support this firmware or not. > Is this okay? Well, they probably could check that but it's not really the point - we don't want to break things at all here, we should try to avoid forcing users to upgrade their userspace with their kernel since that makes life harder for everyone.
On Wed, Oct 07, 2015 at 12:04:19PM +0200, Takashi Iwai wrote: > How about to allow leaving this name empty as default? Or better to > ask: what's the exact merit of having strict binding to DAI link? Well, if the topology is describing something like a phone at some point you're going to need to work out what is connected to for example the modem and the bluetooth, and you're probably going to want to have different processing pipelines in front of each.
diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h index 88210a8..2181480 100644 --- a/include/uapi/sound/asoc.h +++ b/include/uapi/sound/asoc.h @@ -217,6 +217,7 @@ struct snd_soc_tplg_stream_caps { */ struct snd_soc_tplg_stream { __le32 size; /* in bytes of this structure */ + char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN]; /* Name of the stream */ __le64 format; /* SNDRV_PCM_FMTBIT_* */ __le32 rate; /* SNDRV_PCM_RATE_* */ __le32 period_bytes; /* size of period in bytes */