[PATCH_RFC_v1 0/4] MC initial changes due to MC workshop

[PATCH_RFC_v1 0/4] MC initial changes due to MC workshop

[Mauro Carvalho Chehab]

This is an initial set of patches showing the approach I'm taking
in order to fulfill all the MC needs that was discussed on the 3
day MC summit in Helsinki.

As suggested, I'll be sending incremental patches and avoiding to
do a large set of changes into one changeset series.

So, this is the first 4 patches of the changes.

Along this patch series, I'll be calling "object" as anything "symbol"
is part of the media graph. Currently: entity, link and pads.

New object types will be needed to represent interfaces. I suspect
we'll also need another type to represent group of objects, but this
will be covered on future patches.

No userspace API changes here, just changes at the internal structs
that contains the media graph objects and some new helper functions.

The goal of this patchset is:

1) to create a common struct that will be embedded on all internal
   structs that represents the comon data that will be used by
   all kinds of objects;

2) to have an unique object ID for each object in the graph. The
   object ID will be needed by the userspace API in the future, as
   discussed during the MC workshop;

3) to be sure that no data used by an entity will be freed too
   early. Latter patches will do similar changes to links and pads.

With regards to (3), this patchset is not complete yet; it currently
changes only the DVB core, as the changes are simpler there.
I'll be working on a similar change for the V4L2 core. Yet, it would
be nice to have a feedback earlier in order to avoid rework.

Mauro Carvalho Chehab (4):
  media: Add some fields to store graph objects
  media: Add a common embeed struct for all media graph objects
  media: add functions to create/remove entities
  dvbdev: Use functions to create/remove media_entity struct

 drivers/media/dvb-core/dvbdev.c | 13 +++---
 drivers/media/media-device.c    |  4 ++
 drivers/media/media-entity.c    | 99 +++++++++++++++++++++++++++++++++++++++++
 include/media/media-device.h    |  4 ++
 include/media/media-entity.h    | 58 ++++++++++++++++++++++++
 5 files changed, 172 insertions(+), 6 deletions(-)

-- 
2.4.3

[PATCH_RFC_v1 1/4] media: Add some fields to store graph objects

[Mauro Carvalho Chehab]

We'll need unique IDs for graph objects and a way to associate
them with the media interface.

So, add an atomic var to be used to create unique IDs and
a list to store such objects.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 7b39440192d6..e627b0b905ad 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -396,6 +396,10 @@ int __must_check __media_device_register(struct media_device *mdev,
 		return ret;
 	}
 
+	/* Initialize media graph object list and ID */
+	atomic_set(&mdev->last_obj_id, 0);
+	INIT_LIST_HEAD(&mdev->object_list);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__media_device_register);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 6e6db78f1ee2..a9d546716e49 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -78,6 +78,10 @@ struct media_device {
 
 	int (*link_notify)(struct media_link *link, u32 flags,
 			   unsigned int notification);
+
+	/* Used by media_graph stuff */
+	atomic_t last_obj_id;
+	struct list_head object_list;
 };
 
 /* Supported link_notify @notification values. */
-- 
2.4.3

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

[Mauro Carvalho Chehab]

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;
+	struct media_device	*mdev;
+	struct kref		kref;
+};
+
 
 struct media_pipeline {
 };
 
 struct media_link {
+	struct media_graph_obj			graph_obj;
 	struct media_pad *source;	/* Source pad */
 	struct media_pad *sink;		/* Sink pad  */
 	struct media_link *reverse;	/* Link in the reverse direction */
@@ -39,6 +82,7 @@ struct media_link {
 };
 
 struct media_pad {
+	struct media_graph_obj			graph_obj;
 	struct media_entity *entity;	/* Entity this pad belongs to */
 	u16 index;			/* Pad index in the entity pads array */
 	unsigned long flags;		/* Pad flags (MEDIA_PAD_FL_*) */
@@ -61,6 +105,7 @@ struct media_entity_operations {
 };
 
 struct media_entity {
+	struct media_graph_obj			graph_obj;
 	struct list_head list;
 	struct media_device *parent;	/* Media device this entity belongs to*/
 	u32 id;				/* Entity ID, unique in the parent media
-- 
2.4.3

[PATCH_RFC_v1 3/4] media: add functions to create/remove entities

[Mauro Carvalho Chehab]

In order to be able to safely create/remove entities
dynamically, we need to use krefs in a way that the
entity memory will only be freed when nobody is using
it anymore.

So, instead of just using kmalloc/kfree, we need to map
those into two functions that will use krefs to protect
memory.

Of course, we need to change all drivers to use the new way,
but they should keep work without such change.
So, let's postpone the driver changes to a separate patch,
in order to be easier for review.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 4d8e01c7b1b2..d6ad6c3fe800 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -27,6 +27,105 @@
 #include <media/media-device.h>
 
 /**
+ *  graph_obj_init - Initialize a graph object
+ *
+ * @mdev:	Pointer to the media_device that contains the object
+ * @type:	Type of the object
+ * @gobj:	Pointer to the object
+*/
+static void graph_obj_init(struct media_device *mdev,
+			   enum media_graph_type type,
+			   struct media_graph_obj *gobj)
+{
+	INIT_LIST_HEAD(&gobj->list);
+
+	list_add_tail(&gobj->list, &mdev->object_list);
+	gobj->obj_id = atomic_inc_return(&mdev->last_obj_id);
+	gobj->type = type;
+	gobj->mdev = mdev;
+	kref_init(&gobj->kref);
+}
+
+/**
+ *  graph_obj_remove - De-initialize a graph object
+ *
+ * @graph_obj:	Pointer to the object
+*/
+static void graph_obj_remove(struct media_graph_obj *gobj)
+{
+	list_del(&gobj->list);
+}
+
+/**
+ *  media_entity_create - Allocates memory and create a media entity
+ *
+ * @mdev:	Pointer to the media_device that contains the object
+ * @name:	Name of the media entity
+ * @flags:	Flags to be used at the media_entity
+ * @ops:	Media entity operations pointer
+ * @gfp_flags:	Flags to be used by kzalloc. Typically: GFP_KERNEL
+*/
+struct media_entity
+*media_entity_create(struct media_device *mdev,
+		     const char *name,
+		     unsigned long flags,
+		     const struct media_entity_operations *ops,
+		     gfp_t gfp_flags)
+{
+	struct media_entity *entity;
+
+	entity = kzalloc(sizeof(*entity), gfp_flags);
+	if (!entity)
+		return NULL;
+
+	/*
+	 * Let's create a copy of the string here, as we should keep a
+	 * reference for the entity until kref count is zero. So, we can't
+	 * rely that the caller will not be freed earlier.
+	 */
+	entity->name = kstrdup(name, gfp_flags);
+	if (!entity)
+		return NULL;
+
+	entity->flags = flags;
+	entity->ops = ops;
+
+	graph_obj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
+
+	return entity;
+}
+
+/**
+ *  media_entity_free - Frees media_entity memory
+ *
+ * @kref:	Kernel reference pointer
+ *
+ * This routine should never be called directly
+ */
+static void media_entity_free(struct kref *ref)
+{
+	struct media_graph_obj *gobj = kref_to_gobj(ref);
+	struct media_entity *entity = gobj_to_entity(gobj);
+
+	graph_obj_remove(gobj);
+	kfree(entity->name);
+	kfree(entity);
+}
+
+/**
+ *  media_entity_remove - Removes a media_entity reference
+ *
+ * @entity:	Pointer to the media entity that will not be used anymore
+ *
+ * This routine decreases the reference for a media entity. When the
+ * reference count is zero, the memory will be freed.
+ */
+void media_entity_remove(struct media_entity *entity)
+{
+	kref_put(&entity->graph_obj.kref, media_entity_free);
+}
+
+/**
  * media_entity_init - Initialize a media entity
  *
  * @num_pads: Total number of sink and source pads.
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index faead169fe32..5f073a8351c1 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -171,6 +171,19 @@ struct media_entity_graph {
 	int top;
 };
 
+#define kref_to_gobj(r) container_of(r, struct media_graph_obj, kref)
+
+#define gobj_to_entity(gobj) \
+		container_of(gobj, struct media_entity, graph_obj)
+
+struct media_entity
+*media_entity_create(struct media_device *mdev,
+		     const char *name,
+		     unsigned long flags,
+		     const struct media_entity_operations *ops,
+		     gfp_t gfp_flags);
+void media_entity_remove(struct media_entity *entity);
+
 int media_entity_init(struct media_entity *entity, u16 num_pads,
 		struct media_pad *pads, u16 extra_links);
 void media_entity_cleanup(struct media_entity *entity);
-- 
2.4.3

[PATCH_RFC_v1 4/4] dvbdev: Use functions to create/remove media_entity struct

[Mauro Carvalho Chehab]

Instead of using kalloc()/kfree(), use the new functions
media_entity_create()/media_entity_remove(), in order to
have the proper kref counts for the usage of the entity
objects at the media graph.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
index 13bb57f0457f..9e0704118ffc 100644
--- a/drivers/media/dvb-core/dvbdev.c
+++ b/drivers/media/dvb-core/dvbdev.c
@@ -189,13 +189,14 @@ static void dvb_register_media_device(struct dvb_device *dvbdev,
 	if (!dvbdev->adapter->mdev)
 		return;
 
-	dvbdev->entity = kzalloc(sizeof(*dvbdev->entity), GFP_KERNEL);
+	dvbdev->entity = media_entity_create(dvbdev->adapter->mdev,
+					     dvbdev->name, 0, NULL,
+					     GFP_KERNEL);
 	if (!dvbdev->entity)
 		return;
 
 	dvbdev->entity->info.dev.major = DVB_MAJOR;
 	dvbdev->entity->info.dev.minor = minor;
-	dvbdev->entity->name = dvbdev->name;
 
 	switch (type) {
 	case DVB_DEVICE_CA:
@@ -214,7 +215,7 @@ static void dvb_register_media_device(struct dvb_device *dvbdev,
 		dvbdev->pads = kcalloc(npads, sizeof(*dvbdev->pads),
 				       GFP_KERNEL);
 		if (!dvbdev->pads) {
-			kfree(dvbdev->entity);
+			media_entity_remove(dvbdev->entity);
 			return;
 		}
 	}
@@ -243,7 +244,7 @@ static void dvb_register_media_device(struct dvb_device *dvbdev,
 		dvbdev->entity->type = MEDIA_ENT_T_DEVNODE_DVB_NET;
 		break;
 	default:
-		kfree(dvbdev->entity);
+		media_entity_remove(dvbdev->entity);
 		dvbdev->entity = NULL;
 		return;
 	}
@@ -258,7 +259,7 @@ static void dvb_register_media_device(struct dvb_device *dvbdev,
 			"%s: media_device_register_entity failed for %s\n",
 			__func__, dvbdev->entity->name);
 		kfree(dvbdev->pads);
-		kfree(dvbdev->entity);
+		media_entity_remove(dvbdev->entity);
 		dvbdev->entity = NULL;
 		return;
 	}
@@ -369,7 +370,7 @@ void dvb_unregister_device(struct dvb_device *dvbdev)
 #if defined(CONFIG_MEDIA_CONTROLLER_DVB)
 	if (dvbdev->entity) {
 		media_device_unregister_entity(dvbdev->entity);
-		kfree(dvbdev->entity);
+		media_entity_remove(dvbdev->entity);
 		kfree(dvbdev->pads);
 	}
 #endif
-- 
2.4.3

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

[Hans Verkuil]

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.

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. It also avoid the need for a connector flag since that would be
a distinct type (although using the same media_entity struct).

This is not a blocker for me, but I'd like to hear what others think.

Otherwise this patch series looks OK.

Regards,

	Hans

> +	struct media_device	*mdev;
> +	struct kref		kref;
> +};
> +
>  
>  struct media_pipeline {
>  };
>  
>  struct media_link {
> +	struct media_graph_obj			graph_obj;
>  	struct media_pad *source;	/* Source pad */
>  	struct media_pad *sink;		/* Sink pad  */
>  	struct media_link *reverse;	/* Link in the reverse direction */
> @@ -39,6 +82,7 @@ struct media_link {
>  };
>  
>  struct media_pad {
> +	struct media_graph_obj			graph_obj;
>  	struct media_entity *entity;	/* Entity this pad belongs to */
>  	u16 index;			/* Pad index in the entity pads array */
>  	unsigned long flags;		/* Pad flags (MEDIA_PAD_FL_*) */
> @@ -61,6 +105,7 @@ struct media_entity_operations {
>  };
>  
>  struct media_entity {
> +	struct media_graph_obj			graph_obj;
>  	struct list_head list;
>  	struct media_device *parent;	/* Media device this entity belongs to*/
>  	u32 id;				/* Entity ID, unique in the parent media
> 

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

[Mauro Carvalho Chehab]

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

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

[Hans Verkuil]

On 08/04/2015 03:26 PM, Mauro Carvalho Chehab wrote:
> 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...

I don't really see why it would be more complex, to be honest.

> 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.

My main concern is that in the new media_link struct we currently have the
two object IDs that are linked, but both userspace and kernelspace needs to
know the type of those objects as well. By encoding the type in the ID this
is O(1) to retrieve. If you have to look it up it will be O(log N) where N
is the number of objects. I really would like to avoid that, esp. when we
get audio graphs with 4000 objects or more.

So I think it should either be encoded in the ID, or it should be a separate
type field in the media_link struct (I'm talking about the userspace API here).
Either is fine by me, although I have a preference for encoding it in the ID
since it is more compact.

Regards,

	Hans

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

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

[Mauro Carvalho Chehab]

Em Wed, 05 Aug 2015 13:27:03 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> 
> 
> On 08/04/2015 03:26 PM, Mauro Carvalho Chehab wrote:
> > 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...
> 
> I don't really see why it would be more complex, to be honest.

Instead of one monotonic counter, we'll need one per type. Plus, if
we consider a device that would be turned on by a long time and that
has lots of dynamic changes, the code will have to handle with
the possibility of a counter overflow, and will need to check if a
given object ID was used already or not. It would also need to have
a switch at the code that gets the object id, in order to use the
correct one, according with the object type.

I'm not saying we should not do it. Just saying that there are already
too many changes for us to spend more time on this ATM. 

Let's do such change at by the end, if we still find need for it.

> > 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.
> 
> My main concern is that in the new media_link struct we currently have the
> two object IDs that are linked, but both userspace and kernelspace needs to
> know the type of those objects as well. By encoding the type in the ID this
> is O(1) to retrieve. If you have to look it up it will be O(log N) where N
> is the number of objects. I really would like to avoid that, esp. when we
> get audio graphs with 4000 objects or more.

I'm not seeing O(log N), if we do:
	struct media_foo {
		u32 id;
		u32 type;
		...
	}

instead of:
	struct media_foo {
		u32 type_id;
		...
	}

The only bad effect is to spend an additional 32 bits per object. Ok, for
4000 objects, that would mean 4 pages (if PAGE_SIZE is 4096) that could be
otherwise be freed. We may end by doing that anyway to save those 4 pages,
but let's focus first on adding the functionality. Then, we can optimize
it.

> So I think it should either be encoded in the ID, or it should be a separate
> type field in the media_link struct (I'm talking about the userspace API here).
> Either is fine by me, although I have a preference for encoding it in the ID
> since it is more compact.

Ah, you were thinking on just using:

	struct media_foo {
		u32 id;
	}

Without any type. Yeah, this would be O(log N) for a binary search.
Userspace might optimize it further using hash tables, if they need
to handle graphs with 4000+ objects. Well, actually for 4000 objects,
that would mean 12 interactions on a binary search. Not enough to
justify a more complex model, and probably not enough to hurt badly
userspace if it needs to do it.

> 
> Regards,
> 
> 	Hans
> 
> > 
> > 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
> >