[PATCH] staging: media: atomisp: refactor pipe graph dump stage formatting

[PATCH] staging: media: atomisp: refactor pipe graph dump stage formatting

[Neal Patalay]

The original implementation of ia_css_debug_pipe_graph_dump_stage()
includes an off-by-one error where the original strscpy size dropped
characters immediately before newlines. It also allocates over 600
bytes across multiple buffers on the stack. Address these shortcomings
and reduce stack usage with a single 256 byte buffer via a new helper
function, ia_css_debug_build_info().

Fixes: ad85094b293e ("Revert "media: staging: atomisp: Remove driver"")
Signed-off-by: Neal Patalay <nealpatalay0@gmail.com>
---
 .../pci/runtime/debug/src/ia_css_debug.c      | 174 ++++++------------
 1 file changed, 59 insertions(+), 115 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
index 5113aa5973f3..9b4dd3b429da 100644
--- a/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
+++ b/drivers/staging/media/atomisp/pci/runtime/debug/src/ia_css_debug.c
@@ -1162,6 +1162,25 @@ void ia_css_debug_pipe_graph_dump_epilogue(void)
 	pg_inst.stream_format = N_ATOMISP_INPUT_FORMAT;
 }
 
+static void ia_css_debug_build_info(char *info, size_t info_size, int *offset, bool flag,
+				    const char *flag_str, size_t flag_str_size, int *line_len)
+{
+	if (flag) {
+		/* If new line length exceeds max line length, replace the last ',' with a "\\n" */
+		if (*line_len > 0 && info_size - *offset >= 2 &&
+		    *line_len + flag_str_size > ENABLE_LINE_MAX_LENGTH) {
+			info[*offset - 1] = '\\';
+			info[*offset] = 'n';
+			*offset += 1;
+			*line_len = 0;
+		}
+
+		int len_written = scnprintf(info + *offset, info_size - *offset, "%s,", flag_str);
+		*offset += len_written;
+		*line_len += len_written;
+	}
+}
+
 void
 ia_css_debug_pipe_graph_dump_stage(
     struct ia_css_pipeline_stage *stage,
@@ -1194,123 +1213,48 @@ ia_css_debug_pipe_graph_dump_stage(
 
 	/* Guard in case of binaries that don't have any binary_info */
 	if (stage->binary_info) {
-		char enable_info1[100];
-		char enable_info2[100];
-		char enable_info3[100];
-		char enable_info[302];
+		char enable_info[256];
+		int offset = 0;
+		int line_len = 0;
 		struct ia_css_binary_info *bi = stage->binary_info;
 
-		/* Split it in 2 function-calls to keep the amount of
-		 * parameters per call "reasonable"
-		 */
-		snprintf(enable_info1, sizeof(enable_info1),
-			 "%s%s%s%s%s%s%s%s%s%s%s%s%s%s",
-			 bi->enable.reduced_pipe ?	"rp," : "",
-			 bi->enable.vf_veceven ?		"vfve," : "",
-			 bi->enable.dis ?		"dis," : "",
-			 bi->enable.dvs_envelope ?	"dvse," : "",
-			 bi->enable.uds ?		"uds," : "",
-			 bi->enable.dvs_6axis ?		"dvs6," : "",
-			 bi->enable.block_output ?	"bo," : "",
-			 bi->enable.ds ?			"ds," : "",
-			 bi->enable.bayer_fir_6db ?	"bf6," : "",
-			 bi->enable.raw_binning ?	"rawb," : "",
-			 bi->enable.continuous ?		"cont," : "",
-			 bi->enable.s3a ?		"s3a," : "",
-			 bi->enable.fpnr ?		"fpnr," : "",
-			 bi->enable.sc ?			"sc," : ""
-			);
-
-		snprintf(enable_info2, sizeof(enable_info2),
-			 "%s%s%s%s%s%s%s%s%s%s%s",
-			 bi->enable.macc ?		"macc," : "",
-			 bi->enable.output ?		"outp," : "",
-			 bi->enable.ref_frame ?		"reff," : "",
-			 bi->enable.tnr ?		"tnr," : "",
-			 bi->enable.xnr ?		"xnr," : "",
-			 bi->enable.params ?		"par," : "",
-			 bi->enable.ca_gdc ?		"cagdc," : "",
-			 bi->enable.isp_addresses ?	"ispa," : "",
-			 bi->enable.in_frame ?		"inf," : "",
-			 bi->enable.out_frame ?		"outf," : "",
-			 bi->enable.high_speed ?		"hs," : ""
-			);
-
-		/* And merge them into one string */
-		snprintf(enable_info, sizeof(enable_info), "%s%s",
-			 enable_info1, enable_info2);
-		{
-			int l, p;
-			char *ei = enable_info;
-
-			l = strlen(ei);
-
-			/* Replace last ',' with \0 if present */
-			if (l && enable_info[l - 1] == ',')
-				enable_info[--l] = '\0';
-
-			if (l > ENABLE_LINE_MAX_LENGTH) {
-				/* Too big for one line, find last comma */
-				p = ENABLE_LINE_MAX_LENGTH;
-				while (ei[p] != ',')
-					p--;
-				/* Last comma found, copy till that comma */
-				strscpy(enable_info1, ei, umin(p, sizeof(enable_info1)));
-
-				ei += p + 1;
-				l = strlen(ei);
-
-				if (l <= ENABLE_LINE_MAX_LENGTH) {
-					/* The 2nd line fits */
-					/* we cannot use ei as argument because
-					 * it is not guaranteed dword aligned
-					 */
-
-					strscpy(enable_info2, ei, umin(l, sizeof(enable_info2)));
-
-					snprintf(enable_info, sizeof(enable_info), "%s\\n%s",
-						 enable_info1, enable_info2);
-
-				} else {
-					/* 2nd line is still too long */
-					p = ENABLE_LINE_MAX_LENGTH;
-					while (ei[p] != ',')
-						p--;
-
-					strscpy(enable_info2, ei, umin(p, sizeof(enable_info2)));
-
-					ei += p + 1;
-					l = strlen(ei);
-
-					if (l <= ENABLE_LINE_MAX_LENGTH) {
-						/* The 3rd line fits */
-						/* we cannot use ei as argument because
-						* it is not guaranteed dword aligned
-						*/
-						strscpy(enable_info3, ei,
-							sizeof(enable_info3));
-						snprintf(enable_info, sizeof(enable_info),
-							 "%s\\n%s\\n%s",
-							 enable_info1, enable_info2,
-							 enable_info3);
-					} else {
-						/* 3rd line is still too long */
-						p = ENABLE_LINE_MAX_LENGTH;
-						while (ei[p] != ',')
-							p--;
-						strscpy(enable_info3, ei,
-							umin(p, sizeof(enable_info3)));
-						ei += p + 1;
-						strscpy(enable_info3, ei,
-							sizeof(enable_info3));
-						snprintf(enable_info, sizeof(enable_info),
-							 "%s\\n%s\\n%s",
-							 enable_info1, enable_info2,
-							 enable_info3);
-					}
-				}
-			}
-		}
+#define ADD_INFO(flag, flag_str) ia_css_debug_build_info(enable_info, sizeof(enable_info), \
+		&offset, bi->enable.flag, flag_str, sizeof(flag_str), &line_len)
+
+		/* Build string in enable_info buffer */
+		ADD_INFO(reduced_pipe, "rp");
+		ADD_INFO(vf_veceven, "vfve");
+		ADD_INFO(dis, "dis");
+		ADD_INFO(dvs_envelope, "dvse");
+		ADD_INFO(uds, "uds");
+		ADD_INFO(dvs_6axis, "dvs6");
+		ADD_INFO(block_output, "bo");
+		ADD_INFO(ds, "ds");
+		ADD_INFO(bayer_fir_6db, "bf6");
+		ADD_INFO(raw_binning, "rawb");
+		ADD_INFO(continuous, "cont");
+		ADD_INFO(s3a, "s3a");
+		ADD_INFO(fpnr, "fpnr");
+		ADD_INFO(sc, "sc");
+		ADD_INFO(macc, "macc");
+		ADD_INFO(output, "outp");
+		ADD_INFO(ref_frame, "reff");
+		ADD_INFO(tnr, "tnr");
+		ADD_INFO(xnr, "xnr");
+		ADD_INFO(params, "par");
+		ADD_INFO(ca_gdc, "cagdc");
+		ADD_INFO(isp_addresses, "ispa");
+		ADD_INFO(in_frame, "inf");
+		ADD_INFO(out_frame, "outf");
+		ADD_INFO(high_speed, "hs");
+
+#undef ADD_INFO
+
+		/* Replace last ',' with '\0' */
+		if (offset > 0)
+			enable_info[offset - 1] = '\0';
+		else
+			enable_info[0] = '\0';
 
 		dtrace_dot("node [shape = circle, fixedsize=true, width=2.5, label=\"%s\\n%s\\n\\n%s\"]; \"%s(pipe%d)\"",
 			   bin_type, blob_name, enable_info, blob_name, id);
-- 
2.54.0

Re: [PATCH] staging: media: atomisp: refactor pipe graph dump stage formatting

[Andy Shevchenko]

On Wed, Jul 01, 2026 at 09:15:34AM -0700, Neal Patalay wrote:
> The original implementation of ia_css_debug_pipe_graph_dump_stage()
> includes an off-by-one error where the original strscpy size dropped

strscpy()

> characters immediately before newlines. It also allocates over 600
> bytes across multiple buffers on the stack. Address these shortcomings
> and reduce stack usage with a single 256 byte buffer via a new helper
> function, ia_css_debug_build_info().

Thanks, this is useful! See my comments below.

> Fixes: ad85094b293e ("Revert "media: staging: atomisp: Remove driver"")

Nope. Find the actual commit in the history.

...

> +static void ia_css_debug_build_info(char *info, size_t info_size, int *offset, bool flag,
> +				    const char *flag_str, size_t flag_str_size, int *line_len)
> +{
> +	if (flag) {

So, this makes code much better if

	if (!flag)
		return;

BUT, the usual way of such functions is to make check in the caller(s) and drop
this "flag" completely.

> +		/* If new line length exceeds max line length, replace the last ',' with a "\\n" */

The media subsystem is quite strict about 80 character limit, this line way
too long.

> +		if (*line_len > 0 && info_size - *offset >= 2 &&
> +		    *line_len + flag_str_size > ENABLE_LINE_MAX_LENGTH) {
> +			info[*offset - 1] = '\\';
> +			info[*offset] = 'n';
> +			*offset += 1;
> +			*line_len = 0;
> +		}
> +
> +		int len_written = scnprintf(info + *offset, info_size - *offset, "%s,", flag_str);
> +		*offset += len_written;
> +		*line_len += len_written;
> +	}
> +}

...

> +#define ADD_INFO(flag, flag_str) ia_css_debug_build_info(enable_info, sizeof(enable_info), \
> +		&offset, bi->enable.flag, flag_str, sizeof(flag_str), &line_len)


Use logical split and make this all to be shorter.

> +		/* Build string in enable_info buffer */
> +		ADD_INFO(reduced_pipe, "rp");
> +		ADD_INFO(vf_veceven, "vfve");
> +		ADD_INFO(dis, "dis");
> +		ADD_INFO(dvs_envelope, "dvse");
> +		ADD_INFO(uds, "uds");
> +		ADD_INFO(dvs_6axis, "dvs6");
> +		ADD_INFO(block_output, "bo");
> +		ADD_INFO(ds, "ds");
> +		ADD_INFO(bayer_fir_6db, "bf6");
> +		ADD_INFO(raw_binning, "rawb");
> +		ADD_INFO(continuous, "cont");
> +		ADD_INFO(s3a, "s3a");
> +		ADD_INFO(fpnr, "fpnr");
> +		ADD_INFO(sc, "sc");
> +		ADD_INFO(macc, "macc");
> +		ADD_INFO(output, "outp");
> +		ADD_INFO(ref_frame, "reff");
> +		ADD_INFO(tnr, "tnr");
> +		ADD_INFO(xnr, "xnr");
> +		ADD_INFO(params, "par");
> +		ADD_INFO(ca_gdc, "cagdc");
> +		ADD_INFO(isp_addresses, "ispa");
> +		ADD_INFO(in_frame, "inf");
> +		ADD_INFO(out_frame, "outf");
> +		ADD_INFO(high_speed, "hs");
> +
> +#undef ADD_INFO

Instead of doing this way, consider making an string literal array and just
loop over it. This might need to reconsider representation of those flags
as well. Yet, don't come to the conclusion, you need to try and see which
one is better. The current approach is okay if my suggestion will look less
readable.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] staging: media: atomisp: refactor pipe graph dump stage formatting

[Neal Patalay]

On Thu, Jul 2, 2026 at 2:11 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> strscpy()

Thanks, I'll fix this.

> Nope. Find the actual commit in the history.

I'll fix this too.

> So, this makes code much better if
>
>         if (!flag)
>                 return;
>
> BUT, the usual way of such functions is to make check in the caller(s) and drop
> this "flag" completely.

I'll drop the flag entirely and do the check directly in the macro.

> Use logical split and make this all to be shorter.

I'll shorten all of the long lines.

> Instead of doing this way, consider making an string literal array and just
> loop over it. This might need to reconsider representation of those flags
> as well. Yet, don't come to the conclusion, you need to try and see which
> one is better. The current approach is okay if my suggestion will look less
> readable.

I considered this, but I found that the string literal array based approach is
less readable in this case as the flags are less obviously mapped to the flag
names compared to the macro approach. However, I can implement this using
that approach at your request.

-- 
Thank you,
Neal Patalay