[RFC 0/2] Memory-to-memory media controller topology

[RFC 0/2] Memory-to-memory media controller topology

[Ezequiel Garcia]

As discussed on IRC, memory-to-memory need to be modeled
properly in order to be supported by the media controller
framework, and thus to support the Request API.

This RFC is a first draft on the memory-to-memory
media controller topology.

The topology looks like this:

Device topology
- entity 1: input (1 pad, 1 link)
            type Node subtype Unknown flags 0
	pad0: Source
		-> "proc":1 [ENABLED,IMMUTABLE]

- entity 3: proc (2 pads, 2 links)
            type Node subtype Unknown flags 0
	pad0: Source
		-> "output":0 [ENABLED,IMMUTABLE]
	pad1: Sink
		<- "input":0 [ENABLED,IMMUTABLE]

- entity 6: output (1 pad, 1 link)
            type Node subtype Unknown flags 0
	pad0: Sink
		<- "proc":0 [ENABLED,IMMUTABLE]

The first commit introduces a register/unregister API,
that creates/destroys all the entities and pads needed,
and links them.

The second commit uses this API to support the vim2m driver.

Notes
-----

* A new device node type is introduced VFL_TYPE_MEM2MEM,
  this is mostly done so the video4linux core doesn't
  try to register other media controller entities.

* Also, a new media entity type is introduced. Memory-to-memory
  devices have a multi-entity description and so can't
  be simply embedded in other structs, or cast from other structs.

Ezequiel Garcia (1):
  media: add helpers for memory-to-memory media controller

Hans Verkuil (1):
  vim2m: add media device

 drivers/media/platform/vim2m.c         |  41 ++++++-
 drivers/media/v4l2-core/v4l2-dev.c     |  23 ++--
 drivers/media/v4l2-core/v4l2-mem2mem.c | 157 +++++++++++++++++++++++++
 include/media/media-entity.h           |   4 +
 include/media/v4l2-dev.h               |   2 +
 include/media/v4l2-mem2mem.h           |   5 +
 include/uapi/linux/media.h             |   2 +
 7 files changed, 222 insertions(+), 12 deletions(-)

-- 
2.17.1

[RFC 1/2] media: add helpers for memory-to-memory media controller

[Ezequiel Garcia]

A memory-to-memory pipeline device consists in three
entities: two DMA engine and one video processing entities.
The DMA engine entities are linked to a V4L interface.

This commit add a new v4l2_m2m_{un}register_media_controller
API to register this topology.

For instance, a typical mem2mem device topology would
look like this:

- entity 1: input (1 pad, 1 link)
            type Node subtype Unknown flags 0
	pad0: Source
		-> "proc":1 [ENABLED,IMMUTABLE]

- entity 3: proc (2 pads, 2 links)
            type Node subtype Unknown flags 0
	pad0: Source
		-> "output":0 [ENABLED,IMMUTABLE]
	pad1: Sink
		<- "input":0 [ENABLED,IMMUTABLE]

- entity 6: output (1 pad, 1 link)
            type Node subtype Unknown flags 0
	pad0: Sink
		<- "proc":0 [ENABLED,IMMUTABLE]

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Suggested-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/v4l2-core/v4l2-dev.c     |  23 ++--
 drivers/media/v4l2-core/v4l2-mem2mem.c | 157 +++++++++++++++++++++++++
 include/media/media-entity.h           |   4 +
 include/media/v4l2-dev.h               |   2 +
 include/media/v4l2-mem2mem.h           |   5 +
 include/uapi/linux/media.h             |   2 +
 6 files changed, 186 insertions(+), 7 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index 4ffd7d60a901..ec8f20f0fdc5 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -202,7 +202,7 @@ static void v4l2_device_release(struct device *cd)
 	mutex_unlock(&videodev_lock);
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
-	if (v4l2_dev->mdev) {
+	if (v4l2_dev->mdev && vdev->vfl_type != VFL_TYPE_MEM2MEM) {
 		/* Remove interfaces and interface links */
 		media_devnode_remove(vdev->intf_devnode);
 		if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
@@ -530,6 +530,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 	bool is_radio = vdev->vfl_type == VFL_TYPE_RADIO;
 	bool is_sdr = vdev->vfl_type == VFL_TYPE_SDR;
 	bool is_tch = vdev->vfl_type == VFL_TYPE_TOUCH;
+	bool is_m2m = vdev->vfl_type == VFL_TYPE_MEM2MEM;
 	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
 	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
 
@@ -576,7 +577,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 	if (ops->vidioc_enum_freq_bands || ops->vidioc_g_tuner || ops->vidioc_g_modulator)
 		set_bit(_IOC_NR(VIDIOC_ENUM_FREQ_BANDS), valid_ioctls);
 
-	if (is_vid || is_tch) {
+	if (is_vid || is_m2m || is_tch) {
 		/* video and metadata specific ioctls */
 		if ((is_rx && (ops->vidioc_enum_fmt_vid_cap ||
 			       ops->vidioc_enum_fmt_vid_cap_mplane ||
@@ -669,7 +670,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
 	}
 
-	if (is_vid || is_vbi || is_sdr || is_tch) {
+	if (is_vid || is_m2m || is_vbi || is_sdr || is_tch) {
 		/* ioctls valid for video, metadata, vbi or sdr */
 		SET_VALID_IOCTL(ops, VIDIOC_REQBUFS, vidioc_reqbufs);
 		SET_VALID_IOCTL(ops, VIDIOC_QUERYBUF, vidioc_querybuf);
@@ -682,7 +683,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 		SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
 	}
 
-	if (is_vid || is_vbi || is_tch) {
+	if (is_vid || is_m2m || is_vbi || is_tch) {
 		/* ioctls valid for video or vbi */
 		if (ops->vidioc_s_std)
 			set_bit(_IOC_NR(VIDIOC_ENUMSTD), valid_ioctls);
@@ -733,7 +734,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
 			BASE_VIDIOC_PRIVATE);
 }
 
-static int video_register_media_controller(struct video_device *vdev, int type)
+static int video_register_media_controller(struct video_device *vdev)
 {
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	u32 intf_type;
@@ -745,7 +746,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
 	vdev->entity.obj_type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
 	vdev->entity.function = MEDIA_ENT_F_UNKNOWN;
 
-	switch (type) {
+	switch (vdev->vfl_type) {
 	case VFL_TYPE_GRABBER:
 		intf_type = MEDIA_INTF_T_V4L_VIDEO;
 		vdev->entity.function = MEDIA_ENT_F_IO_V4L;
@@ -774,6 +775,10 @@ static int video_register_media_controller(struct video_device *vdev, int type)
 		intf_type = MEDIA_INTF_T_V4L_SUBDEV;
 		/* Entity will be created via v4l2_device_register_subdev() */
 		break;
+	case VFL_TYPE_MEM2MEM:
+		/* Memory-to-memory devices are more complex and use
+		 * their own function to register.
+		 */
 	default:
 		return 0;
 	}
@@ -869,6 +874,10 @@ int __video_register_device(struct video_device *vdev,
 	case VFL_TYPE_TOUCH:
 		name_base = "v4l-touch";
 		break;
+	case VFL_TYPE_MEM2MEM:
+		/* Maintain this name for backwards compatibility */
+		name_base = "video";
+		break;
 	default:
 		pr_err("%s called with unknown type: %d\n",
 		       __func__, type);
@@ -993,7 +1002,7 @@ int __video_register_device(struct video_device *vdev,
 	v4l2_device_get(vdev->v4l2_dev);
 
 	/* Part 5: Register the entity. */
-	ret = video_register_media_controller(vdev, type);
+	ret = video_register_media_controller(vdev);
 
 	/* Part 6: Activate this minor. The char device can now be used. */
 	set_bit(V4L2_FL_REGISTERED, &vdev->flags);
diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
index c4f963d96a79..0505b65bfa68 100644
--- a/drivers/media/v4l2-core/v4l2-mem2mem.c
+++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
@@ -17,9 +17,11 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 
+#include <media/media-device.h>
 #include <media/videobuf2-v4l2.h>
 #include <media/v4l2-mem2mem.h>
 #include <media/v4l2-dev.h>
+#include <media/v4l2-device.h>
 #include <media/v4l2-fh.h>
 #include <media/v4l2-event.h>
 
@@ -50,6 +52,11 @@ module_param(debug, bool, 0644);
  * offsets but for different queues */
 #define DST_QUEUE_OFF_BASE	(1 << 30)
 
+struct v4l2_m2m_entity {
+	struct media_entity entity;
+	struct media_pad pads[2];
+	char name[64];
+};
 
 /**
  * struct v4l2_m2m_dev - per-device context
@@ -60,6 +67,10 @@ module_param(debug, bool, 0644);
  */
 struct v4l2_m2m_dev {
 	struct v4l2_m2m_ctx	*curr_ctx;
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct v4l2_m2m_entity	entities[3];
+	struct media_intf_devnode *intf_devnode;
+#endif
 
 	struct list_head	job_queue;
 	spinlock_t		job_spinlock;
@@ -595,6 +606,152 @@ int v4l2_m2m_mmap(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
 }
 EXPORT_SYMBOL(v4l2_m2m_mmap);
 
+void v4l2_m2m_unregister_media_controller(struct v4l2_m2m_dev *m2m_dev)
+{
+	int i;
+
+	media_remove_intf_links(&m2m_dev->intf_devnode->intf);
+	media_devnode_remove(m2m_dev->intf_devnode);
+
+	for (i = 0; i < 3; i++)
+		media_entity_remove_links(&m2m_dev->entities[i].entity);
+	for (i = 0; i < 3; i++)
+		media_device_unregister_entity(&m2m_dev->entities[i].entity);
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_unregister_media_controller);
+
+#define MEM2MEM_ENT_TYPE_INPUT	1
+#define MEM2MEM_ENT_TYPE_OUTPUT	2
+#define MEM2MEM_ENT_TYPE_PROC	3
+
+static int v4l2_m2m_register_entity(struct media_device *mdev,
+		struct v4l2_m2m_entity *m2m_entity, int type)
+{
+	unsigned int function;
+	int num_pads;
+	int ret;
+
+	switch (type) {
+	case MEM2MEM_ENT_TYPE_INPUT:
+		function = MEDIA_ENT_F_IO_DMAENGINE;
+		m2m_entity->pads[0].flags = MEDIA_PAD_FL_SOURCE;
+		strlcpy(m2m_entity->name, "input", sizeof(m2m_entity->name));
+		num_pads = 1;
+		break;
+	case MEM2MEM_ENT_TYPE_OUTPUT:
+		function = MEDIA_ENT_F_IO_DMAENGINE;
+		m2m_entity->pads[0].flags = MEDIA_PAD_FL_SINK;
+		strlcpy(m2m_entity->name, "output", sizeof(m2m_entity->name));
+		num_pads = 1;
+		break;
+	case MEM2MEM_ENT_TYPE_PROC:
+		function = MEDIA_ENT_F_PROC_VIDEO_TRANSFORM;
+		m2m_entity->pads[0].flags = MEDIA_PAD_FL_SOURCE;
+		m2m_entity->pads[1].flags = MEDIA_PAD_FL_SINK;
+		strlcpy(m2m_entity->name, "proc", sizeof(m2m_entity->name));
+		num_pads = 2;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = media_entity_pads_init(&m2m_entity->entity, num_pads, m2m_entity->pads);
+	if (ret)
+		return ret;
+
+	m2m_entity->entity.obj_type = MEDIA_ENTITY_TYPE_MEM2MEM;
+	m2m_entity->entity.function = function;
+	m2m_entity->entity.name = m2m_entity->name;
+	ret = media_device_register_entity(mdev, &m2m_entity->entity);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+int v4l2_m2m_register_media_controller(struct v4l2_m2m_dev *m2m_dev, struct video_device *vdev)
+{
+#if defined(CONFIG_MEDIA_CONTROLLER)
+	struct media_device *mdev = vdev->v4l2_dev->mdev;
+	struct media_link *link;
+	int ret;
+
+	if (!mdev)
+		return 0;
+
+	/* A memory-to-memory device consists in two
+	 * DMA engine and one video processing entities.
+	 * The DMA engine entities are linked to a V4L interface
+	 */
+
+	/* Create the three entities with their pads */
+	ret = v4l2_m2m_register_entity(mdev, &m2m_dev->entities[0], MEM2MEM_ENT_TYPE_INPUT);
+	if (ret)
+		return ret;
+	ret = v4l2_m2m_register_entity(mdev, &m2m_dev->entities[1], MEM2MEM_ENT_TYPE_PROC);
+	if (ret)
+		goto err_rel_entity0;
+	ret = v4l2_m2m_register_entity(mdev, &m2m_dev->entities[2], MEM2MEM_ENT_TYPE_OUTPUT);
+	if (ret)
+		goto err_rel_entity1;
+
+	/* Connect the three entities */
+        ret = media_create_pad_link(&m2m_dev->entities[0].entity, 0,
+			&m2m_dev->entities[1].entity, 1,
+                        MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
+	if (ret)
+		goto err_rel_entity2;
+
+        ret = media_create_pad_link(&m2m_dev->entities[1].entity, 0,
+			&m2m_dev->entities[2].entity, 0,
+                        MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
+	if (ret)
+		goto err_rm_links0;
+
+	/* Create video interface */
+	m2m_dev->intf_devnode = media_devnode_create(mdev, MEDIA_INTF_T_V4L_VIDEO, 0, VIDEO_MAJOR, vdev->minor);
+	if (!m2m_dev->intf_devnode) {
+		ret = -ENOMEM;
+		goto err_rm_links1;
+	}
+
+	/* Connect the two DMA engines to the interface */
+	link = media_create_intf_link(&m2m_dev->entities[0].entity, &m2m_dev->intf_devnode->intf,
+				MEDIA_LNK_FL_ENABLED);
+	if (!link) {
+		ret = -ENOMEM;
+		goto err_rm_devnode;
+	}
+
+	link = media_create_intf_link(&m2m_dev->entities[1].entity, &m2m_dev->intf_devnode->intf,
+				MEDIA_LNK_FL_ENABLED);
+	if (!link) {
+		ret = -ENOMEM;
+		goto err_rm_intf_link;
+	}
+	return 0;
+
+err_rm_intf_link:
+	media_remove_intf_links(&m2m_dev->intf_devnode->intf);
+err_rm_devnode:
+	media_devnode_remove(m2m_dev->intf_devnode);
+err_rm_links1:
+	media_entity_remove_links(&m2m_dev->entities[2].entity);
+err_rm_links0:
+	media_entity_remove_links(&m2m_dev->entities[1].entity);
+	media_entity_remove_links(&m2m_dev->entities[0].entity);
+err_rel_entity2:
+	media_device_unregister_entity(&m2m_dev->entities[2].entity);
+err_rel_entity1:
+	media_device_unregister_entity(&m2m_dev->entities[1].entity);
+err_rel_entity0:
+	media_device_unregister_entity(&m2m_dev->entities[0].entity);
+	return ret;
+#endif
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_m2m_register_media_controller);
+
 struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops)
 {
 	struct v4l2_m2m_dev *m2m_dev;
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 3aa3d58d1d58..ff6fbe8333e1 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -206,6 +206,9 @@ struct media_entity_operations {
  *	The entity is embedded in a struct video_device instance.
  * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
  *	The entity is embedded in a struct v4l2_subdev instance.
+ * @MEDIA_ENTITY_TYPE_V4L2_MEM2MEM:
+ *	The entity is not embedded in any struct, but part of
+ *	a memory-to-memory topology.
  *
  * Media entity objects are often not instantiated directly, but the media
  * entity structure is inherited by (through embedding) other subsystem-specific
@@ -222,6 +225,7 @@ enum media_entity_type {
 	MEDIA_ENTITY_TYPE_BASE,
 	MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
 	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
+	MEDIA_ENTITY_TYPE_MEM2MEM,
 };
 
 /**
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 456ac13eca1d..a9df949bb9c3 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -30,6 +30,7 @@
  * @VFL_TYPE_SUBDEV:	for V4L2 subdevices
  * @VFL_TYPE_SDR:	for Software Defined Radio tuners
  * @VFL_TYPE_TOUCH:	for touch sensors
+ * @VFL_TYPE_MEM2MEM:	for mem2mem devices
  * @VFL_TYPE_MAX:	number of VFL types, must always be last in the enum
  */
 enum vfl_devnode_type {
@@ -39,6 +40,7 @@ enum vfl_devnode_type {
 	VFL_TYPE_SUBDEV,
 	VFL_TYPE_SDR,
 	VFL_TYPE_TOUCH,
+	VFL_TYPE_MEM2MEM,
 	VFL_TYPE_MAX /* Shall be the last one */
 };
 
diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
index 3d07ba3a8262..9dfe9bd23f89 100644
--- a/include/media/v4l2-mem2mem.h
+++ b/include/media/v4l2-mem2mem.h
@@ -53,6 +53,7 @@ struct v4l2_m2m_ops {
 	void (*unlock)(void *priv);
 };
 
+struct video_device;
 struct v4l2_m2m_dev;
 
 /**
@@ -328,6 +329,10 @@ int v4l2_m2m_mmap(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
  */
 struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops);
 
+int v4l2_m2m_register_media_controller(struct v4l2_m2m_dev *m2m_dev, struct video_device *vdev);
+
+void v4l2_m2m_unregister_media_controller(struct v4l2_m2m_dev *m2m_dev);
+
 /**
  * v4l2_m2m_release() - cleans up and frees a m2m_dev structure
  *
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index c7e9a5cba24e..becb7db77f6a 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -81,6 +81,7 @@ struct media_device_info {
 #define MEDIA_ENT_F_IO_DTV			(MEDIA_ENT_F_BASE + 0x01001)
 #define MEDIA_ENT_F_IO_VBI			(MEDIA_ENT_F_BASE + 0x01002)
 #define MEDIA_ENT_F_IO_SWRADIO			(MEDIA_ENT_F_BASE + 0x01003)
+#define MEDIA_ENT_F_IO_DMAENGINE		(MEDIA_ENT_F_BASE + 0x01004)
 
 /*
  * Sensor functions
@@ -132,6 +133,7 @@ struct media_device_info {
 #define MEDIA_ENT_F_PROC_VIDEO_LUT		(MEDIA_ENT_F_BASE + 0x4004)
 #define MEDIA_ENT_F_PROC_VIDEO_SCALER		(MEDIA_ENT_F_BASE + 0x4005)
 #define MEDIA_ENT_F_PROC_VIDEO_STATISTICS	(MEDIA_ENT_F_BASE + 0x4006)
+#define MEDIA_ENT_F_PROC_VIDEO_TRANSFORM	(MEDIA_ENT_F_BASE + 0x4007)
 
 /*
  * Switch and bridge entity functions
-- 
2.17.1

[RFC 2/2] vim2m: add media device

[Ezequiel Garcia]

From: Hans Verkuil <hans.verkuil@cisco.com>

Request API requires a media node. Add one to the vim2m driver so we can
use requests with it.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/vim2m.c | 41 +++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/vim2m.c b/drivers/media/platform/vim2m.c
index 065483e62db4..3b521c4f6def 100644
--- a/drivers/media/platform/vim2m.c
+++ b/drivers/media/platform/vim2m.c
@@ -140,6 +140,9 @@ static struct vim2m_fmt *find_format(struct v4l2_format *f)
 struct vim2m_dev {
 	struct v4l2_device	v4l2_dev;
 	struct video_device	vfd;
+#ifdef CONFIG_MEDIA_CONTROLLER
+	struct media_device	mdev;
+#endif
 
 	atomic_t		num_inst;
 	struct mutex		dev_mutex;
@@ -1013,10 +1016,10 @@ static int vim2m_probe(struct platform_device *pdev)
 	vfd->lock = &dev->dev_mutex;
 	vfd->v4l2_dev = &dev->v4l2_dev;
 
-	ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
+	ret = video_register_device(vfd, VFL_TYPE_MEM2MEM, 0);
 	if (ret) {
 		v4l2_err(&dev->v4l2_dev, "Failed to register video device\n");
-		goto unreg_dev;
+		goto unreg_v4l2;
 	}
 
 	video_set_drvdata(vfd, dev);
@@ -1031,15 +1034,38 @@ static int vim2m_probe(struct platform_device *pdev)
 	if (IS_ERR(dev->m2m_dev)) {
 		v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem device\n");
 		ret = PTR_ERR(dev->m2m_dev);
-		goto err_m2m;
+		goto unreg_dev;
+	}
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+	dev->mdev.dev = &pdev->dev;
+	strlcpy(dev->mdev.model, "vim2m", sizeof(dev->mdev.model));
+	media_device_init(&dev->mdev);
+	dev->v4l2_dev.mdev = &dev->mdev;
+
+	ret = v4l2_m2m_register_media_controller(dev->m2m_dev, vfd);
+	if (ret) {
+		v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem media controller\n");
+		goto unreg_m2m;
 	}
 
+	ret = media_device_register(&dev->mdev);
+	if (ret) {
+		v4l2_err(&dev->v4l2_dev, "Failed to register mem2mem media device\n");
+		goto unreg_m2m_mc;
+	}
+#endif
 	return 0;
 
-err_m2m:
+#ifdef CONFIG_MEDIA_CONTROLLER
+unreg_m2m_mc:
+	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
+unreg_m2m:
 	v4l2_m2m_release(dev->m2m_dev);
-	video_unregister_device(&dev->vfd);
+#endif
 unreg_dev:
+	video_unregister_device(&dev->vfd);
+unreg_v4l2:
 	v4l2_device_unregister(&dev->v4l2_dev);
 
 	return ret;
@@ -1050,6 +1076,11 @@ static int vim2m_remove(struct platform_device *pdev)
 	struct vim2m_dev *dev = platform_get_drvdata(pdev);
 
 	v4l2_info(&dev->v4l2_dev, "Removing " MEM2MEM_NAME);
+
+#ifdef CONFIG_MEDIA_CONTROLLER
+	v4l2_m2m_unregister_media_controller(dev->m2m_dev);
+	media_device_cleanup(&dev->mdev);
+#endif
 	v4l2_m2m_release(dev->m2m_dev);
 	del_timer_sync(&dev->timer);
 	video_unregister_device(&dev->vfd);
-- 
2.17.1

Re: [RFC 0/2] Memory-to-memory media controller topology

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

Le mardi 12 juin 2018 à 07:48 -0300, Ezequiel Garcia a écrit :
> As discussed on IRC, memory-to-memory need to be modeled
> properly in order to be supported by the media controller
> framework, and thus to support the Request API.
> 
> This RFC is a first draft on the memory-to-memory
> media controller topology.
> 
> The topology looks like this:
> 
> Device topology
> - entity 1: input (1 pad, 1 link)
>             type Node subtype Unknown flags 0
> 	pad0: Source
> 		-> "proc":1 [ENABLED,IMMUTABLE]
> 
> - entity 3: proc (2 pads, 2 links)
>             type Node subtype Unknown flags 0
> 	pad0: Source
> 		-> "output":0 [ENABLED,IMMUTABLE]
> 	pad1: Sink
> 		<- "input":0 [ENABLED,IMMUTABLE]
> 
> - entity 6: output (1 pad, 1 link)
>             type Node subtype Unknown flags 0
> 	pad0: Sink
> 		<- "proc":0 [ENABLED,IMMUTABLE]

Will the end result have "device node name /dev/..." on both entity 1
and 6 ? I got told that in the long run, one should be able to map a
device (/dev/mediaN) to it's nodes (/dev/video*). In a way that if we
keep going this way, all the media devices can be enumerated from media
node rather then a mixed between media nodes and orphaned video nodes.
> 
> The first commit introduces a register/unregister API,
> that creates/destroys all the entities and pads needed,
> and links them.
> 
> The second commit uses this API to support the vim2m driver.
> 
> Notes
> -----
> 
> * A new device node type is introduced VFL_TYPE_MEM2MEM,
>   this is mostly done so the video4linux core doesn't
>   try to register other media controller entities.
> 
> * Also, a new media entity type is introduced. Memory-to-memory
>   devices have a multi-entity description and so can't
>   be simply embedded in other structs, or cast from other structs.
> 
> Ezequiel Garcia (1):
>   media: add helpers for memory-to-memory media controller
> 
> Hans Verkuil (1):
>   vim2m: add media device
> 
>  drivers/media/platform/vim2m.c         |  41 ++++++-
>  drivers/media/v4l2-core/v4l2-dev.c     |  23 ++--
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 157
> +++++++++++++++++++++++++
>  include/media/media-entity.h           |   4 +
>  include/media/v4l2-dev.h               |   2 +
>  include/media/v4l2-mem2mem.h           |   5 +
>  include/uapi/linux/media.h             |   2 +
>  7 files changed, 222 insertions(+), 12 deletions(-)
> 
> -- 
> 2.17.1
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC 2/2] vim2m: add media device

[emil.velikov]

Hi Ezequiel,  

On Tue, Jun 12, 2018 at 07:48:27AM -0300, Ezequiel Garcia wrote:

> @@ -1013,10 +1016,10 @@ static int vim2m_probe(struct platform_device *pdev)
>  	vfd->lock = &dev->dev_mutex;
>  	vfd->v4l2_dev = &dev->v4l2_dev;
>  
> -	ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> +	ret = video_register_device(vfd, VFL_TYPE_MEM2MEM, 0);
Shouldn't the original type be used when building without
CONFIG_MEDIA_CONTROLLER?


> @@ -1050,6 +1076,11 @@ static int vim2m_remove(struct platform_device *pdev)
>  	struct vim2m_dev *dev = platform_get_drvdata(pdev);
>  
>  	v4l2_info(&dev->v4l2_dev, "Removing " MEM2MEM_NAME);
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER
Gut suggests that media_device_unregister() should be called here.

Then again my experience in media/ is limited so I could be miles off
;-)


HTH
Emil

Re: [RFC 0/2] Memory-to-memory media controller topology

[Hans Verkuil]

On 12/06/18 12:48, Ezequiel Garcia wrote:
> As discussed on IRC, memory-to-memory need to be modeled
> properly in order to be supported by the media controller
> framework, and thus to support the Request API.
> 
> This RFC is a first draft on the memory-to-memory
> media controller topology.
> 
> The topology looks like this:
> 
> Device topology
> - entity 1: input (1 pad, 1 link)
>             type Node subtype Unknown flags 0
> 	pad0: Source
> 		-> "proc":1 [ENABLED,IMMUTABLE]
> 
> - entity 3: proc (2 pads, 2 links)
>             type Node subtype Unknown flags 0
> 	pad0: Source
> 		-> "output":0 [ENABLED,IMMUTABLE]
> 	pad1: Sink
> 		<- "input":0 [ENABLED,IMMUTABLE]
> 
> - entity 6: output (1 pad, 1 link)
>             type Node subtype Unknown flags 0
> 	pad0: Sink
> 		<- "proc":0 [ENABLED,IMMUTABLE]
> 
> The first commit introduces a register/unregister API,
> that creates/destroys all the entities and pads needed,
> and links them.
> 
> The second commit uses this API to support the vim2m driver.
> 
> Notes
> -----
> 
> * A new device node type is introduced VFL_TYPE_MEM2MEM,
>   this is mostly done so the video4linux core doesn't
>   try to register other media controller entities.

There is no need for this. You can check if vfl_dir == VFL_DIR_M2M
instead. I'd rather not add a new VFL_TYPE.

Regards,

	Hans

> 
> * Also, a new media entity type is introduced. Memory-to-memory
>   devices have a multi-entity description and so can't
>   be simply embedded in other structs, or cast from other structs.
> 
> Ezequiel Garcia (1):
>   media: add helpers for memory-to-memory media controller
> 
> Hans Verkuil (1):
>   vim2m: add media device
> 
>  drivers/media/platform/vim2m.c         |  41 ++++++-
>  drivers/media/v4l2-core/v4l2-dev.c     |  23 ++--
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 157 +++++++++++++++++++++++++
>  include/media/media-entity.h           |   4 +
>  include/media/v4l2-dev.h               |   2 +
>  include/media/v4l2-mem2mem.h           |   5 +
>  include/uapi/linux/media.h             |   2 +
>  7 files changed, 222 insertions(+), 12 deletions(-)
> 

Re: [RFC 1/2] media: add helpers for memory-to-memory media controller

[Hans Verkuil]

On 12/06/18 12:48, Ezequiel Garcia wrote:
> A memory-to-memory pipeline device consists in three
> entities: two DMA engine and one video processing entities.
> The DMA engine entities are linked to a V4L interface.
> 
> This commit add a new v4l2_m2m_{un}register_media_controller
> API to register this topology.
> 
> For instance, a typical mem2mem device topology would
> look like this:
> 
> - entity 1: input (1 pad, 1 link)
>             type Node subtype Unknown flags 0
> 	pad0: Source
> 		-> "proc":1 [ENABLED,IMMUTABLE]
> 
> - entity 3: proc (2 pads, 2 links)
>             type Node subtype Unknown flags 0
> 	pad0: Source
> 		-> "output":0 [ENABLED,IMMUTABLE]
> 	pad1: Sink
> 		<- "input":0 [ENABLED,IMMUTABLE]
> 
> - entity 6: output (1 pad, 1 link)
>             type Node subtype Unknown flags 0
> 	pad0: Sink
> 		<- "proc":0 [ENABLED,IMMUTABLE]
> 
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Suggested-by: Hans Verkuil <hans.verkuil@cisco.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  drivers/media/v4l2-core/v4l2-dev.c     |  23 ++--
>  drivers/media/v4l2-core/v4l2-mem2mem.c | 157 +++++++++++++++++++++++++
>  include/media/media-entity.h           |   4 +
>  include/media/v4l2-dev.h               |   2 +
>  include/media/v4l2-mem2mem.h           |   5 +
>  include/uapi/linux/media.h             |   2 +
>  6 files changed, 186 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index 4ffd7d60a901..ec8f20f0fdc5 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -202,7 +202,7 @@ static void v4l2_device_release(struct device *cd)
>  	mutex_unlock(&videodev_lock);
>  
>  #if defined(CONFIG_MEDIA_CONTROLLER)
> -	if (v4l2_dev->mdev) {
> +	if (v4l2_dev->mdev && vdev->vfl_type != VFL_TYPE_MEM2MEM) {

As mentioned this should be vfl_dir != VFL_DIR_M2M. No need for a new VFL_TYPE.

>  		/* Remove interfaces and interface links */
>  		media_devnode_remove(vdev->intf_devnode);
>  		if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
> @@ -530,6 +530,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  	bool is_radio = vdev->vfl_type == VFL_TYPE_RADIO;
>  	bool is_sdr = vdev->vfl_type == VFL_TYPE_SDR;
>  	bool is_tch = vdev->vfl_type == VFL_TYPE_TOUCH;
> +	bool is_m2m = vdev->vfl_type == VFL_TYPE_MEM2MEM;

And that means that this is also no longer needed.

>  	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
>  	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
>  
> @@ -576,7 +577,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  	if (ops->vidioc_enum_freq_bands || ops->vidioc_g_tuner || ops->vidioc_g_modulator)
>  		set_bit(_IOC_NR(VIDIOC_ENUM_FREQ_BANDS), valid_ioctls);
>  
> -	if (is_vid || is_tch) {
> +	if (is_vid || is_m2m || is_tch) {
>  		/* video and metadata specific ioctls */
>  		if ((is_rx && (ops->vidioc_enum_fmt_vid_cap ||
>  			       ops->vidioc_enum_fmt_vid_cap_mplane ||
> @@ -669,7 +670,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
>  	}
>  
> -	if (is_vid || is_vbi || is_sdr || is_tch) {
> +	if (is_vid || is_m2m || is_vbi || is_sdr || is_tch) {
>  		/* ioctls valid for video, metadata, vbi or sdr */
>  		SET_VALID_IOCTL(ops, VIDIOC_REQBUFS, vidioc_reqbufs);
>  		SET_VALID_IOCTL(ops, VIDIOC_QUERYBUF, vidioc_querybuf);
> @@ -682,7 +683,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  		SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
>  	}
>  
> -	if (is_vid || is_vbi || is_tch) {
> +	if (is_vid || is_m2m || is_vbi || is_tch) {
>  		/* ioctls valid for video or vbi */
>  		if (ops->vidioc_s_std)
>  			set_bit(_IOC_NR(VIDIOC_ENUMSTD), valid_ioctls);
> @@ -733,7 +734,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
>  			BASE_VIDIOC_PRIVATE);
>  }
>  
> -static int video_register_media_controller(struct video_device *vdev, int type)
> +static int video_register_media_controller(struct video_device *vdev)
>  {
>  #if defined(CONFIG_MEDIA_CONTROLLER)
>  	u32 intf_type;
> @@ -745,7 +746,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
>  	vdev->entity.obj_type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
>  	vdev->entity.function = MEDIA_ENT_F_UNKNOWN;
>  
> -	switch (type) {
> +	switch (vdev->vfl_type) {
>  	case VFL_TYPE_GRABBER:
>  		intf_type = MEDIA_INTF_T_V4L_VIDEO;
>  		vdev->entity.function = MEDIA_ENT_F_IO_V4L;
> @@ -774,6 +775,10 @@ static int video_register_media_controller(struct video_device *vdev, int type)
>  		intf_type = MEDIA_INTF_T_V4L_SUBDEV;
>  		/* Entity will be created via v4l2_device_register_subdev() */
>  		break;
> +	case VFL_TYPE_MEM2MEM:
> +		/* Memory-to-memory devices are more complex and use
> +		 * their own function to register.
> +		 */
>  	default:
>  		return 0;
>  	}
> @@ -869,6 +874,10 @@ int __video_register_device(struct video_device *vdev,
>  	case VFL_TYPE_TOUCH:
>  		name_base = "v4l-touch";
>  		break;
> +	case VFL_TYPE_MEM2MEM:
> +		/* Maintain this name for backwards compatibility */
> +		name_base = "video";
> +		break;
>  	default:
>  		pr_err("%s called with unknown type: %d\n",
>  		       __func__, type);
> @@ -993,7 +1002,7 @@ int __video_register_device(struct video_device *vdev,
>  	v4l2_device_get(vdev->v4l2_dev);
>  
>  	/* Part 5: Register the entity. */
> -	ret = video_register_media_controller(vdev, type);
> +	ret = video_register_media_controller(vdev);
>  
>  	/* Part 6: Activate this minor. The char device can now be used. */
>  	set_bit(V4L2_FL_REGISTERED, &vdev->flags);
> diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> index c4f963d96a79..0505b65bfa68 100644
> --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> @@ -17,9 +17,11 @@
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  
> +#include <media/media-device.h>
>  #include <media/videobuf2-v4l2.h>
>  #include <media/v4l2-mem2mem.h>
>  #include <media/v4l2-dev.h>
> +#include <media/v4l2-device.h>
>  #include <media/v4l2-fh.h>
>  #include <media/v4l2-event.h>
>  
> @@ -50,6 +52,11 @@ module_param(debug, bool, 0644);
>   * offsets but for different queues */
>  #define DST_QUEUE_OFF_BASE	(1 << 30)
>  
> +struct v4l2_m2m_entity {
> +	struct media_entity entity;
> +	struct media_pad pads[2];
> +	char name[64];
> +};
>  
>  /**
>   * struct v4l2_m2m_dev - per-device context
> @@ -60,6 +67,10 @@ module_param(debug, bool, 0644);
>   */
>  struct v4l2_m2m_dev {
>  	struct v4l2_m2m_ctx	*curr_ctx;
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	struct v4l2_m2m_entity	entities[3];
> +	struct media_intf_devnode *intf_devnode;
> +#endif
>  
>  	struct list_head	job_queue;
>  	spinlock_t		job_spinlock;
> @@ -595,6 +606,152 @@ int v4l2_m2m_mmap(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>  }
>  EXPORT_SYMBOL(v4l2_m2m_mmap);
>  
> +void v4l2_m2m_unregister_media_controller(struct v4l2_m2m_dev *m2m_dev)
> +{
> +	int i;
> +
> +	media_remove_intf_links(&m2m_dev->intf_devnode->intf);
> +	media_devnode_remove(m2m_dev->intf_devnode);
> +
> +	for (i = 0; i < 3; i++)

ARRAY_SIZE? Or use a define.

> +		media_entity_remove_links(&m2m_dev->entities[i].entity);
> +	for (i = 0; i < 3; i++)

Ditto.

> +		media_device_unregister_entity(&m2m_dev->entities[i].entity);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_unregister_media_controller);
> +
> +#define MEM2MEM_ENT_TYPE_INPUT	1
> +#define MEM2MEM_ENT_TYPE_OUTPUT	2
> +#define MEM2MEM_ENT_TYPE_PROC	3
> +
> +static int v4l2_m2m_register_entity(struct media_device *mdev,
> +		struct v4l2_m2m_entity *m2m_entity, int type)
> +{
> +	unsigned int function;
> +	int num_pads;
> +	int ret;
> +
> +	switch (type) {
> +	case MEM2MEM_ENT_TYPE_INPUT:
> +		function = MEDIA_ENT_F_IO_DMAENGINE;
> +		m2m_entity->pads[0].flags = MEDIA_PAD_FL_SOURCE;
> +		strlcpy(m2m_entity->name, "input", sizeof(m2m_entity->name));
> +		num_pads = 1;
> +		break;
> +	case MEM2MEM_ENT_TYPE_OUTPUT:
> +		function = MEDIA_ENT_F_IO_DMAENGINE;
> +		m2m_entity->pads[0].flags = MEDIA_PAD_FL_SINK;
> +		strlcpy(m2m_entity->name, "output", sizeof(m2m_entity->name));

Either use "capture" and "output" (to conform to the V4L2_BUF_TYPE naming) or
"source" and "sink".

> +		num_pads = 1;
> +		break;
> +	case MEM2MEM_ENT_TYPE_PROC:
> +		function = MEDIA_ENT_F_PROC_VIDEO_TRANSFORM;
> +		m2m_entity->pads[0].flags = MEDIA_PAD_FL_SOURCE;
> +		m2m_entity->pads[1].flags = MEDIA_PAD_FL_SINK;
> +		strlcpy(m2m_entity->name, "proc", sizeof(m2m_entity->name));
> +		num_pads = 2;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = media_entity_pads_init(&m2m_entity->entity, num_pads, m2m_entity->pads);
> +	if (ret)
> +		return ret;
> +
> +	m2m_entity->entity.obj_type = MEDIA_ENTITY_TYPE_MEM2MEM;
> +	m2m_entity->entity.function = function;
> +	m2m_entity->entity.name = m2m_entity->name;

Why not just strlcpy into the m2m_entity->entity.name field? Then you can
drop m2m_entity->name. Or am I missing something?

> +	ret = media_device_register_entity(mdev, &m2m_entity->entity);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +int v4l2_m2m_register_media_controller(struct v4l2_m2m_dev *m2m_dev, struct video_device *vdev)
> +{
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +	struct media_device *mdev = vdev->v4l2_dev->mdev;
> +	struct media_link *link;
> +	int ret;
> +
> +	if (!mdev)
> +		return 0;
> +
> +	/* A memory-to-memory device consists in two
> +	 * DMA engine and one video processing entities.
> +	 * The DMA engine entities are linked to a V4L interface
> +	 */
> +
> +	/* Create the three entities with their pads */
> +	ret = v4l2_m2m_register_entity(mdev, &m2m_dev->entities[0], MEM2MEM_ENT_TYPE_INPUT);
> +	if (ret)
> +		return ret;
> +	ret = v4l2_m2m_register_entity(mdev, &m2m_dev->entities[1], MEM2MEM_ENT_TYPE_PROC);
> +	if (ret)
> +		goto err_rel_entity0;
> +	ret = v4l2_m2m_register_entity(mdev, &m2m_dev->entities[2], MEM2MEM_ENT_TYPE_OUTPUT);
> +	if (ret)
> +		goto err_rel_entity1;
> +
> +	/* Connect the three entities */
> +        ret = media_create_pad_link(&m2m_dev->entities[0].entity, 0,
> +			&m2m_dev->entities[1].entity, 1,
> +                        MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);

Weird indentation.

> +	if (ret)
> +		goto err_rel_entity2;
> +
> +        ret = media_create_pad_link(&m2m_dev->entities[1].entity, 0,
> +			&m2m_dev->entities[2].entity, 0,
> +                        MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);

Ditto.

> +	if (ret)
> +		goto err_rm_links0;
> +
> +	/* Create video interface */
> +	m2m_dev->intf_devnode = media_devnode_create(mdev, MEDIA_INTF_T_V4L_VIDEO, 0, VIDEO_MAJOR, vdev->minor);
> +	if (!m2m_dev->intf_devnode) {
> +		ret = -ENOMEM;
> +		goto err_rm_links1;
> +	}
> +
> +	/* Connect the two DMA engines to the interface */
> +	link = media_create_intf_link(&m2m_dev->entities[0].entity, &m2m_dev->intf_devnode->intf,
> +				MEDIA_LNK_FL_ENABLED);
> +	if (!link) {
> +		ret = -ENOMEM;
> +		goto err_rm_devnode;
> +	}
> +
> +	link = media_create_intf_link(&m2m_dev->entities[1].entity, &m2m_dev->intf_devnode->intf,
> +				MEDIA_LNK_FL_ENABLED);
> +	if (!link) {
> +		ret = -ENOMEM;
> +		goto err_rm_intf_link;
> +	}
> +	return 0;
> +
> +err_rm_intf_link:
> +	media_remove_intf_links(&m2m_dev->intf_devnode->intf);
> +err_rm_devnode:
> +	media_devnode_remove(m2m_dev->intf_devnode);
> +err_rm_links1:
> +	media_entity_remove_links(&m2m_dev->entities[2].entity);
> +err_rm_links0:
> +	media_entity_remove_links(&m2m_dev->entities[1].entity);
> +	media_entity_remove_links(&m2m_dev->entities[0].entity);
> +err_rel_entity2:
> +	media_device_unregister_entity(&m2m_dev->entities[2].entity);
> +err_rel_entity1:
> +	media_device_unregister_entity(&m2m_dev->entities[1].entity);
> +err_rel_entity0:
> +	media_device_unregister_entity(&m2m_dev->entities[0].entity);
> +	return ret;
> +#endif
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_m2m_register_media_controller);
> +
>  struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops)
>  {
>  	struct v4l2_m2m_dev *m2m_dev;

Looking at this code I think it would be much cleaner if you drop the array
and make it explicit instead:

struct v4l2_m2m_entities {
	struct media_entity source;
	struct media_pad source_pad;
	struct media_entity sink;
	struct media_pad sink_pad;
	struct media_entity proc;
	struct media_pad proc_pads[2];
};

I think this will simplify the code quite a bit.

Note: one of these media_entity structs can be removed since struct video_device
already has an entity you can use. It probably makes the most sense to assign
that one the role of the sink or source entity.

Perhaps it might be easiest to change e.g. struct media_entity source to
'struct media_entity *source' and have it point to &vdev->entity.

Up to you.

> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 3aa3d58d1d58..ff6fbe8333e1 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -206,6 +206,9 @@ struct media_entity_operations {
>   *	The entity is embedded in a struct video_device instance.
>   * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
>   *	The entity is embedded in a struct v4l2_subdev instance.
> + * @MEDIA_ENTITY_TYPE_V4L2_MEM2MEM:
> + *	The entity is not embedded in any struct, but part of
> + *	a memory-to-memory topology.

I see no need for this. An M2M device is of type VIDEO_DEVICE, no need to
change that.

>   *
>   * Media entity objects are often not instantiated directly, but the media
>   * entity structure is inherited by (through embedding) other subsystem-specific
> @@ -222,6 +225,7 @@ enum media_entity_type {
>  	MEDIA_ENTITY_TYPE_BASE,
>  	MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
>  	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
> +	MEDIA_ENTITY_TYPE_MEM2MEM,
>  };
>  
>  /**
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 456ac13eca1d..a9df949bb9c3 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -30,6 +30,7 @@
>   * @VFL_TYPE_SUBDEV:	for V4L2 subdevices
>   * @VFL_TYPE_SDR:	for Software Defined Radio tuners
>   * @VFL_TYPE_TOUCH:	for touch sensors
> + * @VFL_TYPE_MEM2MEM:	for mem2mem devices
>   * @VFL_TYPE_MAX:	number of VFL types, must always be last in the enum
>   */
>  enum vfl_devnode_type {
> @@ -39,6 +40,7 @@ enum vfl_devnode_type {
>  	VFL_TYPE_SUBDEV,
>  	VFL_TYPE_SDR,
>  	VFL_TYPE_TOUCH,
> +	VFL_TYPE_MEM2MEM,
>  	VFL_TYPE_MAX /* Shall be the last one */
>  };
>  
> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> index 3d07ba3a8262..9dfe9bd23f89 100644
> --- a/include/media/v4l2-mem2mem.h
> +++ b/include/media/v4l2-mem2mem.h
> @@ -53,6 +53,7 @@ struct v4l2_m2m_ops {
>  	void (*unlock)(void *priv);
>  };
>  
> +struct video_device;
>  struct v4l2_m2m_dev;
>  
>  /**
> @@ -328,6 +329,10 @@ int v4l2_m2m_mmap(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>   */
>  struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops);
>  
> +int v4l2_m2m_register_media_controller(struct v4l2_m2m_dev *m2m_dev, struct video_device *vdev);
> +
> +void v4l2_m2m_unregister_media_controller(struct v4l2_m2m_dev *m2m_dev);
> +
>  /**
>   * v4l2_m2m_release() - cleans up and frees a m2m_dev structure
>   *
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index c7e9a5cba24e..becb7db77f6a 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -81,6 +81,7 @@ struct media_device_info {
>  #define MEDIA_ENT_F_IO_DTV			(MEDIA_ENT_F_BASE + 0x01001)
>  #define MEDIA_ENT_F_IO_VBI			(MEDIA_ENT_F_BASE + 0x01002)
>  #define MEDIA_ENT_F_IO_SWRADIO			(MEDIA_ENT_F_BASE + 0x01003)
> +#define MEDIA_ENT_F_IO_DMAENGINE		(MEDIA_ENT_F_BASE + 0x01004)

Drop this as well. Just stick to MEDIA_ENT_F_IO_V4L which is what we've decided to
call such entities (for better or worse).

>  
>  /*
>   * Sensor functions
> @@ -132,6 +133,7 @@ struct media_device_info {
>  #define MEDIA_ENT_F_PROC_VIDEO_LUT		(MEDIA_ENT_F_BASE + 0x4004)
>  #define MEDIA_ENT_F_PROC_VIDEO_SCALER		(MEDIA_ENT_F_BASE + 0x4005)
>  #define MEDIA_ENT_F_PROC_VIDEO_STATISTICS	(MEDIA_ENT_F_BASE + 0x4006)
> +#define MEDIA_ENT_F_PROC_VIDEO_TRANSFORM	(MEDIA_ENT_F_BASE + 0x4007)

I think we need to be a bit more specific here:

#define MEDIA_ENT_F_PROC_VIDEO_DECODER
#define MEDIA_ENT_F_PROC_VIDEO_ENCODER
#define MEDIA_ENT_F_PROC_VIDEO_DEINTERLACER
// others?

>  
>  /*
>   * Switch and bridge entity functions
> 

Regards,

	Hans

Re: [RFC 0/2] Memory-to-memory media controller topology

[Ezequiel Garcia]

On Fri, 2018-06-15 at 10:59 +0200, Hans Verkuil wrote:
> On 12/06/18 12:48, Ezequiel Garcia wrote:
> > As discussed on IRC, memory-to-memory need to be modeled
> > properly in order to be supported by the media controller
> > framework, and thus to support the Request API.
> > 
> > This RFC is a first draft on the memory-to-memory
> > media controller topology.
> > 
> > The topology looks like this:
> > 
> > Device topology
> > - entity 1: input (1 pad, 1 link)
> >             type Node subtype Unknown flags 0
> > 	pad0: Source
> > 		-> "proc":1 [ENABLED,IMMUTABLE]
> > 
> > - entity 3: proc (2 pads, 2 links)
> >             type Node subtype Unknown flags 0
> > 	pad0: Source
> > 		-> "output":0 [ENABLED,IMMUTABLE]
> > 	pad1: Sink
> > 		<- "input":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 6: output (1 pad, 1 link)
> >             type Node subtype Unknown flags 0
> > 	pad0: Sink
> > 		<- "proc":0 [ENABLED,IMMUTABLE]
> > 
> > The first commit introduces a register/unregister API,
> > that creates/destroys all the entities and pads needed,
> > and links them.
> > 
> > The second commit uses this API to support the vim2m driver.
> > 
> > Notes
> > -----
> > 
> > * A new device node type is introduced VFL_TYPE_MEM2MEM,
> >   this is mostly done so the video4linux core doesn't
> >   try to register other media controller entities.
> 
> There is no need for this. You can check if vfl_dir == VFL_DIR_M2M
> instead. I'd rather not add a new VFL_TYPE.
> 

OK, sounds good.

Thanks,
Eze

Re: [RFC 1/2] media: add helpers for memory-to-memory media controller

[Ezequiel Garcia]

Hi Hans,

Thanks for the review.

On Fri, 2018-06-15 at 11:24 +0200, Hans Verkuil wrote:
> On 12/06/18 12:48, Ezequiel Garcia wrote:
> > A memory-to-memory pipeline device consists in three
> > entities: two DMA engine and one video processing entities.
> > The DMA engine entities are linked to a V4L interface.
> > 
> > This commit add a new v4l2_m2m_{un}register_media_controller
> > API to register this topology.
> > 
> > For instance, a typical mem2mem device topology would
> > look like this:
> > 
> > - entity 1: input (1 pad, 1 link)
> >             type Node subtype Unknown flags 0
> > 	pad0: Source
> > 		-> "proc":1 [ENABLED,IMMUTABLE]
> > 
> > - entity 3: proc (2 pads, 2 links)
> >             type Node subtype Unknown flags 0
> > 	pad0: Source
> > 		-> "output":0 [ENABLED,IMMUTABLE]
> > 	pad1: Sink
> > 		<- "input":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 6: output (1 pad, 1 link)
> >             type Node subtype Unknown flags 0
> > 	pad0: Sink
> > 		<- "proc":0 [ENABLED,IMMUTABLE]
> > 
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Suggested-by: Hans Verkuil <hans.verkuil@cisco.com>
> > Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  drivers/media/v4l2-core/v4l2-dev.c     |  23 ++--
> >  drivers/media/v4l2-core/v4l2-mem2mem.c | 157 +++++++++++++++++++++++++
> >  include/media/media-entity.h           |   4 +
> >  include/media/v4l2-dev.h               |   2 +
> >  include/media/v4l2-mem2mem.h           |   5 +
> >  include/uapi/linux/media.h             |   2 +
> >  6 files changed, 186 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> > index 4ffd7d60a901..ec8f20f0fdc5 100644
> > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > @@ -202,7 +202,7 @@ static void v4l2_device_release(struct device *cd)
> >  	mutex_unlock(&videodev_lock);
> >  
> >  #if defined(CONFIG_MEDIA_CONTROLLER)
> > -	if (v4l2_dev->mdev) {
> > +	if (v4l2_dev->mdev && vdev->vfl_type != VFL_TYPE_MEM2MEM) {
> 
> As mentioned this should be vfl_dir != VFL_DIR_M2M. No need for a new VFL_TYPE.
> 

Right.

> >  		/* Remove interfaces and interface links */
> >  		media_devnode_remove(vdev->intf_devnode);
> >  		if (vdev->entity.function != MEDIA_ENT_F_UNKNOWN)
> > @@ -530,6 +530,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >  	bool is_radio = vdev->vfl_type == VFL_TYPE_RADIO;
> >  	bool is_sdr = vdev->vfl_type == VFL_TYPE_SDR;
> >  	bool is_tch = vdev->vfl_type == VFL_TYPE_TOUCH;
> > +	bool is_m2m = vdev->vfl_type == VFL_TYPE_MEM2MEM;
> 
> And that means that this is also no longer needed.
> 

Right, it should be simplified a lot. I hated to introduce
a new type, just thought it was the cleaner way.

> >  	bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
> >  	bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
> >  
> > @@ -576,7 +577,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >  	if (ops->vidioc_enum_freq_bands || ops->vidioc_g_tuner || ops->vidioc_g_modulator)
> >  		set_bit(_IOC_NR(VIDIOC_ENUM_FREQ_BANDS), valid_ioctls);
> >  
> > -	if (is_vid || is_tch) {
> > +	if (is_vid || is_m2m || is_tch) {
> >  		/* video and metadata specific ioctls */
> >  		if ((is_rx && (ops->vidioc_enum_fmt_vid_cap ||
> >  			       ops->vidioc_enum_fmt_vid_cap_mplane ||
> > @@ -669,7 +670,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >  			set_bit(_IOC_NR(VIDIOC_TRY_FMT), valid_ioctls);
> >  	}
> >  
> > -	if (is_vid || is_vbi || is_sdr || is_tch) {
> > +	if (is_vid || is_m2m || is_vbi || is_sdr || is_tch) {
> >  		/* ioctls valid for video, metadata, vbi or sdr */
> >  		SET_VALID_IOCTL(ops, VIDIOC_REQBUFS, vidioc_reqbufs);
> >  		SET_VALID_IOCTL(ops, VIDIOC_QUERYBUF, vidioc_querybuf);
> > @@ -682,7 +683,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >  		SET_VALID_IOCTL(ops, VIDIOC_STREAMOFF, vidioc_streamoff);
> >  	}
> >  
> > -	if (is_vid || is_vbi || is_tch) {
> > +	if (is_vid || is_m2m || is_vbi || is_tch) {
> >  		/* ioctls valid for video or vbi */
> >  		if (ops->vidioc_s_std)
> >  			set_bit(_IOC_NR(VIDIOC_ENUMSTD), valid_ioctls);
> > @@ -733,7 +734,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
> >  			BASE_VIDIOC_PRIVATE);
> >  }
> >  
> > -static int video_register_media_controller(struct video_device *vdev, int type)
> > +static int video_register_media_controller(struct video_device *vdev)
> >  {
> >  #if defined(CONFIG_MEDIA_CONTROLLER)
> >  	u32 intf_type;
> > @@ -745,7 +746,7 @@ static int video_register_media_controller(struct video_device *vdev, int type)
> >  	vdev->entity.obj_type = MEDIA_ENTITY_TYPE_VIDEO_DEVICE;
> >  	vdev->entity.function = MEDIA_ENT_F_UNKNOWN;
> >  
> > -	switch (type) {
> > +	switch (vdev->vfl_type) {
> >  	case VFL_TYPE_GRABBER:
> >  		intf_type = MEDIA_INTF_T_V4L_VIDEO;
> >  		vdev->entity.function = MEDIA_ENT_F_IO_V4L;
> > @@ -774,6 +775,10 @@ static int video_register_media_controller(struct video_device *vdev, int type)
> >  		intf_type = MEDIA_INTF_T_V4L_SUBDEV;
> >  		/* Entity will be created via v4l2_device_register_subdev() */
> >  		break;
> > +	case VFL_TYPE_MEM2MEM:
> > +		/* Memory-to-memory devices are more complex and use
> > +		 * their own function to register.
> > +		 */
> >  	default:
> >  		return 0;
> >  	}
> > @@ -869,6 +874,10 @@ int __video_register_device(struct video_device *vdev,
> >  	case VFL_TYPE_TOUCH:
> >  		name_base = "v4l-touch";
> >  		break;
> > +	case VFL_TYPE_MEM2MEM:
> > +		/* Maintain this name for backwards compatibility */
> > +		name_base = "video";
> > +		break;
> >  	default:
> >  		pr_err("%s called with unknown type: %d\n",
> >  		       __func__, type);
> > @@ -993,7 +1002,7 @@ int __video_register_device(struct video_device *vdev,
> >  	v4l2_device_get(vdev->v4l2_dev);
> >  
> >  	/* Part 5: Register the entity. */
> > -	ret = video_register_media_controller(vdev, type);
> > +	ret = video_register_media_controller(vdev);
> >  
> >  	/* Part 6: Activate this minor. The char device can now be used. */
> >  	set_bit(V4L2_FL_REGISTERED, &vdev->flags);
> > diff --git a/drivers/media/v4l2-core/v4l2-mem2mem.c b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > index c4f963d96a79..0505b65bfa68 100644
> > --- a/drivers/media/v4l2-core/v4l2-mem2mem.c
> > +++ b/drivers/media/v4l2-core/v4l2-mem2mem.c
> > @@ -17,9 +17,11 @@
> >  #include <linux/sched.h>
> >  #include <linux/slab.h>
> >  
> > +#include <media/media-device.h>
> >  #include <media/videobuf2-v4l2.h>
> >  #include <media/v4l2-mem2mem.h>
> >  #include <media/v4l2-dev.h>
> > +#include <media/v4l2-device.h>
> >  #include <media/v4l2-fh.h>
> >  #include <media/v4l2-event.h>
> >  
> > @@ -50,6 +52,11 @@ module_param(debug, bool, 0644);
> >   * offsets but for different queues */
> >  #define DST_QUEUE_OFF_BASE	(1 << 30)
> >  
> > +struct v4l2_m2m_entity {
> > +	struct media_entity entity;
> > +	struct media_pad pads[2];
> > +	char name[64];
> > +};
> >  
> >  /**
> >   * struct v4l2_m2m_dev - per-device context
> > @@ -60,6 +67,10 @@ module_param(debug, bool, 0644);
> >   */
> >  struct v4l2_m2m_dev {
> >  	struct v4l2_m2m_ctx	*curr_ctx;
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> > +	struct v4l2_m2m_entity	entities[3];
> > +	struct media_intf_devnode *intf_devnode;
> > +#endif
> >  
> >  	struct list_head	job_queue;
> >  	spinlock_t		job_spinlock;
> > @@ -595,6 +606,152 @@ int v4l2_m2m_mmap(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
> >  }
> >  EXPORT_SYMBOL(v4l2_m2m_mmap);
> >  
> > +void v4l2_m2m_unregister_media_controller(struct v4l2_m2m_dev *m2m_dev)
> > +{
> > +	int i;
> > +
> > +	media_remove_intf_links(&m2m_dev->intf_devnode->intf);
> > +	media_devnode_remove(m2m_dev->intf_devnode);
> > +
> > +	for (i = 0; i < 3; i++)
> 
> ARRAY_SIZE? Or use a define.
> 

Ugh, yes, you are right.

> > +		media_entity_remove_links(&m2m_dev->entities[i].entity);
> > +	for (i = 0; i < 3; i++)
> 
> Ditto.
> 
> > +		media_device_unregister_entity(&m2m_dev->entities[i].entity);
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_m2m_unregister_media_controller);
> > +
> > +#define MEM2MEM_ENT_TYPE_INPUT	1
> > +#define MEM2MEM_ENT_TYPE_OUTPUT	2
> > +#define MEM2MEM_ENT_TYPE_PROC	3
> > +
> > +static int v4l2_m2m_register_entity(struct media_device *mdev,
> > +		struct v4l2_m2m_entity *m2m_entity, int type)
> > +{
> > +	unsigned int function;
> > +	int num_pads;
> > +	int ret;
> > +
> > +	switch (type) {
> > +	case MEM2MEM_ENT_TYPE_INPUT:
> > +		function = MEDIA_ENT_F_IO_DMAENGINE;
> > +		m2m_entity->pads[0].flags = MEDIA_PAD_FL_SOURCE;
> > +		strlcpy(m2m_entity->name, "input", sizeof(m2m_entity->name));
> > +		num_pads = 1;
> > +		break;
> > +	case MEM2MEM_ENT_TYPE_OUTPUT:
> > +		function = MEDIA_ENT_F_IO_DMAENGINE;
> > +		m2m_entity->pads[0].flags = MEDIA_PAD_FL_SINK;
> > +		strlcpy(m2m_entity->name, "output", sizeof(m2m_entity->name));
> 
> Either use "capture" and "output" (to conform to the V4L2_BUF_TYPE naming) or
> "source" and "sink".
> 

Right.

> > +		num_pads = 1;
> > +		break;
> > +	case MEM2MEM_ENT_TYPE_PROC:
> > +		function = MEDIA_ENT_F_PROC_VIDEO_TRANSFORM;
> > +		m2m_entity->pads[0].flags = MEDIA_PAD_FL_SOURCE;
> > +		m2m_entity->pads[1].flags = MEDIA_PAD_FL_SINK;
> > +		strlcpy(m2m_entity->name, "proc", sizeof(m2m_entity->name));
> > +		num_pads = 2;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = media_entity_pads_init(&m2m_entity->entity, num_pads, m2m_entity->pads);
> > +	if (ret)
> > +		return ret;
> > +
> > +	m2m_entity->entity.obj_type = MEDIA_ENTITY_TYPE_MEM2MEM;
> > +	m2m_entity->entity.function = function;
> > +	m2m_entity->entity.name = m2m_entity->name;
> 
> Why not just strlcpy into the m2m_entity->entity.name field? Then you can
> drop m2m_entity->name. Or am I missing something?
> 

Hm, I think you are right.

> > +	ret = media_device_register_entity(mdev, &m2m_entity->entity);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> > +
> > +int v4l2_m2m_register_media_controller(struct v4l2_m2m_dev *m2m_dev, struct video_device *vdev)
> > +{
> > +#if defined(CONFIG_MEDIA_CONTROLLER)
> > +	struct media_device *mdev = vdev->v4l2_dev->mdev;
> > +	struct media_link *link;
> > +	int ret;
> > +
> > +	if (!mdev)
> > +		return 0;
> > +
> > +	/* A memory-to-memory device consists in two
> > +	 * DMA engine and one video processing entities.
> > +	 * The DMA engine entities are linked to a V4L interface
> > +	 */
> > +
> > +	/* Create the three entities with their pads */
> > +	ret = v4l2_m2m_register_entity(mdev, &m2m_dev->entities[0], MEM2MEM_ENT_TYPE_INPUT);
> > +	if (ret)
> > +		return ret;
> > +	ret = v4l2_m2m_register_entity(mdev, &m2m_dev->entities[1], MEM2MEM_ENT_TYPE_PROC);
> > +	if (ret)
> > +		goto err_rel_entity0;
> > +	ret = v4l2_m2m_register_entity(mdev, &m2m_dev->entities[2], MEM2MEM_ENT_TYPE_OUTPUT);
> > +	if (ret)
> > +		goto err_rel_entity1;
> > +
> > +	/* Connect the three entities */
> > +        ret = media_create_pad_link(&m2m_dev->entities[0].entity, 0,
> > +			&m2m_dev->entities[1].entity, 1,
> > +                        MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> 
> Weird indentation.
> 
> > +	if (ret)
> > +		goto err_rel_entity2;
> > +
> > +        ret = media_create_pad_link(&m2m_dev->entities[1].entity, 0,
> > +			&m2m_dev->entities[2].entity, 0,
> > +                        MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED);
> 
> Ditto.
> 
> > +	if (ret)
> > +		goto err_rm_links0;
> > +
> > +	/* Create video interface */
> > +	m2m_dev->intf_devnode = media_devnode_create(mdev, MEDIA_INTF_T_V4L_VIDEO, 0, VIDEO_MAJOR, vdev->minor);
> > +	if (!m2m_dev->intf_devnode) {
> > +		ret = -ENOMEM;
> > +		goto err_rm_links1;
> > +	}
> > +
> > +	/* Connect the two DMA engines to the interface */
> > +	link = media_create_intf_link(&m2m_dev->entities[0].entity, &m2m_dev->intf_devnode->intf,
> > +				MEDIA_LNK_FL_ENABLED);
> > +	if (!link) {
> > +		ret = -ENOMEM;
> > +		goto err_rm_devnode;
> > +	}
> > +
> > +	link = media_create_intf_link(&m2m_dev->entities[1].entity, &m2m_dev->intf_devnode->intf,
> > +				MEDIA_LNK_FL_ENABLED);
> > +	if (!link) {
> > +		ret = -ENOMEM;
> > +		goto err_rm_intf_link;
> > +	}
> > +	return 0;
> > +
> > +err_rm_intf_link:
> > +	media_remove_intf_links(&m2m_dev->intf_devnode->intf);
> > +err_rm_devnode:
> > +	media_devnode_remove(m2m_dev->intf_devnode);
> > +err_rm_links1:
> > +	media_entity_remove_links(&m2m_dev->entities[2].entity);
> > +err_rm_links0:
> > +	media_entity_remove_links(&m2m_dev->entities[1].entity);
> > +	media_entity_remove_links(&m2m_dev->entities[0].entity);
> > +err_rel_entity2:
> > +	media_device_unregister_entity(&m2m_dev->entities[2].entity);
> > +err_rel_entity1:
> > +	media_device_unregister_entity(&m2m_dev->entities[1].entity);
> > +err_rel_entity0:
> > +	media_device_unregister_entity(&m2m_dev->entities[0].entity);
> > +	return ret;
> > +#endif
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_m2m_register_media_controller);
> > +
> >  struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops)
> >  {
> >  	struct v4l2_m2m_dev *m2m_dev;
> 
> Looking at this code I think it would be much cleaner if you drop the array
> and make it explicit instead:
> 
> struct v4l2_m2m_entities {
> 	struct media_entity source;
> 	struct media_pad source_pad;
> 	struct media_entity sink;
> 	struct media_pad sink_pad;
> 	struct media_entity proc;
> 	struct media_pad proc_pads[2];
> };
> 
> I think this will simplify the code quite a bit.
> 
> Note: one of these media_entity structs can be removed since struct video_device
> already has an entity you can use. It probably makes the most sense to assign
> that one the role of the sink or source entity.
> 
> Perhaps it might be easiest to change e.g. struct media_entity source to
> 'struct media_entity *source' and have it point to &vdev->entity.
> 
> Up to you.
> 

Right, I'll take a look.

> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 3aa3d58d1d58..ff6fbe8333e1 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -206,6 +206,9 @@ struct media_entity_operations {
> >   *	The entity is embedded in a struct video_device instance.
> >   * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
> >   *	The entity is embedded in a struct v4l2_subdev instance.
> > + * @MEDIA_ENTITY_TYPE_V4L2_MEM2MEM:
> > + *	The entity is not embedded in any struct, but part of
> > + *	a memory-to-memory topology.
> 
> I see no need for this. An M2M device is of type VIDEO_DEVICE, no need to
> change that.
> 

Well, the problem is that this type is used to cast the media_entity
using container_of macro, by means of is_media_entity_v4l2_video_device
and is_media_entity_v4l2_subdev.

So, by using one these types we'd be breaking that assumption. 

> >   *
> >   * Media entity objects are often not instantiated directly, but the media
> >   * entity structure is inherited by (through embedding) other subsystem-specific
> > @@ -222,6 +225,7 @@ enum media_entity_type {
> >  	MEDIA_ENTITY_TYPE_BASE,
> >  	MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
> >  	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
> > +	MEDIA_ENTITY_TYPE_MEM2MEM,
> >  };
> >  
> >  /**
> > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > index 456ac13eca1d..a9df949bb9c3 100644
> > --- a/include/media/v4l2-dev.h
> > +++ b/include/media/v4l2-dev.h
> > @@ -30,6 +30,7 @@
> >   * @VFL_TYPE_SUBDEV:	for V4L2 subdevices
> >   * @VFL_TYPE_SDR:	for Software Defined Radio tuners
> >   * @VFL_TYPE_TOUCH:	for touch sensors
> > + * @VFL_TYPE_MEM2MEM:	for mem2mem devices
> >   * @VFL_TYPE_MAX:	number of VFL types, must always be last in the enum
> >   */
> >  enum vfl_devnode_type {
> > @@ -39,6 +40,7 @@ enum vfl_devnode_type {
> >  	VFL_TYPE_SUBDEV,
> >  	VFL_TYPE_SDR,
> >  	VFL_TYPE_TOUCH,
> > +	VFL_TYPE_MEM2MEM,
> >  	VFL_TYPE_MAX /* Shall be the last one */
> >  };
> >  
> > diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
> > index 3d07ba3a8262..9dfe9bd23f89 100644
> > --- a/include/media/v4l2-mem2mem.h
> > +++ b/include/media/v4l2-mem2mem.h
> > @@ -53,6 +53,7 @@ struct v4l2_m2m_ops {
> >  	void (*unlock)(void *priv);
> >  };
> >  
> > +struct video_device;
> >  struct v4l2_m2m_dev;
> >  
> >  /**
> > @@ -328,6 +329,10 @@ int v4l2_m2m_mmap(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
> >   */
> >  struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops);
> >  
> > +int v4l2_m2m_register_media_controller(struct v4l2_m2m_dev *m2m_dev, struct video_device *vdev);
> > +
> > +void v4l2_m2m_unregister_media_controller(struct v4l2_m2m_dev *m2m_dev);
> > +
> >  /**
> >   * v4l2_m2m_release() - cleans up and frees a m2m_dev structure
> >   *
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index c7e9a5cba24e..becb7db77f6a 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -81,6 +81,7 @@ struct media_device_info {
> >  #define MEDIA_ENT_F_IO_DTV			(MEDIA_ENT_F_BASE + 0x01001)
> >  #define MEDIA_ENT_F_IO_VBI			(MEDIA_ENT_F_BASE + 0x01002)
> >  #define MEDIA_ENT_F_IO_SWRADIO			(MEDIA_ENT_F_BASE + 0x01003)
> > +#define MEDIA_ENT_F_IO_DMAENGINE		(MEDIA_ENT_F_BASE + 0x01004)
> 
> Drop this as well. Just stick to MEDIA_ENT_F_IO_V4L which is what we've decided to
> call such entities (for better or worse).
> 

Will do.

> >  
> >  /*
> >   * Sensor functions
> > @@ -132,6 +133,7 @@ struct media_device_info {
> >  #define MEDIA_ENT_F_PROC_VIDEO_LUT		(MEDIA_ENT_F_BASE + 0x4004)
> >  #define MEDIA_ENT_F_PROC_VIDEO_SCALER		(MEDIA_ENT_F_BASE + 0x4005)
> >  #define MEDIA_ENT_F_PROC_VIDEO_STATISTICS	(MEDIA_ENT_F_BASE + 0x4006)
> > +#define MEDIA_ENT_F_PROC_VIDEO_TRANSFORM	(MEDIA_ENT_F_BASE + 0x4007)
> 
> I think we need to be a bit more specific here:
> 
> #define MEDIA_ENT_F_PROC_VIDEO_DECODER
> #define MEDIA_ENT_F_PROC_VIDEO_ENCODER
> #define MEDIA_ENT_F_PROC_VIDEO_DEINTERLACER
> // others?
> 

OK. And what about "composite" devices that can encode and perform
other transforms, e.g. scale, rotation, etc. 

Regards,
Eze

Re: [RFC 2/2] vim2m: add media device

[Ezequiel Garcia]

On Tue, 2018-06-12 at 20:44 +0100, emil.velikov@collabora.com wrote:
> Hi Ezequiel,  
> 
> On Tue, Jun 12, 2018 at 07:48:27AM -0300, Ezequiel Garcia wrote:
> 
> > @@ -1013,10 +1016,10 @@ static int vim2m_probe(struct
> > platform_device *pdev)
> >  	vfd->lock = &dev->dev_mutex;
> >  	vfd->v4l2_dev = &dev->v4l2_dev;
> >  
> > -	ret = video_register_device(vfd, VFL_TYPE_GRABBER, 0);
> > +	ret = video_register_device(vfd, VFL_TYPE_MEM2MEM, 0);
> 
> Shouldn't the original type be used when building without
> CONFIG_MEDIA_CONTROLLER?
> 

No, the idea was to introduce a new type for mem2mem,
mainly to avoid the video2linux core from registering
mc entities.

Anyway, Hans dislikes this and suggested to drop it.

> 
> > @@ -1050,6 +1076,11 @@ static int vim2m_remove(struct
> > platform_device *pdev)
> >  	struct vim2m_dev *dev = platform_get_drvdata(pdev);
> >  
> >  	v4l2_info(&dev->v4l2_dev, "Removing " MEM2MEM_NAME);
> > +
> > +#ifdef CONFIG_MEDIA_CONTROLLER
> 
> Gut suggests that media_device_unregister() should be called here.
> 
> Then again my experience in media/ is limited so I could be miles off
> ;-)
> 

Good catch, it seems it's indeed missing.

Thanks,
Eze

Re: [RFC 0/2] Memory-to-memory media controller topology

[Ezequiel Garcia]

On Tue, 2018-06-12 at 10:42 -0400, Nicolas Dufresne wrote:
> Le mardi 12 juin 2018 à 07:48 -0300, Ezequiel Garcia a écrit :
> > As discussed on IRC, memory-to-memory need to be modeled
> > properly in order to be supported by the media controller
> > framework, and thus to support the Request API.
> > 
> > This RFC is a first draft on the memory-to-memory
> > media controller topology.
> > 
> > The topology looks like this:
> > 
> > Device topology
> > - entity 1: input (1 pad, 1 link)
> >             type Node subtype Unknown flags 0
> > 	pad0: Source
> > 		-> "proc":1 [ENABLED,IMMUTABLE]
> > 
> > - entity 3: proc (2 pads, 2 links)
> >             type Node subtype Unknown flags 0
> > 	pad0: Source
> > 		-> "output":0 [ENABLED,IMMUTABLE]
> > 	pad1: Sink
> > 		<- "input":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 6: output (1 pad, 1 link)
> >             type Node subtype Unknown flags 0
> > 	pad0: Sink
> > 		<- "proc":0 [ENABLED,IMMUTABLE]
> 
> Will the end result have "device node name /dev/..." on both entity 1
> and 6 ? 

No. There is just one devnode /dev/videoX, which is accepts
both CAPTURE and OUTPUT directions.

> I got told that in the long run, one should be able to map a
> device (/dev/mediaN) to it's nodes (/dev/video*). In a way that if we
> keep going this way, all the media devices can be enumerated from
> media
> node rather then a mixed between media nodes and orphaned video
> nodes.
> 

Yes, that is the idea I think. For instance, for devices with
multiple audio and video channels, there is currently no
clean way to discover them and correlate e.g. video devices
to audio devices.

Not that this series help on that either :) 

Re: [RFC 0/2] Memory-to-memory media controller topology

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 394 bytes --]

Le vendredi 15 juin 2018 à 17:05 -0300, Ezequiel Garcia a écrit :
> > Will the end result have "device node name /dev/..." on both entity
> > 1
> > and 6 ? 
> 
> No. There is just one devnode /dev/videoX, which is accepts
> both CAPTURE and OUTPUT directions.

My question is more ifthe dev node path will be provided somehow,
because it's not displayed in this topologyé

Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [RFC 1/2] media: add helpers for memory-to-memory media controller

[Hans Verkuil]

On 06/15/2018 06:22 PM, Ezequiel Garcia wrote:
> Hi Hans,
> 
> Thanks for the review.
> 
> On Fri, 2018-06-15 at 11:24 +0200, Hans Verkuil wrote:
>> On 12/06/18 12:48, Ezequiel Garcia wrote:

<snip>

>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>> index 3aa3d58d1d58..ff6fbe8333e1 100644
>>> --- a/include/media/media-entity.h
>>> +++ b/include/media/media-entity.h
>>> @@ -206,6 +206,9 @@ struct media_entity_operations {
>>>   *	The entity is embedded in a struct video_device instance.
>>>   * @MEDIA_ENTITY_TYPE_V4L2_SUBDEV:
>>>   *	The entity is embedded in a struct v4l2_subdev instance.
>>> + * @MEDIA_ENTITY_TYPE_V4L2_MEM2MEM:
>>> + *	The entity is not embedded in any struct, but part of
>>> + *	a memory-to-memory topology.
>>
>> I see no need for this. An M2M device is of type VIDEO_DEVICE, no need to
>> change that.
>>
> 
> Well, the problem is that this type is used to cast the media_entity
> using container_of macro, by means of is_media_entity_v4l2_video_device
> and is_media_entity_v4l2_subdev.
> 
> So, by using one these types we'd be breaking that assumption. 

Good point. But in that case I would just use MEDIA_ENTITY_TYPE_V4L2_BASE.
That's fine here.

> 
>>>   *
>>>   * Media entity objects are often not instantiated directly, but the media
>>>   * entity structure is inherited by (through embedding) other subsystem-specific
>>> @@ -222,6 +225,7 @@ enum media_entity_type {
>>>  	MEDIA_ENTITY_TYPE_BASE,
>>>  	MEDIA_ENTITY_TYPE_VIDEO_DEVICE,
>>>  	MEDIA_ENTITY_TYPE_V4L2_SUBDEV,
>>> +	MEDIA_ENTITY_TYPE_MEM2MEM,
>>>  };
>>>  
>>>  /**
>>> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
>>> index 456ac13eca1d..a9df949bb9c3 100644
>>> --- a/include/media/v4l2-dev.h
>>> +++ b/include/media/v4l2-dev.h
>>> @@ -30,6 +30,7 @@
>>>   * @VFL_TYPE_SUBDEV:	for V4L2 subdevices
>>>   * @VFL_TYPE_SDR:	for Software Defined Radio tuners
>>>   * @VFL_TYPE_TOUCH:	for touch sensors
>>> + * @VFL_TYPE_MEM2MEM:	for mem2mem devices
>>>   * @VFL_TYPE_MAX:	number of VFL types, must always be last in the enum
>>>   */
>>>  enum vfl_devnode_type {
>>> @@ -39,6 +40,7 @@ enum vfl_devnode_type {
>>>  	VFL_TYPE_SUBDEV,
>>>  	VFL_TYPE_SDR,
>>>  	VFL_TYPE_TOUCH,
>>> +	VFL_TYPE_MEM2MEM,
>>>  	VFL_TYPE_MAX /* Shall be the last one */
>>>  };
>>>  
>>> diff --git a/include/media/v4l2-mem2mem.h b/include/media/v4l2-mem2mem.h
>>> index 3d07ba3a8262..9dfe9bd23f89 100644
>>> --- a/include/media/v4l2-mem2mem.h
>>> +++ b/include/media/v4l2-mem2mem.h
>>> @@ -53,6 +53,7 @@ struct v4l2_m2m_ops {
>>>  	void (*unlock)(void *priv);
>>>  };
>>>  
>>> +struct video_device;
>>>  struct v4l2_m2m_dev;
>>>  
>>>  /**
>>> @@ -328,6 +329,10 @@ int v4l2_m2m_mmap(struct file *file, struct v4l2_m2m_ctx *m2m_ctx,
>>>   */
>>>  struct v4l2_m2m_dev *v4l2_m2m_init(const struct v4l2_m2m_ops *m2m_ops);
>>>  
>>> +int v4l2_m2m_register_media_controller(struct v4l2_m2m_dev *m2m_dev, struct video_device *vdev);
>>> +
>>> +void v4l2_m2m_unregister_media_controller(struct v4l2_m2m_dev *m2m_dev);
>>> +
>>>  /**
>>>   * v4l2_m2m_release() - cleans up and frees a m2m_dev structure
>>>   *
>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>> index c7e9a5cba24e..becb7db77f6a 100644
>>> --- a/include/uapi/linux/media.h
>>> +++ b/include/uapi/linux/media.h
>>> @@ -81,6 +81,7 @@ struct media_device_info {
>>>  #define MEDIA_ENT_F_IO_DTV			(MEDIA_ENT_F_BASE + 0x01001)
>>>  #define MEDIA_ENT_F_IO_VBI			(MEDIA_ENT_F_BASE + 0x01002)
>>>  #define MEDIA_ENT_F_IO_SWRADIO			(MEDIA_ENT_F_BASE + 0x01003)
>>> +#define MEDIA_ENT_F_IO_DMAENGINE		(MEDIA_ENT_F_BASE + 0x01004)
>>
>> Drop this as well. Just stick to MEDIA_ENT_F_IO_V4L which is what we've decided to
>> call such entities (for better or worse).
>>
> 
> Will do.
> 
>>>  
>>>  /*
>>>   * Sensor functions
>>> @@ -132,6 +133,7 @@ struct media_device_info {
>>>  #define MEDIA_ENT_F_PROC_VIDEO_LUT		(MEDIA_ENT_F_BASE + 0x4004)
>>>  #define MEDIA_ENT_F_PROC_VIDEO_SCALER		(MEDIA_ENT_F_BASE + 0x4005)
>>>  #define MEDIA_ENT_F_PROC_VIDEO_STATISTICS	(MEDIA_ENT_F_BASE + 0x4006)
>>> +#define MEDIA_ENT_F_PROC_VIDEO_TRANSFORM	(MEDIA_ENT_F_BASE + 0x4007)
>>
>> I think we need to be a bit more specific here:
>>
>> #define MEDIA_ENT_F_PROC_VIDEO_DECODER
>> #define MEDIA_ENT_F_PROC_VIDEO_ENCODER
>> #define MEDIA_ENT_F_PROC_VIDEO_DEINTERLACER
>> // others?
>>
> 
> OK. And what about "composite" devices that can encode and perform
> other transforms, e.g. scale, rotation, etc. 

The intention is to eventually allow for more than one function to be specified
for an entity. That should be done through 'properties', but that still hasn't
been implemented. For now we use 'function' to specify the primary function of
the device.

Regards,

	Hans

Re: [RFC 0/2] Memory-to-memory media controller topology

[Ezequiel Garcia]

On Sat, 2018-06-16 at 20:31 -0400, Nicolas Dufresne wrote:
> Le vendredi 15 juin 2018 à 17:05 -0300, Ezequiel Garcia a écrit :
> > > Will the end result have "device node name /dev/..." on both
> > > entity
> > > 1
> > > and 6 ? 
> > 
> > No. There is just one devnode /dev/videoX, which is accepts
> > both CAPTURE and OUTPUT directions.
> 
> My question is more ifthe dev node path will be provided somehow,
> because it's not displayed in this topologyé
> 

AFAIU, the device node associated to each media interface object
can be discovered via the G_TOPOLOGY[1] ioctl.

User gets the major:minor tuple, which can be used to get
the node path.

[1] https://linuxtv.org/downloads/v4l-dvb-apis/uapi/mediactl/media-ioc-
g-topology.html