[RFC/PATCH] media: Add video bus switch

Commit Message

Pavel Machek Dec. 22, 2016, 10:42 p.m. UTC

On Thu 2016-12-22 15:32:44, Sebastian Reichel wrote:
> Hi Pavel,
> 
> On Thu, Dec 22, 2016 at 02:39:38PM +0100, Pavel Machek wrote:
> > N900 contains front and back camera, with a switch between the
> > two. This adds support for the swich component.
> > 
> > Signed-off-by: Sebastian Reichel <[e-mail]>
> > Signed-off-by: Ivaylo Dimitrov <[e-mail]>
> > Signed-off-by: Pavel Machek <[e-mail]>
> > 
> > --
> > 
> > I see this needs dts documentation, anything else than needs to be
> > done?
> 
> Yes. This driver takes care of the switch gpio, but the cameras also
> use different bus settings. Currently omap3isp gets the bus-settings
> from the link connected to the CCP2 port in DT at probe time (*).
> 
> So there are two general problems:
> 
> 1. Settings must be applied before the streaming starts instead of
> at probe time, since the settings may change (based one the selected
> camera). That should be fairly easy to implement by just moving the
> code to the s_stream callback as far as I can see.
> 
> 2. omap3isp should try to get the bus settings from using a callback
> in the connected driver instead of loading it from DT. Then the
> video-bus-switch can load the bus-settings from its downstream links
> in DT and propagate the correct ones to omap3isp based on the
> selected port. The DT loading part should actually remain in omap3isp
> as fallback, in case it does not find a callback in the connected driver.
> That way everything is backward compatible and the DT variant is
> nice for 1-on-1 scenarios.

So... did I understood it correctly? (Needs some work to be done...)

Comments

Sebastian Reichel Dec. 22, 2016, 11:40 p.m. UTC | #1

Hi,

On Thu, Dec 22, 2016 at 11:42:26PM +0100, Pavel Machek wrote:
> On Thu 2016-12-22 15:32:44, Sebastian Reichel wrote:
> > Hi Pavel,
> > 
> > On Thu, Dec 22, 2016 at 02:39:38PM +0100, Pavel Machek wrote:
> > > N900 contains front and back camera, with a switch between the
> > > two. This adds support for the swich component.
> > > 
> > > Signed-off-by: Sebastian Reichel <[e-mail]>
> > > Signed-off-by: Ivaylo Dimitrov <[e-mail]>
> > > Signed-off-by: Pavel Machek <[e-mail]>
> > > 
> > > --
> > > 
> > > I see this needs dts documentation, anything else than needs to be
> > > done?
> > 
> > Yes. This driver takes care of the switch gpio, but the cameras also
> > use different bus settings. Currently omap3isp gets the bus-settings
> > from the link connected to the CCP2 port in DT at probe time (*).
> > 
> > So there are two general problems:
> > 
> > 1. Settings must be applied before the streaming starts instead of
> > at probe time, since the settings may change (based one the selected
> > camera). That should be fairly easy to implement by just moving the
> > code to the s_stream callback as far as I can see.
> > 
> > 2. omap3isp should try to get the bus settings from using a callback
> > in the connected driver instead of loading it from DT. Then the
> > video-bus-switch can load the bus-settings from its downstream links
> > in DT and propagate the correct ones to omap3isp based on the
> > selected port. The DT loading part should actually remain in omap3isp
> > as fallback, in case it does not find a callback in the connected driver.
> > That way everything is backward compatible and the DT variant is
> > nice for 1-on-1 scenarios.
> 
> So... did I understood it correctly? (Needs some work to be done...)

I had a quick look and yes, that's basically what I had in mind to
solve the issue. If callback is not available the old system should
be used of course.

> [...]
>
>  static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> diff --git a/drivers/media/platform/video-bus-switch.c b/drivers/media/platform/video-bus-switch.c
> index 1a5d944..3a2d442 100644
> --- a/drivers/media/platform/video-bus-switch.c
> +++ b/drivers/media/platform/video-bus-switch.c
> @@ -247,12 +247,21 @@ static int vbs_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct v4l2_subdev *subdev = vbs_get_remote_subdev(sd);
>  
> +	/* FIXME: we need to set the GPIO here */
> +

The gpio is set when the pad is selected, so no need to do it again.
The gpio selection actually works with your branch (assuming its
based on Ivo's).

>  	if (IS_ERR(subdev))
>  		return PTR_ERR(subdev);
>  
>  	return v4l2_subdev_call(subdev, video, s_stream, enable);
>  }
>  
> +static int vbs_g_endpoint_config(struct v4l2_subdev *sd, struct isp_bus_cfg *cfg)
> +{
> +	printk("vbs_g_endpoint_config...\n");
> +	return 0;
> +}

Would be nice to find something more abstract than isp_bus_cfg,
which is specific to omap3isp.

-- Sebastian

Pavel Machek Dec. 24, 2016, 2:26 p.m. UTC | #2

Hi!

> > So... did I understood it correctly? (Needs some work to be done...)
> 
> I had a quick look and yes, that's basically what I had in mind to
> solve the issue. If callback is not available the old system should
> be used of course.

Ok, got it to work, thanks for all the help. I'll clean it up now.

Best regards,
									Pavel

Patch

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 45c69ed..1f44da1 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -702,6 +704,33 @@  static int isp_pipeline_enable(struct isp_pipeline *pipe,
 
 	entity = &pipe->output->video.entity;
 	while (1) {
+		struct v4l2_of_endpoint vep;
+		pad = &entity->pads[0];
+		if (!(pad->flags & MEDIA_PAD_FL_SINK))
+			break;
+
+		pad = media_entity_remote_pad(pad);
+		if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
+			break;
+
+		entity = pad->entity;
+		subdev = media_entity_to_v4l2_subdev(entity);
+
+	       	printk("Entity = %p\n", entity);
+		ret = v4l2_subdev_call(subdev, video, g_endpoint_config, &vep);
+		/* Is there better method than walking a list?
+		   Can I easily get dev and isd pointers here? */
+#if 0
+		if (ret == 0) {
+			printk("success\n");
+			/* notifier->subdevs[notifier->num_subdevs] ... contains isd */
+			isp_endpoint_to_buscfg(dev, vep, isd->bus);
+		}
+#endif
+	}
+
+	entity = &pipe->output->video.entity;
+	while (1) {
 		pad = &entity->pads[0];
 		if (!(pad->flags & MEDIA_PAD_FL_SINK))
 			break;
@@ -2099,27 +2128,8 @@  static void isp_of_parse_node_csi2(struct device *dev,
 	buscfg->bus.csi2.crc = 1;
 }
 
-static int isp_of_parse_node_endpoint(struct device *dev,
-				      struct device_node *node,
-				      struct isp_async_subdev *isd)
+static int isp_endpoint_to_buscfg(struct device *dev, struct v4l2_of_endpoint vep, struct isp_bus_cfg *buscfg)
 {
-	struct isp_bus_cfg *buscfg;
-	struct v4l2_of_endpoint vep;
-	int ret;
-
-	isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
-	if (!isd->bus)
-		return -ENOMEM;
-
-	buscfg = isd->bus;
-
-	ret = v4l2_of_parse_endpoint(node, &vep);
-	if (ret)
-		return ret;
-
-	dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
-		vep.base.port);
-
 	switch (vep.base.port) {
 	case ISP_OF_PHY_PARALLEL:
 		buscfg->interface = ISP_INTERFACE_PARALLEL;
@@ -2147,10 +2157,35 @@  static int isp_of_parse_node_endpoint(struct device *dev,
 		break;
 
 	default:
+		return -1;
+	}
+	return 0;
+}
+
+static int isp_of_parse_node_endpoint(struct device *dev,
+				      struct device_node *node,
+				      struct isp_async_subdev *isd)
+{
+	struct isp_bus_cfg *buscfg;
+	struct v4l2_of_endpoint vep;
+	int ret;
+
+	isd->bus = devm_kzalloc(dev, sizeof(*isd->bus), GFP_KERNEL);
+	if (!isd->bus)
+		return -ENOMEM;
+
+	buscfg = isd->bus;
+
+	ret = v4l2_of_parse_endpoint(node, &vep);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "parsing endpoint %s, interface %u\n", node->full_name,
+		vep.base.port);
+
+	if (isp_endpoint_to_buscfg(dev, vep, buscfg))
 		dev_warn(dev, "%s: invalid interface %u\n", node->full_name,
 			 vep.base.port);
-		break;
-	}
 
 	return 0;
 }
@@ -2262,6 +2297,10 @@  static int isp_of_parse_nodes(struct device *dev,
 	}
 
 	return notifier->num_subdevs;
+
+error:
+	of_node_put(node);
+	return -EINVAL;
 }
 
 static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
diff --git a/drivers/media/platform/video-bus-switch.c b/drivers/media/platform/video-bus-switch.c
index 1a5d944..3a2d442 100644
--- a/drivers/media/platform/video-bus-switch.c
+++ b/drivers/media/platform/video-bus-switch.c
@@ -247,12 +247,21 @@  static int vbs_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct v4l2_subdev *subdev = vbs_get_remote_subdev(sd);
 
+	/* FIXME: we need to set the GPIO here */
+
 	if (IS_ERR(subdev))
 		return PTR_ERR(subdev);
 
 	return v4l2_subdev_call(subdev, video, s_stream, enable);
 }
 
+static int vbs_g_endpoint_config(struct v4l2_subdev *sd, struct isp_bus_cfg *cfg)
+{
+	printk("vbs_g_endpoint_config...\n");
+	return 0;
+}
+
+
 static const struct v4l2_subdev_internal_ops vbs_internal_ops = {
 	.registered = &vbs_registered,
 };
@@ -265,6 +274,7 @@  static const struct media_entity_operations vbs_media_ops = {
 /* subdev video operations */
 static const struct v4l2_subdev_video_ops vbs_video_ops = {
 	.s_stream = vbs_s_stream,
+	.g_endpoint_config = vbs_g_endpoint_config,
 };
 
 static const struct v4l2_subdev_ops vbs_ops = {
diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index cf778c5..30457b0 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -415,6 +415,8 @@  struct v4l2_subdev_video_ops {
 			     const struct v4l2_mbus_config *cfg);
 	int (*s_rx_buffer)(struct v4l2_subdev *sd, void *buf,
 			   unsigned int *size);
+	int (*g_endpoint_config)(struct v4l2_subdev *sd,
+			    struct v4l2_of_endpoint *cfg);
 };
 
 /**