Re: [PATCH_RFC_v1 2/4] media: Add a common embeed struct for all media graph objects

[Mauro Carvalho Chehab <mchehab@osg.samsung.com>]

Em Tue, 04 Aug 2015 14:26:04 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> 
> 
> On 08/04/2015 01:41 PM, Mauro Carvalho Chehab wrote:
> > Due to the MC API proposed changes, we'll need to:
> > 	- have an unique object ID for all graph objects;
> > 	- be able to dynamically create/remove objects;
> > 	- be able to group objects;
> > 	- keep the object in memory until we stop use it.
> > 
> > Due to that, create a struct media_graph_obj and put there the
> > common elements that all media objects will have in common.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > 
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 0c003d817493..faead169fe32 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -27,11 +27,54 @@
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> >  #include <linux/media.h>
> > +#include <linux/kref.h>
> > +
> > +/* Enums used internally at the media controller to represent graphs */
> > +
> > +/**
> > + * enum media_graph_type - type of a graph element
> > + *
> > + * @MEDIA_GRAPH_ENTITY:		Identify a media entity
> > + * @MEDIA_GRAPH_PAD:		Identify a media PAD
> > + * @MEDIA_GRAPH_LINK:		Identify a media link
> > + */
> > +enum media_graph_type {
> > +	MEDIA_GRAPH_ENTITY,
> > +	MEDIA_GRAPH_PAD,
> > +	MEDIA_GRAPH_LINK,
> > +};
> > +
> > +
> > +/* Structs to represent the objects that belong to a media graph */
> > +
> > +/**
> > + * struct media_graph_obj - Define a graph object.
> > + *
> > + * @list:		List of media graph objects
> > + * @obj_id:		Non-zero object ID identifier. The ID should be unique
> > + *			inside a media_device
> > + * @type:		Type of the graph object
> > + * @mdev:		Media device that contains the object
> > + * @kref:		pointer to struct kref, used to avoid destroying the
> > + *			object before stopping using it
> > + *
> > + * All elements on the media graph should have this struct embedded
> > + */
> > +struct media_graph_obj {
> > +	struct list_head	list;
> > +	struct list_head	group;
> > +	u32			obj_id;
> > +	enum media_graph_type	type;
> 
> I think that using the top 8 bits of the ID for the type and the lower 24 bits for
> the ID is more efficient. I proposed this in my RFC.

Oh, somehow I missed it. I'll read and comment it in a few.
> 
> The IDs are still unique, but you don't need to keep track of the type since that's
> encoded in the ID. You'd need an array of MAX_TYPE atomics to get per-type unique IDs,
> but that's trivial. 

That could be done, but I'm not sure about that. The code becomes a
little more complex. The interface will also be more complex. We're
already having enough complexity without that...

So, I would prefer to keep this in separate, at least for now. We
can latter change it (of course before finishing the final interface).
This is not a blocker to me, though.

Maybe, for now, I can add one define to get object type. This way, we can
easily replace the code latter to merge both ID and types into one u32. 
Something like:

#define obj_type(gobj)	((gobj)->type)

> It also avoid the need for a connector flag since that would be
> a distinct type (although using the same media_entity struct).

Well, this can be done anyway, no matter if we encap type/ID into a
single u32 or not.

For me, a connector entity makes sense. It also makes sense, at least
at Kernel level, to have more than one interface type, but I'll discuss
that latter on, when I add the devnode interface at a new patch series.

> This is not a blocker for me, but I'd like to hear what others think.
> 
> Otherwise this patch series looks OK.

Thanks for review!
Mauro