Re: [RFC/PATCH 03/10] media: Entities, pads and links
Hi Hans,
Thanks for the review.
On Sunday 18 July 2010 13:53:51 Hans Verkuil wrote:
On Wednesday 14 July 2010 15:30:12 Laurent Pinchart wrote:
[snip]
+Links have flags that describe the link capabilities and state.
+
+ MEDIA_LINK_FLAG_ACTIVE indicates that the link is active and can be
+ used to transfer media data. When two or more links target a sink pad,
+ only one of them can be active at a time.
+ MEDIA_LINK_FLAG_IMMUTABLE indicates that the link active state can't
+ be modified at runtime. An immutable link is always active.
I would rephrase the last sentence to:
If MEDIA_LINK_FLAG_IMMUTABLE is set, then MEDIA_LINK_FLAG_ACTIVE must also
be set since an immutable link is always active.
OK, I'll change that.
[snip]
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
new file mode 100644
index 000..0929a90
--- /dev/null
+++ b/include/media/media-entity.h
@@ -0,0 +1,79 @@
+#ifndef _MEDIA_ENTITY_H
+#define _MEDIA_ENTITY_H
+
+#include linux/list.h
+
+#define MEDIA_ENTITY_TYPE_NODE 1
+#define MEDIA_ENTITY_TYPE_SUBDEV 2
+
+#define MEDIA_NODE_TYPE_V4L1
+#define MEDIA_NODE_TYPE_FB 2
+#define MEDIA_NODE_TYPE_ALSA 3
+#define MEDIA_NODE_TYPE_DVB4
+
+#define MEDIA_SUBDEV_TYPE_VID_DECODER 1
+#define MEDIA_SUBDEV_TYPE_VID_ENCODER 2
+#define MEDIA_SUBDEV_TYPE_MISC 3
Are these the subtypes? If so, I would rename this to
MEDIA_ENTITY_SUBTYPE_VID_DECODER, etc.
Those are subtypes relative to the node and subdev types. Their name should
thus start with the type they refer to. What about
MEDIA_ENTITY_SUBTYPE_NODE_V4L
MEDIA_ENTITY_SUBTYPE_NODE_FB
MEDIA_ENTITY_SUBTYPE_NODE_ALSA
MEDIA_ENTITY_SUBTYPE_NODE_DVB
MEDIA_ENTITY_SUBTYPE_SUBDEV_VID_DECODER
MEDIA_ENTITY_SUBTYPE_SUBDEV_VID_ENCODER
MEDIA_ENTITY_SUBTYPE_SUBDEV_MISC
It might be a bit long though.
The subdev subtypes need more attention. I don't think that video decoder,
video encoder and misc are good enough. Maybe some kind of capabilities
bitflag would be better.
+#define MEDIA_LINK_FLAG_ACTIVE (1 0)
+#define MEDIA_LINK_FLAG_IMMUTABLE (1 1)
+
+#define MEDIA_PAD_TYPE_INPUT 1
+#define MEDIA_PAD_TYPE_OUTPUT 2
+
+struct media_entity_link {
+ struct media_entity_pad *source;/* Source pad */
+ struct media_entity_pad *sink; /* Sink pad */
+ struct media_entity_link *other;/* Link in the reverse direction */
+ u32 flags; /* Link flags (MEDIA_LINK_FLAG_*) */
+};
+
+struct media_entity_pad {
+ struct media_entity *entity;/* Entity this pad belongs to */
+ u32 type; /* Pad type (MEDIA_PAD_TYPE_*) */
+ u32 index; /* Pad index in the entity pads array */
u32 seems unnecessarily wasteful. u8 should be sufficient.
OK.
I don't really like the name 'type'. Why not 'dir' for direction?
Another reason for not using the name 'type' for this is that I think we
need an actual 'type' field that describes the type of data being streamed
to/from the pad. While for now we mainly have video pads, we may also get
audio pads and perhaps vbi pads as well.
Agreed. Do you think we should have a capabilities bitflag ? The direction
could be encoded as 2 bits, one for input and one for output.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 03/10] media: Entities, pads and links
Hi Hans,
Thanks for the review.
On Sunday 18 July 2010 13:53:51 Hans Verkuil wrote:
On Wednesday 14 July 2010 15:30:12 Laurent Pinchart wrote:
[snip]
+Links have flags that describe the link capabilities and state.
+
+ MEDIA_LINK_FLAG_ACTIVE indicates that the link is active and can be
+ used to transfer media data. When two or more links target a sink
pad,
+ only one of them can be active at a time.
+ MEDIA_LINK_FLAG_IMMUTABLE indicates that the link active state can't
+ be modified at runtime. An immutable link is always active.
I would rephrase the last sentence to:
If MEDIA_LINK_FLAG_IMMUTABLE is set, then MEDIA_LINK_FLAG_ACTIVE must
also
be set since an immutable link is always active.
OK, I'll change that.
[snip]
diff --git a/include/media/media-entity.h
b/include/media/media-entity.h
new file mode 100644
index 000..0929a90
--- /dev/null
+++ b/include/media/media-entity.h
@@ -0,0 +1,79 @@
+#ifndef _MEDIA_ENTITY_H
+#define _MEDIA_ENTITY_H
+
+#include linux/list.h
+
+#define MEDIA_ENTITY_TYPE_NODE1
+#define MEDIA_ENTITY_TYPE_SUBDEV 2
+
+#define MEDIA_NODE_TYPE_V4L 1
+#define MEDIA_NODE_TYPE_FB2
+#define MEDIA_NODE_TYPE_ALSA 3
+#define MEDIA_NODE_TYPE_DVB 4
+
+#define MEDIA_SUBDEV_TYPE_VID_DECODER 1
+#define MEDIA_SUBDEV_TYPE_VID_ENCODER 2
+#define MEDIA_SUBDEV_TYPE_MISC3
Are these the subtypes? If so, I would rename this to
MEDIA_ENTITY_SUBTYPE_VID_DECODER, etc.
Those are subtypes relative to the node and subdev types. Their name
should
thus start with the type they refer to. What about
MEDIA_ENTITY_SUBTYPE_NODE_V4L
MEDIA_ENTITY_SUBTYPE_NODE_FB
MEDIA_ENTITY_SUBTYPE_NODE_ALSA
MEDIA_ENTITY_SUBTYPE_NODE_DVB
MEDIA_ENTITY_SUBTYPE_SUBDEV_VID_DECODER
MEDIA_ENTITY_SUBTYPE_SUBDEV_VID_ENCODER
MEDIA_ENTITY_SUBTYPE_SUBDEV_MISC
It might be a bit long though.
Perhaps, but now I understand it. I really didn't get the original names.
The subdev subtypes need more attention. I don't think that video decoder,
video encoder and misc are good enough. Maybe some kind of capabilities
bitflag would be better.
I don't think so. The problem with bitflags is that you run out of them so
quickly. We definitely need more subtypes, though, but we can just add
them as needed.
+#define MEDIA_LINK_FLAG_ACTIVE(1 0)
+#define MEDIA_LINK_FLAG_IMMUTABLE (1 1)
+
+#define MEDIA_PAD_TYPE_INPUT 1
+#define MEDIA_PAD_TYPE_OUTPUT 2
+
+struct media_entity_link {
+ struct media_entity_pad *source;/* Source pad */
+ struct media_entity_pad *sink; /* Sink pad */
+ struct media_entity_link *other;/* Link in the reverse direction */
+ u32 flags; /* Link flags (MEDIA_LINK_FLAG_*) */
+};
+
+struct media_entity_pad {
+ struct media_entity *entity;/* Entity this pad belongs to */
+ u32 type; /* Pad type (MEDIA_PAD_TYPE_*) */
+ u32 index; /* Pad index in the entity pads array */
u32 seems unnecessarily wasteful. u8 should be sufficient.
OK.
I don't really like the name 'type'. Why not 'dir' for direction?
Another reason for not using the name 'type' for this is that I think we
need an actual 'type' field that describes the type of data being
streamed
to/from the pad. While for now we mainly have video pads, we may also
get
audio pads and perhaps vbi pads as well.
Agreed. Do you think we should have a capabilities bitflag ? The direction
could be encoded as 2 bits, one for input and one for output.
I don't really like that. It makes for awkward ANDs in the code whenever
you need to detect the direction.
If this is only used internally, then we might consider using a bitfield.
That would work as well.
Regards.
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 03/10] media: Entities, pads and links
Hi Sergio,
Thanks for the review.
On Thursday 15 July 2010 16:35:20 Aguirre, Sergio wrote:
On Wednesday 14 July 2010 08:30:00 Laurent Pinchart wrote:
[snip]
diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index a4d3db5..6361367 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
[snip]
@@ -72,6 +77,54 @@ EXPORT_SYMBOL_GPL(media_device_register);
[snip]
+
+/**
+ * media_device_register_entity - Register an entity with a media device
+ * @mdev:The media device
+ * @entity: The entity
+ */
+int __must_check media_device_register_entity(struct media_device *mdev,
+ struct media_entity *entity)
+{
What if mdev == NULL OR entity == NULL?
It's not a valid use case. Drivers must not try to register a NULL entity, or
an entity to a NULL media device.
+ /* Warn if we apparently re-register an entity */
+ WARN_ON(entity-parent != NULL);
Shouldn't we exit with -EBUSY here instead? Or is there a usecase
In which this is a valid scenario?
entity-parent != NULL isn't a valid scenario. It's a driver bug, and WARN_ON
will result in a backtrace being printed, notifying the driver developer that
something is seriously wrong.
+ entity-parent = mdev;
+
+ spin_lock(mdev-lock);
+ entity-id = mdev-entity_id++;
+ list_add_tail(entity-list, mdev-entities);
+ spin_unlock(mdev-lock);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(media_device_register_entity);
+
+/**
+ * media_device_unregister_entity - Unregister an entity
+ * @entity: The entity
+ *
+ * If the entity has never been registered this function will return
+ * immediately.
+ */
+void media_device_unregister_entity(struct media_entity *entity)
+{
+ struct media_device *mdev = entity-parent;
What if entity == NULL?
Still not a valid use case, don't unregister NULL.
+
+ if (mdev == NULL)
+ return;
+
+ spin_lock(mdev-lock);
+ list_del(entity-list);
+ spin_unlock(mdev-lock);
+ entity-parent = NULL;
+}
+EXPORT_SYMBOL_GPL(media_device_unregister_entity);
diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
new file mode 100644
index 000..d5a4b4c
--- /dev/null
+++ b/drivers/media/media-entity.c
@@ -0,0 +1,145 @@
+/*
+ * Media Entity support
+ *
+ * Copyright (C) 2009 Laurent Pinchart
laurent.pinch...@ideasonboard.com
2010?
Oops, yes, will fix.
[snip]
+int
+media_entity_create_link(struct media_entity *source, u8 source_pad,
+ struct media_entity *sink, u8 sink_pad, u32 flags)
+{
+ struct media_entity_link *link;
+ struct media_entity_link *backlink;
+
+ BUG_ON(source == NULL || sink == NULL);
+ BUG_ON(source_pad = source-num_pads);
+ BUG_ON(sink_pad = sink-num_pads);
Isn't this too dramatic? :)
I mean, does the entire system needs to be halted because you won't be
able to link your video subdevices?
If source or sink is NULL, the second or third BUG_ON will result in a crash.
If the source or sink pad numbers are out of bounds, undefined memory will be
dereferenced later. Both conditions will likely result in a crash, so it's
better to have a predictable, easy to understand crash.
Once again this should never happen. It's a driver bug if it does, and should
not happen randomly at runtime.
[snip]
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
new file mode 100644
index 000..0929a90
--- /dev/null
+++ b/include/media/media-entity.h
@@ -0,0 +1,79 @@
+#ifndef _MEDIA_ENTITY_H
+#define _MEDIA_ENTITY_H
+
+#include linux/list.h
+
+#define MEDIA_ENTITY_TYPE_NODE 1
+#define MEDIA_ENTITY_TYPE_SUBDEV 2
+
+#define MEDIA_NODE_TYPE_V4L 1
+#define MEDIA_NODE_TYPE_FB 2
+#define MEDIA_NODE_TYPE_ALSA 3
+#define MEDIA_NODE_TYPE_DVB 4
+
+#define MEDIA_SUBDEV_TYPE_VID_DECODER1
+#define MEDIA_SUBDEV_TYPE_VID_ENCODER2
+#define MEDIA_SUBDEV_TYPE_MISC 3
+
+#define MEDIA_LINK_FLAG_ACTIVE (1 0)
+#define MEDIA_LINK_FLAG_IMMUTABLE(1 1)
+
+#define MEDIA_PAD_TYPE_INPUT 1
+#define MEDIA_PAD_TYPE_OUTPUT2
Shouldn't all the above be enums? (except of course the
MEDIA_LINK_FLAG_* defines)
They can be enums, but the structures used in ioctls must use integer types as
enum sizes vary depending on the ABI on some platforms (most notably ARM). I
have no strong opinion for or against enums for the definitions of the values.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC/PATCH 03/10] media: Entities, pads and links
Hi Laurent,
-Original Message-
From: Laurent Pinchart [mailto:laurent.pinch...@ideasonboard.com]
Sent: Friday, July 16, 2010 4:10 AM
To: Aguirre, Sergio
Cc: linux-media@vger.kernel.org; sakari.ai...@maxwell.research.nokia.com
Subject: Re: [RFC/PATCH 03/10] media: Entities, pads and links
Hi Sergio,
Thanks for the review.
On Thursday 15 July 2010 16:35:20 Aguirre, Sergio wrote:
On Wednesday 14 July 2010 08:30:00 Laurent Pinchart wrote:
[snip]
diff --git a/drivers/media/media-device.c b/drivers/media/media-
device.c
index a4d3db5..6361367 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
[snip]
@@ -72,6 +77,54 @@ EXPORT_SYMBOL_GPL(media_device_register);
[snip]
+
+/**
+ * media_device_register_entity - Register an entity with a media
device
+ * @mdev:The media device
+ * @entity: The entity
+ */
+int __must_check media_device_register_entity(struct media_device
*mdev,
+ struct media_entity
*entity)
+{
What if mdev == NULL OR entity == NULL?
It's not a valid use case. Drivers must not try to register a NULL entity,
or
an entity to a NULL media device.
Ok.
+ /* Warn if we apparently re-register an entity */
+ WARN_ON(entity-parent != NULL);
Shouldn't we exit with -EBUSY here instead? Or is there a usecase
In which this is a valid scenario?
entity-parent != NULL isn't a valid scenario. It's a driver bug, and
WARN_ON
will result in a backtrace being printed, notifying the driver developer
that
something is seriously wrong.
Ok.
+ entity-parent = mdev;
+
+ spin_lock(mdev-lock);
+ entity-id = mdev-entity_id++;
+ list_add_tail(entity-list, mdev-entities);
+ spin_unlock(mdev-lock);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(media_device_register_entity);
+
+/**
+ * media_device_unregister_entity - Unregister an entity
+ * @entity: The entity
+ *
+ * If the entity has never been registered this function will return
+ * immediately.
+ */
+void media_device_unregister_entity(struct media_entity *entity)
+{
+ struct media_device *mdev = entity-parent;
What if entity == NULL?
Still not a valid use case, don't unregister NULL.
Ok.
+
+ if (mdev == NULL)
+ return;
+
+ spin_lock(mdev-lock);
+ list_del(entity-list);
+ spin_unlock(mdev-lock);
+ entity-parent = NULL;
+}
+EXPORT_SYMBOL_GPL(media_device_unregister_entity);
diff --git a/drivers/media/media-entity.c b/drivers/media/media-
entity.c
new file mode 100644
index 000..d5a4b4c
--- /dev/null
+++ b/drivers/media/media-entity.c
@@ -0,0 +1,145 @@
+/*
+ * Media Entity support
+ *
+ * Copyright (C) 2009 Laurent Pinchart
laurent.pinch...@ideasonboard.com
2010?
Oops, yes, will fix.
[snip]
+int
+media_entity_create_link(struct media_entity *source, u8 source_pad,
+ struct media_entity *sink, u8 sink_pad, u32
flags)
+{
+ struct media_entity_link *link;
+ struct media_entity_link *backlink;
+
+ BUG_ON(source == NULL || sink == NULL);
+ BUG_ON(source_pad = source-num_pads);
+ BUG_ON(sink_pad = sink-num_pads);
Isn't this too dramatic? :)
I mean, does the entire system needs to be halted because you won't be
able to link your video subdevices?
If source or sink is NULL, the second or third BUG_ON will result in a
crash.
If the source or sink pad numbers are out of bounds, undefined memory will
be
dereferenced later. Both conditions will likely result in a crash, so it's
better to have a predictable, easy to understand crash.
Once again this should never happen. It's a driver bug if it does, and
should
not happen randomly at runtime.
Ok.
[snip]
diff --git a/include/media/media-entity.h b/include/media/media-
entity.h
new file mode 100644
index 000..0929a90
--- /dev/null
+++ b/include/media/media-entity.h
@@ -0,0 +1,79 @@
+#ifndef _MEDIA_ENTITY_H
+#define _MEDIA_ENTITY_H
+
+#include linux/list.h
+
+#define MEDIA_ENTITY_TYPE_NODE 1
+#define MEDIA_ENTITY_TYPE_SUBDEV 2
+
+#define MEDIA_NODE_TYPE_V4L 1
+#define MEDIA_NODE_TYPE_FB 2
+#define MEDIA_NODE_TYPE_ALSA 3
+#define MEDIA_NODE_TYPE_DVB 4
+
+#define MEDIA_SUBDEV_TYPE_VID_DECODER1
+#define MEDIA_SUBDEV_TYPE_VID_ENCODER2
+#define MEDIA_SUBDEV_TYPE_MISC 3
+
+#define MEDIA_LINK_FLAG_ACTIVE (1 0)
+#define MEDIA_LINK_FLAG_IMMUTABLE(1 1)
+
+#define MEDIA_PAD_TYPE_INPUT 1
+#define MEDIA_PAD_TYPE_OUTPUT2
Shouldn't all the above be enums? (except of course the
MEDIA_LINK_FLAG_* defines)
They can be enums
RE: [RFC/PATCH 03/10] media: Entities, pads and links
Hi Laurent, -Original Message- From: linux-media-ow...@vger.kernel.org [mailto:linux-media- ow...@vger.kernel.org] On Behalf Of Laurent Pinchart Sent: Wednesday, July 14, 2010 8:30 AM To: linux-media@vger.kernel.org Cc: sakari.ai...@maxwell.research.nokia.com Subject: [RFC/PATCH 03/10] media: Entities, pads and links As video hardware pipelines become increasingly complex and configurable, the current hardware description through v4l2 subdevices reaches its limits. In addition to enumerating and configuring subdevices, video camera drivers need a way to discover and modify at runtime how those subdevices are connected. This is done through new elements called entities, pads and links. An entity is a basic media hardware building block. It can correspond to a large variety of logical blocks such as physical hardware devices (CMOS sensor for instance), logical hardware devices (a building block in a System-on-Chip image processing pipeline), DMA channels or physical connectors. A pad is a connection endpoint through which an entity can interact with other entities. Data (not restricted to video) produced by an entity flows from the entity's output to one or more entity inputs. Pads should not be confused with physical pins at chip boundaries. A link is a point-to-point oriented connection between two pads, either on the same entity or on different entities. Data flows from a source pad to a sink pad. Links are stored in the source entity. To make backwards graph walk faster, a copy of all links is also stored in the sink entity. The copy is known as a backlink and is only used to help graph traversal. The entity API is made of three functions: - media_entity_init() initializes an entity. The caller must provide an array of pads as well as an estimated number of links. The links array is allocated dynamically and will be reallocated if it grows beyond the initial estimate. - media_entity_cleanup() frees resources allocated for an entity. It must be called during the cleanup phase after unregistering the entity and before freeing it. - media_entity_create_link() creates a link between two entities. An entry in the link array of each entity is allocated and stores pointers to source and sink pads. When a media device is unregistered, all its entities are unregistered automatically. The code is based on Hans Verkuil hverk...@xs4all.nl initial work. Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com Signed-off-by: Sakari Ailus sakari.ai...@maxwell.research.nokia.com --- Documentation/media-framework.txt | 125 drivers/media/Makefile|2 +- drivers/media/media-device.c | 53 ++ drivers/media/media-entity.c | 145 + include/media/media-device.h | 16 include/media/media-entity.h | 79 6 files changed, 419 insertions(+), 1 deletions(-) create mode 100644 drivers/media/media-entity.c create mode 100644 include/media/media-entity.h diff --git a/Documentation/media-framework.txt b/Documentation/media- framework.txt index b942c8f..4a8f379 100644 --- a/Documentation/media-framework.txt +++ b/Documentation/media-framework.txt @@ -35,6 +35,30 @@ belong to userspace. The media kernel API aims at solving those problems. +Abstract media device model +--- + +Discovering a device internal topology, and configuring it at runtime, is one +of the goals of the media framework. To achieve this, hardware devices are +modeled as an oriented graph of building blocks called entities connected +through pads. + +An entity is a basic media hardware building block. It can correspond to +a large variety of logical blocks such as physical hardware devices +(CMOS sensor for instance), logical hardware devices (a building block +in a System-on-Chip image processing pipeline), DMA channels or physical +connectors. + +A pad is a connection endpoint through which an entity can interact with +other entities. Data (not restricted to video) produced by an entity +flows from the entity's output to one or more entity inputs. Pads should +not be confused with physical pins at chip boundaries. + +A link is a point-to-point oriented connection between two pads, either +on the same entity or on different entities. Data flows from a source +pad to a sink pad. + + Media device @@ -66,3 +90,104 @@ Drivers unregister media device instances by calling Unregistering a media device that hasn't been registered is *NOT* safe. + +Entities, pads and links + + +- Entities + +Entities are represented by a struct media_entity instance, defined in +include/media/media-entity.h. The structure is usually embedded into a +higher-level structure, such as a v4l2_subdev or video_device instance, +although drivers can
