| Message ID | 20240115103029.28055-7-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Fix mishandling of MEDIA_PAD_FL_MUST_CONNECT flag | expand |
Hi Laurent, Many thanks for working on this. 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@ideasonboard.com> > --- > .../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? 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. It may require reworking this to apply it to the pre-rework implementation and that's outside the scope of this set obviously. For the set: Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > 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",
Hi Laurent, A small addtional comment... On Mon, Jan 15, 2024 at 12:30:28PM +0200, Laurent Pinchart wrote: > + - 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 How about, instead, to make the meaning clearer: - If this flag is set, then for this pad to be able to stream, it must be connected by at least one enabled link. > + 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.
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@ideasonboard.com> > > --- > > .../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@linux.intel.com> > > > 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",
On Mon, Jan 15, 2024 at 10:54:11AM +0000, Sakari Ailus wrote: > Hi Laurent, > > A small addtional comment... > > On Mon, Jan 15, 2024 at 12:30:28PM +0200, Laurent Pinchart wrote: > > + - 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 > > How about, instead, to make the meaning clearer: > > - If this flag is set, then for this pad to be able to stream, > it must be connected by at least one enabled link. Works for me. > > + 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.
Hi Laurent, On Mon, Jan 15, 2024 at 12:55:25PM +0200, Laurent Pinchart wrote: > 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@ideasonboard.com> > > > --- > > > .../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. How certain are you this only affects the imx8-isi driver? There are many drivers using the MUST_CONNECT flag and I'm not sure all those have the necessary checks in place. There of course could be drivers just missing the flag, too, and that's a different issue.
On Mon, Jan 15, 2024 at 10:59:14AM +0000, Sakari Ailus wrote: > On Mon, Jan 15, 2024 at 12:55:25PM +0200, Laurent Pinchart wrote: > > 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@ideasonboard.com> > > > > --- > > > > .../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. > > How certain are you this only affects the imx8-isi driver? There are many > drivers using the MUST_CONNECT flag and I'm not sure all those have the > necessary checks in place. There of course could be drivers just missing > the flag, too, and that's a different issue. The imx8-isi was not using the flag, so it's a different issue. The ISI is special in the sense that it has an input crossbar switch that can have Asome non-connected inputs in valid configurations. For drivers using the flag, in most cases there should be at least one link connected to MUST_CONNECT pads. Many drivers are for 1-sink, 1-source subdevs, and those should not get registered in the first place if they have nothing connected to their source. One exception may be the ipu3-cio2 driver which has multiple independent CSI-2 RXs. Some of them will not have any sensor connected. This could be addressed in the ipu3-cio2 driver to not register those CSI-2 RXs (and the corresponding video nodes). Alternatively I'm fine if you try to backport this series before v6.1, but I won't :-)
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. 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",
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@ideasonboard.com> --- .../media/mediactl/media-types.rst | 11 ++-- drivers/media/mc/mc-entity.c | 53 +++++++++++++++---- 2 files changed, 48 insertions(+), 16 deletions(-)