| Message ID | 20170428141330.16187-1-p.zabel@pengutronix.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
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"); > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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"); >> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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>; + }; + }; + }; +};