[PATCH 1/2] [media] dt-bindings: Add bindings for video-multiplexer device

[PATCH 1/2] [media] dt-bindings: Add bindings for video-multiplexer device

[Philipp Zabel]

Add bindings documentation for the video multiplexer device.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
This has been last sent as part of Steve's i.MX media series. Since the binding
changed, I've dropped Rob's ack.

Changes since https://patchwork.kernel.org/patch/9647951/:
 - Replaced re, bit-mask/shift, and gpios properties with a single mux-controls
   property, leaving the actual operation of the mux to a separate mux
   controller, as described by Peter's mux-controller bindings:
   https://patchwork.kernel.org/patch/9695835/
 - Shortened 'video-multiplexer' compatible to 'video-mux', aligning with the
   other mux bindings.
 - Added a comment about the optional ports node and a link to the OF graph
   bindings document.
---
 .../devicetree/bindings/media/video-mux.txt        | 60 ++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/video-mux.txt

diff --git a/Documentation/devicetree/bindings/media/video-mux.txt b/Documentation/devicetree/bindings/media/video-mux.txt
new file mode 100644
index 0000000000000..63b9dc913e456
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/video-mux.txt
@@ -0,0 +1,60 @@
+Video Multiplexer
+=================
+
+Video multiplexers allow to select between multiple input ports. Video received
+on the active input port is passed through to the output port. Muxes described
+by this binding are controlled by a multiplexer controller that is described by
+the bindings in Documentation/devicetree/bindings/mux/mux-controller.txt
+
+Required properties:
+- compatible : should be "video-mux"
+- mux-controls : mux controller node to use for operating the mux
+- #address-cells: should be <1>
+- #size-cells: should be <0>
+- port@*: at least three port nodes containing endpoints connecting to the
+  source and sink devices according to of_graph bindings. The last port is
+  the output port, all others are inputs.
+
+Optionally, #address-cells, #size-cells, and port nodes can be grouped under a
+ports node as described in Documentation/devicetree/bindings/graph.txt.
+
+Example:
+
+	mux: mux-controller {
+		compatible = "gpio-mux";
+		#mux-control-cells = <0>;
+
+		mux-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH>;
+	};
+
+	video-mux {
+		compatible = "video-mux";
+		mux-controls = <&mux>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			mux_in0: endpoint {
+				remote-endpoint = <&video_source0_out>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			mux_in1: endpoint {
+				remote-endpoint = <&video_source1_out>;
+			};
+		};
+
+		port@2 {
+			reg = <2>;
+
+			mux_out: endpoint {
+				remote-endpoint = <&capture_interface_in>;
+			};
+		};
+	};
+};
-- 
2.11.0

[PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Philipp Zabel]

This driver can handle SoC internal and external video bus multiplexers,
controlled by mux controllers provided by the mux controller framework,
such as MMIO register bitfields or GPIOs. The subdevice passes through
the mbus configuration of the active input to the output side.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
This has been last sent as part of the i.MX media series.

Changes since https://patchwork.kernel.org/patch/9647869/:
 - Split out the actual mux operation to be provided by the mux controller
   framework [1]. GPIO and MMIO control can be provided by individual mux
   controller drivers [2][3].
   [1] https://patchwork.kernel.org/patch/9695837/
   [2] https://patchwork.kernel.org/patch/9695839/
   [3] https://patchwork.kernel.org/patch/9704509/
 - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
   vidsw with video_mux.
 - Made the mux inactive by default, only activated by user interaction.
 - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
 - Reuse subdev.entity.num_pads instead of keeping our own count.
 - Removed implicit link disabling. Instead, trying to enable a second
   sink pad link yields -EBUSY.
 - Merged _async_init into _probe.
 - Removed superfluous pad index check from _set_format.
 - Added is_source_pad helper to tell source and sink pads apart.
 - Removed test for status property in endpoint nodes. Disable the remote
   device or sever the endpoint link to disable a sink pad.
---
 drivers/media/platform/Kconfig     |   6 +
 drivers/media/platform/Makefile    |   2 +
 drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
 3 files changed, 349 insertions(+)
 create mode 100644 drivers/media/platform/video-mux.c

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index c9106e105baba..b046a6d39fee5 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
 	  To compile this driver as a module, choose M here: the
 	  module will be called arv.
 
+config VIDEO_MUX
+	tristate "Video Multiplexer"
+	depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
+	help
+	  This driver provides support for N:1 video bus multiplexers.
+
 config VIDEO_OMAP3
 	tristate "OMAP 3 Camera support"
 	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 349ddf6a69da2..fd2735ca3ff75 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
 
 obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
 
+obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
+
 obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
 obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
 obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
new file mode 100644
index 0000000000000..419541729f67e
--- /dev/null
+++ b/drivers/media/platform/video-mux.c
@@ -0,0 +1,341 @@
+/*
+ * video stream multiplexer controlled via mux control
+ *
+ * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
+ * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/module.h>
+#include <linux/mux/consumer.h>
+#include <linux/of.h>
+#include <linux/of_graph.h>
+#include <linux/platform_device.h>
+#include <media/v4l2-async.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-of.h>
+
+struct video_mux {
+	struct v4l2_subdev subdev;
+	struct media_pad *pads;
+	struct v4l2_mbus_framefmt *format_mbus;
+	struct v4l2_of_endpoint *endpoint;
+	struct mux_control *mux;
+	int active;
+};
+
+static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
+{
+	return container_of(sd, struct video_mux, subdev);
+}
+
+static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
+{
+	return pad == vmux->subdev.entity.num_pads - 1;
+}
+
+static int video_mux_link_setup(struct media_entity *entity,
+				const struct media_pad *local,
+				const struct media_pad *remote, u32 flags)
+{
+	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
+	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
+	int ret;
+
+	/*
+	 * The mux state is determined by the enabled sink pad link.
+	 * Enabling or disabling the source pad link has no effect.
+	 */
+	if (is_source_pad(vmux, local->index))
+		return 0;
+
+	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
+		remote->entity->name, remote->index, local->entity->name,
+		local->index, flags & MEDIA_LNK_FL_ENABLED);
+
+	if (flags & MEDIA_LNK_FL_ENABLED) {
+		if (vmux->active == local->index)
+			return 0;
+
+		if (vmux->active >= 0)
+			return -EBUSY;
+
+		dev_dbg(sd->dev, "setting %d active\n", local->index);
+		ret = mux_control_try_select(vmux->mux, local->index);
+		if (ret < 0)
+			return ret;
+		vmux->active = local->index;
+	} else {
+		if (vmux->active != local->index)
+			return 0;
+
+		dev_dbg(sd->dev, "going inactive\n");
+		mux_control_deselect(vmux->mux);
+		vmux->active = -1;
+	}
+
+	return 0;
+}
+
+static struct media_entity_operations video_mux_ops = {
+	.link_setup = video_mux_link_setup,
+	.link_validate = v4l2_subdev_link_validate,
+};
+
+static bool video_mux_endpoint_disabled(struct device_node *ep)
+{
+	struct device_node *rpp = of_graph_get_remote_port_parent(ep);
+
+	return !of_device_is_available(rpp);
+}
+
+static int video_mux_g_mbus_config(struct v4l2_subdev *sd,
+				   struct v4l2_mbus_config *cfg)
+{
+	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
+	struct v4l2_of_endpoint *endpoint;
+	struct media_pad *pad;
+	int ret;
+
+	if (vmux->active == -1) {
+		dev_err(sd->dev, "no configuration for inactive mux\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Retrieve media bus configuration from the entity connected to the
+	 * active input
+	 */
+	pad = media_entity_remote_pad(&vmux->pads[vmux->active]);
+	if (pad) {
+		sd = media_entity_to_v4l2_subdev(pad->entity);
+		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
+		if (ret == -ENOIOCTLCMD)
+			pad = NULL;
+		else if (ret < 0) {
+			dev_err(sd->dev, "failed to get source configuration\n");
+			return ret;
+		}
+	}
+	if (!pad) {
+		endpoint = &vmux->endpoint[vmux->active];
+
+		/* Mirror the input side on the output side */
+		cfg->type = endpoint->bus_type;
+		if (cfg->type == V4L2_MBUS_PARALLEL ||
+		    cfg->type == V4L2_MBUS_BT656)
+			cfg->flags = endpoint->bus.parallel.flags;
+	}
+
+	return 0;
+}
+
+static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)
+{
+	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
+	struct v4l2_subdev *upstream_sd;
+	struct media_pad *pad;
+
+	if (vmux->active == -1) {
+		dev_err(sd->dev, "Can not start streaming on inactive mux\n");
+		return -EINVAL;
+	}
+
+	pad = media_entity_remote_pad(&sd->entity.pads[vmux->active]);
+	if (!pad) {
+		dev_err(sd->dev, "Failed to find remote source pad\n");
+		return -ENOLINK;
+	}
+
+	if (!is_media_entity_v4l2_subdev(pad->entity)) {
+		dev_err(sd->dev, "Upstream entity is not a v4l2 subdev\n");
+		return -ENODEV;
+	}
+
+	upstream_sd = media_entity_to_v4l2_subdev(pad->entity);
+
+	return v4l2_subdev_call(upstream_sd, video, s_stream, enable);
+}
+
+static const struct v4l2_subdev_video_ops video_mux_subdev_video_ops = {
+	.g_mbus_config = video_mux_g_mbus_config,
+	.s_stream = video_mux_s_stream,
+};
+
+static struct v4l2_mbus_framefmt *
+__video_mux_get_pad_format(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_pad_config *cfg,
+			   unsigned int pad, u32 which)
+{
+	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
+
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_format(sd, cfg, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &vmux->format_mbus[pad];
+	default:
+		return NULL;
+	}
+}
+
+static int video_mux_get_format(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_pad_config *cfg,
+			    struct v4l2_subdev_format *sdformat)
+{
+	sdformat->format = *__video_mux_get_pad_format(sd, cfg, sdformat->pad,
+						   sdformat->which);
+	return 0;
+}
+
+static int video_mux_set_format(struct v4l2_subdev *sd,
+			    struct v4l2_subdev_pad_config *cfg,
+			    struct v4l2_subdev_format *sdformat)
+{
+	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
+	struct v4l2_mbus_framefmt *mbusformat;
+
+	mbusformat = __video_mux_get_pad_format(sd, cfg, sdformat->pad,
+					    sdformat->which);
+	if (!mbusformat)
+		return -EINVAL;
+
+	/* Source pad mirrors active sink pad, no limitations on sink pads */
+	if (is_source_pad(vmux, sdformat->pad) && vmux->active >= 0)
+		sdformat->format = vmux->format_mbus[vmux->active];
+
+	*mbusformat = sdformat->format;
+
+	return 0;
+}
+
+static struct v4l2_subdev_pad_ops video_mux_pad_ops = {
+	.get_fmt = video_mux_get_format,
+	.set_fmt = video_mux_set_format,
+};
+
+static struct v4l2_subdev_ops video_mux_subdev_ops = {
+	.pad = &video_mux_pad_ops,
+	.video = &video_mux_subdev_video_ops,
+};
+
+static int video_mux_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	struct v4l2_of_endpoint endpoint;
+	struct device_node *ep;
+	struct video_mux *vmux;
+	unsigned int num_pads = 0;
+	int ret;
+	int i;
+
+	vmux = devm_kzalloc(dev, sizeof(*vmux), GFP_KERNEL);
+	if (!vmux)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, vmux);
+
+	v4l2_subdev_init(&vmux->subdev, &video_mux_subdev_ops);
+	snprintf(vmux->subdev.name, sizeof(vmux->subdev.name), "%s", np->name);
+	vmux->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+	vmux->subdev.dev = dev;
+
+	/*
+	 * The largest numbered port is the output port. It determines
+	 * total number of pads.
+	 */
+	for_each_endpoint_of_node(np, ep) {
+		of_graph_parse_endpoint(ep, &endpoint.base);
+		num_pads = max(num_pads, endpoint.base.port + 1);
+	}
+
+	if (num_pads < 2) {
+		dev_err(dev, "Not enough ports %d\n", num_pads);
+		return -EINVAL;
+	}
+
+	vmux->mux = devm_mux_control_get(dev, NULL);
+	if (IS_ERR(vmux->mux)) {
+		ret = PTR_ERR(vmux->mux);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Failed to get mux: %d\n", ret);
+		return ret;
+	}
+
+	vmux->active = -1;
+	vmux->pads = devm_kzalloc(dev, sizeof(*vmux->pads) * num_pads,
+				  GFP_KERNEL);
+	vmux->format_mbus = devm_kzalloc(dev, sizeof(*vmux->format_mbus) *
+					 num_pads, GFP_KERNEL);
+	vmux->endpoint = devm_kzalloc(dev, sizeof(*vmux->endpoint) *
+				      (num_pads - 1), GFP_KERNEL);
+
+	for (i = 0; i < num_pads - 1; i++)
+		vmux->pads[i].flags = MEDIA_PAD_FL_SINK;
+	vmux->pads[num_pads - 1].flags = MEDIA_PAD_FL_SOURCE;
+
+	vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
+	ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
+				     vmux->pads);
+	if (ret < 0)
+		return ret;
+
+	vmux->subdev.entity.ops = &video_mux_ops;
+
+	for_each_endpoint_of_node(np, ep) {
+		v4l2_of_parse_endpoint(ep, &endpoint);
+
+		if (video_mux_endpoint_disabled(ep)) {
+			dev_dbg(dev, "port %d disabled\n", endpoint.base.port);
+			continue;
+		}
+
+		vmux->endpoint[endpoint.base.port] = endpoint;
+	}
+
+	return v4l2_async_register_subdev(&vmux->subdev);
+}
+
+static int video_mux_remove(struct platform_device *pdev)
+{
+	struct video_mux *vmux = platform_get_drvdata(pdev);
+	struct v4l2_subdev *sd = &vmux->subdev;
+
+	v4l2_async_unregister_subdev(sd);
+	media_entity_cleanup(&sd->entity);
+
+	return 0;
+}
+
+static const struct of_device_id video_mux_dt_ids[] = {
+	{ .compatible = "video-mux", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, video_mux_dt_ids);
+
+static struct platform_driver video_mux_driver = {
+	.probe		= video_mux_probe,
+	.remove		= video_mux_remove,
+	.driver		= {
+		.of_match_table = video_mux_dt_ids,
+		.name = "video-mux",
+	},
+};
+
+module_platform_driver(video_mux_driver);
+
+MODULE_DESCRIPTION("video stream multiplexer");
+MODULE_AUTHOR("Sascha Hauer, Pengutronix");
+MODULE_AUTHOR("Philipp Zabel, Pengutronix");
+MODULE_LICENSE("GPL");
-- 
2.11.0

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Peter Rosin]

On 2017-04-28 16:13, Philipp Zabel wrote:
> This driver can handle SoC internal and external video bus multiplexers,
> controlled by mux controllers provided by the mux controller framework,
> such as MMIO register bitfields or GPIOs. The subdevice passes through
> the mbus configuration of the active input to the output side.
> 
> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> Signed-off-by: Steve Longerbeam <steve_longerbeam-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> ---
> This has been last sent as part of the i.MX media series.
> 
> Changes since https://patchwork.kernel.org/patch/9647869/:
>  - Split out the actual mux operation to be provided by the mux controller
>    framework [1]. GPIO and MMIO control can be provided by individual mux
>    controller drivers [2][3].
>    [1] https://patchwork.kernel.org/patch/9695837/
>    [2] https://patchwork.kernel.org/patch/9695839/
>    [3] https://patchwork.kernel.org/patch/9704509/
>  - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
>    vidsw with video_mux.
>  - Made the mux inactive by default, only activated by user interaction.
>  - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
>  - Reuse subdev.entity.num_pads instead of keeping our own count.
>  - Removed implicit link disabling. Instead, trying to enable a second
>    sink pad link yields -EBUSY.
>  - Merged _async_init into _probe.
>  - Removed superfluous pad index check from _set_format.
>  - Added is_source_pad helper to tell source and sink pads apart.
>  - Removed test for status property in endpoint nodes. Disable the remote
>    device or sever the endpoint link to disable a sink pad.
> ---
>  drivers/media/platform/Kconfig     |   6 +
>  drivers/media/platform/Makefile    |   2 +
>  drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 349 insertions(+)
>  create mode 100644 drivers/media/platform/video-mux.c
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index c9106e105baba..b046a6d39fee5 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called arv.
>  
> +config VIDEO_MUX
> +	tristate "Video Multiplexer"
> +	depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
> +	help
> +	  This driver provides support for N:1 video bus multiplexers.
> +
>  config VIDEO_OMAP3
>  	tristate "OMAP 3 Camera support"
>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 349ddf6a69da2..fd2735ca3ff75 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
>  
>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
>  
> +obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
> +
>  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> new file mode 100644
> index 0000000000000..419541729f67e
> --- /dev/null
> +++ b/drivers/media/platform/video-mux.c
> @@ -0,0 +1,341 @@
> +/*
> + * video stream multiplexer controlled via mux control
> + *
> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

2017?

> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/mux/consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-of.h>
> +
> +struct video_mux {
> +	struct v4l2_subdev subdev;
> +	struct media_pad *pads;
> +	struct v4l2_mbus_framefmt *format_mbus;
> +	struct v4l2_of_endpoint *endpoint;
> +	struct mux_control *mux;
> +	int active;
> +};
> +
> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct video_mux, subdev);
> +}
> +
> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
> +{
> +	return pad == vmux->subdev.entity.num_pads - 1;
> +}
> +
> +static int video_mux_link_setup(struct media_entity *entity,
> +				const struct media_pad *local,
> +				const struct media_pad *remote, u32 flags)
> +{
> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +	int ret;
> +
> +	/*
> +	 * The mux state is determined by the enabled sink pad link.
> +	 * Enabling or disabling the source pad link has no effect.
> +	 */
> +	if (is_source_pad(vmux, local->index))
> +		return 0;
> +
> +	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
> +		remote->entity->name, remote->index, local->entity->name,
> +		local->index, flags & MEDIA_LNK_FL_ENABLED);
> +
> +	if (flags & MEDIA_LNK_FL_ENABLED) {
> +		if (vmux->active == local->index)

Here, you shortcut the mux_control_select_trylock test and return "OK"
based on a driver-local variable that is intended to keep track of mux
ownership.

> +			return 0;
> +
> +		if (vmux->active >= 0)

Here too (and this check is not needed, the situation will be covered by
the mux_control_try_select call).

> +			return -EBUSY;
> +
> +		dev_dbg(sd->dev, "setting %d active\n", local->index);
> +		ret = mux_control_try_select(vmux->mux, local->index);
> +		if (ret < 0)
> +			return ret;
> +		vmux->active = local->index;
> +	} else {
> +		if (vmux->active != local->index)
> +			return 0;
> +
> +		dev_dbg(sd->dev, "going inactive\n");
> +		mux_control_deselect(vmux->mux);

But here you let go of the mux *before* you clear the driver-local
ownership indicator. That looks suspicious. My guess is that this is
"safe" because the upper layers has some serialization, but I don't
know. Anyway, even if there is something saving you in the upper
layers, it looks out of order and unneeded. I would have moved the
below vmux->active = -1; statement up to before the above deselect.

With that fixed, mux usage looks good to me, so you can add an Acked-
by from me if you wish (goes for the bindings patch as well).

Cheers,
peda

> +		vmux->active = -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct media_entity_operations video_mux_ops = {
> +	.link_setup = video_mux_link_setup,
> +	.link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static bool video_mux_endpoint_disabled(struct device_node *ep)
> +{
> +	struct device_node *rpp = of_graph_get_remote_port_parent(ep);
> +
> +	return !of_device_is_available(rpp);
> +}
> +
> +static int video_mux_g_mbus_config(struct v4l2_subdev *sd,
> +				   struct v4l2_mbus_config *cfg)
> +{
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +	struct v4l2_of_endpoint *endpoint;
> +	struct media_pad *pad;
> +	int ret;
> +
> +	if (vmux->active == -1) {
> +		dev_err(sd->dev, "no configuration for inactive mux\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Retrieve media bus configuration from the entity connected to the
> +	 * active input
> +	 */
> +	pad = media_entity_remote_pad(&vmux->pads[vmux->active]);
> +	if (pad) {
> +		sd = media_entity_to_v4l2_subdev(pad->entity);
> +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
> +		if (ret == -ENOIOCTLCMD)
> +			pad = NULL;
> +		else if (ret < 0) {
> +			dev_err(sd->dev, "failed to get source configuration\n");
> +			return ret;
> +		}
> +	}
> +	if (!pad) {
> +		endpoint = &vmux->endpoint[vmux->active];
> +
> +		/* Mirror the input side on the output side */
> +		cfg->type = endpoint->bus_type;
> +		if (cfg->type == V4L2_MBUS_PARALLEL ||
> +		    cfg->type == V4L2_MBUS_BT656)
> +			cfg->flags = endpoint->bus.parallel.flags;
> +	}
> +
> +	return 0;
> +}
> +
> +static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +	struct v4l2_subdev *upstream_sd;
> +	struct media_pad *pad;
> +
> +	if (vmux->active == -1) {
> +		dev_err(sd->dev, "Can not start streaming on inactive mux\n");
> +		return -EINVAL;
> +	}
> +
> +	pad = media_entity_remote_pad(&sd->entity.pads[vmux->active]);
> +	if (!pad) {
> +		dev_err(sd->dev, "Failed to find remote source pad\n");
> +		return -ENOLINK;
> +	}
> +
> +	if (!is_media_entity_v4l2_subdev(pad->entity)) {
> +		dev_err(sd->dev, "Upstream entity is not a v4l2 subdev\n");
> +		return -ENODEV;
> +	}
> +
> +	upstream_sd = media_entity_to_v4l2_subdev(pad->entity);
> +
> +	return v4l2_subdev_call(upstream_sd, video, s_stream, enable);
> +}
> +
> +static const struct v4l2_subdev_video_ops video_mux_subdev_video_ops = {
> +	.g_mbus_config = video_mux_g_mbus_config,
> +	.s_stream = video_mux_s_stream,
> +};
> +
> +static struct v4l2_mbus_framefmt *
> +__video_mux_get_pad_format(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_pad_config *cfg,
> +			   unsigned int pad, u32 which)
> +{
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		return v4l2_subdev_get_try_format(sd, cfg, pad);
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		return &vmux->format_mbus[pad];
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static int video_mux_get_format(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_pad_config *cfg,
> +			    struct v4l2_subdev_format *sdformat)
> +{
> +	sdformat->format = *__video_mux_get_pad_format(sd, cfg, sdformat->pad,
> +						   sdformat->which);
> +	return 0;
> +}
> +
> +static int video_mux_set_format(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_pad_config *cfg,
> +			    struct v4l2_subdev_format *sdformat)
> +{
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +	struct v4l2_mbus_framefmt *mbusformat;
> +
> +	mbusformat = __video_mux_get_pad_format(sd, cfg, sdformat->pad,
> +					    sdformat->which);
> +	if (!mbusformat)
> +		return -EINVAL;
> +
> +	/* Source pad mirrors active sink pad, no limitations on sink pads */
> +	if (is_source_pad(vmux, sdformat->pad) && vmux->active >= 0)
> +		sdformat->format = vmux->format_mbus[vmux->active];
> +
> +	*mbusformat = sdformat->format;
> +
> +	return 0;
> +}
> +
> +static struct v4l2_subdev_pad_ops video_mux_pad_ops = {
> +	.get_fmt = video_mux_get_format,
> +	.set_fmt = video_mux_set_format,
> +};
> +
> +static struct v4l2_subdev_ops video_mux_subdev_ops = {
> +	.pad = &video_mux_pad_ops,
> +	.video = &video_mux_subdev_video_ops,
> +};
> +
> +static int video_mux_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct v4l2_of_endpoint endpoint;
> +	struct device_node *ep;
> +	struct video_mux *vmux;
> +	unsigned int num_pads = 0;
> +	int ret;
> +	int i;
> +
> +	vmux = devm_kzalloc(dev, sizeof(*vmux), GFP_KERNEL);
> +	if (!vmux)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, vmux);
> +
> +	v4l2_subdev_init(&vmux->subdev, &video_mux_subdev_ops);
> +	snprintf(vmux->subdev.name, sizeof(vmux->subdev.name), "%s", np->name);
> +	vmux->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	vmux->subdev.dev = dev;
> +
> +	/*
> +	 * The largest numbered port is the output port. It determines
> +	 * total number of pads.
> +	 */
> +	for_each_endpoint_of_node(np, ep) {
> +		of_graph_parse_endpoint(ep, &endpoint.base);
> +		num_pads = max(num_pads, endpoint.base.port + 1);
> +	}
> +
> +	if (num_pads < 2) {
> +		dev_err(dev, "Not enough ports %d\n", num_pads);
> +		return -EINVAL;
> +	}
> +
> +	vmux->mux = devm_mux_control_get(dev, NULL);
> +	if (IS_ERR(vmux->mux)) {
> +		ret = PTR_ERR(vmux->mux);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get mux: %d\n", ret);
> +		return ret;
> +	}
> +
> +	vmux->active = -1;
> +	vmux->pads = devm_kzalloc(dev, sizeof(*vmux->pads) * num_pads,
> +				  GFP_KERNEL);
> +	vmux->format_mbus = devm_kzalloc(dev, sizeof(*vmux->format_mbus) *
> +					 num_pads, GFP_KERNEL);
> +	vmux->endpoint = devm_kzalloc(dev, sizeof(*vmux->endpoint) *
> +				      (num_pads - 1), GFP_KERNEL);
> +
> +	for (i = 0; i < num_pads - 1; i++)
> +		vmux->pads[i].flags = MEDIA_PAD_FL_SINK;
> +	vmux->pads[num_pads - 1].flags = MEDIA_PAD_FL_SOURCE;
> +
> +	vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
> +	ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
> +				     vmux->pads);
> +	if (ret < 0)
> +		return ret;
> +
> +	vmux->subdev.entity.ops = &video_mux_ops;
> +
> +	for_each_endpoint_of_node(np, ep) {
> +		v4l2_of_parse_endpoint(ep, &endpoint);
> +
> +		if (video_mux_endpoint_disabled(ep)) {
> +			dev_dbg(dev, "port %d disabled\n", endpoint.base.port);
> +			continue;
> +		}
> +
> +		vmux->endpoint[endpoint.base.port] = endpoint;
> +	}
> +
> +	return v4l2_async_register_subdev(&vmux->subdev);
> +}
> +
> +static int video_mux_remove(struct platform_device *pdev)
> +{
> +	struct video_mux *vmux = platform_get_drvdata(pdev);
> +	struct v4l2_subdev *sd = &vmux->subdev;
> +
> +	v4l2_async_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id video_mux_dt_ids[] = {
> +	{ .compatible = "video-mux", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, video_mux_dt_ids);
> +
> +static struct platform_driver video_mux_driver = {
> +	.probe		= video_mux_probe,
> +	.remove		= video_mux_remove,
> +	.driver		= {
> +		.of_match_table = video_mux_dt_ids,
> +		.name = "video-mux",
> +	},
> +};
> +
> +module_platform_driver(video_mux_driver);
> +
> +MODULE_DESCRIPTION("video stream multiplexer");
> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
> +MODULE_LICENSE("GPL");
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Peter Rosin]

On 2017-04-28 16:13, Philipp Zabel wrote:
> This driver can handle SoC internal and external video bus multiplexers,
> controlled by mux controllers provided by the mux controller framework,
> such as MMIO register bitfields or GPIOs. The subdevice passes through
> the mbus configuration of the active input to the output side.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
> This has been last sent as part of the i.MX media series.
> 
> Changes since https://patchwork.kernel.org/patch/9647869/:
>  - Split out the actual mux operation to be provided by the mux controller
>    framework [1]. GPIO and MMIO control can be provided by individual mux
>    controller drivers [2][3].
>    [1] https://patchwork.kernel.org/patch/9695837/
>    [2] https://patchwork.kernel.org/patch/9695839/
>    [3] https://patchwork.kernel.org/patch/9704509/
>  - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
>    vidsw with video_mux.
>  - Made the mux inactive by default, only activated by user interaction.
>  - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
>  - Reuse subdev.entity.num_pads instead of keeping our own count.
>  - Removed implicit link disabling. Instead, trying to enable a second
>    sink pad link yields -EBUSY.
>  - Merged _async_init into _probe.
>  - Removed superfluous pad index check from _set_format.
>  - Added is_source_pad helper to tell source and sink pads apart.
>  - Removed test for status property in endpoint nodes. Disable the remote
>    device or sever the endpoint link to disable a sink pad.
> ---
>  drivers/media/platform/Kconfig     |   6 +
>  drivers/media/platform/Makefile    |   2 +
>  drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 349 insertions(+)
>  create mode 100644 drivers/media/platform/video-mux.c
> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index c9106e105baba..b046a6d39fee5 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called arv.
>  
> +config VIDEO_MUX
> +	tristate "Video Multiplexer"
> +	depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
> +	help
> +	  This driver provides support for N:1 video bus multiplexers.
> +
>  config VIDEO_OMAP3
>  	tristate "OMAP 3 Camera support"
>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 349ddf6a69da2..fd2735ca3ff75 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
>  
>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
>  
> +obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
> +
>  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> new file mode 100644
> index 0000000000000..419541729f67e
> --- /dev/null
> +++ b/drivers/media/platform/video-mux.c
> @@ -0,0 +1,341 @@
> +/*
> + * video stream multiplexer controlled via mux control
> + *
> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel@pengutronix.de>

2017?

> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/module.h>
> +#include <linux/mux/consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/platform_device.h>
> +#include <media/v4l2-async.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-of.h>
> +
> +struct video_mux {
> +	struct v4l2_subdev subdev;
> +	struct media_pad *pads;
> +	struct v4l2_mbus_framefmt *format_mbus;
> +	struct v4l2_of_endpoint *endpoint;
> +	struct mux_control *mux;
> +	int active;
> +};
> +
> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
> +{
> +	return container_of(sd, struct video_mux, subdev);
> +}
> +
> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
> +{
> +	return pad == vmux->subdev.entity.num_pads - 1;
> +}
> +
> +static int video_mux_link_setup(struct media_entity *entity,
> +				const struct media_pad *local,
> +				const struct media_pad *remote, u32 flags)
> +{
> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +	int ret;
> +
> +	/*
> +	 * The mux state is determined by the enabled sink pad link.
> +	 * Enabling or disabling the source pad link has no effect.
> +	 */
> +	if (is_source_pad(vmux, local->index))
> +		return 0;
> +
> +	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
> +		remote->entity->name, remote->index, local->entity->name,
> +		local->index, flags & MEDIA_LNK_FL_ENABLED);
> +
> +	if (flags & MEDIA_LNK_FL_ENABLED) {
> +		if (vmux->active == local->index)

Here, you shortcut the mux_control_select_trylock test and return "OK"
based on a driver-local variable that is intended to keep track of mux
ownership.

> +			return 0;
> +
> +		if (vmux->active >= 0)

Here too (and this check is not needed, the situation will be covered by
the mux_control_try_select call).

> +			return -EBUSY;
> +
> +		dev_dbg(sd->dev, "setting %d active\n", local->index);
> +		ret = mux_control_try_select(vmux->mux, local->index);
> +		if (ret < 0)
> +			return ret;
> +		vmux->active = local->index;
> +	} else {
> +		if (vmux->active != local->index)
> +			return 0;
> +
> +		dev_dbg(sd->dev, "going inactive\n");
> +		mux_control_deselect(vmux->mux);

But here you let go of the mux *before* you clear the driver-local
ownership indicator. That looks suspicious. My guess is that this is
"safe" because the upper layers has some serialization, but I don't
know. Anyway, even if there is something saving you in the upper
layers, it looks out of order and unneeded. I would have moved the
below vmux->active = -1; statement up to before the above deselect.

With that fixed, mux usage looks good to me, so you can add an Acked-
by from me if you wish (goes for the bindings patch as well).

Cheers,
peda

> +		vmux->active = -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct media_entity_operations video_mux_ops = {
> +	.link_setup = video_mux_link_setup,
> +	.link_validate = v4l2_subdev_link_validate,
> +};
> +
> +static bool video_mux_endpoint_disabled(struct device_node *ep)
> +{
> +	struct device_node *rpp = of_graph_get_remote_port_parent(ep);
> +
> +	return !of_device_is_available(rpp);
> +}
> +
> +static int video_mux_g_mbus_config(struct v4l2_subdev *sd,
> +				   struct v4l2_mbus_config *cfg)
> +{
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +	struct v4l2_of_endpoint *endpoint;
> +	struct media_pad *pad;
> +	int ret;
> +
> +	if (vmux->active == -1) {
> +		dev_err(sd->dev, "no configuration for inactive mux\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * Retrieve media bus configuration from the entity connected to the
> +	 * active input
> +	 */
> +	pad = media_entity_remote_pad(&vmux->pads[vmux->active]);
> +	if (pad) {
> +		sd = media_entity_to_v4l2_subdev(pad->entity);
> +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
> +		if (ret == -ENOIOCTLCMD)
> +			pad = NULL;
> +		else if (ret < 0) {
> +			dev_err(sd->dev, "failed to get source configuration\n");
> +			return ret;
> +		}
> +	}
> +	if (!pad) {
> +		endpoint = &vmux->endpoint[vmux->active];
> +
> +		/* Mirror the input side on the output side */
> +		cfg->type = endpoint->bus_type;
> +		if (cfg->type == V4L2_MBUS_PARALLEL ||
> +		    cfg->type == V4L2_MBUS_BT656)
> +			cfg->flags = endpoint->bus.parallel.flags;
> +	}
> +
> +	return 0;
> +}
> +
> +static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)
> +{
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +	struct v4l2_subdev *upstream_sd;
> +	struct media_pad *pad;
> +
> +	if (vmux->active == -1) {
> +		dev_err(sd->dev, "Can not start streaming on inactive mux\n");
> +		return -EINVAL;
> +	}
> +
> +	pad = media_entity_remote_pad(&sd->entity.pads[vmux->active]);
> +	if (!pad) {
> +		dev_err(sd->dev, "Failed to find remote source pad\n");
> +		return -ENOLINK;
> +	}
> +
> +	if (!is_media_entity_v4l2_subdev(pad->entity)) {
> +		dev_err(sd->dev, "Upstream entity is not a v4l2 subdev\n");
> +		return -ENODEV;
> +	}
> +
> +	upstream_sd = media_entity_to_v4l2_subdev(pad->entity);
> +
> +	return v4l2_subdev_call(upstream_sd, video, s_stream, enable);
> +}
> +
> +static const struct v4l2_subdev_video_ops video_mux_subdev_video_ops = {
> +	.g_mbus_config = video_mux_g_mbus_config,
> +	.s_stream = video_mux_s_stream,
> +};
> +
> +static struct v4l2_mbus_framefmt *
> +__video_mux_get_pad_format(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_pad_config *cfg,
> +			   unsigned int pad, u32 which)
> +{
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		return v4l2_subdev_get_try_format(sd, cfg, pad);
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		return &vmux->format_mbus[pad];
> +	default:
> +		return NULL;
> +	}
> +}
> +
> +static int video_mux_get_format(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_pad_config *cfg,
> +			    struct v4l2_subdev_format *sdformat)
> +{
> +	sdformat->format = *__video_mux_get_pad_format(sd, cfg, sdformat->pad,
> +						   sdformat->which);
> +	return 0;
> +}
> +
> +static int video_mux_set_format(struct v4l2_subdev *sd,
> +			    struct v4l2_subdev_pad_config *cfg,
> +			    struct v4l2_subdev_format *sdformat)
> +{
> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> +	struct v4l2_mbus_framefmt *mbusformat;
> +
> +	mbusformat = __video_mux_get_pad_format(sd, cfg, sdformat->pad,
> +					    sdformat->which);
> +	if (!mbusformat)
> +		return -EINVAL;
> +
> +	/* Source pad mirrors active sink pad, no limitations on sink pads */
> +	if (is_source_pad(vmux, sdformat->pad) && vmux->active >= 0)
> +		sdformat->format = vmux->format_mbus[vmux->active];
> +
> +	*mbusformat = sdformat->format;
> +
> +	return 0;
> +}
> +
> +static struct v4l2_subdev_pad_ops video_mux_pad_ops = {
> +	.get_fmt = video_mux_get_format,
> +	.set_fmt = video_mux_set_format,
> +};
> +
> +static struct v4l2_subdev_ops video_mux_subdev_ops = {
> +	.pad = &video_mux_pad_ops,
> +	.video = &video_mux_subdev_video_ops,
> +};
> +
> +static int video_mux_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	struct v4l2_of_endpoint endpoint;
> +	struct device_node *ep;
> +	struct video_mux *vmux;
> +	unsigned int num_pads = 0;
> +	int ret;
> +	int i;
> +
> +	vmux = devm_kzalloc(dev, sizeof(*vmux), GFP_KERNEL);
> +	if (!vmux)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, vmux);
> +
> +	v4l2_subdev_init(&vmux->subdev, &video_mux_subdev_ops);
> +	snprintf(vmux->subdev.name, sizeof(vmux->subdev.name), "%s", np->name);
> +	vmux->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +	vmux->subdev.dev = dev;
> +
> +	/*
> +	 * The largest numbered port is the output port. It determines
> +	 * total number of pads.
> +	 */
> +	for_each_endpoint_of_node(np, ep) {
> +		of_graph_parse_endpoint(ep, &endpoint.base);
> +		num_pads = max(num_pads, endpoint.base.port + 1);
> +	}
> +
> +	if (num_pads < 2) {
> +		dev_err(dev, "Not enough ports %d\n", num_pads);
> +		return -EINVAL;
> +	}
> +
> +	vmux->mux = devm_mux_control_get(dev, NULL);
> +	if (IS_ERR(vmux->mux)) {
> +		ret = PTR_ERR(vmux->mux);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get mux: %d\n", ret);
> +		return ret;
> +	}
> +
> +	vmux->active = -1;
> +	vmux->pads = devm_kzalloc(dev, sizeof(*vmux->pads) * num_pads,
> +				  GFP_KERNEL);
> +	vmux->format_mbus = devm_kzalloc(dev, sizeof(*vmux->format_mbus) *
> +					 num_pads, GFP_KERNEL);
> +	vmux->endpoint = devm_kzalloc(dev, sizeof(*vmux->endpoint) *
> +				      (num_pads - 1), GFP_KERNEL);
> +
> +	for (i = 0; i < num_pads - 1; i++)
> +		vmux->pads[i].flags = MEDIA_PAD_FL_SINK;
> +	vmux->pads[num_pads - 1].flags = MEDIA_PAD_FL_SOURCE;
> +
> +	vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
> +	ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
> +				     vmux->pads);
> +	if (ret < 0)
> +		return ret;
> +
> +	vmux->subdev.entity.ops = &video_mux_ops;
> +
> +	for_each_endpoint_of_node(np, ep) {
> +		v4l2_of_parse_endpoint(ep, &endpoint);
> +
> +		if (video_mux_endpoint_disabled(ep)) {
> +			dev_dbg(dev, "port %d disabled\n", endpoint.base.port);
> +			continue;
> +		}
> +
> +		vmux->endpoint[endpoint.base.port] = endpoint;
> +	}
> +
> +	return v4l2_async_register_subdev(&vmux->subdev);
> +}
> +
> +static int video_mux_remove(struct platform_device *pdev)
> +{
> +	struct video_mux *vmux = platform_get_drvdata(pdev);
> +	struct v4l2_subdev *sd = &vmux->subdev;
> +
> +	v4l2_async_unregister_subdev(sd);
> +	media_entity_cleanup(&sd->entity);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id video_mux_dt_ids[] = {
> +	{ .compatible = "video-mux", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, video_mux_dt_ids);
> +
> +static struct platform_driver video_mux_driver = {
> +	.probe		= video_mux_probe,
> +	.remove		= video_mux_remove,
> +	.driver		= {
> +		.of_match_table = video_mux_dt_ids,
> +		.name = "video-mux",
> +	},
> +};
> +
> +module_platform_driver(video_mux_driver);
> +
> +MODULE_DESCRIPTION("video stream multiplexer");
> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Peter Rosin]

On 2017-04-29 23:29, Peter Rosin wrote:
> On 2017-04-28 16:13, Philipp Zabel wrote:
>> This driver can handle SoC internal and external video bus multiplexers,
>> controlled by mux controllers provided by the mux controller framework,
>> such as MMIO register bitfields or GPIOs. The subdevice passes through
>> the mbus configuration of the active input to the output side.
>>
>> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
>> ---
>> This has been last sent as part of the i.MX media series.
>>
>> Changes since https://patchwork.kernel.org/patch/9647869/:
>>  - Split out the actual mux operation to be provided by the mux controller
>>    framework [1]. GPIO and MMIO control can be provided by individual mux
>>    controller drivers [2][3].
>>    [1] https://patchwork.kernel.org/patch/9695837/
>>    [2] https://patchwork.kernel.org/patch/9695839/
>>    [3] https://patchwork.kernel.org/patch/9704509/
>>  - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
>>    vidsw with video_mux.
>>  - Made the mux inactive by default, only activated by user interaction.
>>  - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
>>  - Reuse subdev.entity.num_pads instead of keeping our own count.
>>  - Removed implicit link disabling. Instead, trying to enable a second
>>    sink pad link yields -EBUSY.
>>  - Merged _async_init into _probe.
>>  - Removed superfluous pad index check from _set_format.
>>  - Added is_source_pad helper to tell source and sink pads apart.
>>  - Removed test for status property in endpoint nodes. Disable the remote
>>    device or sever the endpoint link to disable a sink pad.
>> ---
>>  drivers/media/platform/Kconfig     |   6 +
>>  drivers/media/platform/Makefile    |   2 +
>>  drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 349 insertions(+)
>>  create mode 100644 drivers/media/platform/video-mux.c
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index c9106e105baba..b046a6d39fee5 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called arv.
>>  
>> +config VIDEO_MUX
>> +	tristate "Video Multiplexer"
>> +	depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
>> +	help
>> +	  This driver provides support for N:1 video bus multiplexers.
>> +
>>  config VIDEO_OMAP3
>>  	tristate "OMAP 3 Camera support"
>>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>> index 349ddf6a69da2..fd2735ca3ff75 100644
>> --- a/drivers/media/platform/Makefile
>> +++ b/drivers/media/platform/Makefile
>> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
>>  
>>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
>>  
>> +obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
>> +
>>  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
>>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
>>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
>> new file mode 100644
>> index 0000000000000..419541729f67e
>> --- /dev/null
>> +++ b/drivers/media/platform/video-mux.c
>> @@ -0,0 +1,341 @@
>> +/*
>> + * video stream multiplexer controlled via mux control
>> + *
>> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> 
> 2017?
> 
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +#include <linux/mux/consumer.h>
>> +#include <linux/of.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/platform_device.h>
>> +#include <media/v4l2-async.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/v4l2-of.h>
>> +
>> +struct video_mux {
>> +	struct v4l2_subdev subdev;
>> +	struct media_pad *pads;
>> +	struct v4l2_mbus_framefmt *format_mbus;
>> +	struct v4l2_of_endpoint *endpoint;
>> +	struct mux_control *mux;
>> +	int active;
>> +};
>> +
>> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
>> +{
>> +	return container_of(sd, struct video_mux, subdev);
>> +}
>> +
>> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
>> +{
>> +	return pad == vmux->subdev.entity.num_pads - 1;
>> +}
>> +
>> +static int video_mux_link_setup(struct media_entity *entity,
>> +				const struct media_pad *local,
>> +				const struct media_pad *remote, u32 flags)
>> +{
>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +	int ret;
>> +
>> +	/*
>> +	 * The mux state is determined by the enabled sink pad link.
>> +	 * Enabling or disabling the source pad link has no effect.
>> +	 */
>> +	if (is_source_pad(vmux, local->index))
>> +		return 0;
>> +
>> +	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
>> +		remote->entity->name, remote->index, local->entity->name,
>> +		local->index, flags & MEDIA_LNK_FL_ENABLED);
>> +
>> +	if (flags & MEDIA_LNK_FL_ENABLED) {
>> +		if (vmux->active == local->index)
> 
> Here, you shortcut the mux_control_select_trylock test and return "OK"
> based on a driver-local variable that is intended to keep track of mux
> ownership.
> 
>> +			return 0;
>> +
>> +		if (vmux->active >= 0)
> 
> Here too (and this check is not needed, the situation will be covered by
> the mux_control_try_select call).
> 
>> +			return -EBUSY;
>> +
>> +		dev_dbg(sd->dev, "setting %d active\n", local->index);
>> +		ret = mux_control_try_select(vmux->mux, local->index);
>> +		if (ret < 0)
>> +			return ret;
>> +		vmux->active = local->index;
>> +	} else {
>> +		if (vmux->active != local->index)
>> +			return 0;
>> +
>> +		dev_dbg(sd->dev, "going inactive\n");
>> +		mux_control_deselect(vmux->mux);
> 
> But here you let go of the mux *before* you clear the driver-local
> ownership indicator. That looks suspicious. My guess is that this is
> "safe" because the upper layers has some serialization, but I don't
> know. Anyway, even if there is something saving you in the upper
> layers, it looks out of order and unneeded. I would have moved the
> below vmux->active = -1; statement up to before the above deselect.
> 
> With that fixed, mux usage looks good to me, so you can add an Acked-
> by from me if you wish (goes for the bindings patch as well).

Ouch, that was a bit too soon. If there is *no* serialization in the
upper layers, this is *not* ok, even with my reordering. There must be
only one call to mux_control_deselect, and w/o serialization there
is a race where you might get multiple deselect calls when several
callers makes it through the active != index check before any of them
manages to set active = -1. That race must be taken care of!

Cheers,
peda

>> +		vmux->active = -1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct media_entity_operations video_mux_ops = {
>> +	.link_setup = video_mux_link_setup,
>> +	.link_validate = v4l2_subdev_link_validate,
>> +};
>> +
>> +static bool video_mux_endpoint_disabled(struct device_node *ep)
>> +{
>> +	struct device_node *rpp = of_graph_get_remote_port_parent(ep);
>> +
>> +	return !of_device_is_available(rpp);
>> +}
>> +
>> +static int video_mux_g_mbus_config(struct v4l2_subdev *sd,
>> +				   struct v4l2_mbus_config *cfg)
>> +{
>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +	struct v4l2_of_endpoint *endpoint;
>> +	struct media_pad *pad;
>> +	int ret;
>> +
>> +	if (vmux->active == -1) {
>> +		dev_err(sd->dev, "no configuration for inactive mux\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Retrieve media bus configuration from the entity connected to the
>> +	 * active input
>> +	 */
>> +	pad = media_entity_remote_pad(&vmux->pads[vmux->active]);
>> +	if (pad) {
>> +		sd = media_entity_to_v4l2_subdev(pad->entity);
>> +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
>> +		if (ret == -ENOIOCTLCMD)
>> +			pad = NULL;
>> +		else if (ret < 0) {
>> +			dev_err(sd->dev, "failed to get source configuration\n");
>> +			return ret;
>> +		}
>> +	}
>> +	if (!pad) {
>> +		endpoint = &vmux->endpoint[vmux->active];
>> +
>> +		/* Mirror the input side on the output side */
>> +		cfg->type = endpoint->bus_type;
>> +		if (cfg->type == V4L2_MBUS_PARALLEL ||
>> +		    cfg->type == V4L2_MBUS_BT656)
>> +			cfg->flags = endpoint->bus.parallel.flags;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +	struct v4l2_subdev *upstream_sd;
>> +	struct media_pad *pad;
>> +
>> +	if (vmux->active == -1) {
>> +		dev_err(sd->dev, "Can not start streaming on inactive mux\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	pad = media_entity_remote_pad(&sd->entity.pads[vmux->active]);
>> +	if (!pad) {
>> +		dev_err(sd->dev, "Failed to find remote source pad\n");
>> +		return -ENOLINK;
>> +	}
>> +
>> +	if (!is_media_entity_v4l2_subdev(pad->entity)) {
>> +		dev_err(sd->dev, "Upstream entity is not a v4l2 subdev\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	upstream_sd = media_entity_to_v4l2_subdev(pad->entity);
>> +
>> +	return v4l2_subdev_call(upstream_sd, video, s_stream, enable);
>> +}
>> +
>> +static const struct v4l2_subdev_video_ops video_mux_subdev_video_ops = {
>> +	.g_mbus_config = video_mux_g_mbus_config,
>> +	.s_stream = video_mux_s_stream,
>> +};
>> +
>> +static struct v4l2_mbus_framefmt *
>> +__video_mux_get_pad_format(struct v4l2_subdev *sd,
>> +			   struct v4l2_subdev_pad_config *cfg,
>> +			   unsigned int pad, u32 which)
>> +{
>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +
>> +	switch (which) {
>> +	case V4L2_SUBDEV_FORMAT_TRY:
>> +		return v4l2_subdev_get_try_format(sd, cfg, pad);
>> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
>> +		return &vmux->format_mbus[pad];
>> +	default:
>> +		return NULL;
>> +	}
>> +}
>> +
>> +static int video_mux_get_format(struct v4l2_subdev *sd,
>> +			    struct v4l2_subdev_pad_config *cfg,
>> +			    struct v4l2_subdev_format *sdformat)
>> +{
>> +	sdformat->format = *__video_mux_get_pad_format(sd, cfg, sdformat->pad,
>> +						   sdformat->which);
>> +	return 0;
>> +}
>> +
>> +static int video_mux_set_format(struct v4l2_subdev *sd,
>> +			    struct v4l2_subdev_pad_config *cfg,
>> +			    struct v4l2_subdev_format *sdformat)
>> +{
>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +	struct v4l2_mbus_framefmt *mbusformat;
>> +
>> +	mbusformat = __video_mux_get_pad_format(sd, cfg, sdformat->pad,
>> +					    sdformat->which);
>> +	if (!mbusformat)
>> +		return -EINVAL;
>> +
>> +	/* Source pad mirrors active sink pad, no limitations on sink pads */
>> +	if (is_source_pad(vmux, sdformat->pad) && vmux->active >= 0)
>> +		sdformat->format = vmux->format_mbus[vmux->active];
>> +
>> +	*mbusformat = sdformat->format;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct v4l2_subdev_pad_ops video_mux_pad_ops = {
>> +	.get_fmt = video_mux_get_format,
>> +	.set_fmt = video_mux_set_format,
>> +};
>> +
>> +static struct v4l2_subdev_ops video_mux_subdev_ops = {
>> +	.pad = &video_mux_pad_ops,
>> +	.video = &video_mux_subdev_video_ops,
>> +};
>> +
>> +static int video_mux_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	struct device *dev = &pdev->dev;
>> +	struct v4l2_of_endpoint endpoint;
>> +	struct device_node *ep;
>> +	struct video_mux *vmux;
>> +	unsigned int num_pads = 0;
>> +	int ret;
>> +	int i;
>> +
>> +	vmux = devm_kzalloc(dev, sizeof(*vmux), GFP_KERNEL);
>> +	if (!vmux)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, vmux);
>> +
>> +	v4l2_subdev_init(&vmux->subdev, &video_mux_subdev_ops);
>> +	snprintf(vmux->subdev.name, sizeof(vmux->subdev.name), "%s", np->name);
>> +	vmux->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +	vmux->subdev.dev = dev;
>> +
>> +	/*
>> +	 * The largest numbered port is the output port. It determines
>> +	 * total number of pads.
>> +	 */
>> +	for_each_endpoint_of_node(np, ep) {
>> +		of_graph_parse_endpoint(ep, &endpoint.base);
>> +		num_pads = max(num_pads, endpoint.base.port + 1);
>> +	}
>> +
>> +	if (num_pads < 2) {
>> +		dev_err(dev, "Not enough ports %d\n", num_pads);
>> +		return -EINVAL;
>> +	}
>> +
>> +	vmux->mux = devm_mux_control_get(dev, NULL);
>> +	if (IS_ERR(vmux->mux)) {
>> +		ret = PTR_ERR(vmux->mux);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "Failed to get mux: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	vmux->active = -1;
>> +	vmux->pads = devm_kzalloc(dev, sizeof(*vmux->pads) * num_pads,
>> +				  GFP_KERNEL);
>> +	vmux->format_mbus = devm_kzalloc(dev, sizeof(*vmux->format_mbus) *
>> +					 num_pads, GFP_KERNEL);
>> +	vmux->endpoint = devm_kzalloc(dev, sizeof(*vmux->endpoint) *
>> +				      (num_pads - 1), GFP_KERNEL);
>> +
>> +	for (i = 0; i < num_pads - 1; i++)
>> +		vmux->pads[i].flags = MEDIA_PAD_FL_SINK;
>> +	vmux->pads[num_pads - 1].flags = MEDIA_PAD_FL_SOURCE;
>> +
>> +	vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
>> +	ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
>> +				     vmux->pads);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	vmux->subdev.entity.ops = &video_mux_ops;
>> +
>> +	for_each_endpoint_of_node(np, ep) {
>> +		v4l2_of_parse_endpoint(ep, &endpoint);
>> +
>> +		if (video_mux_endpoint_disabled(ep)) {
>> +			dev_dbg(dev, "port %d disabled\n", endpoint.base.port);
>> +			continue;
>> +		}
>> +
>> +		vmux->endpoint[endpoint.base.port] = endpoint;
>> +	}
>> +
>> +	return v4l2_async_register_subdev(&vmux->subdev);
>> +}
>> +
>> +static int video_mux_remove(struct platform_device *pdev)
>> +{
>> +	struct video_mux *vmux = platform_get_drvdata(pdev);
>> +	struct v4l2_subdev *sd = &vmux->subdev;
>> +
>> +	v4l2_async_unregister_subdev(sd);
>> +	media_entity_cleanup(&sd->entity);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id video_mux_dt_ids[] = {
>> +	{ .compatible = "video-mux", },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, video_mux_dt_ids);
>> +
>> +static struct platform_driver video_mux_driver = {
>> +	.probe		= video_mux_probe,
>> +	.remove		= video_mux_remove,
>> +	.driver		= {
>> +		.of_match_table = video_mux_dt_ids,
>> +		.name = "video-mux",
>> +	},
>> +};
>> +
>> +module_platform_driver(video_mux_driver);
>> +
>> +MODULE_DESCRIPTION("video stream multiplexer");
>> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
>> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
>> +MODULE_LICENSE("GPL");
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Peter Rosin]

On 2017-04-29 23:29, Peter Rosin wrote:
> On 2017-04-28 16:13, Philipp Zabel wrote:
>> This driver can handle SoC internal and external video bus multiplexers,
>> controlled by mux controllers provided by the mux controller framework,
>> such as MMIO register bitfields or GPIOs. The subdevice passes through
>> the mbus configuration of the active input to the output side.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>> This has been last sent as part of the i.MX media series.
>>
>> Changes since https://patchwork.kernel.org/patch/9647869/:
>>  - Split out the actual mux operation to be provided by the mux controller
>>    framework [1]. GPIO and MMIO control can be provided by individual mux
>>    controller drivers [2][3].
>>    [1] https://patchwork.kernel.org/patch/9695837/
>>    [2] https://patchwork.kernel.org/patch/9695839/
>>    [3] https://patchwork.kernel.org/patch/9704509/
>>  - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
>>    vidsw with video_mux.
>>  - Made the mux inactive by default, only activated by user interaction.
>>  - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
>>  - Reuse subdev.entity.num_pads instead of keeping our own count.
>>  - Removed implicit link disabling. Instead, trying to enable a second
>>    sink pad link yields -EBUSY.
>>  - Merged _async_init into _probe.
>>  - Removed superfluous pad index check from _set_format.
>>  - Added is_source_pad helper to tell source and sink pads apart.
>>  - Removed test for status property in endpoint nodes. Disable the remote
>>    device or sever the endpoint link to disable a sink pad.
>> ---
>>  drivers/media/platform/Kconfig     |   6 +
>>  drivers/media/platform/Makefile    |   2 +
>>  drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 349 insertions(+)
>>  create mode 100644 drivers/media/platform/video-mux.c
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index c9106e105baba..b046a6d39fee5 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called arv.
>>  
>> +config VIDEO_MUX
>> +	tristate "Video Multiplexer"
>> +	depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
>> +	help
>> +	  This driver provides support for N:1 video bus multiplexers.
>> +
>>  config VIDEO_OMAP3
>>  	tristate "OMAP 3 Camera support"
>>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>> index 349ddf6a69da2..fd2735ca3ff75 100644
>> --- a/drivers/media/platform/Makefile
>> +++ b/drivers/media/platform/Makefile
>> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
>>  
>>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
>>  
>> +obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
>> +
>>  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
>>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
>>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
>> new file mode 100644
>> index 0000000000000..419541729f67e
>> --- /dev/null
>> +++ b/drivers/media/platform/video-mux.c
>> @@ -0,0 +1,341 @@
>> +/*
>> + * video stream multiplexer controlled via mux control
>> + *
>> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
>> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
> 
> 2017?
> 
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version 2
>> + * of the License, or (at your option) any later version.
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/err.h>
>> +#include <linux/module.h>
>> +#include <linux/mux/consumer.h>
>> +#include <linux/of.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/platform_device.h>
>> +#include <media/v4l2-async.h>
>> +#include <media/v4l2-device.h>
>> +#include <media/v4l2-subdev.h>
>> +#include <media/v4l2-of.h>
>> +
>> +struct video_mux {
>> +	struct v4l2_subdev subdev;
>> +	struct media_pad *pads;
>> +	struct v4l2_mbus_framefmt *format_mbus;
>> +	struct v4l2_of_endpoint *endpoint;
>> +	struct mux_control *mux;
>> +	int active;
>> +};
>> +
>> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
>> +{
>> +	return container_of(sd, struct video_mux, subdev);
>> +}
>> +
>> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
>> +{
>> +	return pad == vmux->subdev.entity.num_pads - 1;
>> +}
>> +
>> +static int video_mux_link_setup(struct media_entity *entity,
>> +				const struct media_pad *local,
>> +				const struct media_pad *remote, u32 flags)
>> +{
>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +	int ret;
>> +
>> +	/*
>> +	 * The mux state is determined by the enabled sink pad link.
>> +	 * Enabling or disabling the source pad link has no effect.
>> +	 */
>> +	if (is_source_pad(vmux, local->index))
>> +		return 0;
>> +
>> +	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
>> +		remote->entity->name, remote->index, local->entity->name,
>> +		local->index, flags & MEDIA_LNK_FL_ENABLED);
>> +
>> +	if (flags & MEDIA_LNK_FL_ENABLED) {
>> +		if (vmux->active == local->index)
> 
> Here, you shortcut the mux_control_select_trylock test and return "OK"
> based on a driver-local variable that is intended to keep track of mux
> ownership.
> 
>> +			return 0;
>> +
>> +		if (vmux->active >= 0)
> 
> Here too (and this check is not needed, the situation will be covered by
> the mux_control_try_select call).
> 
>> +			return -EBUSY;
>> +
>> +		dev_dbg(sd->dev, "setting %d active\n", local->index);
>> +		ret = mux_control_try_select(vmux->mux, local->index);
>> +		if (ret < 0)
>> +			return ret;
>> +		vmux->active = local->index;
>> +	} else {
>> +		if (vmux->active != local->index)
>> +			return 0;
>> +
>> +		dev_dbg(sd->dev, "going inactive\n");
>> +		mux_control_deselect(vmux->mux);
> 
> But here you let go of the mux *before* you clear the driver-local
> ownership indicator. That looks suspicious. My guess is that this is
> "safe" because the upper layers has some serialization, but I don't
> know. Anyway, even if there is something saving you in the upper
> layers, it looks out of order and unneeded. I would have moved the
> below vmux->active = -1; statement up to before the above deselect.
> 
> With that fixed, mux usage looks good to me, so you can add an Acked-
> by from me if you wish (goes for the bindings patch as well).

Ouch, that was a bit too soon. If there is *no* serialization in the
upper layers, this is *not* ok, even with my reordering. There must be
only one call to mux_control_deselect, and w/o serialization there
is a race where you might get multiple deselect calls when several
callers makes it through the active != index check before any of them
manages to set active = -1. That race must be taken care of!

Cheers,
peda

>> +		vmux->active = -1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct media_entity_operations video_mux_ops = {
>> +	.link_setup = video_mux_link_setup,
>> +	.link_validate = v4l2_subdev_link_validate,
>> +};
>> +
>> +static bool video_mux_endpoint_disabled(struct device_node *ep)
>> +{
>> +	struct device_node *rpp = of_graph_get_remote_port_parent(ep);
>> +
>> +	return !of_device_is_available(rpp);
>> +}
>> +
>> +static int video_mux_g_mbus_config(struct v4l2_subdev *sd,
>> +				   struct v4l2_mbus_config *cfg)
>> +{
>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +	struct v4l2_of_endpoint *endpoint;
>> +	struct media_pad *pad;
>> +	int ret;
>> +
>> +	if (vmux->active == -1) {
>> +		dev_err(sd->dev, "no configuration for inactive mux\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/*
>> +	 * Retrieve media bus configuration from the entity connected to the
>> +	 * active input
>> +	 */
>> +	pad = media_entity_remote_pad(&vmux->pads[vmux->active]);
>> +	if (pad) {
>> +		sd = media_entity_to_v4l2_subdev(pad->entity);
>> +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
>> +		if (ret == -ENOIOCTLCMD)
>> +			pad = NULL;
>> +		else if (ret < 0) {
>> +			dev_err(sd->dev, "failed to get source configuration\n");
>> +			return ret;
>> +		}
>> +	}
>> +	if (!pad) {
>> +		endpoint = &vmux->endpoint[vmux->active];
>> +
>> +		/* Mirror the input side on the output side */
>> +		cfg->type = endpoint->bus_type;
>> +		if (cfg->type == V4L2_MBUS_PARALLEL ||
>> +		    cfg->type == V4L2_MBUS_BT656)
>> +			cfg->flags = endpoint->bus.parallel.flags;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int video_mux_s_stream(struct v4l2_subdev *sd, int enable)
>> +{
>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +	struct v4l2_subdev *upstream_sd;
>> +	struct media_pad *pad;
>> +
>> +	if (vmux->active == -1) {
>> +		dev_err(sd->dev, "Can not start streaming on inactive mux\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	pad = media_entity_remote_pad(&sd->entity.pads[vmux->active]);
>> +	if (!pad) {
>> +		dev_err(sd->dev, "Failed to find remote source pad\n");
>> +		return -ENOLINK;
>> +	}
>> +
>> +	if (!is_media_entity_v4l2_subdev(pad->entity)) {
>> +		dev_err(sd->dev, "Upstream entity is not a v4l2 subdev\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	upstream_sd = media_entity_to_v4l2_subdev(pad->entity);
>> +
>> +	return v4l2_subdev_call(upstream_sd, video, s_stream, enable);
>> +}
>> +
>> +static const struct v4l2_subdev_video_ops video_mux_subdev_video_ops = {
>> +	.g_mbus_config = video_mux_g_mbus_config,
>> +	.s_stream = video_mux_s_stream,
>> +};
>> +
>> +static struct v4l2_mbus_framefmt *
>> +__video_mux_get_pad_format(struct v4l2_subdev *sd,
>> +			   struct v4l2_subdev_pad_config *cfg,
>> +			   unsigned int pad, u32 which)
>> +{
>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +
>> +	switch (which) {
>> +	case V4L2_SUBDEV_FORMAT_TRY:
>> +		return v4l2_subdev_get_try_format(sd, cfg, pad);
>> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
>> +		return &vmux->format_mbus[pad];
>> +	default:
>> +		return NULL;
>> +	}
>> +}
>> +
>> +static int video_mux_get_format(struct v4l2_subdev *sd,
>> +			    struct v4l2_subdev_pad_config *cfg,
>> +			    struct v4l2_subdev_format *sdformat)
>> +{
>> +	sdformat->format = *__video_mux_get_pad_format(sd, cfg, sdformat->pad,
>> +						   sdformat->which);
>> +	return 0;
>> +}
>> +
>> +static int video_mux_set_format(struct v4l2_subdev *sd,
>> +			    struct v4l2_subdev_pad_config *cfg,
>> +			    struct v4l2_subdev_format *sdformat)
>> +{
>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>> +	struct v4l2_mbus_framefmt *mbusformat;
>> +
>> +	mbusformat = __video_mux_get_pad_format(sd, cfg, sdformat->pad,
>> +					    sdformat->which);
>> +	if (!mbusformat)
>> +		return -EINVAL;
>> +
>> +	/* Source pad mirrors active sink pad, no limitations on sink pads */
>> +	if (is_source_pad(vmux, sdformat->pad) && vmux->active >= 0)
>> +		sdformat->format = vmux->format_mbus[vmux->active];
>> +
>> +	*mbusformat = sdformat->format;
>> +
>> +	return 0;
>> +}
>> +
>> +static struct v4l2_subdev_pad_ops video_mux_pad_ops = {
>> +	.get_fmt = video_mux_get_format,
>> +	.set_fmt = video_mux_set_format,
>> +};
>> +
>> +static struct v4l2_subdev_ops video_mux_subdev_ops = {
>> +	.pad = &video_mux_pad_ops,
>> +	.video = &video_mux_subdev_video_ops,
>> +};
>> +
>> +static int video_mux_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	struct device *dev = &pdev->dev;
>> +	struct v4l2_of_endpoint endpoint;
>> +	struct device_node *ep;
>> +	struct video_mux *vmux;
>> +	unsigned int num_pads = 0;
>> +	int ret;
>> +	int i;
>> +
>> +	vmux = devm_kzalloc(dev, sizeof(*vmux), GFP_KERNEL);
>> +	if (!vmux)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, vmux);
>> +
>> +	v4l2_subdev_init(&vmux->subdev, &video_mux_subdev_ops);
>> +	snprintf(vmux->subdev.name, sizeof(vmux->subdev.name), "%s", np->name);
>> +	vmux->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>> +	vmux->subdev.dev = dev;
>> +
>> +	/*
>> +	 * The largest numbered port is the output port. It determines
>> +	 * total number of pads.
>> +	 */
>> +	for_each_endpoint_of_node(np, ep) {
>> +		of_graph_parse_endpoint(ep, &endpoint.base);
>> +		num_pads = max(num_pads, endpoint.base.port + 1);
>> +	}
>> +
>> +	if (num_pads < 2) {
>> +		dev_err(dev, "Not enough ports %d\n", num_pads);
>> +		return -EINVAL;
>> +	}
>> +
>> +	vmux->mux = devm_mux_control_get(dev, NULL);
>> +	if (IS_ERR(vmux->mux)) {
>> +		ret = PTR_ERR(vmux->mux);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "Failed to get mux: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	vmux->active = -1;
>> +	vmux->pads = devm_kzalloc(dev, sizeof(*vmux->pads) * num_pads,
>> +				  GFP_KERNEL);
>> +	vmux->format_mbus = devm_kzalloc(dev, sizeof(*vmux->format_mbus) *
>> +					 num_pads, GFP_KERNEL);
>> +	vmux->endpoint = devm_kzalloc(dev, sizeof(*vmux->endpoint) *
>> +				      (num_pads - 1), GFP_KERNEL);
>> +
>> +	for (i = 0; i < num_pads - 1; i++)
>> +		vmux->pads[i].flags = MEDIA_PAD_FL_SINK;
>> +	vmux->pads[num_pads - 1].flags = MEDIA_PAD_FL_SOURCE;
>> +
>> +	vmux->subdev.entity.function = MEDIA_ENT_F_VID_MUX;
>> +	ret = media_entity_pads_init(&vmux->subdev.entity, num_pads,
>> +				     vmux->pads);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	vmux->subdev.entity.ops = &video_mux_ops;
>> +
>> +	for_each_endpoint_of_node(np, ep) {
>> +		v4l2_of_parse_endpoint(ep, &endpoint);
>> +
>> +		if (video_mux_endpoint_disabled(ep)) {
>> +			dev_dbg(dev, "port %d disabled\n", endpoint.base.port);
>> +			continue;
>> +		}
>> +
>> +		vmux->endpoint[endpoint.base.port] = endpoint;
>> +	}
>> +
>> +	return v4l2_async_register_subdev(&vmux->subdev);
>> +}
>> +
>> +static int video_mux_remove(struct platform_device *pdev)
>> +{
>> +	struct video_mux *vmux = platform_get_drvdata(pdev);
>> +	struct v4l2_subdev *sd = &vmux->subdev;
>> +
>> +	v4l2_async_unregister_subdev(sd);
>> +	media_entity_cleanup(&sd->entity);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id video_mux_dt_ids[] = {
>> +	{ .compatible = "video-mux", },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, video_mux_dt_ids);
>> +
>> +static struct platform_driver video_mux_driver = {
>> +	.probe		= video_mux_probe,
>> +	.remove		= video_mux_remove,
>> +	.driver		= {
>> +		.of_match_table = video_mux_dt_ids,
>> +		.name = "video-mux",
>> +	},
>> +};
>> +
>> +module_platform_driver(video_mux_driver);
>> +
>> +MODULE_DESCRIPTION("video stream multiplexer");
>> +MODULE_AUTHOR("Sascha Hauer, Pengutronix");
>> +MODULE_AUTHOR("Philipp Zabel, Pengutronix");
>> +MODULE_LICENSE("GPL");
>>
> 

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Philipp Zabel]

On Sat, 2017-04-29 at 23:42 +0200, Peter Rosin wrote:
> On 2017-04-29 23:29, Peter Rosin wrote:
> > On 2017-04-28 16:13, Philipp Zabel wrote:
> >> This driver can handle SoC internal and external video bus multiplexers,
> >> controlled by mux controllers provided by the mux controller framework,
> >> such as MMIO register bitfields or GPIOs. The subdevice passes through
> >> the mbus configuration of the active input to the output side.
> >>
> >> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> Signed-off-by: Steve Longerbeam <steve_longerbeam-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> >> ---
> >> This has been last sent as part of the i.MX media series.
> >>
> >> Changes since https://patchwork.kernel.org/patch/9647869/:
> >>  - Split out the actual mux operation to be provided by the mux controller
> >>    framework [1]. GPIO and MMIO control can be provided by individual mux
> >>    controller drivers [2][3].
> >>    [1] https://patchwork.kernel.org/patch/9695837/
> >>    [2] https://patchwork.kernel.org/patch/9695839/
> >>    [3] https://patchwork.kernel.org/patch/9704509/
> >>  - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
> >>    vidsw with video_mux.
> >>  - Made the mux inactive by default, only activated by user interaction.
> >>  - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
> >>  - Reuse subdev.entity.num_pads instead of keeping our own count.
> >>  - Removed implicit link disabling. Instead, trying to enable a second
> >>    sink pad link yields -EBUSY.
> >>  - Merged _async_init into _probe.
> >>  - Removed superfluous pad index check from _set_format.
> >>  - Added is_source_pad helper to tell source and sink pads apart.
> >>  - Removed test for status property in endpoint nodes. Disable the remote
> >>    device or sever the endpoint link to disable a sink pad.
> >> ---
> >>  drivers/media/platform/Kconfig     |   6 +
> >>  drivers/media/platform/Makefile    |   2 +
> >>  drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 349 insertions(+)
> >>  create mode 100644 drivers/media/platform/video-mux.c
> >>
> >> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> >> index c9106e105baba..b046a6d39fee5 100644
> >> --- a/drivers/media/platform/Kconfig
> >> +++ b/drivers/media/platform/Kconfig
> >> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
> >>  	  To compile this driver as a module, choose M here: the
> >>  	  module will be called arv.
> >>  
> >> +config VIDEO_MUX
> >> +	tristate "Video Multiplexer"
> >> +	depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
> >> +	help
> >> +	  This driver provides support for N:1 video bus multiplexers.
> >> +
> >>  config VIDEO_OMAP3
> >>  	tristate "OMAP 3 Camera support"
> >>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> >> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> >> index 349ddf6a69da2..fd2735ca3ff75 100644
> >> --- a/drivers/media/platform/Makefile
> >> +++ b/drivers/media/platform/Makefile
> >> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
> >>  
> >>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
> >>  
> >> +obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
> >> +
> >>  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
> >>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
> >>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
> >> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> >> new file mode 100644
> >> index 0000000000000..419541729f67e
> >> --- /dev/null
> >> +++ b/drivers/media/platform/video-mux.c
> >> @@ -0,0 +1,341 @@
> >> +/*
> >> + * video stream multiplexer controlled via mux control
> >> + *
> >> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> > 
> > 2017?
> > 
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU General Public License
> >> + * as published by the Free Software Foundation; either version 2
> >> + * of the License, or (at your option) any later version.
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + */
> >> +
> >> +#include <linux/err.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mux/consumer.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_graph.h>
> >> +#include <linux/platform_device.h>
> >> +#include <media/v4l2-async.h>
> >> +#include <media/v4l2-device.h>
> >> +#include <media/v4l2-subdev.h>
> >> +#include <media/v4l2-of.h>
> >> +
> >> +struct video_mux {
> >> +	struct v4l2_subdev subdev;
> >> +	struct media_pad *pads;
> >> +	struct v4l2_mbus_framefmt *format_mbus;
> >> +	struct v4l2_of_endpoint *endpoint;
> >> +	struct mux_control *mux;
> >> +	int active;
> >> +};
> >> +
> >> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
> >> +{
> >> +	return container_of(sd, struct video_mux, subdev);
> >> +}
> >> +
> >> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
> >> +{
> >> +	return pad == vmux->subdev.entity.num_pads - 1;
> >> +}
> >> +
> >> +static int video_mux_link_setup(struct media_entity *entity,
> >> +				const struct media_pad *local,
> >> +				const struct media_pad *remote, u32 flags)
> >> +{
> >> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> >> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> >> +	int ret;
> >> +
> >> +	/*
> >> +	 * The mux state is determined by the enabled sink pad link.
> >> +	 * Enabling or disabling the source pad link has no effect.
> >> +	 */
> >> +	if (is_source_pad(vmux, local->index))
> >> +		return 0;
> >> +
> >> +	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
> >> +		remote->entity->name, remote->index, local->entity->name,
> >> +		local->index, flags & MEDIA_LNK_FL_ENABLED);
> >> +
> >> +	if (flags & MEDIA_LNK_FL_ENABLED) {
> >> +		if (vmux->active == local->index)
> > 
> > Here, you shortcut the mux_control_select_trylock test and return "OK"
> > based on a driver-local variable that is intended to keep track of mux
> > ownership.
> > 
> >> +			return 0;
> >> +
> >> +		if (vmux->active >= 0)
> > 
> > Here too (and this check is not needed, the situation will be covered by
> > the mux_control_try_select call).
> > 
> >> +			return -EBUSY;
> >> +
> >> +		dev_dbg(sd->dev, "setting %d active\n", local->index);
> >> +		ret = mux_control_try_select(vmux->mux, local->index);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +		vmux->active = local->index;
> >> +	} else {
> >> +		if (vmux->active != local->index)
> >> +			return 0;
> >> +
> >> +		dev_dbg(sd->dev, "going inactive\n");
> >> +		mux_control_deselect(vmux->mux);
> > 
> > But here you let go of the mux *before* you clear the driver-local
> > ownership indicator. That looks suspicious. My guess is that this is
> > "safe" because the upper layers has some serialization, but I don't
> > know. Anyway, even if there is something saving you in the upper
> > layers, it looks out of order and unneeded. I would have moved the
> > below vmux->active = -1; statement up to before the above deselect.
> > 
> > With that fixed, mux usage looks good to me, so you can add an Acked-
> > by from me if you wish (goes for the bindings patch as well).
> 
> Ouch, that was a bit too soon. If there is *no* serialization in the
> upper layers, this is *not* ok, even with my reordering. There must be
> only one call to mux_control_deselect, and w/o serialization there
> is a race where you might get multiple deselect calls when several
> callers makes it through the active != index check before any of them
> manages to set active = -1. That race must be taken care of!

Thank you, I've resent a version with a mutex lock around vmux->active.

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Philipp Zabel]

On Sat, 2017-04-29 at 23:42 +0200, Peter Rosin wrote:
> On 2017-04-29 23:29, Peter Rosin wrote:
> > On 2017-04-28 16:13, Philipp Zabel wrote:
> >> This driver can handle SoC internal and external video bus multiplexers,
> >> controlled by mux controllers provided by the mux controller framework,
> >> such as MMIO register bitfields or GPIOs. The subdevice passes through
> >> the mbus configuration of the active input to the output side.
> >>
> >> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >> ---
> >> This has been last sent as part of the i.MX media series.
> >>
> >> Changes since https://patchwork.kernel.org/patch/9647869/:
> >>  - Split out the actual mux operation to be provided by the mux controller
> >>    framework [1]. GPIO and MMIO control can be provided by individual mux
> >>    controller drivers [2][3].
> >>    [1] https://patchwork.kernel.org/patch/9695837/
> >>    [2] https://patchwork.kernel.org/patch/9695839/
> >>    [3] https://patchwork.kernel.org/patch/9704509/
> >>  - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
> >>    vidsw with video_mux.
> >>  - Made the mux inactive by default, only activated by user interaction.
> >>  - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
> >>  - Reuse subdev.entity.num_pads instead of keeping our own count.
> >>  - Removed implicit link disabling. Instead, trying to enable a second
> >>    sink pad link yields -EBUSY.
> >>  - Merged _async_init into _probe.
> >>  - Removed superfluous pad index check from _set_format.
> >>  - Added is_source_pad helper to tell source and sink pads apart.
> >>  - Removed test for status property in endpoint nodes. Disable the remote
> >>    device or sever the endpoint link to disable a sink pad.
> >> ---
> >>  drivers/media/platform/Kconfig     |   6 +
> >>  drivers/media/platform/Makefile    |   2 +
> >>  drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 349 insertions(+)
> >>  create mode 100644 drivers/media/platform/video-mux.c
> >>
> >> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> >> index c9106e105baba..b046a6d39fee5 100644
> >> --- a/drivers/media/platform/Kconfig
> >> +++ b/drivers/media/platform/Kconfig
> >> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
> >>  	  To compile this driver as a module, choose M here: the
> >>  	  module will be called arv.
> >>  
> >> +config VIDEO_MUX
> >> +	tristate "Video Multiplexer"
> >> +	depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
> >> +	help
> >> +	  This driver provides support for N:1 video bus multiplexers.
> >> +
> >>  config VIDEO_OMAP3
> >>  	tristate "OMAP 3 Camera support"
> >>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> >> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> >> index 349ddf6a69da2..fd2735ca3ff75 100644
> >> --- a/drivers/media/platform/Makefile
> >> +++ b/drivers/media/platform/Makefile
> >> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
> >>  
> >>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
> >>  
> >> +obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
> >> +
> >>  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
> >>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
> >>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
> >> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> >> new file mode 100644
> >> index 0000000000000..419541729f67e
> >> --- /dev/null
> >> +++ b/drivers/media/platform/video-mux.c
> >> @@ -0,0 +1,341 @@
> >> +/*
> >> + * video stream multiplexer controlled via mux control
> >> + *
> >> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
> >> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
> > 
> > 2017?
> > 
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> + * modify it under the terms of the GNU General Public License
> >> + * as published by the Free Software Foundation; either version 2
> >> + * of the License, or (at your option) any later version.
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + * GNU General Public License for more details.
> >> + */
> >> +
> >> +#include <linux/err.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mux/consumer.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_graph.h>
> >> +#include <linux/platform_device.h>
> >> +#include <media/v4l2-async.h>
> >> +#include <media/v4l2-device.h>
> >> +#include <media/v4l2-subdev.h>
> >> +#include <media/v4l2-of.h>
> >> +
> >> +struct video_mux {
> >> +	struct v4l2_subdev subdev;
> >> +	struct media_pad *pads;
> >> +	struct v4l2_mbus_framefmt *format_mbus;
> >> +	struct v4l2_of_endpoint *endpoint;
> >> +	struct mux_control *mux;
> >> +	int active;
> >> +};
> >> +
> >> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
> >> +{
> >> +	return container_of(sd, struct video_mux, subdev);
> >> +}
> >> +
> >> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
> >> +{
> >> +	return pad == vmux->subdev.entity.num_pads - 1;
> >> +}
> >> +
> >> +static int video_mux_link_setup(struct media_entity *entity,
> >> +				const struct media_pad *local,
> >> +				const struct media_pad *remote, u32 flags)
> >> +{
> >> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> >> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> >> +	int ret;
> >> +
> >> +	/*
> >> +	 * The mux state is determined by the enabled sink pad link.
> >> +	 * Enabling or disabling the source pad link has no effect.
> >> +	 */
> >> +	if (is_source_pad(vmux, local->index))
> >> +		return 0;
> >> +
> >> +	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
> >> +		remote->entity->name, remote->index, local->entity->name,
> >> +		local->index, flags & MEDIA_LNK_FL_ENABLED);
> >> +
> >> +	if (flags & MEDIA_LNK_FL_ENABLED) {
> >> +		if (vmux->active == local->index)
> > 
> > Here, you shortcut the mux_control_select_trylock test and return "OK"
> > based on a driver-local variable that is intended to keep track of mux
> > ownership.
> > 
> >> +			return 0;
> >> +
> >> +		if (vmux->active >= 0)
> > 
> > Here too (and this check is not needed, the situation will be covered by
> > the mux_control_try_select call).
> > 
> >> +			return -EBUSY;
> >> +
> >> +		dev_dbg(sd->dev, "setting %d active\n", local->index);
> >> +		ret = mux_control_try_select(vmux->mux, local->index);
> >> +		if (ret < 0)
> >> +			return ret;
> >> +		vmux->active = local->index;
> >> +	} else {
> >> +		if (vmux->active != local->index)
> >> +			return 0;
> >> +
> >> +		dev_dbg(sd->dev, "going inactive\n");
> >> +		mux_control_deselect(vmux->mux);
> > 
> > But here you let go of the mux *before* you clear the driver-local
> > ownership indicator. That looks suspicious. My guess is that this is
> > "safe" because the upper layers has some serialization, but I don't
> > know. Anyway, even if there is something saving you in the upper
> > layers, it looks out of order and unneeded. I would have moved the
> > below vmux->active = -1; statement up to before the above deselect.
> > 
> > With that fixed, mux usage looks good to me, so you can add an Acked-
> > by from me if you wish (goes for the bindings patch as well).
> 
> Ouch, that was a bit too soon. If there is *no* serialization in the
> upper layers, this is *not* ok, even with my reordering. There must be
> only one call to mux_control_deselect, and w/o serialization there
> is a race where you might get multiple deselect calls when several
> callers makes it through the active != index check before any of them
> manages to set active = -1. That race must be taken care of!

Thank you, I've resent a version with a mutex lock around vmux->active.

regards
Philipp

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Peter Rosin]

On 2017-05-02 17:21, Philipp Zabel wrote:
> On Sat, 2017-04-29 at 23:42 +0200, Peter Rosin wrote:
>> On 2017-04-29 23:29, Peter Rosin wrote:
>>> On 2017-04-28 16:13, Philipp Zabel wrote:
>>>> This driver can handle SoC internal and external video bus multiplexers,
>>>> controlled by mux controllers provided by the mux controller framework,
>>>> such as MMIO register bitfields or GPIOs. The subdevice passes through
>>>> the mbus configuration of the active input to the output side.
>>>>
>>>> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>> This has been last sent as part of the i.MX media series.
>>>>
>>>> Changes since https://patchwork.kernel.org/patch/9647869/:
>>>>  - Split out the actual mux operation to be provided by the mux controller
>>>>    framework [1]. GPIO and MMIO control can be provided by individual mux
>>>>    controller drivers [2][3].
>>>>    [1] https://patchwork.kernel.org/patch/9695837/
>>>>    [2] https://patchwork.kernel.org/patch/9695839/
>>>>    [3] https://patchwork.kernel.org/patch/9704509/
>>>>  - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
>>>>    vidsw with video_mux.
>>>>  - Made the mux inactive by default, only activated by user interaction.
>>>>  - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
>>>>  - Reuse subdev.entity.num_pads instead of keeping our own count.
>>>>  - Removed implicit link disabling. Instead, trying to enable a second
>>>>    sink pad link yields -EBUSY.
>>>>  - Merged _async_init into _probe.
>>>>  - Removed superfluous pad index check from _set_format.
>>>>  - Added is_source_pad helper to tell source and sink pads apart.
>>>>  - Removed test for status property in endpoint nodes. Disable the remote
>>>>    device or sever the endpoint link to disable a sink pad.
>>>> ---
>>>>  drivers/media/platform/Kconfig     |   6 +
>>>>  drivers/media/platform/Makefile    |   2 +
>>>>  drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 349 insertions(+)
>>>>  create mode 100644 drivers/media/platform/video-mux.c
>>>>
>>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>>> index c9106e105baba..b046a6d39fee5 100644
>>>> --- a/drivers/media/platform/Kconfig
>>>> +++ b/drivers/media/platform/Kconfig
>>>> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
>>>>  	  To compile this driver as a module, choose M here: the
>>>>  	  module will be called arv.
>>>>  
>>>> +config VIDEO_MUX
>>>> +	tristate "Video Multiplexer"
>>>> +	depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
>>>> +	help
>>>> +	  This driver provides support for N:1 video bus multiplexers.
>>>> +
>>>>  config VIDEO_OMAP3
>>>>  	tristate "OMAP 3 Camera support"
>>>>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
>>>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>>>> index 349ddf6a69da2..fd2735ca3ff75 100644
>>>> --- a/drivers/media/platform/Makefile
>>>> +++ b/drivers/media/platform/Makefile
>>>> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
>>>>  
>>>>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
>>>>  
>>>> +obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
>>>> +
>>>>  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
>>>>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
>>>>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
>>>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
>>>> new file mode 100644
>>>> index 0000000000000..419541729f67e
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/video-mux.c
>>>> @@ -0,0 +1,341 @@
>>>> +/*
>>>> + * video stream multiplexer controlled via mux control
>>>> + *
>>>> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>>>
>>> 2017?
>>>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU General Public License
>>>> + * as published by the Free Software Foundation; either version 2
>>>> + * of the License, or (at your option) any later version.
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mux/consumer.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_graph.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <media/v4l2-async.h>
>>>> +#include <media/v4l2-device.h>
>>>> +#include <media/v4l2-subdev.h>
>>>> +#include <media/v4l2-of.h>
>>>> +
>>>> +struct video_mux {
>>>> +	struct v4l2_subdev subdev;
>>>> +	struct media_pad *pads;
>>>> +	struct v4l2_mbus_framefmt *format_mbus;
>>>> +	struct v4l2_of_endpoint *endpoint;
>>>> +	struct mux_control *mux;
>>>> +	int active;
>>>> +};
>>>> +
>>>> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
>>>> +{
>>>> +	return container_of(sd, struct video_mux, subdev);
>>>> +}
>>>> +
>>>> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
>>>> +{
>>>> +	return pad == vmux->subdev.entity.num_pads - 1;
>>>> +}
>>>> +
>>>> +static int video_mux_link_setup(struct media_entity *entity,
>>>> +				const struct media_pad *local,
>>>> +				const struct media_pad *remote, u32 flags)
>>>> +{
>>>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>>>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>>>> +	int ret;
>>>> +
>>>> +	/*
>>>> +	 * The mux state is determined by the enabled sink pad link.
>>>> +	 * Enabling or disabling the source pad link has no effect.
>>>> +	 */
>>>> +	if (is_source_pad(vmux, local->index))
>>>> +		return 0;
>>>> +
>>>> +	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
>>>> +		remote->entity->name, remote->index, local->entity->name,
>>>> +		local->index, flags & MEDIA_LNK_FL_ENABLED);
>>>> +
>>>> +	if (flags & MEDIA_LNK_FL_ENABLED) {
>>>> +		if (vmux->active == local->index)
>>>
>>> Here, you shortcut the mux_control_select_trylock test and return "OK"
>>> based on a driver-local variable that is intended to keep track of mux
>>> ownership.
>>>
>>>> +			return 0;
>>>> +
>>>> +		if (vmux->active >= 0)
>>>
>>> Here too (and this check is not needed, the situation will be covered by
>>> the mux_control_try_select call).
>>>
>>>> +			return -EBUSY;
>>>> +
>>>> +		dev_dbg(sd->dev, "setting %d active\n", local->index);
>>>> +		ret = mux_control_try_select(vmux->mux, local->index);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +		vmux->active = local->index;
>>>> +	} else {
>>>> +		if (vmux->active != local->index)
>>>> +			return 0;
>>>> +
>>>> +		dev_dbg(sd->dev, "going inactive\n");
>>>> +		mux_control_deselect(vmux->mux);
>>>
>>> But here you let go of the mux *before* you clear the driver-local
>>> ownership indicator. That looks suspicious. My guess is that this is
>>> "safe" because the upper layers has some serialization, but I don't
>>> know. Anyway, even if there is something saving you in the upper
>>> layers, it looks out of order and unneeded. I would have moved the
>>> below vmux->active = -1; statement up to before the above deselect.
>>>
>>> With that fixed, mux usage looks good to me, so you can add an Acked-
>>> by from me if you wish (goes for the bindings patch as well).
>>
>> Ouch, that was a bit too soon. If there is *no* serialization in the
>> upper layers, this is *not* ok, even with my reordering. There must be
>> only one call to mux_control_deselect, and w/o serialization there
>> is a race where you might get multiple deselect calls when several
>> callers makes it through the active != index check before any of them
>> manages to set active = -1. That race must be taken care of!
> 
> Thank you, I've resent a version with a mutex lock around vmux->active.

I had a bunch of ifs in the above message, so I'm not sure it's needed.
I would expect there to be a lock outside somewhere in the media layer.
A cursory look gets me to media-entity.c and media_entity_setup_link()
which does have a mutex. But I'm no media expert, so maybe there are other
ways of getting to video_mux_link_setup that I'm not aware of?

If you do end up relying on external locking, a comment saying so would
be nice. Or even better, some __must_hold markup if possible?

Cheers,
peda

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Peter Rosin]

On 2017-05-02 17:21, Philipp Zabel wrote:
> On Sat, 2017-04-29 at 23:42 +0200, Peter Rosin wrote:
>> On 2017-04-29 23:29, Peter Rosin wrote:
>>> On 2017-04-28 16:13, Philipp Zabel wrote:
>>>> This driver can handle SoC internal and external video bus multiplexers,
>>>> controlled by mux controllers provided by the mux controller framework,
>>>> such as MMIO register bitfields or GPIOs. The subdevice passes through
>>>> the mbus configuration of the active input to the output side.
>>>>
>>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>> ---
>>>> This has been last sent as part of the i.MX media series.
>>>>
>>>> Changes since https://patchwork.kernel.org/patch/9647869/:
>>>>  - Split out the actual mux operation to be provided by the mux controller
>>>>    framework [1]. GPIO and MMIO control can be provided by individual mux
>>>>    controller drivers [2][3].
>>>>    [1] https://patchwork.kernel.org/patch/9695837/
>>>>    [2] https://patchwork.kernel.org/patch/9695839/
>>>>    [3] https://patchwork.kernel.org/patch/9704509/
>>>>  - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
>>>>    vidsw with video_mux.
>>>>  - Made the mux inactive by default, only activated by user interaction.
>>>>  - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
>>>>  - Reuse subdev.entity.num_pads instead of keeping our own count.
>>>>  - Removed implicit link disabling. Instead, trying to enable a second
>>>>    sink pad link yields -EBUSY.
>>>>  - Merged _async_init into _probe.
>>>>  - Removed superfluous pad index check from _set_format.
>>>>  - Added is_source_pad helper to tell source and sink pads apart.
>>>>  - Removed test for status property in endpoint nodes. Disable the remote
>>>>    device or sever the endpoint link to disable a sink pad.
>>>> ---
>>>>  drivers/media/platform/Kconfig     |   6 +
>>>>  drivers/media/platform/Makefile    |   2 +
>>>>  drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 349 insertions(+)
>>>>  create mode 100644 drivers/media/platform/video-mux.c
>>>>
>>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>>> index c9106e105baba..b046a6d39fee5 100644
>>>> --- a/drivers/media/platform/Kconfig
>>>> +++ b/drivers/media/platform/Kconfig
>>>> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
>>>>  	  To compile this driver as a module, choose M here: the
>>>>  	  module will be called arv.
>>>>  
>>>> +config VIDEO_MUX
>>>> +	tristate "Video Multiplexer"
>>>> +	depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
>>>> +	help
>>>> +	  This driver provides support for N:1 video bus multiplexers.
>>>> +
>>>>  config VIDEO_OMAP3
>>>>  	tristate "OMAP 3 Camera support"
>>>>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
>>>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>>>> index 349ddf6a69da2..fd2735ca3ff75 100644
>>>> --- a/drivers/media/platform/Makefile
>>>> +++ b/drivers/media/platform/Makefile
>>>> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
>>>>  
>>>>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
>>>>  
>>>> +obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
>>>> +
>>>>  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
>>>>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
>>>>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
>>>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
>>>> new file mode 100644
>>>> index 0000000000000..419541729f67e
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/video-mux.c
>>>> @@ -0,0 +1,341 @@
>>>> +/*
>>>> + * video stream multiplexer controlled via mux control
>>>> + *
>>>> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
>>>> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
>>>
>>> 2017?
>>>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU General Public License
>>>> + * as published by the Free Software Foundation; either version 2
>>>> + * of the License, or (at your option) any later version.
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>> + * GNU General Public License for more details.
>>>> + */
>>>> +
>>>> +#include <linux/err.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mux/consumer.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_graph.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <media/v4l2-async.h>
>>>> +#include <media/v4l2-device.h>
>>>> +#include <media/v4l2-subdev.h>
>>>> +#include <media/v4l2-of.h>
>>>> +
>>>> +struct video_mux {
>>>> +	struct v4l2_subdev subdev;
>>>> +	struct media_pad *pads;
>>>> +	struct v4l2_mbus_framefmt *format_mbus;
>>>> +	struct v4l2_of_endpoint *endpoint;
>>>> +	struct mux_control *mux;
>>>> +	int active;
>>>> +};
>>>> +
>>>> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
>>>> +{
>>>> +	return container_of(sd, struct video_mux, subdev);
>>>> +}
>>>> +
>>>> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
>>>> +{
>>>> +	return pad == vmux->subdev.entity.num_pads - 1;
>>>> +}
>>>> +
>>>> +static int video_mux_link_setup(struct media_entity *entity,
>>>> +				const struct media_pad *local,
>>>> +				const struct media_pad *remote, u32 flags)
>>>> +{
>>>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
>>>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
>>>> +	int ret;
>>>> +
>>>> +	/*
>>>> +	 * The mux state is determined by the enabled sink pad link.
>>>> +	 * Enabling or disabling the source pad link has no effect.
>>>> +	 */
>>>> +	if (is_source_pad(vmux, local->index))
>>>> +		return 0;
>>>> +
>>>> +	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
>>>> +		remote->entity->name, remote->index, local->entity->name,
>>>> +		local->index, flags & MEDIA_LNK_FL_ENABLED);
>>>> +
>>>> +	if (flags & MEDIA_LNK_FL_ENABLED) {
>>>> +		if (vmux->active == local->index)
>>>
>>> Here, you shortcut the mux_control_select_trylock test and return "OK"
>>> based on a driver-local variable that is intended to keep track of mux
>>> ownership.
>>>
>>>> +			return 0;
>>>> +
>>>> +		if (vmux->active >= 0)
>>>
>>> Here too (and this check is not needed, the situation will be covered by
>>> the mux_control_try_select call).
>>>
>>>> +			return -EBUSY;
>>>> +
>>>> +		dev_dbg(sd->dev, "setting %d active\n", local->index);
>>>> +		ret = mux_control_try_select(vmux->mux, local->index);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +		vmux->active = local->index;
>>>> +	} else {
>>>> +		if (vmux->active != local->index)
>>>> +			return 0;
>>>> +
>>>> +		dev_dbg(sd->dev, "going inactive\n");
>>>> +		mux_control_deselect(vmux->mux);
>>>
>>> But here you let go of the mux *before* you clear the driver-local
>>> ownership indicator. That looks suspicious. My guess is that this is
>>> "safe" because the upper layers has some serialization, but I don't
>>> know. Anyway, even if there is something saving you in the upper
>>> layers, it looks out of order and unneeded. I would have moved the
>>> below vmux->active = -1; statement up to before the above deselect.
>>>
>>> With that fixed, mux usage looks good to me, so you can add an Acked-
>>> by from me if you wish (goes for the bindings patch as well).
>>
>> Ouch, that was a bit too soon. If there is *no* serialization in the
>> upper layers, this is *not* ok, even with my reordering. There must be
>> only one call to mux_control_deselect, and w/o serialization there
>> is a race where you might get multiple deselect calls when several
>> callers makes it through the active != index check before any of them
>> manages to set active = -1. That race must be taken care of!
> 
> Thank you, I've resent a version with a mutex lock around vmux->active.

I had a bunch of ifs in the above message, so I'm not sure it's needed.
I would expect there to be a lock outside somewhere in the media layer.
A cursory look gets me to media-entity.c and media_entity_setup_link()
which does have a mutex. But I'm no media expert, so maybe there are other
ways of getting to video_mux_link_setup that I'm not aware of?

If you do end up relying on external locking, a comment saying so would
be nice. Or even better, some __must_hold markup if possible?

Cheers,
peda

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Philipp Zabel]

On Tue, 2017-05-02 at 19:42 +0200, Peter Rosin wrote:
> On 2017-05-02 17:21, Philipp Zabel wrote:
> > On Sat, 2017-04-29 at 23:42 +0200, Peter Rosin wrote:
> >> On 2017-04-29 23:29, Peter Rosin wrote:
> >>> On 2017-04-28 16:13, Philipp Zabel wrote:
> >>>> This driver can handle SoC internal and external video bus multiplexers,
> >>>> controlled by mux controllers provided by the mux controller framework,
> >>>> such as MMIO register bitfields or GPIOs. The subdevice passes through
> >>>> the mbus configuration of the active input to the output side.
> >>>>
> >>>> Signed-off-by: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >>>> Signed-off-by: Philipp Zabel <p.zabel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >>>> Signed-off-by: Steve Longerbeam <steve_longerbeam-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> >>>> ---
> >>>> This has been last sent as part of the i.MX media series.
> >>>>
> >>>> Changes since https://patchwork.kernel.org/patch/9647869/:
> >>>>  - Split out the actual mux operation to be provided by the mux controller
> >>>>    framework [1]. GPIO and MMIO control can be provided by individual mux
> >>>>    controller drivers [2][3].
> >>>>    [1] https://patchwork.kernel.org/patch/9695837/
> >>>>    [2] https://patchwork.kernel.org/patch/9695839/
> >>>>    [3] https://patchwork.kernel.org/patch/9704509/
> >>>>  - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
> >>>>    vidsw with video_mux.
> >>>>  - Made the mux inactive by default, only activated by user interaction.
> >>>>  - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
> >>>>  - Reuse subdev.entity.num_pads instead of keeping our own count.
> >>>>  - Removed implicit link disabling. Instead, trying to enable a second
> >>>>    sink pad link yields -EBUSY.
> >>>>  - Merged _async_init into _probe.
> >>>>  - Removed superfluous pad index check from _set_format.
> >>>>  - Added is_source_pad helper to tell source and sink pads apart.
> >>>>  - Removed test for status property in endpoint nodes. Disable the remote
> >>>>    device or sever the endpoint link to disable a sink pad.
> >>>> ---
> >>>>  drivers/media/platform/Kconfig     |   6 +
> >>>>  drivers/media/platform/Makefile    |   2 +
> >>>>  drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 349 insertions(+)
> >>>>  create mode 100644 drivers/media/platform/video-mux.c
> >>>>
> >>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> >>>> index c9106e105baba..b046a6d39fee5 100644
> >>>> --- a/drivers/media/platform/Kconfig
> >>>> +++ b/drivers/media/platform/Kconfig
> >>>> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
> >>>>  	  To compile this driver as a module, choose M here: the
> >>>>  	  module will be called arv.
> >>>>  
> >>>> +config VIDEO_MUX
> >>>> +	tristate "Video Multiplexer"
> >>>> +	depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
> >>>> +	help
> >>>> +	  This driver provides support for N:1 video bus multiplexers.
> >>>> +
> >>>>  config VIDEO_OMAP3
> >>>>  	tristate "OMAP 3 Camera support"
> >>>>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> >>>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> >>>> index 349ddf6a69da2..fd2735ca3ff75 100644
> >>>> --- a/drivers/media/platform/Makefile
> >>>> +++ b/drivers/media/platform/Makefile
> >>>> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
> >>>>  
> >>>>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
> >>>>  
> >>>> +obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
> >>>> +
> >>>>  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
> >>>>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
> >>>>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
> >>>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> >>>> new file mode 100644
> >>>> index 0000000000000..419541729f67e
> >>>> --- /dev/null
> >>>> +++ b/drivers/media/platform/video-mux.c
> >>>> @@ -0,0 +1,341 @@
> >>>> +/*
> >>>> + * video stream multiplexer controlled via mux control
> >>>> + *
> >>>> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >>>> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >>>
> >>> 2017?
> >>>
> >>>> + *
> >>>> + * This program is free software; you can redistribute it and/or
> >>>> + * modify it under the terms of the GNU General Public License
> >>>> + * as published by the Free Software Foundation; either version 2
> >>>> + * of the License, or (at your option) any later version.
> >>>> + * This program is distributed in the hope that it will be useful,
> >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>>> + * GNU General Public License for more details.
> >>>> + */
> >>>> +
> >>>> +#include <linux/err.h>
> >>>> +#include <linux/module.h>
> >>>> +#include <linux/mux/consumer.h>
> >>>> +#include <linux/of.h>
> >>>> +#include <linux/of_graph.h>
> >>>> +#include <linux/platform_device.h>
> >>>> +#include <media/v4l2-async.h>
> >>>> +#include <media/v4l2-device.h>
> >>>> +#include <media/v4l2-subdev.h>
> >>>> +#include <media/v4l2-of.h>
> >>>> +
> >>>> +struct video_mux {
> >>>> +	struct v4l2_subdev subdev;
> >>>> +	struct media_pad *pads;
> >>>> +	struct v4l2_mbus_framefmt *format_mbus;
> >>>> +	struct v4l2_of_endpoint *endpoint;
> >>>> +	struct mux_control *mux;
> >>>> +	int active;
> >>>> +};
> >>>> +
> >>>> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
> >>>> +{
> >>>> +	return container_of(sd, struct video_mux, subdev);
> >>>> +}
> >>>> +
> >>>> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
> >>>> +{
> >>>> +	return pad == vmux->subdev.entity.num_pads - 1;
> >>>> +}
> >>>> +
> >>>> +static int video_mux_link_setup(struct media_entity *entity,
> >>>> +				const struct media_pad *local,
> >>>> +				const struct media_pad *remote, u32 flags)
> >>>> +{
> >>>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> >>>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> >>>> +	int ret;
> >>>> +
> >>>> +	/*
> >>>> +	 * The mux state is determined by the enabled sink pad link.
> >>>> +	 * Enabling or disabling the source pad link has no effect.
> >>>> +	 */
> >>>> +	if (is_source_pad(vmux, local->index))
> >>>> +		return 0;
> >>>> +
> >>>> +	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
> >>>> +		remote->entity->name, remote->index, local->entity->name,
> >>>> +		local->index, flags & MEDIA_LNK_FL_ENABLED);
> >>>> +
> >>>> +	if (flags & MEDIA_LNK_FL_ENABLED) {
> >>>> +		if (vmux->active == local->index)
> >>>
> >>> Here, you shortcut the mux_control_select_trylock test and return "OK"
> >>> based on a driver-local variable that is intended to keep track of mux
> >>> ownership.
> >>>
> >>>> +			return 0;
> >>>> +
> >>>> +		if (vmux->active >= 0)
> >>>
> >>> Here too (and this check is not needed, the situation will be covered by
> >>> the mux_control_try_select call).
> >>>
> >>>> +			return -EBUSY;
> >>>> +
> >>>> +		dev_dbg(sd->dev, "setting %d active\n", local->index);
> >>>> +		ret = mux_control_try_select(vmux->mux, local->index);
> >>>> +		if (ret < 0)
> >>>> +			return ret;
> >>>> +		vmux->active = local->index;
> >>>> +	} else {
> >>>> +		if (vmux->active != local->index)
> >>>> +			return 0;
> >>>> +
> >>>> +		dev_dbg(sd->dev, "going inactive\n");
> >>>> +		mux_control_deselect(vmux->mux);
> >>>
> >>> But here you let go of the mux *before* you clear the driver-local
> >>> ownership indicator. That looks suspicious. My guess is that this is
> >>> "safe" because the upper layers has some serialization, but I don't
> >>> know. Anyway, even if there is something saving you in the upper
> >>> layers, it looks out of order and unneeded. I would have moved the
> >>> below vmux->active = -1; statement up to before the above deselect.
> >>>
> >>> With that fixed, mux usage looks good to me, so you can add an Acked-
> >>> by from me if you wish (goes for the bindings patch as well).
> >>
> >> Ouch, that was a bit too soon. If there is *no* serialization in the
> >> upper layers, this is *not* ok, even with my reordering. There must be
> >> only one call to mux_control_deselect, and w/o serialization there
> >> is a race where you might get multiple deselect calls when several
> >> callers makes it through the active != index check before any of them
> >> manages to set active = -1. That race must be taken care of!
> > 
> > Thank you, I've resent a version with a mutex lock around vmux->active.
> 
> I had a bunch of ifs in the above message, so I'm not sure it's needed.
> I would expect there to be a lock outside somewhere in the media layer.
> A cursory look gets me to media-entity.c and media_entity_setup_link()
> which does have a mutex. But I'm no media expert, so maybe there are other
> ways of getting to video_mux_link_setup that I'm not aware of?

link_setup is always called under the graph mutex of the /dev/media
device. That is why I didn't think about locking too hard. In fact, I
initially wrote this expecting mux_control_get_exclusive to exist and
the mux select/deselect not to be locked at all.

But set_format is called from an unlocked ioctl on a /dev/v4l-subdev
device. Until your comments I didn't notice that it would be possible to
let link_setup set active = -1 in the middle of the set_format call,
causing it to return garbage.

> If you do end up relying on external locking, a comment saying so would
> be nice. Or even better, some __must_hold markup if possible?
> 
> Cheers,
> peda

regards
Philipp

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Philipp Zabel]

On Tue, 2017-05-02 at 19:42 +0200, Peter Rosin wrote:
> On 2017-05-02 17:21, Philipp Zabel wrote:
> > On Sat, 2017-04-29 at 23:42 +0200, Peter Rosin wrote:
> >> On 2017-04-29 23:29, Peter Rosin wrote:
> >>> On 2017-04-28 16:13, Philipp Zabel wrote:
> >>>> This driver can handle SoC internal and external video bus multiplexers,
> >>>> controlled by mux controllers provided by the mux controller framework,
> >>>> such as MMIO register bitfields or GPIOs. The subdevice passes through
> >>>> the mbus configuration of the active input to the output side.
> >>>>
> >>>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> >>>> ---
> >>>> This has been last sent as part of the i.MX media series.
> >>>>
> >>>> Changes since https://patchwork.kernel.org/patch/9647869/:
> >>>>  - Split out the actual mux operation to be provided by the mux controller
> >>>>    framework [1]. GPIO and MMIO control can be provided by individual mux
> >>>>    controller drivers [2][3].
> >>>>    [1] https://patchwork.kernel.org/patch/9695837/
> >>>>    [2] https://patchwork.kernel.org/patch/9695839/
> >>>>    [3] https://patchwork.kernel.org/patch/9704509/
> >>>>  - Shortened 'video-multiplexer' to 'video-mux', replaced all instances of
> >>>>    vidsw with video_mux.
> >>>>  - Made the mux inactive by default, only activated by user interaction.
> >>>>  - Added CONFIG_OF and CONFIG_MULTIPLEXER dependencies.
> >>>>  - Reuse subdev.entity.num_pads instead of keeping our own count.
> >>>>  - Removed implicit link disabling. Instead, trying to enable a second
> >>>>    sink pad link yields -EBUSY.
> >>>>  - Merged _async_init into _probe.
> >>>>  - Removed superfluous pad index check from _set_format.
> >>>>  - Added is_source_pad helper to tell source and sink pads apart.
> >>>>  - Removed test for status property in endpoint nodes. Disable the remote
> >>>>    device or sever the endpoint link to disable a sink pad.
> >>>> ---
> >>>>  drivers/media/platform/Kconfig     |   6 +
> >>>>  drivers/media/platform/Makefile    |   2 +
> >>>>  drivers/media/platform/video-mux.c | 341 +++++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 349 insertions(+)
> >>>>  create mode 100644 drivers/media/platform/video-mux.c
> >>>>
> >>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> >>>> index c9106e105baba..b046a6d39fee5 100644
> >>>> --- a/drivers/media/platform/Kconfig
> >>>> +++ b/drivers/media/platform/Kconfig
> >>>> @@ -74,6 +74,12 @@ config VIDEO_M32R_AR_M64278
> >>>>  	  To compile this driver as a module, choose M here: the
> >>>>  	  module will be called arv.
> >>>>  
> >>>> +config VIDEO_MUX
> >>>> +	tristate "Video Multiplexer"
> >>>> +	depends on OF && VIDEO_V4L2_SUBDEV_API && MEDIA_CONTROLLER && MULTIPLEXER
> >>>> +	help
> >>>> +	  This driver provides support for N:1 video bus multiplexers.
> >>>> +
> >>>>  config VIDEO_OMAP3
> >>>>  	tristate "OMAP 3 Camera support"
> >>>>  	depends on VIDEO_V4L2 && I2C && VIDEO_V4L2_SUBDEV_API && ARCH_OMAP3
> >>>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> >>>> index 349ddf6a69da2..fd2735ca3ff75 100644
> >>>> --- a/drivers/media/platform/Makefile
> >>>> +++ b/drivers/media/platform/Makefile
> >>>> @@ -27,6 +27,8 @@ obj-$(CONFIG_VIDEO_SH_VEU)		+= sh_veu.o
> >>>>  
> >>>>  obj-$(CONFIG_VIDEO_MEM2MEM_DEINTERLACE)	+= m2m-deinterlace.o
> >>>>  
> >>>> +obj-$(CONFIG_VIDEO_MUX)			+= video-mux.o
> >>>> +
> >>>>  obj-$(CONFIG_VIDEO_S3C_CAMIF) 		+= s3c-camif/
> >>>>  obj-$(CONFIG_VIDEO_SAMSUNG_EXYNOS4_IS) 	+= exynos4-is/
> >>>>  obj-$(CONFIG_VIDEO_SAMSUNG_S5P_JPEG)	+= s5p-jpeg/
> >>>> diff --git a/drivers/media/platform/video-mux.c b/drivers/media/platform/video-mux.c
> >>>> new file mode 100644
> >>>> index 0000000000000..419541729f67e
> >>>> --- /dev/null
> >>>> +++ b/drivers/media/platform/video-mux.c
> >>>> @@ -0,0 +1,341 @@
> >>>> +/*
> >>>> + * video stream multiplexer controlled via mux control
> >>>> + *
> >>>> + * Copyright (C) 2013 Pengutronix, Sascha Hauer <kernel@pengutronix.de>
> >>>> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel@pengutronix.de>
> >>>
> >>> 2017?
> >>>
> >>>> + *
> >>>> + * This program is free software; you can redistribute it and/or
> >>>> + * modify it under the terms of the GNU General Public License
> >>>> + * as published by the Free Software Foundation; either version 2
> >>>> + * of the License, or (at your option) any later version.
> >>>> + * This program is distributed in the hope that it will be useful,
> >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>>> + * GNU General Public License for more details.
> >>>> + */
> >>>> +
> >>>> +#include <linux/err.h>
> >>>> +#include <linux/module.h>
> >>>> +#include <linux/mux/consumer.h>
> >>>> +#include <linux/of.h>
> >>>> +#include <linux/of_graph.h>
> >>>> +#include <linux/platform_device.h>
> >>>> +#include <media/v4l2-async.h>
> >>>> +#include <media/v4l2-device.h>
> >>>> +#include <media/v4l2-subdev.h>
> >>>> +#include <media/v4l2-of.h>
> >>>> +
> >>>> +struct video_mux {
> >>>> +	struct v4l2_subdev subdev;
> >>>> +	struct media_pad *pads;
> >>>> +	struct v4l2_mbus_framefmt *format_mbus;
> >>>> +	struct v4l2_of_endpoint *endpoint;
> >>>> +	struct mux_control *mux;
> >>>> +	int active;
> >>>> +};
> >>>> +
> >>>> +static inline struct video_mux *v4l2_subdev_to_video_mux(struct v4l2_subdev *sd)
> >>>> +{
> >>>> +	return container_of(sd, struct video_mux, subdev);
> >>>> +}
> >>>> +
> >>>> +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
> >>>> +{
> >>>> +	return pad == vmux->subdev.entity.num_pads - 1;
> >>>> +}
> >>>> +
> >>>> +static int video_mux_link_setup(struct media_entity *entity,
> >>>> +				const struct media_pad *local,
> >>>> +				const struct media_pad *remote, u32 flags)
> >>>> +{
> >>>> +	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> >>>> +	struct video_mux *vmux = v4l2_subdev_to_video_mux(sd);
> >>>> +	int ret;
> >>>> +
> >>>> +	/*
> >>>> +	 * The mux state is determined by the enabled sink pad link.
> >>>> +	 * Enabling or disabling the source pad link has no effect.
> >>>> +	 */
> >>>> +	if (is_source_pad(vmux, local->index))
> >>>> +		return 0;
> >>>> +
> >>>> +	dev_dbg(sd->dev, "link setup '%s':%d->'%s':%d[%d]",
> >>>> +		remote->entity->name, remote->index, local->entity->name,
> >>>> +		local->index, flags & MEDIA_LNK_FL_ENABLED);
> >>>> +
> >>>> +	if (flags & MEDIA_LNK_FL_ENABLED) {
> >>>> +		if (vmux->active == local->index)
> >>>
> >>> Here, you shortcut the mux_control_select_trylock test and return "OK"
> >>> based on a driver-local variable that is intended to keep track of mux
> >>> ownership.
> >>>
> >>>> +			return 0;
> >>>> +
> >>>> +		if (vmux->active >= 0)
> >>>
> >>> Here too (and this check is not needed, the situation will be covered by
> >>> the mux_control_try_select call).
> >>>
> >>>> +			return -EBUSY;
> >>>> +
> >>>> +		dev_dbg(sd->dev, "setting %d active\n", local->index);
> >>>> +		ret = mux_control_try_select(vmux->mux, local->index);
> >>>> +		if (ret < 0)
> >>>> +			return ret;
> >>>> +		vmux->active = local->index;
> >>>> +	} else {
> >>>> +		if (vmux->active != local->index)
> >>>> +			return 0;
> >>>> +
> >>>> +		dev_dbg(sd->dev, "going inactive\n");
> >>>> +		mux_control_deselect(vmux->mux);
> >>>
> >>> But here you let go of the mux *before* you clear the driver-local
> >>> ownership indicator. That looks suspicious. My guess is that this is
> >>> "safe" because the upper layers has some serialization, but I don't
> >>> know. Anyway, even if there is something saving you in the upper
> >>> layers, it looks out of order and unneeded. I would have moved the
> >>> below vmux->active = -1; statement up to before the above deselect.
> >>>
> >>> With that fixed, mux usage looks good to me, so you can add an Acked-
> >>> by from me if you wish (goes for the bindings patch as well).
> >>
> >> Ouch, that was a bit too soon. If there is *no* serialization in the
> >> upper layers, this is *not* ok, even with my reordering. There must be
> >> only one call to mux_control_deselect, and w/o serialization there
> >> is a race where you might get multiple deselect calls when several
> >> callers makes it through the active != index check before any of them
> >> manages to set active = -1. That race must be taken care of!
> > 
> > Thank you, I've resent a version with a mutex lock around vmux->active.
> 
> I had a bunch of ifs in the above message, so I'm not sure it's needed.
> I would expect there to be a lock outside somewhere in the media layer.
> A cursory look gets me to media-entity.c and media_entity_setup_link()
> which does have a mutex. But I'm no media expert, so maybe there are other
> ways of getting to video_mux_link_setup that I'm not aware of?

link_setup is always called under the graph mutex of the /dev/media
device. That is why I didn't think about locking too hard. In fact, I
initially wrote this expecting mux_control_get_exclusive to exist and
the mux select/deselect not to be locked at all.

But set_format is called from an unlocked ioctl on a /dev/v4l-subdev
device. Until your comments I didn't notice that it would be possible to
let link_setup set active = -1 in the middle of the set_format call,
causing it to return garbage.

> If you do end up relying on external locking, a comment saying so would
> be nice. Or even better, some __must_hold markup if possible?
> 
> Cheers,
> peda

regards
Philipp

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Peter Rosin]

On 2017-05-03 10:35, Philipp Zabel wrote:
> On Tue, 2017-05-02 at 19:42 +0200, Peter Rosin wrote:
>> On 2017-05-02 17:21, Philipp Zabel wrote:
>>> Thank you, I've resent a version with a mutex lock around vmux->active.
>>
>> I had a bunch of ifs in the above message, so I'm not sure it's needed.
>> I would expect there to be a lock outside somewhere in the media layer.
>> A cursory look gets me to media-entity.c and media_entity_setup_link()
>> which does have a mutex. But I'm no media expert, so maybe there are other
>> ways of getting to video_mux_link_setup that I'm not aware of?
> 
> link_setup is always called under the graph mutex of the /dev/media
> device. That is why I didn't think about locking too hard. In fact, I
> initially wrote this expecting mux_control_get_exclusive to exist and
> the mux select/deselect not to be locked at all.
> 
> But set_format is called from an unlocked ioctl on a /dev/v4l-subdev
> device. Until your comments I didn't notice that it would be possible to
> let link_setup set active = -1 in the middle of the set_format call,
> causing it to return garbage.

Obviously, now that you point it out. I completely missed set_format...

Cheers,
peda

Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Peter Rosin]

On 2017-05-03 10:35, Philipp Zabel wrote:
> On Tue, 2017-05-02 at 19:42 +0200, Peter Rosin wrote:
>> On 2017-05-02 17:21, Philipp Zabel wrote:
>>> Thank you, I've resent a version with a mutex lock around vmux->active.
>>
>> I had a bunch of ifs in the above message, so I'm not sure it's needed.
>> I would expect there to be a lock outside somewhere in the media layer.
>> A cursory look gets me to media-entity.c and media_entity_setup_link()
>> which does have a mutex. But I'm no media expert, so maybe there are other
>> ways of getting to video_mux_link_setup that I'm not aware of?
> 
> link_setup is always called under the graph mutex of the /dev/media
> device. That is why I didn't think about locking too hard. In fact, I
> initially wrote this expecting mux_control_get_exclusive to exist and
> the mux select/deselect not to be locked at all.
> 
> But set_format is called from an unlocked ioctl on a /dev/v4l-subdev
> device. Until your comments I didn't notice that it would be possible to
> let link_setup set active = -1 in the middle of the set_format call,
> causing it to return garbage.

Obviously, now that you point it out. I completely missed set_format...

Cheers,
peda