Hi,
As scheduled on IRC, we'll have a followup discussion today in order to address some details that weren't discussed at the MC workshop, and popped up during the last patch series and its review.
Items for discussion on today's meeting:
1) Links namespace:
From my answer to a Hans review:
> > +#define MEDIA_NEW_LNK_FL_ENABLED MEDIA_LNK_FL_ENABLED > > +#define MEDIA_NEW_LNK_FL_IMMUTABLE MEDIA_LNK_FL_IMMUTABLE > > +#define MEDIA_NEW_LNK_FL_DYNAMIC MEDIA_NEW_FL_DYNAMIC > > +#define MEDIA_NEW_LNK_FL_INTERFACE_LINK (1 << 3)
Yes, this can be deduced from the type of the objects inside the link.
I guess I added it because of some comment from your media.h RFC proposal.
Right now, I'm using it at the application to better represent the graph elements:
http://git.linuxtv.org/cgit.cgi/mchehab/experimental-v4l-utils.git/tree/cont... http://git.linuxtv.org/cgit.cgi/mchehab/experimental-v4l-utils.git/tree/cont...
But it could, instead, be doing something like:
if media_type(link->gobj1.id == MEDIA_GRAPH_PAD) link_is_pad = true; else link_is_pad = false;
Btw, I'm using the same type for both data and interface links, as I don't see any reason why to differentiate internally: they all share the same linked list at mdev and the same object ID range.
There are two questions here:
1) should we have a flag to distinguish interface links? Not really required, but it makes a little easier for userspace and doesn't consume more than a bit.
2) should we create a new namespace for the V2 API, or should we keep the same namespace?
2) Entities namespace:
From Hans review:
> > Entities should do something, and just saying 'V4L2_VIDEO' doesn't really convey > that meaning. It is also very easy to confuse with INTF_T_V4L_* types. BTW, we > should decide whether V4L2 or V4L is used here (interfaces now use V4L, entities > V4L2). Since entities already use V4L2, I think the interface defines should > use V4L2 as well.
Yes, agreed. We actually need to discuss a little more about namespacing and do some additional renaming stuff.
For example, calling a tuner entity as MEDIA_ENT_T_V4L2_SUBDEV_TUNER is wrong, because the tuner can be used only at the DVB side.
So, both V4L2 and SUBDEV prefixes there are wrong. Yet, this should be under the V4L2_SUBDEV range to avoid breaking userspace.
> > > + > > +/* V4L2 Sub-device entities */ > > +#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 1) > > +#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 2) > > +#define MEDIA_ENT_T_V4L2_SUBDEV_LENS (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 3) > > + /* A converter of analogue video to its digital representation. */ > > +#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 4) > > + /* Tuner entity is actually both V4L2 and DVB subdev */ > > +#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 5) > > + > > +/* DVB entities */ > > +#define MEDIA_ENT_T_DVB_DEMOD (MEDIA_ENT_T_DVB_BASE) > > After changing DVB_BASE to 0, change this to DVB_BASE + 1, and adjust the other DVB > entity types accordingly. > > This keeps the base defines consistent (i.e. the lower 16 bits are always 0). > > It surprised me when reading this patch, so I'm probably not the only one.
This is another thing for discussion: keeping the MEDIA_ENT_T_foo_BASE unused opens space for API abuse.
There are several entities at OMAP3 driver, for example, that keeps entity->type undefined. So, they got whatever is the default at v4l2-device.c (before this series: MEDIA_ENT_T_V4L2_SUBDEV, after that: MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN):
$ media-ctl --print-t|grep -B1 Unknown - entity 1: OMAP3 ISP CCP2 (2 pads, 2 links) type V4L2 subdev subtype Unknown flags 0 -- - entity 3: OMAP3 ISP CSI2a (2 pads, 2 links) type V4L2 subdev subtype Unknown flags 0 -- - entity 5: OMAP3 ISP CCDC (3 pads, 8 links) type V4L2 subdev subtype Unknown flags 0 -- - entity 7: OMAP3 ISP preview (2 pads, 4 links) type V4L2 subdev subtype Unknown flags 0 -- - entity 10: OMAP3 ISP resizer (2 pads, 4 links) type V4L2 subdev subtype Unknown flags 0 -- - entity 13: OMAP3 ISP AEWB (1 pad, 1 link) type V4L2 subdev subtype Unknown flags 0 -- - entity 14: OMAP3 ISP AF (1 pad, 1 link) type V4L2 subdev subtype Unknown flags 0 -- - entity 15: OMAP3 ISP histogram (1 pad, 1 link) type V4L2 subdev subtype Unknown flags 0
I guess all the above entities are processing units, so they should have, instead, some type like: MEDIA_ENT_T_V4L2_SUBDEV_PROCESSING
Or, even some of the above would actually deserves to have an specific type, like: MEDIA_ENT_T_V4L2_SUBDEV_HISTOGRAM MEDIA_ENT_T_V4L2_SUBDEV_RESIZER ...
Let's try to find some time to discuss the entities namespace on IRC during this week.
In summary, now that we've split interfaces from entities, MEDIA_ENT_T_V4L2_SUBDEV_* namespace is messy/wrong because:
- entities should represent hardware, and not Linux implementation. So, calling them as "V4L2" is wrong, because V4L2 is not a hardware block.
- entities are always part of a device. So, calling them as SUBDEV is also wrong.
- some entities like a tuner can be used outside V4L2 (and even outside drivers/media).
The current patch series removes the type/subtype usage inside the Kernelspace, allowing further patches to fix that mess. Yet, I opted to keep the messy/wrong namespaces there for now, as fixing this can be done on a separate patch series, and won't interfere at the implementation of the new G_TOPOLOGY ioctl or at the implementation of the properties API. However, we should fix this earlier than latter.
Back on May, 7 I did a proposal on a 18 patches series: http://www.spinics.net/lists/linux-media/msg89521.html This is a little outdated, but the basic idea there was to split entities by their function. So, we would have things like:
MEDIA_ENT_T_CAMERA_foo - for camera stuff (sensor, flash, ...) MEDIA_ENT_T_RF_foo - for things like tuner, LNBf, LNA MEDIA_ENT_T_DTV_foo - for digital TV stuff MEDIA_ENT_T_ATV_foo - for analog TV stuff MEDIA_ENT_T_MEDIA_foo - for common media blocks (scaler, resizer)
On my patch series, I actually created numberspace bases, just to keep things easier to maintain:
#define MEDIA_ENT_T_DVB_BASE 0x00000000 #define MEDIA_ENT_T_V4L2_BASE 0x00010000 #define MEDIA_ENT_T_V4L2_SUBDEV_BASE 0x00020000 #define MEDIA_ENT_T_CONNECTOR_BASE 0x00030000
but, after thinking a little bit more about that, I think that this may not be a good idea, as userspace programs should not rely on it, and the things at MEDIA_ENT_T_V4L2_SUBDEV_BASE are under that "messy" state:
#define MEDIA_ENT_T_V4L2_SUBDEV_UNKNOWN MEDIA_ENT_T_V4L2_SUBDEV_BASE
#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 1) #define MEDIA_ENT_T_V4L2_SUBDEV_FLASH (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 2) #define MEDIA_ENT_T_V4L2_SUBDEV_LENS (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 3) #define MEDIA_ENT_T_V4L2_SUBDEV_DECODER (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 4) #define MEDIA_ENT_T_V4L2_SUBDEV_TUNER (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 5)
on the above: - SENSOR/FLASH/LENS are camera related entities - DECODER is an analog TV entity - TUNER is a RF entity shared with analog, digital, software radio and, in the future could even be used on wifi drivers, if we ever need the media controller to be used there
A side discussion that we can do if we have time is how to fix those "type V4L2 subdev subtype Unknown" subdevs at OMAP3 (and likely other drivers) that are just putting all non-camera subdevices under a generic umbrella (MEDIA_ENT_T_V4L2_SUBDEV).
Regards, Mauro