[PATCH v2 0/2] ASoC topology header parsing refinement

[PATCH v2 0/2] ASoC topology header parsing refinement

[Keyon Jie]

This small series is to optimize the header logging during the topology
parsing. This is verified work fine on both SOF and SST drivers
(bxt_rt298 with dfw_sst.bin).

Change History:
v2:
- Change the internal used array to be 'static' to fix the issue
  reported by: kbuild test robot <lkp@intel.com>
- Add testing coverage including Intel SST driver also.

v1:
- Initial version.

Keyon Jie (2):
  ASoC: topology: refine and log the header in the correct pass
  ASoC: topology: remove the redundant pass checks

 sound/soc/soc-topology.c | 99 ++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 55 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] ASoC: topology: refine and log the header in the correct pass

[Keyon Jie]

The check (tplg->pass == le32_to_cpu(hdr->type)) makes no sense as it is
comparing two different enums, refine the element loading functions, and
log the information when the header is being parsed in the corresponding
parsing pass.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
---
 sound/soc/soc-topology.c | 53 +++++++++++++++++++++++++++++++---------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 49875978a1ce..70c0ff7ce13f 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -2636,6 +2636,17 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg,
 	return ret;
 }
 
+static int (*elem_load[])(struct soc_tplg *, struct snd_soc_tplg_hdr *) = {
+	[SOC_TPLG_PASS_MANIFEST]	= soc_tplg_manifest_load,
+	[SOC_TPLG_PASS_VENDOR]		= soc_tplg_vendor_load,
+	[SOC_TPLG_PASS_MIXER]		= soc_tplg_kcontrol_elems_load,
+	[SOC_TPLG_PASS_WIDGET]		= soc_tplg_dapm_widget_elems_load,
+	[SOC_TPLG_PASS_PCM_DAI]		= soc_tplg_pcm_elems_load,
+	[SOC_TPLG_PASS_GRAPH]		= soc_tplg_dapm_graph_elems_load,
+	[SOC_TPLG_PASS_BE_DAI]		= soc_tplg_dai_elems_load,
+	[SOC_TPLG_PASS_LINK]		= soc_tplg_link_elems_load,
+};
+
 /* validate header magic, size and type */
 static int soc_valid_header(struct soc_tplg *tplg,
 	struct snd_soc_tplg_hdr *hdr)
@@ -2685,19 +2696,31 @@ static int soc_valid_header(struct soc_tplg *tplg,
 		return -EINVAL;
 	}
 
-	if (tplg->pass == le32_to_cpu(hdr->type))
+	return 1;
+}
+
+/* check and load the element for the current pass */
+static int soc_pass_load(struct soc_tplg *tplg,
+			 struct snd_soc_tplg_hdr *hdr,
+			 unsigned int hdr_pass)
+{
+	if (tplg->pass == hdr_pass) {
 		dev_dbg(tplg->dev,
 			"ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass %d\n",
 			hdr->payload_size, hdr->type, hdr->version,
 			hdr->vendor_type, tplg->pass);
+		return elem_load[hdr_pass](tplg, hdr);
+	}
 
-	return 1;
+	return 0;
 }
 
 /* check header type and call appropriate handler */
 static int soc_tplg_load_header(struct soc_tplg *tplg,
 	struct snd_soc_tplg_hdr *hdr)
 {
+	unsigned int hdr_pass;
+
 	tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);
 
 	/* check for matching ID */
@@ -2711,27 +2734,35 @@ static int soc_tplg_load_header(struct soc_tplg *tplg,
 	case SND_SOC_TPLG_TYPE_MIXER:
 	case SND_SOC_TPLG_TYPE_ENUM:
 	case SND_SOC_TPLG_TYPE_BYTES:
-		return soc_tplg_kcontrol_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_MIXER;
+		break;
 	case SND_SOC_TPLG_TYPE_DAPM_GRAPH:
-		return soc_tplg_dapm_graph_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_GRAPH;
+		break;
 	case SND_SOC_TPLG_TYPE_DAPM_WIDGET:
-		return soc_tplg_dapm_widget_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_WIDGET;
+		break;
 	case SND_SOC_TPLG_TYPE_PCM:
-		return soc_tplg_pcm_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_PCM_DAI;
+		break;
 	case SND_SOC_TPLG_TYPE_DAI:
-		return soc_tplg_dai_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_BE_DAI;
+		break;
 	case SND_SOC_TPLG_TYPE_DAI_LINK:
 	case SND_SOC_TPLG_TYPE_BACKEND_LINK:
 		/* physical link configurations */
-		return soc_tplg_link_elems_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_LINK;
+		break;
 	case SND_SOC_TPLG_TYPE_MANIFEST:
-		return soc_tplg_manifest_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_MANIFEST;
+		break;
 	default:
 		/* bespoke vendor data object */
-		return soc_tplg_vendor_load(tplg, hdr);
+		hdr_pass = SOC_TPLG_PASS_VENDOR;
+		break;
 	}
 
-	return 0;
+	return soc_pass_load(tplg, hdr, hdr_pass);
 }
 
 /* process the topology file headers */
-- 
2.25.1

Re: [PATCH v2 1/2] ASoC: topology: refine and log the header in the correct pass

[Cezary Rojewski]

On 2020-05-21 9:48 AM, Keyon Jie wrote:
> The check (tplg->pass == le32_to_cpu(hdr->type)) makes no sense as it is
> comparing two different enums, refine the element loading functions, and
> log the information when the header is being parsed in the corresponding
> parsing pass.
> 
> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
> ---
>   sound/soc/soc-topology.c | 53 +++++++++++++++++++++++++++++++---------
>   1 file changed, 42 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 49875978a1ce..70c0ff7ce13f 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -2636,6 +2636,17 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg,
>   	return ret;
>   }
>   
> +static int (*elem_load[])(struct soc_tplg *, struct snd_soc_tplg_hdr *) = {
> +	[SOC_TPLG_PASS_MANIFEST]	= soc_tplg_manifest_load,
> +	[SOC_TPLG_PASS_VENDOR]		= soc_tplg_vendor_load,
> +	[SOC_TPLG_PASS_MIXER]		= soc_tplg_kcontrol_elems_load,
> +	[SOC_TPLG_PASS_WIDGET]		= soc_tplg_dapm_widget_elems_load,
> +	[SOC_TPLG_PASS_PCM_DAI]		= soc_tplg_pcm_elems_load,
> +	[SOC_TPLG_PASS_GRAPH]		= soc_tplg_dapm_graph_elems_load,
> +	[SOC_TPLG_PASS_BE_DAI]		= soc_tplg_dai_elems_load,
> +	[SOC_TPLG_PASS_LINK]		= soc_tplg_link_elems_load,
> +};
> +

Do we really need private array for this? There is a separate 
soc_tplg_load_header function already. By separate, I mean an isolate 
scope for single task only - validating tplg headers. While I agree with 
the idea behind second patch of the series - removal of individual 
checks in favor of generic one - I believe said checks could be 
"inlined". See below.

>   /* validate header magic, size and type */
>   static int soc_valid_header(struct soc_tplg *tplg,
>   	struct snd_soc_tplg_hdr *hdr)
> @@ -2685,19 +2696,31 @@ static int soc_valid_header(struct soc_tplg *tplg,
>   		return -EINVAL;
>   	}
>   
> -	if (tplg->pass == le32_to_cpu(hdr->type))
> +	return 1;
> +}
> +
> +/* check and load the element for the current pass */
> +static int soc_pass_load(struct soc_tplg *tplg,
> +			 struct snd_soc_tplg_hdr *hdr,
> +			 unsigned int hdr_pass)
> +{
> +	if (tplg->pass == hdr_pass) {
>   		dev_dbg(tplg->dev,
>   			"ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass %d\n",
>   			hdr->payload_size, hdr->type, hdr->version,
>   			hdr->vendor_type, tplg->pass);
> +		return elem_load[hdr_pass](tplg, hdr);
> +	}
>   
> -	return 1;
> +	return 0;
>   }

Remove the function and merge its body into soc_tplg_load_header.

>   
>   /* check header type and call appropriate handler */
>   static int soc_tplg_load_header(struct soc_tplg *tplg,
>   	struct snd_soc_tplg_hdr *hdr)
>   {
> +	unsigned int hdr_pass;
> +
>   	tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);
>   
>   	/* check for matching ID */
> @@ -2711,27 +2734,35 @@ static int soc_tplg_load_header(struct soc_tplg *tplg,
>   	case SND_SOC_TPLG_TYPE_MIXER:
>   	case SND_SOC_TPLG_TYPE_ENUM:
>   	case SND_SOC_TPLG_TYPE_BYTES:
> -		return soc_tplg_kcontrol_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_MIXER;
> +		break;
>   	case SND_SOC_TPLG_TYPE_DAPM_GRAPH:
> -		return soc_tplg_dapm_graph_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_GRAPH;
> +		break;
>   	case SND_SOC_TPLG_TYPE_DAPM_WIDGET:
> -		return soc_tplg_dapm_widget_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_WIDGET;
> +		break;
>   	case SND_SOC_TPLG_TYPE_PCM:
> -		return soc_tplg_pcm_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_PCM_DAI;
> +		break;
>   	case SND_SOC_TPLG_TYPE_DAI:
> -		return soc_tplg_dai_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_BE_DAI;
> +		break;
>   	case SND_SOC_TPLG_TYPE_DAI_LINK:
>   	case SND_SOC_TPLG_TYPE_BACKEND_LINK:
>   		/* physical link configurations */
> -		return soc_tplg_link_elems_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_LINK;
> +		break;
>   	case SND_SOC_TPLG_TYPE_MANIFEST:
> -		return soc_tplg_manifest_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_MANIFEST;
> +		break;
>   	default:
>   		/* bespoke vendor data object */
> -		return soc_tplg_vendor_load(tplg, hdr);
> +		hdr_pass = SOC_TPLG_PASS_VENDOR;
> +		break;
>   	}
>   
> -	return 0;
> +	return soc_pass_load(tplg, hdr, hdr_pass);

By having "log" code here we have one place for hdr validation, rather 
than two (the second being just an "if" to be fair..) and private array 
is no longer necessary. Local func ptr variable would take care of 
storing adequate function to call.

>   }
>   
>   /* process the topology file headers */
> 

Re: [PATCH v2 1/2] ASoC: topology: refine and log the header in the correct pass

[Keyon Jie]

On 5/26/20 8:38 PM, Cezary Rojewski wrote:
> On 2020-05-21 9:48 AM, Keyon Jie wrote:
>> The check (tplg->pass == le32_to_cpu(hdr->type)) makes no sense as it is
>> comparing two different enums, refine the element loading functions, and
>> log the information when the header is being parsed in the corresponding
>> parsing pass.
>>
>> Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
>> ---
>>   sound/soc/soc-topology.c | 53 +++++++++++++++++++++++++++++++---------
>>   1 file changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
>> index 49875978a1ce..70c0ff7ce13f 100644
>> --- a/sound/soc/soc-topology.c
>> +++ b/sound/soc/soc-topology.c
>> @@ -2636,6 +2636,17 @@ static int soc_tplg_manifest_load(struct 
>> soc_tplg *tplg,
>>       return ret;
>>   }
>> +static int (*elem_load[])(struct soc_tplg *, struct snd_soc_tplg_hdr 
>> *) = {
>> +    [SOC_TPLG_PASS_MANIFEST]    = soc_tplg_manifest_load,
>> +    [SOC_TPLG_PASS_VENDOR]        = soc_tplg_vendor_load,
>> +    [SOC_TPLG_PASS_MIXER]        = soc_tplg_kcontrol_elems_load,
>> +    [SOC_TPLG_PASS_WIDGET]        = soc_tplg_dapm_widget_elems_load,
>> +    [SOC_TPLG_PASS_PCM_DAI]        = soc_tplg_pcm_elems_load,
>> +    [SOC_TPLG_PASS_GRAPH]        = soc_tplg_dapm_graph_elems_load,
>> +    [SOC_TPLG_PASS_BE_DAI]        = soc_tplg_dai_elems_load,
>> +    [SOC_TPLG_PASS_LINK]        = soc_tplg_link_elems_load,
>> +};
>> +
> 
> Do we really need private array for this? There is a separate 
> soc_tplg_load_header function already. By separate, I mean an isolate 
> scope for single task only - validating tplg headers. While I agree with 
> the idea behind second patch of the series - removal of individual 
> checks in favor of generic one - I believe said checks could be 
> "inlined". See below.
> 
>>   /* validate header magic, size and type */
>>   static int soc_valid_header(struct soc_tplg *tplg,
>>       struct snd_soc_tplg_hdr *hdr)
>> @@ -2685,19 +2696,31 @@ static int soc_valid_header(struct soc_tplg 
>> *tplg,
>>           return -EINVAL;
>>       }
>> -    if (tplg->pass == le32_to_cpu(hdr->type))
>> +    return 1;
>> +}
>> +
>> +/* check and load the element for the current pass */
>> +static int soc_pass_load(struct soc_tplg *tplg,
>> +             struct snd_soc_tplg_hdr *hdr,
>> +             unsigned int hdr_pass)
>> +{
>> +    if (tplg->pass == hdr_pass) {
>>           dev_dbg(tplg->dev,
>>               "ASoC: Got 0x%x bytes of type %d version %d vendor %d at 
>> pass %d\n",
>>               hdr->payload_size, hdr->type, hdr->version,
>>               hdr->vendor_type, tplg->pass);
>> +        return elem_load[hdr_pass](tplg, hdr);
>> +    }
>> -    return 1;
>> +    return 0;
>>   }
> 
> Remove the function and merge its body into soc_tplg_load_header.
> 
>>   /* check header type and call appropriate handler */we
>>   static int soc_tplg_load_header(struct soc_tplg *tplg,
>>       struct snd_soc_tplg_hdr *hdr)
>>   {
>> +    unsigned int hdr_pass;
>> +
>>       tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);
>>       /* check for matching ID */
>> @@ -2711,27 +2734,35 @@ static int soc_tplg_load_header(struct 
>> soc_tplg *tplg,
>>       case SND_SOC_TPLG_TYPE_MIXER:
>>       case SND_SOC_TPLG_TYPE_ENUM:
>>       case SND_SOC_TPLG_TYPE_BYTES:
>> -        return soc_tplg_kcontrol_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_MIXER;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_DAPM_GRAPH:
>> -        return soc_tplg_dapm_graph_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_GRAPH;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_DAPM_WIDGET:
>> -        return soc_tplg_dapm_widget_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_WIDGET;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_PCM:
>> -        return soc_tplg_pcm_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_PCM_DAI;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_DAI:
>> -        return soc_tplg_dai_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_BE_DAI;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_DAI_LINK:
>>       case SND_SOC_TPLG_TYPE_BACKEND_LINK:
>>           /* physical link configurations */
>> -        return soc_tplg_link_elems_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_LINK;
>> +        break;
>>       case SND_SOC_TPLG_TYPE_MANIFEST:
>> -        return soc_tplg_manifest_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_MANIFEST;
>> +        break;
>>       default:
>>           /* bespoke vendor data object */
>> -        return soc_tplg_vendor_load(tplg, hdr);
>> +        hdr_pass = SOC_TPLG_PASS_VENDOR;
>> +        break;
>>       }
>> -    return 0;
>> +    return soc_pass_load(tplg, hdr, hdr_pass);
> 
> By having "log" code here we have one place for hdr validation, rather 
> than two (the second being just an "if" to be fair..) and private array 
> is no longer necessary. Local func ptr variable would take care of 
> storing adequate function to call.

Hi Cezary, so what you suggested above is changing the 
soc_tplg_load_header() to be something like this, right?


static int soc_tplg_load_header(struct soc_tplg *tplg,
	struct snd_soc_tplg_hdr *hdr)
{
	unsigned int hdr_pass;
	int (*elem_load)(struct soc_tplg *, struct snd_soc_tplg_hdr *);

	tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);

	/* check for matching ID */
	if (le32_to_cpu(hdr->index) != tplg->req_index &&
		tplg->req_index != SND_SOC_TPLG_INDEX_ALL)
		return 0;

	tplg->index = le32_to_cpu(hdr->index);

	switch (le32_to_cpu(hdr->type)) {
	case SND_SOC_TPLG_TYPE_MIXER:
	case SND_SOC_TPLG_TYPE_ENUM:
	case SND_SOC_TPLG_TYPE_BYTES:
		hdr_pass = SOC_TPLG_PASS_MIXER;
		elem_load = soc_tplg_kcontrol_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_DAPM_GRAPH:
		hdr_pass = SOC_TPLG_PASS_GRAPH;
		elem_load = soc_tplg_dapm_graph_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_DAPM_WIDGET:
		hdr_pass = SOC_TPLG_PASS_WIDGET;
		elem_load = soc_tplg_dapm_widget_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_PCM:
		hdr_pass = SOC_TPLG_PASS_PCM_DAI;
		elem_load = soc_tplg_pcm_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_DAI:
		hdr_pass = SOC_TPLG_PASS_BE_DAI;
		elem_load = soc_tplg_dai_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_DAI_LINK:
	case SND_SOC_TPLG_TYPE_BACKEND_LINK:
		/* physical link configurations */
		hdr_pass = SOC_TPLG_PASS_LINK;
		elem_load = soc_tplg_link_elems_load;
		break;
	case SND_SOC_TPLG_TYPE_MANIFEST:
		hdr_pass = SOC_TPLG_PASS_MANIFEST;
		elem_load = soc_tplg_manifest_load;
		break;
	default:
		/* bespoke vendor data object */
		hdr_pass = SOC_TPLG_PASS_VENDOR;
		elem_load = soc_tplg_vendor_load;
		break;
	}

	if (tplg->pass == hdr_pass) {
		dev_dbg(tplg->dev,
			"ASoC: Got 0x%x bytes of type %d version %d vendor %d at pass %d\n",
			hdr->payload_size, hdr->type, hdr->version,
			hdr->vendor_type, tplg->pass);
		return elem_load(tplg, hdr);
	}

	return 0;
}


I am also fine with this, though I thought my previous version looks 
more organized and not so error-prone as we need 8 more assignation here.

Mark, Pierre, preference about this?

Thanks,
~Keyon

> 
>>   }
>>   /* process the topology file headers */
>>

Re: [PATCH v2 1/2] ASoC: topology: refine and log the header in the correct pass

[Cezary Rojewski]

On 2020-05-26 4:45 PM, Keyon Jie wrote:
> On 5/26/20 8:38 PM, Cezary Rojewski wrote:
>> On 2020-05-21 9:48 AM, Keyon Jie wrote:

>> By having "log" code here we have one place for hdr validation, rather 
>> than two (the second being just an "if" to be fair..) and private 
>> array is no longer necessary. Local func ptr variable would take care 
>> of storing adequate function to call.
> 
> Hi Cezary, so what you suggested above is changing the 
> soc_tplg_load_header() to be something like this, right?
> 
> 
> static int soc_tplg_load_header(struct soc_tplg *tplg,
>      struct snd_soc_tplg_hdr *hdr)
> {
>      unsigned int hdr_pass;
>      int (*elem_load)(struct soc_tplg *, struct snd_soc_tplg_hdr *);
> 
>      tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);
> 
>      /* check for matching ID */
>      if (le32_to_cpu(hdr->index) != tplg->req_index &&
>          tplg->req_index != SND_SOC_TPLG_INDEX_ALL)
>          return 0;
> 
>      tplg->index = le32_to_cpu(hdr->index);
> 
>      switch (le32_to_cpu(hdr->type)) {
>      case SND_SOC_TPLG_TYPE_MIXER:
>      case SND_SOC_TPLG_TYPE_ENUM:
>      case SND_SOC_TPLG_TYPE_BYTES:
>          hdr_pass = SOC_TPLG_PASS_MIXER;
>          elem_load = soc_tplg_kcontrol_elems_load;
>          break;
>      case SND_SOC_TPLG_TYPE_DAPM_GRAPH:
>          hdr_pass = SOC_TPLG_PASS_GRAPH;
>          elem_load = soc_tplg_dapm_graph_elems_load;
>          break;
>      case SND_SOC_TPLG_TYPE_DAPM_WIDGET:
>          hdr_pass = SOC_TPLG_PASS_WIDGET;
>          elem_load = soc_tplg_dapm_widget_elems_load;
>          break;
>      case SND_SOC_TPLG_TYPE_PCM:
>          hdr_pass = SOC_TPLG_PASS_PCM_DAI;
>          elem_load = soc_tplg_pcm_elems_load;
>          break;
>      case SND_SOC_TPLG_TYPE_DAI:
>          hdr_pass = SOC_TPLG_PASS_BE_DAI;
>          elem_load = soc_tplg_dai_elems_load;
>          break;
>      case SND_SOC_TPLG_TYPE_DAI_LINK:
>      case SND_SOC_TPLG_TYPE_BACKEND_LINK:
>          /* physical link configurations */
>          hdr_pass = SOC_TPLG_PASS_LINK;
>          elem_load = soc_tplg_link_elems_load;
>          break;
>      case SND_SOC_TPLG_TYPE_MANIFEST:
>          hdr_pass = SOC_TPLG_PASS_MANIFEST;
>          elem_load = soc_tplg_manifest_load;
>          break;
>      default:
>          /* bespoke vendor data object */
>          hdr_pass = SOC_TPLG_PASS_VENDOR;
>          elem_load = soc_tplg_vendor_load;
>          break;
>      }
> 
>      if (tplg->pass == hdr_pass) {
>          dev_dbg(tplg->dev,
>              "ASoC: Got 0x%x bytes of type %d version %d vendor %d at 
> pass %d\n",
>              hdr->payload_size, hdr->type, hdr->version,
>              hdr->vendor_type, tplg->pass);
>          return elem_load(tplg, hdr);
>      }
> 
>      return 0;
> }
> 
> 
> I am also fine with this, though I thought my previous version looks 
> more organized and not so error-prone as we need 8 more assignation here.
> 
> Mark, Pierre, preference about this?
> 
> Thanks,
> ~Keyon
> 

Another option, if you want to reduce assignment count, is to keep 
soc_pass_load while still removing the private map. Said soc_pass_load 
would require declaration update to accept function ptr on top of what's 
already there.

In consequence, soc_tplg_load_header becomes a switch-case with a bunch of
case X:
	return soc_pass_load(tplg, hdr,
			pass=_MY_PASS
				(e.g. SOC_TPLG_PASS_VENDOR),
			elem_load=_MY_LOAD_FUNC
				(e.g. soc_tplg_vendor_load))

Czarek

Re: [PATCH v2 1/2] ASoC: topology: refine and log the header in the correct pass

[Keyon Jie]

On 5/26/20 11:30 PM, Cezary Rojewski wrote:
> On 2020-05-26 4:45 PM, Keyon Jie wrote:
>> On 5/26/20 8:38 PM, Cezary Rojewski wrote:
>>> On 2020-05-21 9:48 AM, Keyon Jie wrote:
> 
>>> By having "log" code here we have one place for hdr validation, 
>>> rather than two (the second being just an "if" to be fair..) and 
>>> private array is no longer necessary. Local func ptr variable would 
>>> take care of storing adequate function to call.
>>
>> Hi Cezary, so what you suggested above is changing the 
>> soc_tplg_load_header() to be something like this, right?
>>
>>
>> static int soc_tplg_load_header(struct soc_tplg *tplg,
>>      struct snd_soc_tplg_hdr *hdr)
>> {
>>      unsigned int hdr_pass;
>>      int (*elem_load)(struct soc_tplg *, struct snd_soc_tplg_hdr *);
>>
>>      tplg->pos = tplg->hdr_pos + sizeof(struct snd_soc_tplg_hdr);
>>
>>      /* check for matching ID */
>>      if (le32_to_cpu(hdr->index) != tplg->req_index &&
>>          tplg->req_index != SND_SOC_TPLG_INDEX_ALL)
>>          return 0;
>>
>>      tplg->index = le32_to_cpu(hdr->index);
>>
>>      switch (le32_to_cpu(hdr->type)) {
>>      case SND_SOC_TPLG_TYPE_MIXER:
>>      case SND_SOC_TPLG_TYPE_ENUM:
>>      case SND_SOC_TPLG_TYPE_BYTES:
>>          hdr_pass = SOC_TPLG_PASS_MIXER;
>>          elem_load = soc_tplg_kcontrol_elems_load;
>>          break;
>>      case SND_SOC_TPLG_TYPE_DAPM_GRAPH:
>>          hdr_pass = SOC_TPLG_PASS_GRAPH;
>>          elem_load = soc_tplg_dapm_graph_elems_load;
>>          break;
>>      case SND_SOC_TPLG_TYPE_DAPM_WIDGET:
>>          hdr_pass = SOC_TPLG_PASS_WIDGET;
>>          elem_load = soc_tplg_dapm_widget_elems_load;
>>          break;
>>      case SND_SOC_TPLG_TYPE_PCM:
>>          hdr_pass = SOC_TPLG_PASS_PCM_DAI;
>>          elem_load = soc_tplg_pcm_elems_load;
>>          break;
>>      case SND_SOC_TPLG_TYPE_DAI:
>>          hdr_pass = SOC_TPLG_PASS_BE_DAI;
>>          elem_load = soc_tplg_dai_elems_load;
>>          break;
>>      case SND_SOC_TPLG_TYPE_DAI_LINK:
>>      case SND_SOC_TPLG_TYPE_BACKEND_LINK:
>>          /* physical link configurations */
>>          hdr_pass = SOC_TPLG_PASS_LINK;
>>          elem_load = soc_tplg_link_elems_load;
>>          break;
>>      case SND_SOC_TPLG_TYPE_MANIFEST:
>>          hdr_pass = SOC_TPLG_PASS_MANIFEST;
>>          elem_load = soc_tplg_manifest_load;
>>          break;
>>      default:
>>          /* bespoke vendor data object */
>>          hdr_pass = SOC_TPLG_PASS_VENDOR;
>>          elem_load = soc_tplg_vendor_load;
>>          break;
>>      }
>>
>>      if (tplg->pass == hdr_pass) {
>>          dev_dbg(tplg->dev,
>>              "ASoC: Got 0x%x bytes of type %d version %d vendor %d at 
>> pass %d\n",
>>              hdr->payload_size, hdr->type, hdr->version,
>>              hdr->vendor_type, tplg->pass);
>>          return elem_load(tplg, hdr);
>>      }
>>
>>      return 0;
>> }
>>
>>
>> I am also fine with this, though I thought my previous version looks 
>> more organized and not so error-prone as we need 8 more assignation here.
>>
>> Mark, Pierre, preference about this?
>>
>> Thanks,
>> ~Keyon
>>
> 
> Another option, if you want to reduce assignment count, is to keep 
> soc_pass_load while still removing the private map. Said soc_pass_load 
> would require declaration update to accept function ptr on top of what's 
> already there.
> 
> In consequence, soc_tplg_load_header becomes a switch-case with a bunch of
> case X:
>      return soc_pass_load(tplg, hdr,
>              pass=_MY_PASS
>                  (e.g. SOC_TPLG_PASS_VENDOR),
>              elem_load=_MY_LOAD_FUNC
>                  (e.g. soc_tplg_vendor_load))

I would prefer to use the assignment one, let me update the series now.

Thanks,
~Keyon

> 
> Czarek

[PATCH v2 2/2] ASoC: topology: remove the redundant pass checks

[Keyon Jie]

As we have check the 'pass' in the soc_elem_pass_load(), so no need to
check it again in each specific elem_load function, at the same time,
the tplg->pos will be reset to the next header base when the pass is
mismatched, so the increasing of the tplg->pos in these cases made no
sense. Here remove all of them.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
---
 sound/soc/soc-topology.c | 46 ++--------------------------------------
 1 file changed, 2 insertions(+), 44 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 70c0ff7ce13f..6f4269ffd84f 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -246,8 +246,8 @@ static inline void soc_control_err(struct soc_tplg *tplg,
 }
 
 /* pass vendor data to component driver for processing */
-static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
-	struct snd_soc_tplg_hdr *hdr)
+static int soc_tplg_vendor_load(struct soc_tplg *tplg,
+				struct snd_soc_tplg_hdr *hdr)
 {
 	int ret = 0;
 
@@ -268,16 +268,6 @@ static int soc_tplg_vendor_load_(struct soc_tplg *tplg,
 	return ret;
 }
 
-/* pass vendor data to component driver for processing */
-static int soc_tplg_vendor_load(struct soc_tplg *tplg,
-	struct snd_soc_tplg_hdr *hdr)
-{
-	if (tplg->pass != SOC_TPLG_PASS_VENDOR)
-		return 0;
-
-	return soc_tplg_vendor_load_(tplg, hdr);
-}
-
 /* optionally pass new dynamic widget to component driver. This is mainly for
  * external widgets where we can assign private data/ops */
 static int soc_tplg_widget_load(struct soc_tplg *tplg,
@@ -1127,12 +1117,6 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg,
 	int ret;
 	int i;
 
-	if (tplg->pass != SOC_TPLG_PASS_MIXER) {
-		tplg->pos += le32_to_cpu(hdr->size) +
-			le32_to_cpu(hdr->payload_size);
-		return 0;
-	}
-
 	dev_dbg(tplg->dev, "ASoC: adding %d kcontrols at 0x%lx\n", hdr->count,
 		soc_tplg_get_offset(tplg));
 
@@ -1204,14 +1188,6 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
 
 	count = le32_to_cpu(hdr->count);
 
-	if (tplg->pass != SOC_TPLG_PASS_GRAPH) {
-		tplg->pos +=
-			le32_to_cpu(hdr->size) +
-			le32_to_cpu(hdr->payload_size);
-
-		return 0;
-	}
-
 	if (soc_tplg_check_elem_count(tplg,
 		sizeof(struct snd_soc_tplg_dapm_graph_elem),
 		count, le32_to_cpu(hdr->payload_size), "graph")) {
@@ -1741,9 +1717,6 @@ static int soc_tplg_dapm_widget_elems_load(struct soc_tplg *tplg,
 
 	count = le32_to_cpu(hdr->count);
 
-	if (tplg->pass != SOC_TPLG_PASS_WIDGET)
-		return 0;
-
 	dev_dbg(tplg->dev, "ASoC: adding %d DAPM widgets\n", count);
 
 	for (i = 0; i < count; i++) {
@@ -2101,9 +2074,6 @@ static int soc_tplg_pcm_elems_load(struct soc_tplg *tplg,
 
 	count = le32_to_cpu(hdr->count);
 
-	if (tplg->pass != SOC_TPLG_PASS_PCM_DAI)
-		return 0;
-
 	/* check the element size and count */
 	pcm = (struct snd_soc_tplg_pcm *)tplg->pos;
 	size = le32_to_cpu(pcm->size);
@@ -2386,12 +2356,6 @@ static int soc_tplg_link_elems_load(struct soc_tplg *tplg,
 
 	count = le32_to_cpu(hdr->count);
 
-	if (tplg->pass != SOC_TPLG_PASS_LINK) {
-		tplg->pos += le32_to_cpu(hdr->size) +
-			le32_to_cpu(hdr->payload_size);
-		return 0;
-	}
-
 	/* check the element size and count */
 	link = (struct snd_soc_tplg_link_config *)tplg->pos;
 	size = le32_to_cpu(link->size);
@@ -2528,9 +2492,6 @@ static int soc_tplg_dai_elems_load(struct soc_tplg *tplg,
 
 	count = le32_to_cpu(hdr->count);
 
-	if (tplg->pass != SOC_TPLG_PASS_BE_DAI)
-		return 0;
-
 	/* config the existing BE DAIs */
 	for (i = 0; i < count; i++) {
 		dai = (struct snd_soc_tplg_dai *)tplg->pos;
@@ -2610,9 +2571,6 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg,
 	bool abi_match;
 	int ret = 0;
 
-	if (tplg->pass != SOC_TPLG_PASS_MANIFEST)
-		return 0;
-
 	manifest = (struct snd_soc_tplg_manifest *)tplg->pos;
 
 	/* check ABI version by size, create a new manifest if abi not match */
-- 
2.25.1

Re: [PATCH v2 0/2] ASoC topology header parsing refinement

[Cezary Rojewski]

On 2020-05-21 9:48 AM, Keyon Jie wrote:
> This small series is to optimize the header logging during the topology
> parsing. This is verified work fine on both SOF and SST drivers
> (bxt_rt298 with dfw_sst.bin).
> 
> Change History:
> v2:
> - Change the internal used array to be 'static' to fix the issue
>    reported by: kbuild test robot <lkp@intel.com>
> - Add testing coverage including Intel SST driver also.
> 
> v1:
> - Initial version.
> 
> Keyon Jie (2):
>    ASoC: topology: refine and log the header in the correct pass
>    ASoC: topology: remove the redundant pass checks
> 
>   sound/soc/soc-topology.c | 99 ++++++++++++++++++----------------------
>   1 file changed, 44 insertions(+), 55 deletions(-)
> 

No regression after applying these and running through SST CI on cAVS 
1.5+ (GLK) and 1.8 (CNL).

Given some comments on 1/2 though, believe it could be simplified.

Czarek

Re: [PATCH v2 0/2] ASoC topology header parsing refinement

[Pierre-Louis Bossart]

On 5/26/20 7:40 AM, Cezary Rojewski wrote:
> On 2020-05-21 9:48 AM, Keyon Jie wrote:
>> This small series is to optimize the header logging during the topology
>> parsing. This is verified work fine on both SOF and SST drivers
>> (bxt_rt298 with dfw_sst.bin).
>>
>> Change History:
>> v2:
>> - Change the internal used array to be 'static' to fix the issue
>>    reported by: kbuild test robot <lkp@intel.com>
>> - Add testing coverage including Intel SST driver also.
>>
>> v1:
>> - Initial version.
>>
>> Keyon Jie (2):
>>    ASoC: topology: refine and log the header in the correct pass
>>    ASoC: topology: remove the redundant pass checks
>>
>>   sound/soc/soc-topology.c | 99 ++++++++++++++++++----------------------
>>   1 file changed, 44 insertions(+), 55 deletions(-)
>>
> 
> No regression after applying these and running through SST CI on cAVS 
> 1.5+ (GLK) and 1.8 (CNL).

Can the tests be run on SKL/KBL? That's where the cAVS driver will be 
used and where this patchset will have an impact.
Thanks!

> Given some comments on 1/2 though, believe it could be simplified.
> 
> Czarek

Re: [PATCH v2 0/2] ASoC topology header parsing refinement

[Keyon Jie]

On 5/26/20 9:29 PM, Pierre-Louis Bossart wrote:
> 
> 
> On 5/26/20 7:40 AM, Cezary Rojewski wrote:
>> On 2020-05-21 9:48 AM, Keyon Jie wrote:
>>> This small series is to optimize the header logging during the topology
>>> parsing. This is verified work fine on both SOF and SST drivers
>>> (bxt_rt298 with dfw_sst.bin).
>>>
>>> Change History:
>>> v2:
>>> - Change the internal used array to be 'static' to fix the issue
>>>    reported by: kbuild test robot <lkp@intel.com>
>>> - Add testing coverage including Intel SST driver also.
>>>
>>> v1:
>>> - Initial version.
>>>
>>> Keyon Jie (2):
>>>    ASoC: topology: refine and log the header in the correct pass
>>>    ASoC: topology: remove the redundant pass checks
>>>
>>>   sound/soc/soc-topology.c | 99 ++++++++++++++++++----------------------
>>>   1 file changed, 44 insertions(+), 55 deletions(-)
>>>
>>
>> No regression after applying these and running through SST CI on cAVS 
>> 1.5+ (GLK) and 1.8 (CNL).
> 
> Can the tests be run on SKL/KBL? That's where the cAVS driver will be 
> used and where this patchset will have an impact.
> Thanks!

Yes, they were just tested on KBL Chrome book with cAVS driver by Vamshi 
and Sathya.

Tested-by: Vamshi Kerishna Gopal <vamshi.krishna.gopal@intel.com>

Thanks,
~Keyon

> 
>> Given some comments on 1/2 though, believe it could be simplified.
>>
>> Czarek

Re: [PATCH v2 0/2] ASoC topology header parsing refinement

[Keyon Jie]

On 5/26/20 8:40 PM, Cezary Rojewski wrote:
> On 2020-05-21 9:48 AM, Keyon Jie wrote:
>> This small series is to optimize the header logging during the topology
>> parsing. This is verified work fine on both SOF and SST drivers
>> (bxt_rt298 with dfw_sst.bin).
>>
>> Change History:
>> v2:
>> - Change the internal used array to be 'static' to fix the issue
>>    reported by: kbuild test robot <lkp@intel.com>
>> - Add testing coverage including Intel SST driver also.
>>
>> v1:
>> - Initial version.
>>
>> Keyon Jie (2):
>>    ASoC: topology: refine and log the header in the correct pass
>>    ASoC: topology: remove the redundant pass checks
>>
>>   sound/soc/soc-topology.c | 99 ++++++++++++++++++----------------------
>>   1 file changed, 44 insertions(+), 55 deletions(-)
>>
> 
> No regression after applying these and running through SST CI on cAVS 
> 1.5+ (GLK) and 1.8 (CNL).

Thanks a lot for the validation.

> 
> Given some comments on 1/2 though, believe it could be simplified.

reply on the 1/2 commit.

~Keyon

> 
> Czarek