Re: [PATCH 2/2] [media] platform: add video-multiplexer subdevice driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

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




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux