From mboxrd@z Thu Jan 1 00:00:00 1970 From: Philipp Zabel Subject: Re: [PATCH v3 2/2] [media] platform: add video-multiplexer subdevice driver Date: Thu, 04 May 2017 17:13:35 +0200 Message-ID: <1493910815.2381.28.camel@pengutronix.de> References: <1493905137-27051-1-git-send-email-p.zabel@pengutronix.de> <1493905137-27051-2-git-send-email-p.zabel@pengutronix.de> <3e26dc24-7135-2f52-9d7e-72e47ae219d4@ideasonboard.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <3e26dc24-7135-2f52-9d7e-72e47ae219d4@ideasonboard.com> Sender: linux-media-owner@vger.kernel.org To: kieran.bingham@ideasonboard.com Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org, Steve Longerbeam , Peter Rosin , Sakari Ailus , Pavel Machek , Rob Herring , Mark Rutland , Vladimir Zapolskiy , kernel@pengutronix.de, Sascha Hauer , Steve Longerbeam List-Id: devicetree@vger.kernel.org Hi Kieran, On Thu, 2017-05-04 at 15:59 +0100, Kieran Bingham wrote: > Hi Philipp, > > Thankyou for the patch, thank you for reviewing. > On 04/05/17 14:38, 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 > > Signed-off-by: Philipp Zabel > > Signed-off-by: Steve Longerbeam > > --- > > Changes since v2 [1]: > > - Extend vmux->lock to protect mbus format against simultaneous access > > from get/set_format calls. > > - Drop is_source_pad(), check pad->flags & MEDIA_PAD_FL_SOURCE directly. > > - Replace v4l2_of_parse_endpoint call with v4l2_fwnode_endpoint_parse, > > include media/v4l2-fwnode.h instead of media/v4l2-of.h. > > - Constify ops structures. > > > > This was previously sent as part of Steve's i.MX media series [2]. > > Tested against > > https://git.linuxtv.org/sailus/media_tree.git/log/?h=v4l2-acpi-merge > > > > [1] https://patchwork.kernel.org/patch/9708237/ > > [2] https://patchwork.kernel.org/patch/9647869/ > > --- > > drivers/media/platform/Kconfig | 6 + > > drivers/media/platform/Makefile | 2 + > > drivers/media/platform/video-mux.c | 318 +++++++++++++++++++++++++++++++++++++ > > 3 files changed, 326 insertions(+) > > create mode 100644 drivers/media/platform/video-mux.c > > > > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig > > index 317f8d41e4ad6..2fea699962624 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 63303d63c64cf..a6363023f981e 100644 > > --- a/drivers/media/platform/Makefile > > +++ b/drivers/media/platform/Makefile > > @@ -28,6 +28,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..afa06febf6c33 > > --- /dev/null > > +++ b/drivers/media/platform/video-mux.c > > @@ -0,0 +1,318 @@ > > +/* > > + * video stream multiplexer controlled via mux control > > + * > > + * Copyright (C) 2013 Pengutronix, Sascha Hauer > > + * Copyright (C) 2016-2017 Pengutronix, Philipp Zabel > > + * > > + * 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +struct video_mux { > > + struct v4l2_subdev subdev; > > + struct media_pad *pads; > > + struct v4l2_mbus_framefmt *format_mbus; > > + struct v4l2_fwnode_endpoint *endpoint; > > + struct mux_control *mux; > > + struct mutex lock; > > + 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 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 = 0; > > + > > + /* > > + * The mux state is determined by the enabled sink pad link. > > + * Enabling or disabling the source pad link has no effect. > > + */ > > + if (local->flags & MEDIA_PAD_FL_SOURCE) > > + 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); > > + > > + mutex_lock(&vmux->lock); > > + > > + if (flags & MEDIA_LNK_FL_ENABLED) { > > + if (vmux->active == local->index) > > + goto out; > > + > > + if (vmux->active >= 0) { > > + ret = -EBUSY; > > + goto out; > > + } > > + > > + dev_dbg(sd->dev, "setting %d active\n", local->index); > > + ret = mux_control_try_select(vmux->mux, local->index); > > + if (ret < 0) > > + goto out; > > + vmux->active = local->index; > > + } else { > > + if (vmux->active != local->index) > > + goto out; > > + > > + dev_dbg(sd->dev, "going inactive\n"); > > + mux_control_deselect(vmux->mux); > > + vmux->active = -1; > > + } > > + > > +out: > > + mutex_unlock(&vmux->lock); > > + return ret; > > +} > > + > > +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_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 = { > > + .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) > > +{ > > + struct video_mux *vmux = v4l2_subdev_to_video_mux(sd); > > + > > + mutex_lock(&vmux->lock); > > + > > + sdformat->format = *__video_mux_get_pad_format(sd, cfg, sdformat->pad, > > + sdformat->which); > > + > > + mutex_unlock(&vmux->lock); > > + > > + 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; > > + struct media_pad *pad = &vmux->pads[sdformat->pad]; > > + > > + mbusformat = __video_mux_get_pad_format(sd, cfg, sdformat->pad, > > + sdformat->which); > > + if (!mbusformat) > > + return -EINVAL; > > + > > + mutex_lock(&vmux->lock); > > + > > + /* Source pad mirrors active sink pad, no limitations on sink pads */ > > + if ((pad->flags & MEDIA_PAD_FL_SOURCE) && vmux->active >= 0) > > + sdformat->format = vmux->format_mbus[vmux->active]; > > + > > + *mbusformat = sdformat->format; > > + > > + mutex_unlock(&vmux->lock); > > + > > + return 0; > > +} > > + > > +static const struct v4l2_subdev_pad_ops video_mux_pad_ops = { > > + .get_fmt = video_mux_get_format, > > + .set_fmt = video_mux_set_format, > > +}; > > + > > +static const 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 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) { > > + struct of_endpoint endpoint; > > + > > + of_graph_parse_endpoint(ep, &endpoint); > > + num_pads = max(num_pads, endpoint.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; > > + } > > + > > + mutex_init(&vmux->lock); > > + 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) { > > + struct v4l2_fwnode_endpoint endpoint; > > + > > + v4l2_fwnode_endpoint_parse(of_fwnode_handle(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; > > There is no verification here that endpoint.base.port is a valid value. > ( <= num_pads - 1 ) I just noticed that when changing to devm_kcalloc as Sebastian suggested. I'll drop the endpoint parsing completely for now, as it isn't even needed anymore with g_mbus_config gone. > As it comes in from the DT, it 'could' be set as any arbitrary value. Worse, we expect the output port (num_pads - 1) to exist and have a valid link. But the vmux->endpoint array length is only num_pads - 1. I have lost the sink port check at some point. > Should it be sanity checked ? Or do we trust the DT at this point ? Since this would avoid possible memory corruption, yes, it would be better to check. regards Philipp