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

[Neal Patalay]

On Sun, Jul 5, 2026 at 10:37 PM Andy Shevchenko wrote:

> Please, slow down! No need to send a new version immediately after
> replying to the previous one without settling down all aspects of the
> change.
>
> This version is no go, sorry.

Understood, no problem.

> > +static void ia_css_debug_build_info(char *info, size_t info_size,
> > +                                   int *offset,
> > +                                   const char *flag_str, size_t flag_str_size,
> > +                                   int *line_len,
> > +                                   int *num_lines)
> > +{
> > +       int len = *line_len;
> > +       int off = *offset;
> > +       int lines = *num_lines;
> > +       int len_written;
> > +
> > +       if (lines > 3)
> > +               return;
> > +
> > +       /*
> > +        * If new line length exceeds max line length,
> > +        * replace the last ',' with a "\\n".
> > +        */
> > +       if (len > 0 && off > 0 && info_size - off >= 2 &&
> > +           len + flag_str_size > ENABLE_LINE_MAX_LENGTH) {
> > +               if (lines >= 3) {
> > +                       *num_lines = lines + 1;
> > +                       return;
> > +               }
> > +               info[off - 1] = '\\';
> > +               info[off] = 'n';
> > +               off += 1;
> > +               len = 0;
> > +               lines += 1;
> > +       }
> > +
> > +       len_written = scnprintf(info + off, info_size - off, "%s,", flag_str);
> > +       *offset = off + len_written;
> > +       *line_len = len + len_written;
> > +       *num_lines = lines;
> > +}
>
> This makes helper too ugly and unreadable. Again, study the case
> first, when this can be true. Do we really need to cut it? This whole
> thing AFAICS depends on the dtrace facility in the driver. Is that
> HW-related? Is it pure SW? You need to perform some homework.

I've taken a look at the driver in greater depth. The flag wrapping
functionality being refactored here is used to construct a graph
visualization for debugging.

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

Each one of these calls generates a node in that graph, and the contents
of the node are decided by the flag string we build in enable_info. Seeing
as we are just collecting debug info and populating a visualization with it,
this refactor seems to be software.

Seeing as it's unlikely that every flag is enabled at once, and even if they
are this only causes a slightly more unreadable graph, avoiding 3 line
truncation seems to be the best option since it leads to more readable code
and avoids omitting information.

Additionally, len > 0 && offset == 0 is an impossible condition under any
feasible helper function use condition. This can be documented in a comment.

Given this information, to me it seems like the best course of action is to
revert the helper function to its simpler state without line count truncation
or the offset > 0 comparison. What do you think?

-- 
Thank you,
Neal Patalay