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@xxxxxxxxxxxxxxxx>
> > Suggested-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> > Signed-off-by: Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>
> > ---
> > 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
