Re: [RFC 1/2] media: add helpers for memory-to-memory media controller
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:
>>> 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 1/2] media: add helpers for memory-to-memory media controller
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
> > Suggested-by: Hans Verkuil
> > Signed-off-by: Ezequiel Garcia
> > ---
> > 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 = M
Re: [RFC 1/2] media: add helpers for memory-to-memory media controller
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
> Suggested-by: Hans Verkuil
> Signed-off-by: Ezequiel Garcia
> ---
> 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
[RFC 1/2] media: add helpers for memory-to-memory media controller
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
Suggested-by: Hans Verkuil
Signed-off-by: Ezequiel Garcia
---
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.
+*/
