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@xxxxxxxxxxxxxx> > >>>> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> > >>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@xxxxxxxxxx> > >>>> --- > >>>> 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@xxxxxxxxxxxxxx> > >>>> + * Copyright (C) 2016 Pengutronix, Philipp Zabel <kernel@xxxxxxxxxxxxxx> > >>> > >>> 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
