All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] media: imx6: Support complex external topologies
@ 2020-04-13  1:14 Laurent Pinchart
  2020-04-13  1:14 ` [PATCH 1/2] media: staging/imx: Don't assume OF port id equals pad index Laurent Pinchart
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Laurent Pinchart @ 2020-04-13  1:14 UTC (permalink / raw)
  To: linux-media; +Cc: Steve Longerbeam, Philipp Zabel

Hello,

This small patch series adds support to the i.MX6 IPU CSI driver for
external (on-board) devices that exhibit complex topologies. My use case
is a CSI-2 camera sensor that has a single DT node (as an I2C device)
and creates two V4L2 subdevs. "Complex" may thus be a bit of an
exageration, but the series nonetheless makes such a sensor usable with
the driver.

There are two issues, fixed by two patches. Patch 1/2 removes the
assumption that OF port ids and pad indices are equivalent. This holds
true in many cases, but is incorrect in my case, with the topology being

+------------+       +--------------+       +---------------------+
|  Sensor   _|       |_   Sensor   _|       |_                    |
| Subdev A |0| ----> |0| Subdev B |1| ----> |0| imx6-mipi-csi2 ...|
|           ¯|       |¯            ¯|       |¯                    |
+------------+       +--------------+       +---------------------+

while the sensor has a single DT node with a single port. The IPU CSI
driver then tries to connect 'Sensor Subdev B':0 to 'imx6-mipi-csi2':0,
which isn't right.

The fix isn't perfect, as we ideally need media entity operations to
handle the translation between port ids and pad indices. Nonetheless,
the heuristic in patch 1/2 should cover most, if not all, use cases and
is in my opinion a good way forward.

Patch 2/2 then stops creating links between external entities. The IPU
CSI driver iterates over the list of subdevs registered with the V4L2
device, and creates links based on the OF graph for all of them. That's
fine for devices internal to the i.MX6, but is oversteps the driver's
responsibility as links between external entities are supposed to be
handled by their respective driver. In my use case, the driver attempts
to link 'Sensor Subdev A' by checking OF graph connections in DT, which
results in 'Sensor Subdev A':0 being linked to 'imx6-mipi-csi2':0. The
patch skips external entities for link creation, which fixes the issue.
The link between the closest external entities and the internal entities
are still created by the IPU CSI driver as part of link creation for all
internal entities.

Laurent Pinchart (2):
  media: staging/imx: Don't assume OF port id equals pad index
  media: staging/imx: Don't create links between external entities

 drivers/staging/media/imx/imx-media-csi.c     | 29 ++++++++++-
 .../staging/media/imx/imx-media-dev-common.c  |  7 +--
 drivers/staging/media/imx/imx-media-of.c      | 52 ++++++++++++++++---
 drivers/staging/media/imx/imx-media.h         |  1 +
 4 files changed, 75 insertions(+), 14 deletions(-)

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] media: staging/imx: Don't assume OF port id equals pad index
  2020-04-13  1:14 [PATCH 0/2] media: imx6: Support complex external topologies Laurent Pinchart
@ 2020-04-13  1:14 ` Laurent Pinchart
  2020-04-13  9:34     ` kbuild test robot
  2020-04-13  1:14 ` [PATCH 2/2] media: staging/imx: Don't create links between external entities Laurent Pinchart
  2020-04-13 18:16 ` [PATCH 0/2] media: imx6: Support complex external topologies Steve Longerbeam
  2 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2020-04-13  1:14 UTC (permalink / raw)
  To: linux-media; +Cc: Steve Longerbeam, Philipp Zabel

When parsing the graph and linking entities, the driver assumes that an
OF port if is always equal to the entity pad index. This isn't a valid
assumption, as for instance a CSI-2 source could be a SMIA++-compatible
sensor that creates two or more subdevs but has a single DT node.

Media entities should provide an operation to map port ids to pad
indexes. Until this is available, work around the issue locally by
picking the first pad that has the expected direction if the pad pointed
to by the port id doesn't have the right direction.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx-media-csi.c | 10 ++++-
 drivers/staging/media/imx/imx-media-of.c  | 52 ++++++++++++++++++++---
 2 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 66468326bcbc..d275decc79be 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -199,10 +199,16 @@ static int csi_get_upstream_endpoint(struct csi_priv *priv,
 	sd = media_entity_to_v4l2_subdev(pad->entity);
 
 	/*
-	 * NOTE: this assumes an OF-graph port id is the same as a
-	 * media pad index.
+	 * Find the port corresponding to the pad. Start by assuming that the
+	 * port id is equal to the pad index. If there's no such port, use the
+	 * first port.
+	 *
+	 * FIXME: Media entities should provide an operation to translate from
+	 * pad index to fwnode port id.
 	 */
 	port = of_graph_get_port_by_id(sd->dev->of_node, pad->index);
+	if (!port)
+		port = of_graph_get_port_by_id(sd->dev->of_node, 0);
 	if (!port)
 		return -ENODEV;
 
diff --git a/drivers/staging/media/imx/imx-media-of.c b/drivers/staging/media/imx/imx-media-of.c
index 2d3efd2a6dde..f07bd05b8fa5 100644
--- a/drivers/staging/media/imx/imx-media-of.c
+++ b/drivers/staging/media/imx/imx-media-of.c
@@ -78,9 +78,8 @@ EXPORT_SYMBOL_GPL(imx_media_add_of_subdevs);
 /*
  * Create a single media link to/from sd using a fwnode link.
  *
- * NOTE: this function assumes an OF port node is equivalent to
- * a media pad (port id equal to media pad index), and that an
- * OF endpoint node is equivalent to a media link.
+ * NOTE: this function assumes that an OF endpoint node is equivalent to a
+ * media link.
  */
 static int create_of_link(struct imx_media_dev *imxmd,
 			  struct v4l2_subdev *sd,
@@ -88,6 +87,8 @@ static int create_of_link(struct imx_media_dev *imxmd,
 {
 	struct v4l2_subdev *remote, *src, *sink;
 	int src_pad, sink_pad;
+	int remote_pad;
+	u32 pad_flags;
 
 	if (link->local_port >= sd->entity.num_pads)
 		return -EINVAL;
@@ -96,19 +97,56 @@ static int create_of_link(struct imx_media_dev *imxmd,
 	if (!remote)
 		return 0;
 
-	if (sd->entity.pads[link->local_port].flags & MEDIA_PAD_FL_SINK) {
+	/*
+	 * Find the remote pad. Try the pad corresponding to the fwnode port id
+	 * first. If its direction doesn't correspond to what we expect, use the
+	 * first pad that has the right direction.
+	 *
+	 * FIXME: Media entities should provide an operation to translate from
+	 * fwnode port id to pad index.
+	 */
+	pad_flags = sd->entity.pads[link->local_port].flags;
+	pad_flags = pad_flags & MEDIA_PAD_FL_SINK
+		  ? MEDIA_PAD_FL_SOURCE : MEDIA_PAD_FL_SINK;
+
+	if (link->remote_port < remote->entity.num_pads &&
+	    remote->entity.pads[link->remote_port].flags & pad_flags) {
+		remote_pad = link->remote_port;
+	} else {
+		unsigned int i;
+
+		remote_pad = -1;
+		for (i = 0; i < remote->entity.num_pads; ++i) {
+			if (remote->entity.pads[i].flags & pad_flags) {
+				remote_pad = i;
+				break;
+			}
+		}
+
+		if (remote_pad == -1) {
+			v4l2_err(sd->v4l2_dev,
+				 "remote entity %s has no %s pad\n",
+				 remote->name,
+				 pad_flags & MEDIA_PAD_FL_SOURCE ?
+				 "source" : "sink");
+			return -EINVAL;
+		}
+	}
+
+	/* Mad the local and remote entities to source and sink. */
+	if (pad_flags & MEDIA_PAD_FL_SOURCE) {
 		src = remote;
-		src_pad = link->remote_port;
+		src_pad = remote_pad;
 		sink = sd;
 		sink_pad = link->local_port;
 	} else {
 		src = sd;
 		src_pad = link->local_port;
 		sink = remote;
-		sink_pad = link->remote_port;
+		sink_pad = remote_pad;
 	}
 
-	/* make sure link doesn't already exist before creating */
+	/* Make sure link doesn't already exist before creating it. */
 	if (media_entity_find_link(&src->entity.pads[src_pad],
 				   &sink->entity.pads[sink_pad]))
 		return 0;
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] media: staging/imx: Don't create links between external entities
  2020-04-13  1:14 [PATCH 0/2] media: imx6: Support complex external topologies Laurent Pinchart
  2020-04-13  1:14 ` [PATCH 1/2] media: staging/imx: Don't assume OF port id equals pad index Laurent Pinchart
@ 2020-04-13  1:14 ` Laurent Pinchart
  2020-04-13 18:16 ` [PATCH 0/2] media: imx6: Support complex external topologies Steve Longerbeam
  2 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2020-04-13  1:14 UTC (permalink / raw)
  To: linux-media; +Cc: Steve Longerbeam, Philipp Zabel

When creating links between media entities, the driver walks all the
subdevs that are part of the media device and attempts to create links
for all of them. This is wrong, as external subdevs are responsible for
creating links outside of the SoC.

To fix this, replace the default case by two explicit cases, to only
create links for the CSI MUXes and the CSI-2 receiver. This requires
assigning a group ID to the CSI MUX subdevs, as they're handled by a
separate driver.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx-media-csi.c     | 19 +++++++++++++++++++
 .../staging/media/imx/imx-media-dev-common.c  |  7 ++-----
 drivers/staging/media/imx/imx-media.h         |  1 +
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index d275decc79be..b7dcdb004cc2 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1875,6 +1875,24 @@ static const struct v4l2_subdev_internal_ops csi_internal_ops = {
 	.unregistered = csi_unregistered,
 };
 
+static int imx_csi_notifier_bound(struct v4l2_async_notifier *notifier,
+				  struct v4l2_subdev *sd,
+				  struct v4l2_async_subdev *asd)
+{
+	/*
+	 * If the subdev has no group id, it must be one of the csi muxes. Mark
+	 * it as such to help with link creation in imx_media_create_links().
+	 */
+	if (!sd->grp_id)
+		sd->grp_id = IMX_MEDIA_GRP_ID_CSI_MUX;
+
+	return 0;
+}
+
+static const struct v4l2_async_notifier_operations imx_csi_notifier_ops = {
+	.bound = imx_csi_notifier_bound,
+};
+
 static int imx_csi_parse_endpoint(struct device *dev,
 				  struct v4l2_fwnode_endpoint *vep,
 				  struct v4l2_async_subdev *asd)
@@ -1894,6 +1912,7 @@ static int imx_csi_async_register(struct csi_priv *priv)
 		return -ENOMEM;
 
 	v4l2_async_notifier_init(notifier);
+	notifier->ops = &imx_csi_notifier_ops;
 
 	fwnode = dev_fwnode(priv->dev);
 
diff --git a/drivers/staging/media/imx/imx-media-dev-common.c b/drivers/staging/media/imx/imx-media-dev-common.c
index 66b505f7e8df..abbb8c74910b 100644
--- a/drivers/staging/media/imx/imx-media-dev-common.c
+++ b/drivers/staging/media/imx/imx-media-dev-common.c
@@ -54,11 +54,8 @@ static int imx_media_create_links(struct v4l2_async_notifier *notifier)
 		case IMX_MEDIA_GRP_ID_CSI:
 			imx_media_create_csi_of_links(imxmd, sd);
 			break;
-		default:
-			/*
-			 * if this subdev has fwnode links, create media
-			 * links for them.
-			 */
+		case IMX_MEDIA_GRP_ID_CSI2:
+		case IMX_MEDIA_GRP_ID_CSI_MUX:
 			imx_media_create_of_links(imxmd, sd);
 			break;
 		}
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index ca36beca16de..b5b7d3245727 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -311,5 +311,6 @@ void imx_media_csc_scaler_device_unregister(struct imx_media_video_dev *vdev);
 #define IMX_MEDIA_GRP_ID_IPU_IC_PRP    BIT(13)
 #define IMX_MEDIA_GRP_ID_IPU_IC_PRPENC BIT(14)
 #define IMX_MEDIA_GRP_ID_IPU_IC_PRPVF  BIT(15)
+#define IMX_MEDIA_GRP_ID_CSI_MUX       BIT(16)
 
 #endif
-- 
Regards,

Laurent Pinchart


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] media: staging/imx: Don't assume OF port id equals pad index
  2020-04-13  1:14 ` [PATCH 1/2] media: staging/imx: Don't assume OF port id equals pad index Laurent Pinchart
@ 2020-04-13  9:34     ` kbuild test robot
  0 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2020-04-13  9:34 UTC (permalink / raw)
  To: Laurent Pinchart; +Cc: kbuild-all, linux-media, Steve Longerbeam, Philipp Zabel

Hi Laurent,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.7-rc1 next-20200413]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Laurent-Pinchart/media-imx6-Support-complex-external-topologies/20200413-091602
base:   git://linuxtv.org/media_tree.git master

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


cppcheck warnings: (new ones prefixed by >>)

   drivers/staging/media/imx/imx-media-of.c:169:6: warning: The scope of the variable 'ret' can be reduced. [variableScope]
    int ret;
        ^
>> drivers/staging/media/imx/imx-media-of.c:110:5: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
       ? MEDIA_PAD_FL_SOURCE : MEDIA_PAD_FL_SINK;
       ^
   drivers/staging/media/imx/imx-media-of.c:130:38: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
        pad_flags & MEDIA_PAD_FL_SOURCE ?
                                        ^

vim +110 drivers/staging/media/imx/imx-media-of.c

    77	
    78	/*
    79	 * Create a single media link to/from sd using a fwnode link.
    80	 *
    81	 * NOTE: this function assumes that an OF endpoint node is equivalent to a
    82	 * media link.
    83	 */
    84	static int create_of_link(struct imx_media_dev *imxmd,
    85				  struct v4l2_subdev *sd,
    86				  struct v4l2_fwnode_link *link)
    87	{
    88		struct v4l2_subdev *remote, *src, *sink;
    89		int src_pad, sink_pad;
    90		int remote_pad;
    91		u32 pad_flags;
    92	
    93		if (link->local_port >= sd->entity.num_pads)
    94			return -EINVAL;
    95	
    96		remote = imx_media_find_subdev_by_fwnode(imxmd, link->remote_node);
    97		if (!remote)
    98			return 0;
    99	
   100		/*
   101		 * Find the remote pad. Try the pad corresponding to the fwnode port id
   102		 * first. If its direction doesn't correspond to what we expect, use the
   103		 * first pad that has the right direction.
   104		 *
   105		 * FIXME: Media entities should provide an operation to translate from
   106		 * fwnode port id to pad index.
   107		 */
   108		pad_flags = sd->entity.pads[link->local_port].flags;
   109		pad_flags = pad_flags & MEDIA_PAD_FL_SINK
 > 110			  ? MEDIA_PAD_FL_SOURCE : MEDIA_PAD_FL_SINK;
   111	
   112		if (link->remote_port < remote->entity.num_pads &&
   113		    remote->entity.pads[link->remote_port].flags & pad_flags) {
   114			remote_pad = link->remote_port;
   115		} else {
   116			unsigned int i;
   117	
   118			remote_pad = -1;
   119			for (i = 0; i < remote->entity.num_pads; ++i) {
   120				if (remote->entity.pads[i].flags & pad_flags) {
   121					remote_pad = i;
   122					break;
   123				}
   124			}
   125	
   126			if (remote_pad == -1) {
   127				v4l2_err(sd->v4l2_dev,
   128					 "remote entity %s has no %s pad\n",
   129					 remote->name,
   130					 pad_flags & MEDIA_PAD_FL_SOURCE ?
   131					 "source" : "sink");
   132				return -EINVAL;
   133			}
   134		}
   135	
   136		/* Mad the local and remote entities to source and sink. */
   137		if (pad_flags & MEDIA_PAD_FL_SOURCE) {
   138			src = remote;
   139			src_pad = remote_pad;
   140			sink = sd;
   141			sink_pad = link->local_port;
   142		} else {
   143			src = sd;
   144			src_pad = link->local_port;
   145			sink = remote;
   146			sink_pad = remote_pad;
   147		}
   148	
   149		/* Make sure link doesn't already exist before creating it. */
   150		if (media_entity_find_link(&src->entity.pads[src_pad],
   151					   &sink->entity.pads[sink_pad]))
   152			return 0;
   153	
   154		v4l2_info(sd->v4l2_dev, "%s:%d -> %s:%d\n",
   155			  src->name, src_pad, sink->name, sink_pad);
   156	
   157		return media_create_pad_link(&src->entity, src_pad,
   158					     &sink->entity, sink_pad, 0);
   159	}
   160	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] media: staging/imx: Don't assume OF port id equals pad index
@ 2020-04-13  9:34     ` kbuild test robot
  0 siblings, 0 replies; 6+ messages in thread
From: kbuild test robot @ 2020-04-13  9:34 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 4346 bytes --]

Hi Laurent,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v5.7-rc1 next-20200413]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Laurent-Pinchart/media-imx6-Support-complex-external-topologies/20200413-091602
base:   git://linuxtv.org/media_tree.git master

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


cppcheck warnings: (new ones prefixed by >>)

   drivers/staging/media/imx/imx-media-of.c:169:6: warning: The scope of the variable 'ret' can be reduced. [variableScope]
    int ret;
        ^
>> drivers/staging/media/imx/imx-media-of.c:110:5: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
       ? MEDIA_PAD_FL_SOURCE : MEDIA_PAD_FL_SINK;
       ^
   drivers/staging/media/imx/imx-media-of.c:130:38: warning: Clarify calculation precedence for '&' and '?'. [clarifyCalculation]
        pad_flags & MEDIA_PAD_FL_SOURCE ?
                                        ^

vim +110 drivers/staging/media/imx/imx-media-of.c

    77	
    78	/*
    79	 * Create a single media link to/from sd using a fwnode link.
    80	 *
    81	 * NOTE: this function assumes that an OF endpoint node is equivalent to a
    82	 * media link.
    83	 */
    84	static int create_of_link(struct imx_media_dev *imxmd,
    85				  struct v4l2_subdev *sd,
    86				  struct v4l2_fwnode_link *link)
    87	{
    88		struct v4l2_subdev *remote, *src, *sink;
    89		int src_pad, sink_pad;
    90		int remote_pad;
    91		u32 pad_flags;
    92	
    93		if (link->local_port >= sd->entity.num_pads)
    94			return -EINVAL;
    95	
    96		remote = imx_media_find_subdev_by_fwnode(imxmd, link->remote_node);
    97		if (!remote)
    98			return 0;
    99	
   100		/*
   101		 * Find the remote pad. Try the pad corresponding to the fwnode port id
   102		 * first. If its direction doesn't correspond to what we expect, use the
   103		 * first pad that has the right direction.
   104		 *
   105		 * FIXME: Media entities should provide an operation to translate from
   106		 * fwnode port id to pad index.
   107		 */
   108		pad_flags = sd->entity.pads[link->local_port].flags;
   109		pad_flags = pad_flags & MEDIA_PAD_FL_SINK
 > 110			  ? MEDIA_PAD_FL_SOURCE : MEDIA_PAD_FL_SINK;
   111	
   112		if (link->remote_port < remote->entity.num_pads &&
   113		    remote->entity.pads[link->remote_port].flags & pad_flags) {
   114			remote_pad = link->remote_port;
   115		} else {
   116			unsigned int i;
   117	
   118			remote_pad = -1;
   119			for (i = 0; i < remote->entity.num_pads; ++i) {
   120				if (remote->entity.pads[i].flags & pad_flags) {
   121					remote_pad = i;
   122					break;
   123				}
   124			}
   125	
   126			if (remote_pad == -1) {
   127				v4l2_err(sd->v4l2_dev,
   128					 "remote entity %s has no %s pad\n",
   129					 remote->name,
   130					 pad_flags & MEDIA_PAD_FL_SOURCE ?
   131					 "source" : "sink");
   132				return -EINVAL;
   133			}
   134		}
   135	
   136		/* Mad the local and remote entities to source and sink. */
   137		if (pad_flags & MEDIA_PAD_FL_SOURCE) {
   138			src = remote;
   139			src_pad = remote_pad;
   140			sink = sd;
   141			sink_pad = link->local_port;
   142		} else {
   143			src = sd;
   144			src_pad = link->local_port;
   145			sink = remote;
   146			sink_pad = remote_pad;
   147		}
   148	
   149		/* Make sure link doesn't already exist before creating it. */
   150		if (media_entity_find_link(&src->entity.pads[src_pad],
   151					   &sink->entity.pads[sink_pad]))
   152			return 0;
   153	
   154		v4l2_info(sd->v4l2_dev, "%s:%d -> %s:%d\n",
   155			  src->name, src_pad, sink->name, sink_pad);
   156	
   157		return media_create_pad_link(&src->entity, src_pad,
   158					     &sink->entity, sink_pad, 0);
   159	}
   160	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/2] media: imx6: Support complex external topologies
  2020-04-13  1:14 [PATCH 0/2] media: imx6: Support complex external topologies Laurent Pinchart
  2020-04-13  1:14 ` [PATCH 1/2] media: staging/imx: Don't assume OF port id equals pad index Laurent Pinchart
  2020-04-13  1:14 ` [PATCH 2/2] media: staging/imx: Don't create links between external entities Laurent Pinchart
@ 2020-04-13 18:16 ` Steve Longerbeam
  2 siblings, 0 replies; 6+ messages in thread
From: Steve Longerbeam @ 2020-04-13 18:16 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media; +Cc: Philipp Zabel

Hi Laurent,

I've already done this work, see [1]. The fwnode related changes are 
awaiting an ack from Sakari.

It fixes this stuff the right way in my opinion. The patchset relies on 
the subdevs to create links to their discovered upstream subdevs, via 
the subdev notifier's bound callbacks. In other words it comprehensively 
accomplishes the distributed link creation that you're partially doing here.

If you were to implement .get_fwnode_pad in the smiapp driver, and use 
patchset [1], I think it should resolve the issues with smiapp subdev, 
or any other subdevs with complex internal topologies (another example I 
can think of is the adv748x). Specifically see [2] which will create the 
link to smiapp in the mipi csi-2 receiver's bound callback with the help 
of a new helper function imx_media_create_fwnode_pad_link().

Steve

[1] https://patchwork.linuxtv.org/patch/61927/
[2] https://patchwork.linuxtv.org/patch/61936/


On 4/12/20 6:14 PM, Laurent Pinchart wrote:
> Hello,
>
> This small patch series adds support to the i.MX6 IPU CSI driver for
> external (on-board) devices that exhibit complex topologies. My use case
> is a CSI-2 camera sensor that has a single DT node (as an I2C device)
> and creates two V4L2 subdevs. "Complex" may thus be a bit of an
> exageration, but the series nonetheless makes such a sensor usable with
> the driver.
>
> There are two issues, fixed by two patches. Patch 1/2 removes the
> assumption that OF port ids and pad indices are equivalent. This holds
> true in many cases, but is incorrect in my case, with the topology being
>
> +------------+       +--------------+       +---------------------+
> |  Sensor   _|       |_   Sensor   _|       |_                    |
> | Subdev A |0| ----> |0| Subdev B |1| ----> |0| imx6-mipi-csi2 ...|
> |           ¯|       |¯            ¯|       |¯                    |
> +------------+       +--------------+       +---------------------+
>
> while the sensor has a single DT node with a single port. The IPU CSI
> driver then tries to connect 'Sensor Subdev B':0 to 'imx6-mipi-csi2':0,
> which isn't right.
>
> The fix isn't perfect, as we ideally need media entity operations to
> handle the translation between port ids and pad indices. Nonetheless,
> the heuristic in patch 1/2 should cover most, if not all, use cases and
> is in my opinion a good way forward.
>
> Patch 2/2 then stops creating links between external entities. The IPU
> CSI driver iterates over the list of subdevs registered with the V4L2
> device, and creates links based on the OF graph for all of them. That's
> fine for devices internal to the i.MX6, but is oversteps the driver's
> responsibility as links between external entities are supposed to be
> handled by their respective driver. In my use case, the driver attempts
> to link 'Sensor Subdev A' by checking OF graph connections in DT, which
> results in 'Sensor Subdev A':0 being linked to 'imx6-mipi-csi2':0. The
> patch skips external entities for link creation, which fixes the issue.
> The link between the closest external entities and the internal entities
> are still created by the IPU CSI driver as part of link creation for all
> internal entities.
>
> Laurent Pinchart (2):
>    media: staging/imx: Don't assume OF port id equals pad index
>    media: staging/imx: Don't create links between external entities
>
>   drivers/staging/media/imx/imx-media-csi.c     | 29 ++++++++++-
>   .../staging/media/imx/imx-media-dev-common.c  |  7 +--
>   drivers/staging/media/imx/imx-media-of.c      | 52 ++++++++++++++++---
>   drivers/staging/media/imx/imx-media.h         |  1 +
>   4 files changed, 75 insertions(+), 14 deletions(-)
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-04-13 18:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-13  1:14 [PATCH 0/2] media: imx6: Support complex external topologies Laurent Pinchart
2020-04-13  1:14 ` [PATCH 1/2] media: staging/imx: Don't assume OF port id equals pad index Laurent Pinchart
2020-04-13  9:34   ` kbuild test robot
2020-04-13  9:34     ` kbuild test robot
2020-04-13  1:14 ` [PATCH 2/2] media: staging/imx: Don't create links between external entities Laurent Pinchart
2020-04-13 18:16 ` [PATCH 0/2] media: imx6: Support complex external topologies Steve Longerbeam

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.