[PATCH 0/3] ASoC: dapm-graph: add component on/off and route names to graph

[PATCH 0/3] ASoC: dapm-graph: add component on/off and route names to graph

[Luca Ceresoli]

This small series adds some improvements to dapm-graph in order to produce
a more correct and informative graph.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (3):
      ASoC: dapm-graph: remove the "ROOT" cluster
      ASoC: dapm-graph: visualize component On/Off bias level
      ASoC: dapm-graph: show path name for non-static routes

 tools/sound/dapm-graph | 44 +++++++++++++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 9 deletions(-)
---
base-commit: c3f38fa61af77b49866b006939479069cd451173
change-id: 20240603-dapm-graph-8e3f7e3fd692

Best regards,
-- 
Luca Ceresoli <luca.ceresoli@bootlin.com>

[PATCH 1/3] ASoC: dapm-graph: remove the "ROOT" cluster

[Luca Ceresoli]

Widgets not belonging to any component are currently represented inside a
cluster labeled "ROOT". This is not a correct representation of the actual
structure, as these widgets are not necessarily related to each other as
the ones inside actual components are.

Improve the graphical representation by not adding a cluster around these
widgets. Now a dot cluster represents a card component faithfully. This
will be particularly important with the upcoming improvements which will
visualize the component bias_level.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 tools/sound/dapm-graph | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/tools/sound/dapm-graph b/tools/sound/dapm-graph
index 57d78f6df041..205783d124d3 100755
--- a/tools/sound/dapm-graph
+++ b/tools/sound/dapm-graph
@@ -150,29 +150,32 @@ process_dapm_widget()
 #
 # $1 = temporary work dir
 # $2 = component directory
-# $3 = forced component name (extracted for path if empty)
+# $3 = "ROOT" for the root card directory, empty otherwise
 process_dapm_component()
 {
     local tmp_dir="${1}"
     local c_dir="${2}"
     local c_name="${3}"
+    local is_component=0
     local dot_file="${tmp_dir}/main.dot"
     local links_file="${tmp_dir}/links.dot"
 
     if [ -z "${c_name}" ]; then
+	is_component=1
+
 	# Extract directory name into component name:
 	#   "./cs42l51.0-004a/dapm" -> "cs42l51.0-004a"
 	c_name="$(basename $(dirname "${c_dir}"))"
+
+	echo ""                           >> "${dot_file}"
+	echo "  subgraph \"${c_name}\" {" >> "${dot_file}"
+	echo "    cluster = true"         >> "${dot_file}"
+	echo "    label = \"${c_name}\""  >> "${dot_file}"
+	echo "    color=dodgerblue"       >> "${dot_file}"
     fi
 
     dbg_echo " * Component: ${c_name}"
 
-    echo ""                           >> "${dot_file}"
-    echo "  subgraph \"${c_name}\" {" >> "${dot_file}"
-    echo "    cluster = true"         >> "${dot_file}"
-    echo "    label = \"${c_name}\""  >> "${dot_file}"
-    echo "    color=dodgerblue"       >> "${dot_file}"
-
     # Create empty file to ensure it will exist in all cases
     >"${links_file}"
 
@@ -181,7 +184,9 @@ process_dapm_component()
 	process_dapm_widget "${tmp_dir}" "${c_name}" "${w_file}"
     done
 
-    echo "  }" >> "${dot_file}"
+    if [ ${is_component} = 1 ]; then
+	echo "  }" >> "${dot_file}"
+    fi
 
     cat "${links_file}" >> "${dot_file}"
 }

-- 
2.34.1

[PATCH 2/3] ASoC: dapm-graph: visualize component On/Off bias level

[Luca Ceresoli]

Read the bias_level debugfs files (ignored so far) and visualize the On/Off
state of each component using different graphic attributes in the generated
graph.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 tools/sound/dapm-graph | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/tools/sound/dapm-graph b/tools/sound/dapm-graph
index 205783d124d3..4e90883912d0 100755
--- a/tools/sound/dapm-graph
+++ b/tools/sound/dapm-graph
@@ -8,6 +8,8 @@
 
 set -eu
 
+STYLE_COMPONENT_ON="color=dodgerblue;style=bold"
+STYLE_COMPONENT_OFF="color=gray40;style=filled;fillcolor=gray90"
 STYLE_NODE_ON="shape=box,style=bold,color=green4"
 STYLE_NODE_OFF="shape=box,style=filled,color=gray30,fillcolor=gray95"
 
@@ -159,6 +161,7 @@ process_dapm_component()
     local is_component=0
     local dot_file="${tmp_dir}/main.dot"
     local links_file="${tmp_dir}/links.dot"
+    local c_attribs=""
 
     if [ -z "${c_name}" ]; then
 	is_component=1
@@ -166,16 +169,28 @@ process_dapm_component()
 	# Extract directory name into component name:
 	#   "./cs42l51.0-004a/dapm" -> "cs42l51.0-004a"
 	c_name="$(basename $(dirname "${c_dir}"))"
+    fi
+
+    dbg_echo " * Component: ${c_name}"
+
+    if [ ${is_component} = 1 ]; then
+	if [ -f "${c_dir}/bias_level" ]; then
+	    c_onoff=$(sed -n -e 1p "${c_dir}/bias_level" | awk '{print $1}')
+	    dbg_echo "   - bias_level: ${c_onoff}"
+	    if [ "$c_onoff" = "On" ]; then
+		c_attribs="${STYLE_COMPONENT_ON}"
+	    elif [ "$c_onoff" = "Off" ]; then
+		c_attribs="${STYLE_COMPONENT_OFF}"
+	    fi
+	fi
 
 	echo ""                           >> "${dot_file}"
 	echo "  subgraph \"${c_name}\" {" >> "${dot_file}"
 	echo "    cluster = true"         >> "${dot_file}"
 	echo "    label = \"${c_name}\""  >> "${dot_file}"
-	echo "    color=dodgerblue"       >> "${dot_file}"
+	echo "    ${c_attribs}"           >> "${dot_file}"
     fi
 
-    dbg_echo " * Component: ${c_name}"
-
     # Create empty file to ensure it will exist in all cases
     >"${links_file}"
 

-- 
2.34.1

[PATCH 3/3] ASoC: dapm-graph: show path name for non-static routes

[Luca Ceresoli]

Many routes are just static, not modifiable at runtime. Show the route name
for all the other routes as an edge label in the generated graph.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 tools/sound/dapm-graph | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/sound/dapm-graph b/tools/sound/dapm-graph
index 4e90883912d0..f14bdfedee8f 100755
--- a/tools/sound/dapm-graph
+++ b/tools/sound/dapm-graph
@@ -134,11 +134,17 @@ process_dapm_widget()
 	    # Collect any links. We could use "in" links or "out" links,
 	    # let's use "in" links
 	    if echo "${line}" | grep -q '^in '; then
+		local w_route=$(echo "$line" | awk -F\" '{print $2}')
 		local w_src=$(echo "$line" |
 				  awk -F\" '{print $6 "_" $4}' |
 				  sed  's/^(null)_/ROOT_/')
 		dbg_echo "     - Input route from: ${w_src}"
-		echo "  \"${w_src}\" -> \"$w_tag\"" >> "${links_file}"
+		dbg_echo "     - Route: ${w_route}"
+		local w_edge_attrs=""
+		if [ "${w_route}" != "static" ]; then
+		    w_edge_attrs=" [label=\"${w_route}\"]"
+		fi
+		echo "  \"${w_src}\" -> \"$w_tag\"${w_edge_attrs}" >> "${links_file}"
 	    fi
 	done
 
@@ -220,7 +226,7 @@ process_dapm_tree()
     echo "digraph G {" > "${dot_file}"
     echo "  fontname=\"sans-serif\"" >> "${dot_file}"
     echo "  node [fontname=\"sans-serif\"]" >> "${dot_file}"
-
+    echo "  edge [fontname=\"sans-serif\"]" >> "${dot_file}"
 
     # Process root directory (no component)
     process_dapm_component "${tmp_dir}" "${dapm_dir}/dapm" "ROOT"

-- 
2.34.1

Re: [PATCH 0/3] ASoC: dapm-graph: add component on/off and route names to graph

[Luca Ceresoli]

Hi Mark, All,

On Fri, 07 Jun 2024 09:41:50 +0200
Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> This small series adds some improvements to dapm-graph in order to produce
> a more correct and informative graph.
> 
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

Gentle ping about this series.

It applies and works fine on current master.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/3] ASoC: dapm-graph: add component on/off and route names to graph

[Mark Brown]

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

On Tue, Aug 20, 2024 at 06:44:06PM +0200, Luca Ceresoli wrote:
> On Fri, 07 Jun 2024 09:41:50 +0200
> Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:

> > This small series adds some improvements to dapm-graph in order to produce
> > a more correct and informative graph.

> Gentle ping about this series.

> It applies and works fine on current master.

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] ASoC: dapm-graph: add component on/off and route names to graph

[Luca Ceresoli]

Hello Mark,

On Tue, 20 Aug 2024 18:04:17 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Tue, Aug 20, 2024 at 06:44:06PM +0200, Luca Ceresoli wrote:
> > On Fri, 07 Jun 2024 09:41:50 +0200
> > Luca Ceresoli <luca.ceresoli@bootlin.com> wrote:  
> 
> > > This small series adds some improvements to dapm-graph in order to produce
> > > a more correct and informative graph.  
> 
> > Gentle ping about this series.  
> 
> > It applies and works fine on current master.  
> 
> Please don't send content free pings and please allow a reasonable time
> for review.  People get busy, go on holiday, attend conferences and so 
> on so unless there is some reason for urgency (like critical bug fixes)
> please allow at least a couple of weeks for review.  If there have been
> review comments then people may be waiting for those to be addressed.

I'm sorry about the noise. I thought it was worth in this case because:

 * this series has been sent 2.5 months ago
 * there was no reply at all
 * AFAIK and according to MAINTAINERS there is no patchwork catching
   tools/sound/

So it was looking much more like something gone into oblivion than
being on someone's TODO list.

> Sending content free pings adds to the mail volume (if they are seen at
> all) which is often the problem and since they can't be reviewed
> directly if something has gone wrong you'll have to resend the patches
> anyway, so sending again is generally a better approach though there are
> some other maintainers who like them - if in doubt look at how patches
> for the subsystem are normally handled.

Apologies, my fault for not having checked the archives. Note taken.

After doing so now however, I must say that while counting the resends
is easy, counting the pings in a somewhat reliable way is very time
consuming. It would be nice to have this info in a more reachable way
(MAINTAINERS?).

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/3] ASoC: dapm-graph: add component on/off and route names to graph

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1409 bytes --]

On Wed, Aug 21, 2024 at 10:17:48AM +0200, Luca Ceresoli wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > Please don't send content free pings and please allow a reasonable time
> > for review.  People get busy, go on holiday, attend conferences and so 
> > on so unless there is some reason for urgency (like critical bug fixes)
> > please allow at least a couple of weeks for review.  If there have been
> > review comments then people may be waiting for those to be addressed.

> I'm sorry about the noise. I thought it was worth in this case because:

>  * this series has been sent 2.5 months ago
>  * there was no reply at all
>  * AFAIK and according to MAINTAINERS there is no patchwork catching
>    tools/sound/

> So it was looking much more like something gone into oblivion than
> being on someone's TODO list.

That would be the resend part of the mail - content free pings can't be
directly acted on, they're just noise.  Note also that you should
generally rebase and resend things after the merge window.

> After doing so now however, I must say that while counting the resends
> is easy, counting the pings in a somewhat reliable way is very time
> consuming. It would be nice to have this info in a more reachable way
> (MAINTAINERS?).

Feel free to take that up with the MAINTAINERS people.  It's hard to
specify things in a snappy one liner kind of way.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]