Re: [PATCH 4/5] ASoC: dapm: Consolidate input and output path handling

[Mark Brown <broonie@kernel.org>]

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

On Sun, Jul 26, 2015 at 07:05:01PM +0200, Lars-Peter Clausen wrote:

> Testing on ARM shows that the combined code size of the specialized
> functions is about 50% larger than the generlized function in relative
> numbers. But in absolute numbers its less than 200 bytes, which is still
> quite small. On the other hand the generalized function increases the
> execution time of dapm_power_one_widget() by 30%. Given that this function
> is one of the most often called functions of the DAPM framework the
> tradeoff seems to be worth it.

This reads like you mean to say that it's worth it to combine things in
spite of the 30% speed hit...  you're not clear which tradeoff you mean.

> To avoid this still keep two versions of these functions around, one for
> input and one for output. But have a generic implementation of the
> algorithm which gets inlined by those two versions and then let the
> compiler take care of optimizing it into specialized versions.

...which the compiler presumably actually manages to do.

> --- a/include/sound/soc-dapm.h
> +++ b/include/sound/soc-dapm.h

> +/**
> + * snd_soc_dapm_widget_for_each_sink_path - Iterates over all paths in the
> + *   specified direction of a widget
> + * @w: The widget
> + * @dir: Whether to iterate over the paths where the specified widget is the
> + *       incoming or outgoing widgets
> + * @p: The path iterator variable
> + */
> +#define snd_soc_dapm_widget_for_each_path(w, dir, p) \
> +	list_for_each_entry(p, &w->edges[dir], list_node[dir])

This is a macro not a static inline?  The other macros that use this are
macros but that's more outdated code than anything I think, might be
worth fixing that while we're at it.

>  #define DAPM_UPDATE_STAT(widget, val) widget->dapm->card->dapm_stats.val++;
>  
> +#define dapm_widget_for_each_path_safe(w, dir, p, next_p) \
> +	list_for_each_entry_safe(p, next_p, &w->edges[dir], list_node[dir])

It's a bit odd not to have this in the header next to the unsafe
version.

> +#define SND_SOC_DAPM_DIR_REVERSE(x) ((x == SND_SOC_DAPM_DIR_IN) ? \
> +	SND_SOC_DAPM_DIR_OUT : SND_SOC_DAPM_DIR_IN)
> +
> +#define dapm_for_each_direction(dir) \
> +	for ((dir) = SND_SOC_DAPM_DIR_IN; (dir) <= SND_SOC_DAPM_DIR_OUT; \
> +		(dir)++)

Eew.

>  	list_for_each_entry(w, &card->widgets, list) {
> -		w->inputs = -1;
> -		w->outputs = -1;
> +		w->endpoints[SND_SOC_DAPM_DIR_IN] = -1;
> +		w->endpoints[SND_SOC_DAPM_DIR_OUT] = -1;

Loop over the endpoints array in some of the users like this perhaps?

> +	dapm_for_each_direction(dir) {
> +		rdir = SND_SOC_DAPM_DIR_REVERSE(dir);
> +		snd_soc_dapm_widget_for_each_path(w, dir, p) {
> +			if (p->connected && !p->connected(w, p->node[rdir]))
> +				continue;

Looks like dapm_for_each_direction should be
snd_soc_dapm_for_each_direction - the above looks wrong.

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

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