[PATCH 0/7] media: Fix mishandling of MEDIA_PAD_FL_MUST_CONNECT flag

[PATCH 0/7] media: Fix mishandling of MEDIA_PAD_FL_MUST_CONNECT flag

[Laurent Pinchart]

Hello,

This patch series fixes mishandling of the MEDIA_PAD_FL_MUST_CONNECT
flag.

The issue has been introduced in commit ae219872834a ("media: mc:
entity: Rewrite media_pipeline_start()") when reworking the media
controller pipeline validation code. It is known to cause crashes in the
imx-mipi-csis driver (see [1]) and most likely affects multiple other
drivers. Patch 1/7 and 2/7 fix the problem.

Patches 3/7 to 7/7 fix an additional related issue affecting the
imx8-isi driver. It is however not a regression introduced by commit
ae219872834a. The problem causes a NULL pointer dereference when
enabling streams if the pipeline is configured with the imx8-isi
crossbar sink pad not being connected. Patch 3/7, initially submitted by
Marek, fixes the issue in the imx8-isi driver itself.

As the problem is very similar to what the MUST_CONNECT flag was
designed to handled, I considered the bug to be better solved by using
the MUST_CONNECT flag in the imx8-isi driver. This requires expanding
the purpose of the flag (patch 6/7), after a few drive-by fixes and
reworks (patches 4/7 and 5/7). Finally, patch 7/7 sets the MUST_CONNECT
flag in the imx8-isi driver, effectively reverting the changes from
patch 3/7.

I have decided to still include patch 3/7 in the series, as in the even
that the changes in patches 4/7 to 7/7 were to be reverted later (or
simply require more discussions and new versions), patches 1/7 to 3/7
could be merged without delay and fix the issue for users.

[1] https://lore.kernel.org/linux-media/7658a15a-80c5-219f-2477-2a94ba6c6ba1@kontron.de

Laurent Pinchart (6):
  media: mc: Add local pad to pipeline regardless of the link state
  media: mc: Fix flags handling when creating pad links
  media: mc: Add num_links flag to media_pad
  media: mc: Rename pad variable to clarify intent
  media: mc: Expand MUST_CONNECT flag to always require an enabled link
  media: nxp: imx8-isi: Mark all crossbar sink pads as MUST_CONNECT

Marek Vasut (1):
  media: nxp: imx8-isi: Check whether crossbar pad is non-NULL before
    access

 .../media/mediactl/media-types.rst            | 11 +--
 drivers/media/mc/mc-entity.c                  | 93 ++++++++++++++-----
 .../platform/nxp/imx8-isi/imx8-isi-crossbar.c |  4 +-
 include/media/media-entity.h                  |  2 +
 4 files changed, 78 insertions(+), 32 deletions(-)


base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
-- 
Regards,

Laurent Pinchart

[PATCH 1/7] media: mc: Add local pad to pipeline regardless of the link state

[Laurent Pinchart]

When building pipelines by following links, the
media_pipeline_explore_next_link() function only traverses enabled
links. The remote pad of a disabled link is not added to the pipeline,
and neither is the local pad. While the former is correct as disabled
links should not be followed, not adding the local pad breaks processing
of the MEDIA_PAD_FL_MUST_CONNECT flag.

The MEDIA_PAD_FL_MUST_CONNECT flag is checked in the
__media_pipeline_start() function that iterates over all pads after
populating the pipeline. If the pad is not present, the check gets
skipped, rendering it useless.

Fix this by adding the local pad of all links regardless of their state,
only skipping the remote pad for disabled links.

Fixes: ae219872834a ("media: mc: entity: Rewrite media_pipeline_start()")
Reported-by: Frieder Schrempf <frieder.schrempf@kontron.de>
Closes: https://lore.kernel.org/linux-media/7658a15a-80c5-219f-2477-2a94ba6c6ba1@kontron.de
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/mc/mc-entity.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 543a392f8635..a6f28366106f 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -620,13 +620,6 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
 		link->source->entity->name, link->source->index,
 		link->sink->entity->name, link->sink->index);
 
-	/* Skip links that are not enabled. */
-	if (!(link->flags & MEDIA_LNK_FL_ENABLED)) {
-		dev_dbg(walk->mdev->dev,
-			"media pipeline: skipping link (disabled)\n");
-		return 0;
-	}
-
 	/* Get the local pad and remote pad. */
 	if (link->source->entity == pad->entity) {
 		local = link->source;
@@ -648,13 +641,20 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
 	}
 
 	/*
-	 * Add the local and remote pads of the link to the pipeline and push
-	 * them to the stack, if they're not already present.
+	 * Add the local pad of the link to the pipeline and push it to the
+	 * stack, if not already present.
 	 */
 	ret = media_pipeline_add_pad(pipe, walk, local);
 	if (ret)
 		return ret;
 
+	/* Similarly, add the remote pad, but only if the link is enabled. */
+	if (!(link->flags & MEDIA_LNK_FL_ENABLED)) {
+		dev_dbg(walk->mdev->dev,
+			"media pipeline: skipping link (disabled)\n");
+		return 0;
+	}
+
 	ret = media_pipeline_add_pad(pipe, walk, remote);
 	if (ret)
 		return ret;
-- 
Regards,

Laurent Pinchart

[PATCH 2/7] media: mc: Fix flags handling when creating pad links

[Laurent Pinchart]

The media_create_pad_link() function doesn't correctly clear reject link
type flags, nor does it set the DATA_LINK flag. It only works because
the MEDIA_LNK_FL_DATA_LINK flag's value is 0.

Fix it by returning an error if any link type flag is set. This doesn't
introduce any regression, as nobody calls the media_create_pad_link()
function with link type flags (easily checked by grepping for the flag
in the source code, there are very few hits).

Set the MEDIA_LNK_FL_DATA_LINK explicitly, which is a no-op that the
compiler will optimize out, but is still useful to make the code more
explicit and easier to understand.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/mc/mc-entity.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index a6f28366106f..7839e3f68efa 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -1092,6 +1092,11 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
 	struct media_link *link;
 	struct media_link *backlink;
 
+	if (flags & MEDIA_LNK_FL_LINK_TYPE)
+		return -EINVAL;
+
+	flags |= MEDIA_LNK_FL_DATA_LINK;
+
 	if (WARN_ON(!source || !sink) ||
 	    WARN_ON(source_pad >= source->num_pads) ||
 	    WARN_ON(sink_pad >= sink->num_pads))
@@ -1107,7 +1112,7 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
 
 	link->source = &source->pads[source_pad];
 	link->sink = &sink->pads[sink_pad];
-	link->flags = flags & ~MEDIA_LNK_FL_INTERFACE_LINK;
+	link->flags = flags;
 
 	/* Initialize graph object embedded at the new link */
 	media_gobj_create(source->graph_obj.mdev, MEDIA_GRAPH_LINK,
-- 
Regards,

Laurent Pinchart

[PATCH 3/7] media: nxp: imx8-isi: Check whether crossbar pad is non-NULL before access

[Laurent Pinchart]

From: Marek Vasut <marex@denx.de>

When translating source to sink streams in the crossbar subdev, the
driver tries to locate the remote subdev connected to the sink pad. The
remote pad may be NULL, if userspace tries to enable a stream that ends
at an unconnected crossbar sink. When that occurs, the driver
dereferences the NULL pad, leading to a crash.

Prevent the crash by checking if the pad is NULL before using it, and
return an error if it is.

Fixes: cf21f328fcaf ("media: nxp: Add i.MX8 ISI driver")
Signed-off-by: Marek Vasut <marex@denx.de>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: Fabio Estevam <festevam@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Link: https://lore.kernel.org/r/20231201150614.63300-1-marex@denx.de
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
index 792f031e032a..44354931cf8a 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
@@ -160,8 +160,14 @@ mxc_isi_crossbar_xlate_streams(struct mxc_isi_crossbar *xbar,
 	}
 
 	pad = media_pad_remote_pad_first(&xbar->pads[sink_pad]);
-	sd = media_entity_to_v4l2_subdev(pad->entity);
+	if (!pad) {
+		dev_dbg(xbar->isi->dev,
+			"no pad connected to crossbar input %u\n",
+			sink_pad);
+		return ERR_PTR(-EPIPE);
+	}
 
+	sd = media_entity_to_v4l2_subdev(pad->entity);
 	if (!sd) {
 		dev_dbg(xbar->isi->dev,
 			"no entity connected to crossbar input %u\n",
-- 
Regards,

Laurent Pinchart

[PATCH 4/7] media: mc: Add num_links flag to media_pad

[Laurent Pinchart]

Maintain a counter of the links connected to a pad in the media_pad
structure. This helps checking if a pad is connected to anything, which
will be used in the pipeline building code.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/mc/mc-entity.c | 6 ++++++
 include/media/media-entity.h | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 7839e3f68efa..c2d8f59b62c1 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -1038,6 +1038,9 @@ static void __media_entity_remove_link(struct media_entity *entity,
 
 	/* Remove the reverse links for a data link. */
 	if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) == MEDIA_LNK_FL_DATA_LINK) {
+		link->source->num_links--;
+		link->sink->num_links--;
+
 		if (link->source->entity == entity)
 			remote = link->sink->entity;
 		else
@@ -1143,6 +1146,9 @@ media_create_pad_link(struct media_entity *source, u16 source_pad,
 	sink->num_links++;
 	source->num_links++;
 
+	link->source->num_links++;
+	link->sink->num_links++;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(media_create_pad_link);
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 2b6cd343ee9e..4d95893c8984 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -225,6 +225,7 @@ enum media_pad_signal_type {
  * @graph_obj:	Embedded structure containing the media object common data
  * @entity:	Entity this pad belongs to
  * @index:	Pad index in the entity pads array, numbered from 0 to n
+ * @num_links:	Number of links connected to this pad
  * @sig_type:	Type of the signal inside a media pad
  * @flags:	Pad flags, as defined in
  *		:ref:`include/uapi/linux/media.h <media_header>`
@@ -236,6 +237,7 @@ struct media_pad {
 	struct media_gobj graph_obj;	/* must be first field in struct */
 	struct media_entity *entity;
 	u16 index;
+	u16 num_links;
 	enum media_pad_signal_type sig_type;
 	unsigned long flags;
 
-- 
Regards,

Laurent Pinchart

[PATCH 5/7] media: mc: Rename pad variable to clarify intent

[Laurent Pinchart]

The pad local variable in the media_pipeline_explore_next_link()
function is used to store the pad through which the entity has been
reached. Rename it to origin to reflect that and make the code easier to
read. This will be even more important in subsequent commits when
expanding the function with additional logic.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/mc/mc-entity.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index c2d8f59b62c1..5907925ffd89 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -605,13 +605,13 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
 					    struct media_pipeline_walk *walk)
 {
 	struct media_pipeline_walk_entry *entry = media_pipeline_walk_top(walk);
-	struct media_pad *pad;
+	struct media_pad *origin;
 	struct media_link *link;
 	struct media_pad *local;
 	struct media_pad *remote;
 	int ret;
 
-	pad = entry->pad;
+	origin = entry->pad;
 	link = list_entry(entry->links, typeof(*link), list);
 	media_pipeline_walk_pop(walk);
 
@@ -621,7 +621,7 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
 		link->sink->entity->name, link->sink->index);
 
 	/* Get the local pad and remote pad. */
-	if (link->source->entity == pad->entity) {
+	if (link->source->entity == origin->entity) {
 		local = link->source;
 		remote = link->sink;
 	} else {
@@ -633,8 +633,9 @@ static int media_pipeline_explore_next_link(struct media_pipeline *pipe,
 	 * Skip links that originate from a different pad than the incoming pad
 	 * that is not connected internally in the entity to the incoming pad.
 	 */
-	if (pad != local &&
-	    !media_entity_has_pad_interdep(pad->entity, pad->index, local->index)) {
+	if (origin != local &&
+	    !media_entity_has_pad_interdep(origin->entity, origin->index,
+					   local->index)) {
 		dev_dbg(walk->mdev->dev,
 			"media pipeline: skipping link (no route)\n");
 		return 0;
-- 
Regards,

Laurent Pinchart

[PATCH 6/7] media: mc: Expand MUST_CONNECT flag to always require an enabled link

[Laurent Pinchart]

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.
 
 
 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

Re: [PATCH 6/7] media: mc: Expand MUST_CONNECT flag to always require an enabled link

[Sakari Ailus]

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",

-- 
Regards,

Sakari Ailus

Re: [PATCH 6/7] media: mc: Expand MUST_CONNECT flag to always require an enabled link

[Laurent Pinchart]

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",

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/7] media: mc: Expand MUST_CONNECT flag to always require an enabled link

[Sakari Ailus]

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.

-- 
Regards,

Sakari Ailus

Re: [PATCH 6/7] media: mc: Expand MUST_CONNECT flag to always require an enabled link

[Laurent Pinchart]

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 :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/7] media: mc: Expand MUST_CONNECT flag to always require an enabled link

[Sakari Ailus]

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.

-- 
Regards,

Sakari Ailus

Re: [PATCH 6/7] media: mc: Expand MUST_CONNECT flag to always require an enabled link

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart

[PATCH 7/7] media: nxp: imx8-isi: Mark all crossbar sink pads as MUST_CONNECT

[Laurent Pinchart]

All the sink pads of the crossbar switch require an active link if
they're part of the pipeline. Mark them with the
MEDIA_PAD_FL_MUST_CONNECT flag to fail pipeline validation if they're
not connected. This allows removing a manual check when translating
streams.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../media/platform/nxp/imx8-isi/imx8-isi-crossbar.c    | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
index 44354931cf8a..c9a4d091b570 100644
--- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
+++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-crossbar.c
@@ -160,13 +160,6 @@ mxc_isi_crossbar_xlate_streams(struct mxc_isi_crossbar *xbar,
 	}
 
 	pad = media_pad_remote_pad_first(&xbar->pads[sink_pad]);
-	if (!pad) {
-		dev_dbg(xbar->isi->dev,
-			"no pad connected to crossbar input %u\n",
-			sink_pad);
-		return ERR_PTR(-EPIPE);
-	}
-
 	sd = media_entity_to_v4l2_subdev(pad->entity);
 	if (!sd) {
 		dev_dbg(xbar->isi->dev,
@@ -471,7 +464,8 @@ int mxc_isi_crossbar_init(struct mxc_isi_dev *isi)
 	}
 
 	for (i = 0; i < xbar->num_sinks; ++i)
-		xbar->pads[i].flags = MEDIA_PAD_FL_SINK;
+		xbar->pads[i].flags = MEDIA_PAD_FL_SINK
+				    | MEDIA_PAD_FL_MUST_CONNECT;
 	for (i = 0; i < xbar->num_sources; ++i)
 		xbar->pads[i + xbar->num_sinks].flags = MEDIA_PAD_FL_SOURCE;
 
-- 
Regards,

Laurent Pinchart