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

[Neal Patalay]

On Tue, Jul 7, 2026 at 12:18 AM Andy Shevchenko wrote:
>
> 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.

Great, I'll send in v5.

-- 
Thank you,
Neal Patalay