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

[Andy Shevchenko]

On Mon, Jul 06, 2026 at 02:59:16PM -0700, Neal Patalay wrote:
> 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?

I agree, just add more comments to the code and elaborate in the commit message
the (potential) difference in the behaviour.

-- 
With Best Regards,
Andy Shevchenko