Re: [PATCH v2 06/14] media: mc: Separate single link validation into a new function

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Mirela,

On Sun, Feb 15, 2026 at 04:42:27PM +0200, Mirela Rabulea wrote:
> Hi Sakari,
> 
> On 2/11/26 11:09, Sakari Ailus wrote:
> > Add a new function __media_pipeline_validate_one() to validate a single
> > link in a pipeline. This will soon be used for performing validation in
> > multiple phases.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >   drivers/media/mc/mc-entity.c | 80 +++++++++++++++++++++---------------
> >   1 file changed, 47 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index 9519a537bfa2..ef959e9bb313 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -768,6 +768,49 @@ static int media_pipeline_populate(struct media_pipeline *pipe,
> >          return ret;
> >   }
> > 
> > +static int
> > +__media_pipeline_validate_one(struct media_pad *origin,
> > +                             struct media_pipeline *pipe,
> pipe parameter is not used

I'll remove it for v3.

> > +                             struct media_pad *pad, struct media_link *link,
> > +                             bool *has_enabled_link)
> > +{
> > +       struct media_device *mdev = origin->graph_obj.mdev;
> > +       struct media_entity *entity = pad->entity;
> > +       int ret;
> > +
> > +       /* Record if the pad has links and enabled links. */
> > +       if (link->flags & MEDIA_LNK_FL_ENABLED && has_enabled_link)
> > +               *has_enabled_link = true;
> > +
> > +       /*
> > +        * Validate the link if it's enabled and has the
> > +        * current pad as its sink.
> > +        */
> > +       if (!(link->flags & MEDIA_LNK_FL_ENABLED))
> > +               return 0;
> > +
> > +       if (link->sink != pad)
> > +               return 0;
> > +
> > +       if (!entity->ops || !entity->ops->link_validate)
> > +               return 0;
> > +
> > +       ret = entity->ops->link_validate(link);
> > +       if (ret) {
> > +               dev_dbg(mdev->dev,
> > +                       "Link '%s':%u -> '%s':%u failed validation: %d\n",
> > +                       link->source->entity->name, link->source->index,
> > +                       link->sink->entity->name, link->sink->index, ret);
> > +               return ret;
> > +       }
> > +
> > +       dev_dbg(mdev->dev, "Link '%s':%u -> '%s':%u is valid\n",
> > +               link->source->entity->name, link->source->index,
> > +               link->sink->entity->name, link->sink->index);
> > +
> > +       return 0;
> > +}
> > +
> >   __must_check int __media_pipeline_start(struct media_pad *origin,
> >                                          struct media_pipeline *pipe)
> >   {
> > @@ -838,40 +881,11 @@ __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.
> > -                        */
> > -                       if (!(link->flags & MEDIA_LNK_FL_ENABLED))
> > -                               continue;
> > -
> > -                       if (link->sink != pad)
> > -                               continue;
> > -
> > -                       if (!entity->ops || !entity->ops->link_validate)
> > -                               continue;
> > -
> > -                       ret = entity->ops->link_validate(link);
> > -                       if (ret) {
> > -                               dev_dbg(mdev->dev,
> > -                                       "Link '%s':%u -> '%s':%u failed validation: %d\n",
> > -                                       link->source->entity->name,
> > -                                       link->source->index,
> > -                                       link->sink->entity->name,
> > -                                       link->sink->index, ret);
> > +                       ret = __media_pipeline_validate_one(origin, pipe,
> > +                                                           origin, link,
> > +                                                           &has_enabled_link);
> 
> Shouldn't this be __media_pipeline_validate_one(origin, pipe, pad, link,
> &has_enabled_link) ?

Yes, good find! This would have been bitten us later...

> 
> Also, before refactoring, there were 2 checks, I think the first one was
> unnecessary:
> 
>                         if (link->sink != pad && link->source != pad)
>                                 continue;
> The second one was more complete, it is the check that should remain in the
> end, either before calling the new function, or inside the function, but
> please adjust also the comments according to the actual code:
>                        if (link->sink != pad)
>                                continue;

There are two checks in order to collect information on enabled links to
the pad (first check) and perform validation on sink pads only (second
check). Both should remain.

There's additional cleanup that can be done on the conditions here; I'll
prepend a new patch for that.

-- 
Kind regards,

Sakari Ailus