Hi Mauro,
On Wed, Oct 14, 2015 at 11:26:00PM -0300, Mauro Carvalho Chehab wrote:
Em Tue, 13 Oct 2015 08:09:53 -0300 Mauro Carvalho Chehab mchehab@osg.samsung.com escreveu:
Em Tue, 13 Oct 2015 12:16:51 +0300 Sakari Ailus sakari.ailus@iki.fi escreveu:
Hi Mauro,
On Mon, Oct 12, 2015 at 01:43:38PM -0300, Mauro Carvalho Chehab wrote:
Add a new ioctl that will report the entire topology on one go.
Change-Id: Ic022469be237dcaca56a077073b404f7de21f655 Signed-off-by: Mauro Carvalho Chehab mchehab@osg.samsung.com
include/media/media-entity.h | 2 + include/uapi/linux/media.h | 88 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 89 insertions(+), 1 deletion(-)
diff --git a/include/media/media-entity.h b/include/media/media-entity.h index 7320cdc45833..2d5ad40254b7 100644 --- a/include/media/media-entity.h +++ b/include/media/media-entity.h @@ -181,6 +181,8 @@ struct media_interface { */ struct media_intf_devnode { struct media_interface intf;
- /* Should match the fields at media_v2_intf_devnode */ u32 major; u32 minor;
}; diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h index a1bd7afba110..b17f6763aff4 100644 --- a/include/uapi/linux/media.h +++ b/include/uapi/linux/media.h @@ -206,6 +206,10 @@ struct media_pad_desc { #define MEDIA_LNK_FL_IMMUTABLE (1 << 1) #define MEDIA_LNK_FL_DYNAMIC (1 << 2)
+#define MEDIA_LNK_FL_LINK_TYPE (0xf << 28) +# define MEDIA_LNK_FL_DATA_LINK (0 << 28) +# define MEDIA_LNK_FL_INTERFACE_LINK (1 << 28)
How about removing the extra spaces between # and define? Or were they intended?
They're intended. This is a common way to tell that the two defines below are related to the first one.
Sorry, I didn't see the other comments. See below.
struct media_link_desc { struct media_pad_desc source; struct media_pad_desc sink; @@ -249,11 +253,93 @@ struct media_links_enum { #define MEDIA_INTF_T_ALSA_RAWMIDI (MEDIA_INTF_T_ALSA_BASE + 4) #define MEDIA_INTF_T_ALSA_HWDEP (MEDIA_INTF_T_ALSA_BASE + 5)
-/* TBD: declare the structs needed for the new G_TOPOLOGY ioctl */ +/*
- MC next gen API definitions
- NOTE: The declarations below are close to the MC RFC for the Media
Controller, the next generation. Yet, there are a few adjustments
to do, as we want to be able to have a functional API before
the MC properties change. Those will be properly marked below.
Please also notice that I removed "num_pads", "num_links",
from the proposal, as a proper userspace application will likely
use lists for pads/links, just as we intend to do in Kernelspace.
The API definition should be freed from fields that are bound to
some specific data structure.
- FIXME: Currently, I opted to name the new types as "media_v2", as this
won't cause any conflict with the Kernelspace namespace, nor with
the previous kAPI media_*_desc namespace. This can be changed
later, before the adding this API upstream.- */
+struct media_v2_entity {
- __u32 id;
- char name[64]; /* FIXME: move to a property? (RFC says so) */
That indeed requires the property API. We can't have it this week or next (and perhaps not next month either). Shall we wait, use a temporary field (and remind the user that the API is experimental) or go without name (which is rather important)?
I think clearly labelling this experimental would be the thing to do, and then after some time remove the name field. Albeit historically APIs marked clearly as experimental have been used by applications and then fixing those APIs has been no-go since the applications already use them. Oh, well...
I added the FIXME due to a purpose: IMHO, despite the discussions we had at the MC summit, I think we should keep the name at the main struct.
The rationale is that, unlikely other properties that are typically optional, the name is mandatory, as we want all entities to have a name. By having it at the struct, we'll be sure about that. Ok, the drawback is that we'll waste space at the ioctl, so, I'm not 100% sure about that.
In any case, we should address this before having the API at a released Kernel.
I mean: we need to decide if the entity name will be at a property or at struct media_v2_entity. If we decide to put it on a property, we should just remove the name there and comment the part of the mc_nextgen_test that uses it, uncommenting (and changing accordingly) only when we merge the properties API.
Otherwise, we should remove the /* FIXME */ from it.
Comments?
I think the name should be visible as a property independently of whether it's part of the struct or not. I have no objections to putting it to the struct, however.
If you do need the name in the user space using this API which I believe you do, having no name field would essentially make the set depend on the property API which will take quite some time to design in detail level, implement and agree on.
- __u16 reserved[14];
How about 32-bit fields instead? I think the only reason to have 16-bit reserved fields is to align 16-bit aligned fields to higher alignment, but this is not the case here.
That came from hverkuil's RFC/suggestion. I don't have any strong opinion about that.
So should be __32's instead.
+};
+/* Should match the specific fields at media_intf_devnode */ +struct media_v2_intf_devnode {
- __u32 major;
- __u32 minor;
+};
+struct media_v2_interface {
- __u32 id;
- __u32 intf_type;
- __u32 flags;
- __u32 reserved[9];
- union {
struct media_v2_intf_devnode devnode;__u32 raw[16];- };
+};
+struct media_v2_pad {
- __u32 id;
- __u32 entity_id;
- __u32 flags;
- __u16 reserved[9];
Any particular reason for an odd number of 16-bit reserved fields? :-)
Ask Hans. I just used his RFC design here ;)
+};
+struct media_v2_link {
- __u32 id;
- __u32 source_id;
- __u32 sink_id;
- __u32 flags;
- __u32 reserved[5];
+};
+struct media_v2_topology {
- __u32 topology_version;
- __u32 num_entities;
- struct media_v2_entity *entities;
- __u32 num_interfaces;
- struct media_v2_interface *interfaces;
If you added reserved fields to align the pointers to 64 bits we'd get away with some error-prone compat code.
Maybe. It seems that arm64 has some differences with regards to struct aligns. I didn't analyze yet what are the arch differences, but I suspect that we may need to add some compat code anyway.
I'm open to suggestions.
These should also have the __user modifier.
I have to check, but I guess __user should not be used at the struct definition at the .h, but when using it at media_device.c.
If you have a pointer in a struct that's used in a user space header, the __user should be there AFAIU. It's stripped out by make headers_install anyway.