[PATCH v2 0/8] ASoC: topology: ABI - Add support for PCM, BE & CC link

[PATCH v2 0/8] ASoC: topology: ABI - Add support for PCM, BE & CC link

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

This series includes topology ABI update to support PCM (FE DAI/DAI links),
BE and CC DAI links. Bump the ABI version to 4.

Mengdong Lin (2):
  ASoC: topology: ABI - Change stream formats to a bitwise flag
  ASoC: topology: ABI - Remove tdm_slot & dai_fmt from
    snd_soc_tplg_stream

Vedang Patel (6):
  ASoC: topology: ABI - Add PCM Support and bump ABI version to 4
  ASoC: topology: ABI - Remove unused struct snd_soc_tplg_stream_config
  ASoC: topology: ABI - Use __le32 instead of __u32 in
    snd_soc_tplg_dapm_widget
  ASoC: topology: ABI - Add name element to snd_soc_tplg_stream
  ASoC: topology: ABI - Add the type for BE DAI link
  ASoC: topology: ABI - Add configuration for BE & Codec-Codec DAI Links

 include/uapi/sound/asoc.h | 76 +++++++++++++++++++++++++----------------------
 sound/soc/soc-topology.c  |  4 +--
 2 files changed, 43 insertions(+), 37 deletions(-)

-- 
1.9.1

[PATCH v2 1/8] ASoC: topology: ABI - Add PCM Support and bump ABI version to 4

[mengdong.lin]

From: Vedang Patel <vedang.patel@intel.com>

The struct snd_soc_tplg_pcm_dai is renamed to snd_soc_tplg_pcm.
This struct will now be used to handle data related to PCMs
(FE DAI & DAI links). It's not for BE, because BE DAI mappings will be
provided by ACPI/FDT data.

Remove the unused struct snd_soc_tplg_pcm_cfg_caps. We are using
snd_soc_tplg_stream and snd_soc_stream_caps instead.

Bump ABI version to 4.

Signed-off-by: Vedang Patel <vedang.patel@intel.com>
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h
index 247c50b..2aa081c 100644
--- a/include/uapi/sound/asoc.h
+++ b/include/uapi/sound/asoc.h
@@ -83,7 +83,7 @@
 #define SND_SOC_TPLG_NUM_TEXTS		16
 
 /* ABI version */
-#define SND_SOC_TPLG_ABI_VERSION	0x3
+#define SND_SOC_TPLG_ABI_VERSION	0x4
 
 /* Max size of TLV data */
 #define SND_SOC_TPLG_TLV_SIZE		32
@@ -378,30 +378,29 @@ struct snd_soc_tplg_dapm_widget {
 	 */
 } __attribute__((packed));
 
-struct snd_soc_tplg_pcm_cfg_caps {
-	struct snd_soc_tplg_stream_caps caps;
-	struct snd_soc_tplg_stream_config configs[SND_SOC_TPLG_STREAM_CONFIG_MAX];
-	__le32 num_configs;	/* number of configs */
-} __attribute__((packed));
 
 /*
- * Describes SW/FW specific features of PCM or DAI link.
+ * Describes SW/FW specific features of PCM (FE DAI & DAI link).
  *
- * File block representation for PCM/DAI-Link :-
+ * File block representation for PCM :-
  * +-----------------------------------+-----+
  * | struct snd_soc_tplg_hdr           |  1  |
  * +-----------------------------------+-----+
- * | struct snd_soc_tplg_dapm_pcm_dai  |  N  |
+ * | struct snd_soc_tplg_pcm           |  N  |
  * +-----------------------------------+-----+
  */
-struct snd_soc_tplg_pcm_dai {
+struct snd_soc_tplg_pcm {
 	__le32 size;		/* in bytes of this structure */
-	char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
-	__le32 id;			/* unique ID - used to match */
-	__le32 playback;		/* supports playback mode */
-	__le32 capture;			/* supports capture mode */
-	__le32 compress;		/* 1 = compressed; 0 = PCM */
-	struct snd_soc_tplg_pcm_cfg_caps capconf[2];	/* capabilities and configs */
+	char pcm_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
+	char dai_name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
+	__le32 pcm_id;		/* unique ID - used to match */
+	__le32 dai_id;		/* unique ID - used to match */
+	__le32 playback;	/* supports playback mode */
+	__le32 capture;		/* supports capture mode */
+	__le32 compress;	/* 1 = compressed; 0 = PCM */
+	struct snd_soc_tplg_stream stream[SND_SOC_TPLG_STREAM_CONFIG_MAX]; /* for DAI link */
+	__le32 num_streams;	/* number of streams */
+	struct snd_soc_tplg_stream_caps caps[2]; /* playback and capture for DAI */
 } __attribute__((packed));
 
 #endif
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 69d01cd..8d7ec80 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1558,7 +1558,7 @@ static int soc_tplg_pcm_dai_elems_load(struct soc_tplg *tplg,
 	pcm_dai = (struct snd_soc_tplg_pcm_dai *)tplg->pos;
 
 	if (soc_tplg_check_elem_count(tplg,
-		sizeof(struct snd_soc_tplg_pcm_dai), count,
+		sizeof(struct snd_soc_tplg_pcm), count,
 		hdr->payload_size, "PCM DAI")) {
 		dev_err(tplg->dev, "ASoC: invalid count %d for PCM DAI elems\n",
 			count);
@@ -1566,7 +1566,7 @@ static int soc_tplg_pcm_dai_elems_load(struct soc_tplg *tplg,
 	}
 
 	dev_dbg(tplg->dev, "ASoC: adding %d PCM DAIs\n", count);
-	tplg->pos += sizeof(struct snd_soc_tplg_pcm_dai) * count;
+	tplg->pos += sizeof(struct snd_soc_tplg_pcm) * count;
 
 	dobj = kzalloc(sizeof(struct snd_soc_dobj), GFP_KERNEL);
 	if (dobj == NULL)
-- 
1.9.1

[PATCH v2 2/8] ASoC: topology: ABI - Remove unused struct snd_soc_tplg_stream_config

[mengdong.lin]

From: Vedang Patel <vedang.patel@intel.com>

The struct snd_soc_tplg_stream_config is no longer used in the ABI.
We are using snd_soc_tplg_stream instead.

Signed-off-by: Vedang Patel <vedang.patel@intel.com>
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h
index 2aa081c..4bef63f 100644
--- a/include/uapi/sound/asoc.h
+++ b/include/uapi/sound/asoc.h
@@ -227,16 +227,6 @@ struct snd_soc_tplg_stream {
 } __attribute__((packed));
 
 /*
- * Duplex stream configuration supported by SW/FW.
- */
-struct snd_soc_tplg_stream_config {
-	__le32 size;		/* in bytes of this structure */
-	char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
-	struct snd_soc_tplg_stream playback;
-	struct snd_soc_tplg_stream capture;
-} __attribute__((packed));
-
-/*
  * Manifest. List totals for each payload type. Not used in parsing, but will
  * be passed to the component driver before any other objects in order for any
  * global component resource allocations.
-- 
1.9.1

[PATCH v2 3/8] ASoC: topology: ABI - Use __le32 instead of __u32 in snd_soc_tplg_dapm_widget

[mengdong.lin]

From: Vedang Patel <vedang.patel@intel.com>

This fixes the endianness of the ABI parameters in the struct.
The field 'num_kcontrols' is also extended from 16 bits to 32 bits.

Signed-off-by: Vedang Patel <vedang.patel@intel.com>
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h
index 4bef63f..88210a8 100644
--- a/include/uapi/sound/asoc.h
+++ b/include/uapi/sound/asoc.h
@@ -356,11 +356,11 @@ struct snd_soc_tplg_dapm_widget {
 	__le32 shift;		/* bits to shift */
 	__le32 mask;		/* non-shifted mask */
 	__le32 subseq;		/* sort within widget type */
-	__u32 invert;		/* invert the power bit */
-	__u32 ignore_suspend;	/* kept enabled over suspend */
-	__u16 event_flags;
-	__u16 event_type;
-	__u16 num_kcontrols;
+	__le32 invert;		/* invert the power bit */
+	__le32 ignore_suspend;	/* kept enabled over suspend */
+	__le16 event_flags;
+	__le16 event_type;
+	__le32 num_kcontrols;
 	struct snd_soc_tplg_private priv;
 	/*
 	 * kcontrols that relate to this widget
-- 
1.9.1

[PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[mengdong.lin]

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.

Signed-off-by: Vedang Patel <vedang.patel@intel.com>
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

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 */
-- 
1.9.1

[PATCH v2 5/8] ASoC: topology: ABI - Change stream formats to a bitwise flag

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

The toplogy user space tool will generate this bitwise flag by using
SNDRV_PCM_FORMAT_* exposed by asound.h, and the topology core will copy
this flag when generating DAI streams.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
Acked-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>

diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h
index 2181480..eeb74a9c 100644
--- a/include/uapi/sound/asoc.h
+++ b/include/uapi/sound/asoc.h
@@ -198,7 +198,7 @@ struct snd_soc_tplg_ctl_hdr {
 struct snd_soc_tplg_stream_caps {
 	__le32 size;		/* in bytes of this structure */
 	char name[SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
-	__le64 formats[SND_SOC_TPLG_MAX_FORMATS];	/* supported formats SNDRV_PCM_FMTBIT_* */
+	__le64 formats;	/* supported formats SNDRV_PCM_FMTBIT_* */
 	__le32 rates;		/* supported rates SNDRV_PCM_RATE_* */
 	__le32 rate_min;	/* min rate */
 	__le32 rate_max;	/* max rate */
-- 
1.9.1

[PATCH v2 6/8] ASoC: topology: ABI - Add the type for BE DAI link

[mengdong.lin]

From: Vedang Patel <vedang.patel@intel.com>

Define the topology type for BE DAI link: SND_SOC_TPLG_TYPE_BACKEND_LINK.

Signed-off-by: Vedang Patel <vedang.patel@intel.com>
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h
index eeb74a9c..f75fd29 100644
--- a/include/uapi/sound/asoc.h
+++ b/include/uapi/sound/asoc.h
@@ -103,7 +103,8 @@
 #define SND_SOC_TPLG_TYPE_PCM		7
 #define SND_SOC_TPLG_TYPE_MANIFEST	8
 #define SND_SOC_TPLG_TYPE_CODEC_LINK	9
-#define SND_SOC_TPLG_TYPE_PDATA		10
+#define SND_SOC_TPLG_TYPE_BACKEND_LINK	10
+#define SND_SOC_TPLG_TYPE_PDATA		11
 #define SND_SOC_TPLG_TYPE_MAX	SND_SOC_TPLG_TYPE_PDATA
 
 /* vendor block IDs - please add new vendor types to end */
-- 
1.9.1

[PATCH v2 7/8] ASoC: topology: ABI - Add configuration for BE & Codec-Codec DAI Links

[mengdong.lin]

From: Vedang Patel <vedang.patel@intel.com>

struct snd_soc_tplg_link_config is defined to configure BE & CC links.

Signed-off-by: Vedang Patel <vedang.patel@intel.com>
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h
index f75fd29..538ea13 100644
--- a/include/uapi/sound/asoc.h
+++ b/include/uapi/sound/asoc.h
@@ -395,4 +395,21 @@ struct snd_soc_tplg_pcm {
 	struct snd_soc_tplg_stream_caps caps[2]; /* playback and capture for DAI */
 } __attribute__((packed));
 
+
+/*
+ * Describes the BE or CC link runtime supported configs or params
+ *
+ * File block representation for BE/CC link config :-
+ * +-----------------------------------+-----+
+ * | struct snd_soc_tplg_hdr           |  1  |
+ * +-----------------------------------+-----+
+ * | struct snd_soc_tplg_link_config   |  N  |
+ * +-----------------------------------+-----+
+ */
+struct snd_soc_tplg_link_config {
+	__le32 size;            /* in bytes of this structure */
+	__le32 id;              /* unique ID - used to match */
+	struct snd_soc_tplg_stream stream[SND_SOC_TPLG_STREAM_CONFIG_MAX]; /* supported configs playback and captrure */
+	__le32 num_streams;     /* number of streams */
+} __attribute__((packed));
 #endif
-- 
1.9.1

[PATCH v2 8/8] ASoC: topology: ABI - Remove tdm_slot & dai_fmt from snd_soc_tplg_stream

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

These two fields are line parameters for BE/CC links and
should not be from toplogy but from ACPI.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/include/uapi/sound/asoc.h b/include/uapi/sound/asoc.h
index 538ea13..26539a7 100644
--- a/include/uapi/sound/asoc.h
+++ b/include/uapi/sound/asoc.h
@@ -224,8 +224,6 @@ struct snd_soc_tplg_stream {
 	__le32 period_bytes;	/* size of period in bytes */
 	__le32 buffer_bytes;	/* size of buffer in bytes */
 	__le32 channels;	/* channels */
-	__le32 tdm_slot;	/* optional BE bitmask of supported TDM slots */
-	__le32 dai_fmt;		/* SND_SOC_DAIFMT_  */
 } __attribute__((packed));
 
 /*
-- 
1.9.1

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 500 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Liam Girdwood]

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

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Lin, Mengdong]

> -----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

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 571 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Lin, Mengdong]

> -----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

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Koul, Vinod]

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.

-- 
~Vinod

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 625 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Lars-Peter Clausen]

[-- Attachment #1.1: Type: text/plain, Size: 1130 bytes --]

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.



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1022 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Lin, Mengdong]

> -----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

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Takashi Iwai]

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

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 854 bytes --]

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.  

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v2 4/8] ASoC: topology: ABI - Add name element to snd_soc_tplg_stream

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 462 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]