Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver
From: Peter Rosin <hidden>
Date: 2017-05-02 17:42:52
Also in:
linux-media
On 2017-05-02 17:21, Philipp Zabel wrote:
On Sat, 2017-04-29 at 23:42 +0200, Peter Rosin wrote:quoted
On 2017-04-29 23:29, Peter Rosin wrote:quoted
On 2017-04-28 16:13, Philipp Zabel wrote:quoted
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 <redacted> Signed-off-by: Philipp Zabel <redacted> Signed-off-by: Steve Longerbeam <redacted> --- 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.cdiff --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_OMAP3diff --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?quoted
+ * + * 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.quoted
+ 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).quoted
+ 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