From mboxrd@z Thu Jan 1 00:00:00 1970
From: Philipp Zabel
Subject: Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice
driver
Date: Tue, 02 May 2017 17:21:31 +0200
Message-ID: <1493738491.2391.20.camel@pengutronix.de>
References: <20170428141330.16187-1-p.zabel@pengutronix.de>
<20170428141330.16187-2-p.zabel@pengutronix.de>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Return-path:
In-Reply-To:
Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
To: Peter Rosin
Cc: linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Steve Longerbeam , kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org, Sascha Hauer , Rob Herring , Sakari Ailus , Pavel Machek , Steve Longerbeam , Vladimir Zapolskiy
List-Id: devicetree@vger.kernel.org
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
> >> Signed-off-by: Philipp Zabel
> >> Signed-off-by: Steve Longerbeam
> >> ---
> >> 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
> >> + * Copyright (C) 2016 Pengutronix, Philipp Zabel
> >
> > 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
> >> +#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_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
From mboxrd@z Thu Jan 1 00:00:00 1970
Return-path:
Received: from metis.ext.4.pengutronix.de ([92.198.50.35]:53707 "EHLO
metis.ext.4.pengutronix.de" rhost-flags-OK-OK-OK-OK)
by vger.kernel.org with ESMTP id S1751154AbdEBPVe (ORCPT
);
Tue, 2 May 2017 11:21:34 -0400
Message-ID: <1493738491.2391.20.camel@pengutronix.de>
Subject: Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice
driver
From: Philipp Zabel
To: Peter Rosin
Cc: linux-media@vger.kernel.org, Mark Rutland ,
devicetree@vger.kernel.org,
Steve Longerbeam ,
kernel@pengutronix.de, Sascha Hauer ,
Rob Herring ,
Sakari Ailus ,
Pavel Machek ,
Steve Longerbeam ,
Vladimir Zapolskiy
Date: Tue, 02 May 2017 17:21:31 +0200
In-Reply-To:
References: <20170428141330.16187-1-p.zabel@pengutronix.de>
<20170428141330.16187-2-p.zabel@pengutronix.de>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-media-owner@vger.kernel.org
List-ID:
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
> >> Signed-off-by: Philipp Zabel
> >> Signed-off-by: Steve Longerbeam
> >> ---
> >> 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
> >> + * Copyright (C) 2016 Pengutronix, Philipp Zabel
> >
> > 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
> >> +#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_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