Em Mon, 17 Aug 2015 15:21:22 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> On 08/17/2015 02:39 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 17 Aug 2015 12:33:30 +0200
> > Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> >> That's not what I meant. I meant using the mc_ prefix instead of the media_
> >> prefix for internal structs. So media_entity in the public API would map to
> >> mc_entity in the internal API. Ditto for media_interface/mc_interface,
> >> media_pad/mc_pad, etc.
> >
> > Ah! that could work. Yet, I guess we decided to not use mc_ internally
> > in the first place several years ago because there was already some
> > namespace conflict.
> >
> > Ah, yes:
> >
> > $ git grep -e \\bmc_
> > ...
> > arch/powerpc/kernel/mce.c: struct machine_check_event *mc_evt;
> > arch/powerpc/kernel/mce.c: mc_evt = this_cpu_ptr(&mce_event[index]);
> > arch/powerpc/kernel/mce.c: *mce = *mc_evt;
> > ...
> > arch/um/drivers/line.h: struct mc_device mc;
> > ...
> > arch/um/drivers/net_kern.c:static struct mc_device net_mc = {
> > ...
> >
> > A total of 302 references... Among them, some arch-dependent stuff,
> > including x86 and powerPC error hanlding stuff (MCE, APEI).
> >
> > I would avoid it, as the risk of namespace collisions are high.
>
> OK.
>
> >
> >> Using graph_ instead of mc_ is something I would be OK with as well.
>
> What about graph_ as the internal prefix? Or mctl_?
>
> >> I am a bit worried about this. To my knowledge applications that use the MC
> >> today are expected to know which pad of an entity does what, and it identifies
> >> the pad by index.
> >>
> >> The new public API should still provide applications with this information in
> >> one way or another. The pad ID won't work, certainly not in the dynamic case,
> >> the PAD_TYPE as suggested here will only work as long as there is only one
> >> pad per type. Suppose there is an entity with two output pads, both for video?
> >> One might be SDTV, one HDTV. How to tell the difference?
> >
> > The first one will have "SDTV" as type, the other one "HDTV". So, no problem
> > with that.
>
> So now we have SDTV and HDTV, the next entity will have YUV_420 and YUV_422 pads,
> or whatever. The number of types will quickly escalate. It's similar to the
> 'entity type' problem (e.g. sensor, flash, video encoder, scaler, etc.).
well, as new pads will require new Kernel patches, nothing prevents to add
a new #define. A string would also work.
> >
> > There are, however, some usages where an index is enough. For example, the
> > hardware demux filter outputs can fully be identified by just an index, and
> > each index could (ideally) be mapped into a different DMA.
> >
> > So, I guess we need to offer the flexibility to keep using indexes where
> > it makes sense.
> >
> > That's why I proposed a find_pad function that would allow to use an
> > index:
> > media_pad *find_pad(struct media_entity entity, enum pad_usage usage, int idx);
> > or
> > media_pad *find_pad(struct media_entity entity, char *pad_name);
> >
> >> One option might be to have a pad_type or data_type for basic type information
> >> to have generic code that just wants to find VBI/VIDEO/AUDIO streaming pads,
> >> and a name[32] field that is uniquely identifying the pad and that can be used
> >> in userspace applications (or drivers for that matter by adding a find_pad_name()).
> >
> > just a name could be enough:
> > vbi_pad = find_pad(entity, "vbi");
> >
> > However, that would require to have a strict namespace if we want the core
> > to rely on it.
>
> I think names would work: we can make a number of 'core' names that should be
> used where appropriate, and leave it up to the driver otherwise.
OK.
> So if there is only one video source pad, then call the pad "video". If there
> are multiple video source pads, then zero or one pads can be called "video",
> the others should get different names "video-sdtv").
>
> To be honest, I'm not terribly keen on this. I think a data/pad type + name
> would work better.
Me too.
> >>> };
> >>>
> >>> And have the structs with (some) properties embed on it, like:
> >>>
> >>> struct media_graph_entity {
> >>> struct media_graph_object obj;
> >>
> >> This doesn't make sense: struct media_graph_topology would fill in an
> >> array of struct media_graph_object would point to media objects that
> >> in turn contain a struct media_graph_object.
> >
> > Sorry, I didn't get your idea. Could you please give a C code example on
> > what you're thinking?
> >
> >>
> >> I wouldn't embed a struct media_graph_object in these structs, there
> >> is no point to that. Thinking about this some more you don't need the
> >> length field either, only an id (this gives you the type, so you know
> >> whether the object_data pointer is for a media_entity or a media_pad
> >> or whatever) and the pointer. Strictly speaking you would only need
> >> the 'type' part of the ID, but I see no reason not to fill in the full
> >> ID.
> >>
> >>> u32 flags;
> >>> /* ... */
> >>> char name[64];
> >>> };
> >>>
> >>> Please notice that we don't need to add reserved fields at the structs,
> >>> as we're now putting the struct length at the media_graph_object.
> >>>
> >>> So, if we need to, for example, add a new "foo" inside the
> >>> media_graph_entity:
> >>>
> >>> struct media_graph_entity {
> >>> struct media_graph_object obj;
> >>> u32 flags;
> >>> /* ... */
> >>> char name[64];
> >>> u32 foo;
> >>> };
> >>
> >> No, you can't. Say you've compiled the application with a header that includes
> >> the foo field, and then you run the same application with an older kernel that
> >> doesn't have the foo field. Any access to foo would give garbage back (or fail).
> >>
> >> You really need those reserved fields. The alternative would be to mess around
> >> with different struct versions, and that's painful.
> >
> > No. The length will do that. All we need to do is to document that the data
> > should be zeroed on userspace before filling the data from G_TOPOLOGY.
> >
> > For example:
> >
> > let's say that, on Kernel 4.3: this is declared as:
> > struct media_graph_entity {
> > struct media_graph_object obj;
> > u32 flags;
> > char name[64];
> > };
> >
> > And, on Kernel 4.4, the new "foo" field got introduced:
> >
> > struct media_graph_entity {
> > struct media_graph_object obj;
> > u32 flags;
> > char name[64];
> > u32 foo;
> > };
> >
> > Of course, just like we do with reserved fields, we should ensure that
> > old apps can live without the "foo" field, and that new apps will
> > keep working with older Kernels.
> >
> > Now, we have:
> >
> > The length of the data on 4.3 is: sizeof(obj) + 68
> > The length of the data on 4.4 is: sizeof(obj) + 72
> >
> > As per my proposal, we need to pass the length for the public API
> > object.
> >
> > struct media_graph_object {
> > u32 length;
> > u32 type;
> > u32 id;
> > void *data;
> > } __attribute__ ((packed));
> >
> > An application for the G_TOPOLOGY would do something like:
> >
> > /* p is the pointer for the data returned from Kernel */
> > struct media_graph_entity
> > get_an_entity_graph_obj(struct media_graph_obj *obj)
> > {
> > struct media_graph_entity *ent;
> > int len;
> >
> > len = sizeof(*ent);
> > if (len > obj->length)
> > len = obj->length;
> >
> > ent = calloc(len, 1);
> > memcpy (ent, obj->data, len);
> >
> > return ent;
> > }
>
> OK, so everything has to be copied in userspace, unpacking the objects.
Well, that's one way, to just quickly express what I meant to say ;)
It is possible to avoid that using other data representations on
userspace, like having a get_field() macro that would return zero
if trying to access past the length.
> >
> > There are three cases:
> >
> > 1) both app and Kernel are compiled against the same headers
> >
> > No problem.
> >
> > 2) app is compiled against v4.4 headers
> > kernel compiled against v4.3 headers
> >
> > As the struct were zeroed before memcpy, all entities will have foo = 0.
> >
> > No problem.
> >
> > 3) app is compiled against v4.3 headers
> > kernel compiled against v4.4 headers
> >
> > As the struct won't have the "foo" field, and it will be ignored.
> >
> > No problem.
> >
> > So, I don't see any usage to add reserved fields.
> >
> >>
> >>>
> >>> There are some advantages of this approach:
> >>> - If the size of the entity will change, and obj.length will be bigger.
> >>> Userspace will allocate more space to store the object, but will be
> >>> backward compatible;
> >>> - We can add new object types anytime. If userspace doesn't know the new
> >>> type, it should simply discard the object and go to the next one. Again,
> >>> backward compatible.
> >>>
> >>> We may eventually add a way for userspace to request only a subset of
> >>> the graph elements or to add an ioctl or some other sort of event that
> >>> will report topology changes.
> >>
> >> It's an option, but the first 'advantage' doesn't actually work, and I am
> >> not sure about the second. Yes, it is an advantage but it comes at a price:
> >> the void pointer. I very much prefer strict typing.
> >
> > See above.
> >
> > We can avoid the void pointer with something like:
> >
> > struct media_graph_object {
> > u32 length;
> > u32 type;
>
> Type can be dropped.
I guess you meant to say that it can be merged with ID. Yes, agreed.
> > u32 id;
> > } __attribute__ ((packed));
> >
> > struct media_graph_entity {
> > struct media_graph_object obj;
> > char name[64];
> > } __attribute__ ((packed));
>
> This makes more sense without the pointer. But this also means that the
> objects pointer in media_graph_topology isn't an array. Instead the
> next graph object starts at ((void *)objects + objects->length).
Either that or we'll have an array of pointers, or, as suggested during
the MC, offsets that will be converted into pointers with something like:
obj[4] += obj_base;
>
> >
> > Of course, length is needed, in order to avoid filling it with reserved
> > stuff.
> >
> >>
> >> BTW, my initial proposal had a __u32 reserved[64] field, I'd redo that as
> >> follows:
> >>
> >> struct
> >> __u32 num_reserved;
> >> void *ptr_reserved;
> >> } reserved[32];
> >>
> >> This will make 32/64 bit pointer size differences much easier to handle.
> >>
> >> And in my opinion, if we end up with so many different object types, then
> >> we have a much bigger problem and another redesign would be required.
> >
> > Redesign the API because we add more things is crappy! We should do
> > something that, ideally, will never require us to redesign it again.
>
> That's not what I meant. If we end up with so many different object types,
> then the MC API has gone crazy and becomes unusable.
Well, we currently have 80+ ioctls at V4L2, and it is still usable.
> The MC is supposed
> to have just a few types: originally entity, pad, link and now we are adding
> interface and property.
And, from your own suggestions: connectors
> Perhaps there will be one or two more, but if we
> end up with 30 types, then there is something seriously wrong.
I agree that 30 sounds a good number, but the thing is: it is hard to
tell that in advance. As we add support for other subsystems like IIO,
different needs could popup.
For example, if I remember well, on IIO, PADs aren't needed. So we
may end by having some pad-less type of entity to fulfill its
needs.
The thing is: APIs generally start simple, but tend to grow as new
needs happen. See the crop/scaler thing... every time we discuss that,
new proposals to extend the API by creating a new ioctl or a new
type of crop that we didn't foresee in the past discussions.
> >> In general, I prefer strict typing over void pointers, and having to cast
> >> all the time is something I'd really like to avoid. This API also gives
> >> you filtering for free (just leave the relevant pointers NULL).
> >
> > Adding a filter field is not hard, and, as pointed above, we don't need
> > void pointers.
>
> Doing it this way means that you return a blob that needs to be unpacked
> first. It's useless without that unpacking step. Is that what I want? I
> would like to hear what others think about this. It makes this a very
> complicated and hard-to-use ioctl.
Using a single ioctl to pack different things will always require some
unpacking/parsing effort on the other side, whatever approach we take.
That's why I think we should have a way to report topology changes as
well.
>
> Regards,
>
> Hans
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
