Hi Sakari,
On Mon, Jan 15, 2024 at 10:43:56AM +0000, Sakari Ailus wrote:
> Hi Laurent,
>
> Many thanks for working on this.
You're welcome. It was somehow fun.
> On Mon, Jan 15, 2024 at 12:30:28PM +0200, Laurent Pinchart wrote:
> > The MEDIA_PAD_FL_MUST_CONNECT flag indicates that the pad requires an
> > enabled link to stream, but only if it has any link at all. This makes
> > little sense, as if a pad is part of a pipeline, there are very few use
> > cases for an active link to be mandatory only if links exist at all. A
> > review of in-tree drivers confirms they all need an enabled link for
> > pads marked with the MEDIA_PAD_FL_MUST_CONNECT flag.
> >
> > Expand the scope of the flag by rejecting pads that have no links at
> > all. This requires modifying the pipeline build code to add those pads
> > to the pipeline.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > ---
> > .../media/mediactl/media-types.rst | 11 ++--
> > drivers/media/mc/mc-entity.c | 53 +++++++++++++++----
> > 2 files changed, 48 insertions(+), 16 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst
> > index 0ffeece1e0c8..1ce87c0b705f 100644
> > --- a/Documentation/userspace-api/media/mediactl/media-types.rst
> > +++ b/Documentation/userspace-api/media/mediactl/media-types.rst
> > @@ -375,12 +375,11 @@ Types and flags used to represent the media graph elements
> > are origins of links.
> >
> > * - ``MEDIA_PAD_FL_MUST_CONNECT``
> > - - If this flag is set and the pad is linked to any other pad, then
> > - at least one of those links must be enabled for the entity to be
> > - able to stream. There could be temporary reasons (e.g. device
> > - configuration dependent) for the pad to need enabled links even
> > - when this flag isn't set; the absence of the flag doesn't imply
> > - there is none.
> > + - If this flag, then at least one link connected to the pad must be
> > + enabled for the pad to be able to stream. There could be temporary
> > + reasons (e.g. device configuration dependent) for the pad to need
> > + enabled links even when this flag isn't set; the absence of the flag
> > + doesn't imply there is none.
>
> Shoudln't you indent by tabs first here?
My text editor picked the option it liked best. I'll fix indentation to
avoid switching from tabs to spaces.
> Would it be possible to backport this, too? I'm pretty sure there will be
> NULL pointer dereferences due to this, even previous to the graph walk
> rework.
Patches 1/7 and 2/7 should be simple to backport (hopefully). Patch 3/7
should as well, which will fix the crash in the imx8-isi driver. Patches
4/7 to 6/7 may be more difficult to backport as they could generate more
conflicts, it depends how far back you want to go. That would be my
preferred option though.
> It may require reworking this to apply it to the pre-rework implementation
> and that's outside the scope of this set obviously.
The rework (v6.1) predates the imx8-isi driver (v6.4), so from an
imx8-isi point of view, we don't have to backport this earlier than
v6.4.
> For the set:
>
> Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>
> > One and only one of ``MEDIA_PAD_FL_SINK`` and ``MEDIA_PAD_FL_SOURCE``
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index 5907925ffd89..0e28b9a7936e 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -535,14 +535,15 @@ static int media_pipeline_walk_push(struct media_pipeline_walk *walk,
> >
> > /*
> > * Move the top entry link cursor to the next link. If all links of the entry
> > - * have been visited, pop the entry itself.
> > + * have been visited, pop the entry itself. Return true if the entry has been
> > + * popped.
> > */
> > -static void media_pipeline_walk_pop(struct media_pipeline_walk *walk)
> > +static bool media_pipeline_walk_pop(struct media_pipeline_walk *walk)
> > {
> > struct media_pipeline_walk_entry *entry;
> >
> > if (WARN_ON(walk->stack.top < 0))
> > - return;
> > + return false;
> >
> > entry = media_pipeline_walk_top(walk);
> >
> > @@ -552,7 +553,7 @@ static void media_pipeline_walk_pop(struct media_pipeline_walk *walk)
> > walk->stack.top);
> >
> > walk->stack.top--;
> > - return;
> > + return true;
> > }
> >
> > entry->links = entry->links->next;
> > @@ -560,6 +561,8 @@ static void media_pipeline_walk_pop(struct media_pipeline_walk *walk)
> > dev_dbg(walk->mdev->dev,
> > "media pipeline: moved entry %u to next link\n",
> > walk->stack.top);
> > +
> > + return false;
> > }
> >
> > /* Free all memory allocated while walking the pipeline. */
> > @@ -609,11 +612,12 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
> > struct media_link *link;
> > struct media_pad *local;
> > struct media_pad *remote;
> > + bool last_link;
> > int ret;
> >
> > origin = entry->pad;
> > link = list_entry(entry->links, typeof(*link), list);
> > - media_pipeline_walk_pop(walk);
> > + last_link = media_pipeline_walk_pop(walk);
> >
> > dev_dbg(walk->mdev->dev,
> > "media pipeline: exploring link '%s':%u -> '%s':%u\n",
> > @@ -638,7 +642,7 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
> > local->index)) {
> > dev_dbg(walk->mdev->dev,
> > "media pipeline: skipping link (no route)\n");
> > - return 0;
> > + goto done;
> > }
> >
> > /*
> > @@ -653,13 +657,44 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
> > if (!(link->flags & MEDIA_LNK_FL_ENABLED)) {
> > dev_dbg(walk->mdev->dev,
> > "media pipeline: skipping link (disabled)\n");
> > - return 0;
> > + goto done;
> > }
> >
> > ret = media_pipeline_add_pad(pipe, walk, remote);
> > if (ret)
> > return ret;
> >
> > +done:
> > + /*
> > + * If we're done iterating over links, iterate over pads of the entity.
> > + * This is necessary to discover pads that are not connected with any
> > + * link. Those are dead ends from a pipeline exploration point of view,
> > + * but are still part of the pipeline and need to be added to enable
> > + * proper validation.
> > + */
> > + if (!last_link)
> > + return 0;
> > +
> > + dev_dbg(walk->mdev->dev,
> > + "media pipeline: adding unconnected pads of '%s'\n",
> > + local->entity->name);
> > +
> > + media_entity_for_each_pad(origin->entity, local) {
> > + /*
> > + * Skip the origin pad (already handled), pad that have links
> > + * (already discovered through iterating over links) and pads
> > + * not internally connected.
> > + */
> > + if (origin == local || !local->num_links ||
> > + !media_entity_has_pad_interdep(origin->entity, origin->index,
> > + local->index))
> > + continue;
> > +
> > + ret = media_pipeline_add_pad(pipe, walk, local);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -771,7 +806,6 @@ __must_check int __media_pipeline_start(struct media_pad *pad,
> > struct media_pad *pad = ppad->pad;
> > struct media_entity *entity = pad->entity;
> > bool has_enabled_link = false;
> > - bool has_link = false;
> > struct media_link *link;
> >
> > dev_dbg(mdev->dev, "Validating pad '%s':%u\n", pad->entity->name,
> > @@ -801,7 +835,6 @@ __must_check int __media_pipeline_start(struct media_pad *pad,
> > /* Record if the pad has links and enabled links. */
> > if (link->flags & MEDIA_LNK_FL_ENABLED)
> > has_enabled_link = true;
> > - has_link = true;
> >
> > /*
> > * Validate the link if it's enabled and has the
> > @@ -839,7 +872,7 @@ __must_check int __media_pipeline_start(struct media_pad *pad,
> > * 3. If the pad has the MEDIA_PAD_FL_MUST_CONNECT flag set,
> > * ensure that it has either no link or an enabled link.
> > */
> > - if ((pad->flags & MEDIA_PAD_FL_MUST_CONNECT) && has_link &&
> > + if ((pad->flags & MEDIA_PAD_FL_MUST_CONNECT) &&
> > !has_enabled_link) {
> > dev_dbg(mdev->dev,
> > "Pad '%s':%u must be connected by an enabled link\n",
--
Regards,
Laurent Pinchart
