Re: [PATCH v4 18/29] media: mc: Simplify link processing in __media_pipeline_start()

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Apr 21, 2026 at 03:37:18PM +0300, Sakari Ailus wrote:
> On Tue, Apr 21, 2026 at 02:18:25PM +0300, Laurent Pinchart wrote:
> > On Tue, Apr 21, 2026 at 01:24:16PM +0300, Sakari Ailus wrote:
> > > On Thu, Apr 16, 2026 at 05:35:24PM +0300, Laurent Pinchart wrote:
> > > > On Wed, Apr 08, 2026 at 06:39:27PM +0300, Sakari Ailus wrote:
> > > > > There are two conditions checking the ENABLED link flag in the loop
> > > > > going through the links related to an entity. Drop the other one and
> > > > > simplify the remaining code.
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > Reviewed-by: Michael Riesch <michael.riesch@collabora.com>
> > > > > ---
> > > > >  drivers/media/mc/mc-entity.c | 11 +++++------
> > > > >  1 file changed, 5 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > > > > index 3fa0bc687851..6bf4730b89d2 100644
> > > > > --- a/drivers/media/mc/mc-entity.c
> > > > > +++ b/drivers/media/mc/mc-entity.c
> > > > > @@ -838,17 +838,16 @@ __must_check int __media_pipeline_start(struct media_pad *origin,
> > > > >  			if (link->sink != pad && link->source != pad)
> > > > >  				continue;
> > > > >  
> > > > > -			/* Record if the pad has links and enabled links. */
> > > > > -			if (link->flags & MEDIA_LNK_FL_ENABLED)
> > > > > -				has_enabled_link = true;
> > > > > -
> > > > >  			/*
> > > > > -			 * Validate the link if it's enabled and has the
> > > > > -			 * current pad as its sink.
> > > > > +			 * Ensure the link is enabled and if so, record
> > > > > +			 * it. Proceed to the next link if the current pad isn't
> > > > > +			 * the sink pad of the link.
> > > > 
> > > > You can reflow this;
> > > > 
> > > > 			 * Ensure the link is enabled and if so, record it.
> > > > 			 * Proceed to the next link if the current pad isn't the
> > > > 			 * sink pad of the link.
> > > > 
> > > > but I find the new comment confusing.
> > > > 
> > > > I would keep the code as-is, I think it's more readable, and the
> > > > compiler will deal with optimization.
> > > 
> > > There's only one flag to test and I can't see how it'd be more readable to
> > > do that twice in the same location. I can keep the comment as-is if you
> > > prefer that.
> > 
> > The flag is tested twice for two different purposes, with two separate
> > comments. A subsequent patch in the series further modifies this code,
> 
> I believe the code wouldn't have looked like this if it wasn't written over
> several iterations. In other words, it was in a need of a cleanup this
> patch does. :-)
> 
> I.e. first see if a link isn't enabled and if so, bail out. Otherwise the
> rest will proceed from there, the link being enabled being a condition for
> that, including marking that there was an enabled link.

First the code records if it has found an enabled link for the pad, and
then it skips validating links that don't need to be validated, that is
links that are disabled or links the originate from the current pad (as
links are validated in the context of their sink, not their source).

Those are two separate and unrelated operations (recording if there are
enabled links, and skipping links that don't need to be validated), and
each of them has its own comment block. This patch moves the first
operation in the middle of the second (between the link->flags and the
link->sink checks), which makes things harder to read, and harder to
document.

> > and makes things less readable as it inserts code in the middle while
> > still keeping a single comment to explain the multiple operations. I'm
> 
> If you look at the resulting media_pipeline_validate_one(), the flow is
> entirely reasonable and would not benefit from testing the enabled flag
> twice. I can add better comments on why what is being done is done.

I looked at the function found the comment very confusing compared to
the current code. I tried to write a better one and wasn't happy with
the result. That's why I realized this patch should be dropped.

You can try writing a better comment, but I want things to be absolutely
crystal clear. That won't be easy with this modification of the code
flow.

As the current code is more readable, and the proposed change does not
improve anything, I strongly think the best option is to drop this
patch.

> > sure we could expand the comment to explain things in more details (and
> > bikeshed how to do so), but I think it will still be less readable than
> > keeping those two steps separate with one comment each.
> > 
> > > > >  			 */
> > > > >  			if (!(link->flags & MEDIA_LNK_FL_ENABLED))
> > > > >  				continue;
> > > > >  
> > > > > +			has_enabled_link = true;
> > > > > +
> > > > >  			if (link->sink != pad)
> > > > >  				continue;
> > > > >  

-- 
Regards,

Laurent Pinchart