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

[Philipp Zabel]

On Thu, 2017-05-04 at 12:48 +0300, Sakari Ailus wrote:
> Hi Philipp,
> 
> On Thu, May 04, 2017 at 11:26:18AM +0200, Philipp Zabel wrote:
> > Hi Sakari,
> > 
> > On Thu, 2017-05-04 at 10:17 +0300, Sakari Ailus wrote:
> > > Hi Philipp,
> > > 
> > > On Thu, May 04, 2017 at 09:07:32AM +0200, Philipp Zabel wrote:
> > > > On Wed, 2017-05-03 at 22:28 +0300, Sakari Ailus wrote:
> > > > > Hi Philipp,
> > > > > 
> > > > > Thanks for continuing working on this!
> > > > > 
> > > > > I have some minor comments below...
> > > > 
> > > > Thank you for the comments.
> > > > 
> > > > [...]
> > > > > Could you rebase this on the V4L2 fwnode patchset here, please?
> > > > > 
> > > > > 
> > > > >
> > > > > The conversion is rather simple, as shown here:
> > > > > 
> > > > > 
> > > > 
> > > > What is the status of this patchset? Will this be merged soon?
> > > 
> > > I intend to send a pull request once the next rc1 tag is pulled on
> > > media-tree master. It depends on patches in linux-pm tree that aren't in
> > > media-tree yet.
> > 
> > I get conflicts trying to merge v4l2-acpi into v4.11 or media-tree
> > master. Could you provide an updated version?
> 
> What kind of conflicts? I wonder if something somewhere is out of sync. :-)
> My v4l2-acpi branch is on top of current media-tree master and appears to
> merge cleanly to v4.11 as well.
> 
> It still wouldn't compile though as it depends on the fwnode graph patches.
> 
> I've merged those here:
> 
> 

My bad, I accidentally used an old git remote that still pointed to
git://git.retiisi.org.uk/~sailus/linux.git. Fixed that now.

thanks
Philipp

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

[Sakari Ailus]

Hi Philipp,

On Thu, May 04, 2017 at 11:26:18AM +0200, Philipp Zabel wrote:
> Hi Sakari,
> 
> On Thu, 2017-05-04 at 10:17 +0300, Sakari Ailus wrote:
> > Hi Philipp,
> > 
> > On Thu, May 04, 2017 at 09:07:32AM +0200, Philipp Zabel wrote:
> > > On Wed, 2017-05-03 at 22:28 +0300, Sakari Ailus wrote:
> > > > Hi Philipp,
> > > > 
> > > > Thanks for continuing working on this!
> > > > 
> > > > I have some minor comments below...
> > > 
> > > Thank you for the comments.
> > > 
> > > [...]
> > > > Could you rebase this on the V4L2 fwnode patchset here, please?
> > > > 
> > > > 
> > > >
> > > > The conversion is rather simple, as shown here:
> > > > 
> > > > 
> > > 
> > > What is the status of this patchset? Will this be merged soon?
> > 
> > I intend to send a pull request once the next rc1 tag is pulled on
> > media-tree master. It depends on patches in linux-pm tree that aren't in
> > media-tree yet.
> 
> I get conflicts trying to merge v4l2-acpi into v4.11 or media-tree
> master. Could you provide an updated version?

What kind of conflicts? I wonder if something somewhere is out of sync. :-)
My v4l2-acpi branch is on top of current media-tree master and appears to
merge cleanly to v4.11 as well.

It still wouldn't compile though as it depends on the fwnode graph patches.

I've merged those here:



-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

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

[Philipp Zabel]

Hi Sakari,

On Thu, 2017-05-04 at 10:17 +0300, Sakari Ailus wrote:
> Hi Philipp,
> 
> On Thu, May 04, 2017 at 09:07:32AM +0200, Philipp Zabel wrote:
> > On Wed, 2017-05-03 at 22:28 +0300, Sakari Ailus wrote:
> > > Hi Philipp,
> > > 
> > > Thanks for continuing working on this!
> > > 
> > > I have some minor comments below...
> > 
> > Thank you for the comments.
> > 
> > [...]
> > > Could you rebase this on the V4L2 fwnode patchset here, please?
> > > 
> > > 
> > >
> > > The conversion is rather simple, as shown here:
> > > 
> > > 
> > 
> > What is the status of this patchset? Will this be merged soon?
> 
> I intend to send a pull request once the next rc1 tag is pulled on
> media-tree master. It depends on patches in linux-pm tree that aren't in
> media-tree yet.

I get conflicts trying to merge v4l2-acpi into v4.11 or media-tree
master. Could you provide an updated version?

regards
Philipp

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

[Sakari Ailus]

Hi Philipp,

On Thu, May 04, 2017 at 09:07:32AM +0200, Philipp Zabel wrote:
> On Wed, 2017-05-03 at 22:28 +0300, Sakari Ailus wrote:
> > Hi Philipp,
> > 
> > Thanks for continuing working on this!
> > 
> > I have some minor comments below...
> 
> Thank you for the comments.
> 
> [...]
> > Could you rebase this on the V4L2 fwnode patchset here, please?
> > 
> > 
> >
> > The conversion is rather simple, as shown here:
> > 
> > 
> 
> What is the status of this patchset? Will this be merged soon?

I intend to send a pull request once the next rc1 tag is pulled on
media-tree master. It depends on patches in linux-pm tree that aren't in
media-tree yet.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk

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

[Philipp Zabel]

On Wed, 2017-05-03 at 22:28 +0300, Sakari Ailus wrote:
> Hi Philipp,
> 
> Thanks for continuing working on this!
> 
> I have some minor comments below...

Thank you for the comments.

[...]
> Could you rebase this on the V4L2 fwnode patchset here, please?
> 
> 
>
> The conversion is rather simple, as shown here:
> 
> 

What is the status of this patchset? Will this be merged soon?

[...]
> > +static inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)
> 
> It's a common practice to test pad flags rather than the pad number.
> Although the pad number here implicitly tells this, too, testing pad flags
> is cleaner.
> 
> The matter was discussed in the past and it was decided not to add helper
> functions to the framework for the purpose as testing the flags is trivial.

Ok, I'll drop is_source_pad and check (pad->flags & MEDIA_PAD_FL_SOURCE)
instead in the next version.

[...]
> > +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;
> > +
> > +   mutex_lock(>lock);
> > +
> > +   /* 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];
> > +
> > +   mutex_unlock(>lock);
> > +
> > +   *mbusformat = sdformat->format;
> 
> Shouldn't you do this before releasing the mutex? The assignment won't be
> an atomic operation. Same for get_format; you should take the mutex.

Yes, I'll extend the mutex to cover the mbus formats.

[...]
> > +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 = {
> 
> Const for both of the structs?

Will do, thanks.

regards
Philipp

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

[Sakari Ailus]

Hi Philipp,

Thanks for continuing working on this!

I have some minor comments below...

On Tue, May 02, 2017 at 05:09:13PM +0200, 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 v1 [1]:
>  - Protect vmux->active with a mutex in link_setup and set_format.
>s_stream does not need to be locked; it is called when the pipeline
>is started and thus the link setup can not be changed anymore.
>  - Remove the unused g_mbus_config.
> 
> This was previously sent as part of Steve's i.MX media series [2].
> 
> [1] https://patchwork.kernel.org/patch/9704791/
> [2] https://patchwork.kernel.org/patch/9647869/
> ---
>  drivers/media/platform/Kconfig |   6 +
>  drivers/media/platform/Makefile|   2 +
>  drivers/media/platform/video-mux.c | 312 
> +
>  3 files changed, 320 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 0..a857f5e89deff
> --- /dev/null
> +++ b/drivers/media/platform/video-mux.c
> @@ -0,0 +1,312 @@
> +/*
> + * 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 

Could you rebase this on the V4L2 fwnode patchset here, please?



The conversion is rather simple, as shown here:



> +
> +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;
> + 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 inline bool is_source_pad(struct video_mux *vmux, unsigned int pad)

It's a common practice to test pad flags rather than the pad number.
Although the pad number here implicitly tells this, too, testing pad flags
is cleaner.

The matter was discussed in the past and it was decided not to add helper
functions to the framework for the purpose as testing the flags is trivial.

> +{
> + return pad == vmux->subdev.entity.num_pads - 1;
> +}
> +

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

[Philipp Zabel]

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 v1 [1]:
 - Protect vmux->active with a mutex in link_setup and set_format.
   s_stream does not need to be locked; it is called when the pipeline
   is started and thus the link setup can not be changed anymore.
 - Remove the unused g_mbus_config.

This was previously sent as part of Steve's i.MX media series [2].

[1] https://patchwork.kernel.org/patch/9704791/
[2] https://patchwork.kernel.org/patch/9647869/
---
 drivers/media/platform/Kconfig |   6 +
 drivers/media/platform/Makefile|   2 +
 drivers/media/platform/video-mux.c | 312 +
 3 files changed, 320 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 0..a857f5e89deff
--- /dev/null
+++ b/drivers/media/platform/video-mux.c
@@ -0,0 +1,312 @@
+/*
+ * 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_of_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 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 = 0;
+
+   /*
+* 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);
+
+   mutex_lock(>lock);
+
+   if (flags & MEDIA_LNK_FL_ENABLED) {
+   if (vmux->active == local->index)
+   goto out;
+
+