* [RFC 0/2] Memory-to-memory media controller topology
@ 2018-06-12 10:48 Ezequiel Garcia
2018-06-12 10:48 ` [RFC 1/2] media: add helpers for memory-to-memory media controller Ezequiel Garcia
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Ezequiel Garcia @ 2018-06-12 10:48 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel, 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
^ permalink raw reply [flat|nested] 14+ messages in thread* [RFC 1/2] media: add helpers for memory-to-memory media controller 2018-06-12 10:48 [RFC 0/2] Memory-to-memory media controller topology Ezequiel Garcia @ 2018-06-12 10:48 ` Ezequiel Garcia 2018-06-15 9:24 ` Hans Verkuil 2018-06-12 10:48 ` [RFC 2/2] vim2m: add media device Ezequiel Garcia ` (2 subsequent siblings) 3 siblings, 1 reply; 14+ messages in thread From: Ezequiel Garcia @ 2018-06-12 10:48 UTC (permalink / raw) To: linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC 1/2] media: add helpers for memory-to-memory media controller 2018-06-12 10:48 ` [RFC 1/2] media: add helpers for memory-to-memory media controller Ezequiel Garcia @ 2018-06-15 9:24 ` Hans Verkuil 2018-06-15 16:22 ` Ezequiel Garcia 0 siblings, 1 reply; 14+ messages in thread From: Hans Verkuil @ 2018-06-15 9:24 UTC (permalink / raw) To: Ezequiel Garcia, linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 1/2] media: add helpers for memory-to-memory media controller 2018-06-15 9:24 ` Hans Verkuil @ 2018-06-15 16:22 ` Ezequiel Garcia 2018-06-18 10:18 ` Hans Verkuil 0 siblings, 1 reply; 14+ messages in thread From: Ezequiel Garcia @ 2018-06-15 16:22 UTC (permalink / raw) To: Hans Verkuil, linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 1/2] media: add helpers for memory-to-memory media controller 2018-06-15 16:22 ` Ezequiel Garcia @ 2018-06-18 10:18 ` Hans Verkuil 0 siblings, 0 replies; 14+ messages in thread From: Hans Verkuil @ 2018-06-18 10:18 UTC (permalink / raw) To: Ezequiel Garcia, linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC 2/2] vim2m: add media device 2018-06-12 10:48 [RFC 0/2] Memory-to-memory media controller topology Ezequiel Garcia 2018-06-12 10:48 ` [RFC 1/2] media: add helpers for memory-to-memory media controller Ezequiel Garcia @ 2018-06-12 10:48 ` Ezequiel Garcia 2018-06-12 19:44 ` emil.velikov 2018-06-12 14:42 ` [RFC 0/2] Memory-to-memory media controller topology Nicolas Dufresne 2018-06-15 8:59 ` Hans Verkuil 3 siblings, 1 reply; 14+ messages in thread From: Ezequiel Garcia @ 2018-06-12 10:48 UTC (permalink / raw) To: linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel, 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC 2/2] vim2m: add media device 2018-06-12 10:48 ` [RFC 2/2] vim2m: add media device Ezequiel Garcia @ 2018-06-12 19:44 ` emil.velikov 2018-06-15 20:01 ` Ezequiel Garcia 0 siblings, 1 reply; 14+ messages in thread From: emil.velikov @ 2018-06-12 19:44 UTC (permalink / raw) To: Ezequiel Garcia; +Cc: linux-media, Hans Verkuil, Laurent Pinchart, kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 2/2] vim2m: add media device 2018-06-12 19:44 ` emil.velikov @ 2018-06-15 20:01 ` Ezequiel Garcia 0 siblings, 0 replies; 14+ messages in thread From: Ezequiel Garcia @ 2018-06-15 20:01 UTC (permalink / raw) To: emil.velikov; +Cc: linux-media, Hans Verkuil, Laurent Pinchart, kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 0/2] Memory-to-memory media controller topology 2018-06-12 10:48 [RFC 0/2] Memory-to-memory media controller topology Ezequiel Garcia 2018-06-12 10:48 ` [RFC 1/2] media: add helpers for memory-to-memory media controller Ezequiel Garcia 2018-06-12 10:48 ` [RFC 2/2] vim2m: add media device Ezequiel Garcia @ 2018-06-12 14:42 ` Nicolas Dufresne 2018-06-15 20:05 ` Ezequiel Garcia 2018-06-15 8:59 ` Hans Verkuil 3 siblings, 1 reply; 14+ messages in thread From: Nicolas Dufresne @ 2018-06-12 14:42 UTC (permalink / raw) To: Ezequiel Garcia, linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 0/2] Memory-to-memory media controller topology 2018-06-12 14:42 ` [RFC 0/2] Memory-to-memory media controller topology Nicolas Dufresne @ 2018-06-15 20:05 ` Ezequiel Garcia 2018-06-17 0:31 ` Nicolas Dufresne 0 siblings, 1 reply; 14+ messages in thread From: Ezequiel Garcia @ 2018-06-15 20:05 UTC (permalink / raw) To: Nicolas Dufresne, linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel 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 :) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 0/2] Memory-to-memory media controller topology 2018-06-15 20:05 ` Ezequiel Garcia @ 2018-06-17 0:31 ` Nicolas Dufresne 2018-06-20 19:25 ` Ezequiel Garcia 0 siblings, 1 reply; 14+ messages in thread From: Nicolas Dufresne @ 2018-06-17 0:31 UTC (permalink / raw) To: Ezequiel Garcia, linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel [-- 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 --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 0/2] Memory-to-memory media controller topology 2018-06-17 0:31 ` Nicolas Dufresne @ 2018-06-20 19:25 ` Ezequiel Garcia 0 siblings, 0 replies; 14+ messages in thread From: Ezequiel Garcia @ 2018-06-20 19:25 UTC (permalink / raw) To: Nicolas Dufresne, linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 0/2] Memory-to-memory media controller topology 2018-06-12 10:48 [RFC 0/2] Memory-to-memory media controller topology Ezequiel Garcia ` (2 preceding siblings ...) 2018-06-12 14:42 ` [RFC 0/2] Memory-to-memory media controller topology Nicolas Dufresne @ 2018-06-15 8:59 ` Hans Verkuil 2018-06-15 12:53 ` Ezequiel Garcia 3 siblings, 1 reply; 14+ messages in thread From: Hans Verkuil @ 2018-06-15 8:59 UTC (permalink / raw) To: Ezequiel Garcia, linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel 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(-) > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 0/2] Memory-to-memory media controller topology 2018-06-15 8:59 ` Hans Verkuil @ 2018-06-15 12:53 ` Ezequiel Garcia 0 siblings, 0 replies; 14+ messages in thread From: Ezequiel Garcia @ 2018-06-15 12:53 UTC (permalink / raw) To: Hans Verkuil, linux-media; +Cc: Hans Verkuil, Laurent Pinchart, kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-06-20 19:25 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-12 10:48 [RFC 0/2] Memory-to-memory media controller topology Ezequiel Garcia 2018-06-12 10:48 ` [RFC 1/2] media: add helpers for memory-to-memory media controller Ezequiel Garcia 2018-06-15 9:24 ` Hans Verkuil 2018-06-15 16:22 ` Ezequiel Garcia 2018-06-18 10:18 ` Hans Verkuil 2018-06-12 10:48 ` [RFC 2/2] vim2m: add media device Ezequiel Garcia 2018-06-12 19:44 ` emil.velikov 2018-06-15 20:01 ` Ezequiel Garcia 2018-06-12 14:42 ` [RFC 0/2] Memory-to-memory media controller topology Nicolas Dufresne 2018-06-15 20:05 ` Ezequiel Garcia 2018-06-17 0:31 ` Nicolas Dufresne 2018-06-20 19:25 ` Ezequiel Garcia 2018-06-15 8:59 ` Hans Verkuil 2018-06-15 12:53 ` Ezequiel Garcia
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.