Re: [RFC] Media Controller, the next generation

Em Mon, 17 Aug 2015 15:21:22 +0200
Hans Verkuil [off-list ref] escreveu:

On 08/17/2015 02:39 PM, Mauro Carvalho Chehab wrote:

quoted

Em Mon, 17 Aug 2015 12:33:30 +0200
Hans Verkuil [off-list ref] escreveu:

quoted

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.

quoted

quoted

Using graph_ instead of mc_ is something I would be OK with as well.

What about graph_ as the internal prefix? Or mctl_?

quoted

quoted

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.

quoted

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);

quoted

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.

quoted

quoted

quoted

};

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?

quoted

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.

quoted

	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.

quoted

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.

quoted

quoted

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.

quoted

 	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;

quoted

Of course, length is needed, in order to avoid filling it with reserved
stuff.

quoted

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.

quoted

quoted

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