[RFC 0/9] Unrestricted media entity ID range support

[RFC 0/9] Unrestricted media entity ID range support

[Sakari Ailus]

Hi all,

This patchset adds an API for managing entity enumerations, i.e. storing a
bit of information per entity. The entity ID is no longer limited to small
integers (e.g. 31 or 63), but MEDIA_ENTITY_MAX_LOW_ID. The drivers are
also converted to use that instead of a fixed number.

If the number of entities in a real use case grows beyond the capabilities
of the approach, the algorithm may be changed. But most importantly, the
API is used to perform the same operation everywhere it's done.

I'm sending this for review only, the code itself is untested.

I haven't entirely made up my mind on the fourth patch. It could be
dropped, as it uses the API for a somewhat different purpose.

-- 
Kind regards,
Sakari

[RFC 1/9] media: Rename MEDIA_ENTITY_ENUM_MAX_ID as MEDIA_ENTITY_MAX_LOW_ID

[Sakari Ailus]

Rename the macro as it no longer is a maximum ID that an entity can have,
but a low ID which is used for internal purposes of enumeration. This is
the maximum number of concurrent entities that may exist in a media
device.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-entity.c | 6 +++---
 include/media/media-entity.h | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 2c984fb..5526e8c 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -332,9 +332,9 @@ void media_entity_graph_walk_start(struct media_entity_graph *graph,
 {
 	graph->top = 0;
 	graph->stack[graph->top].entity = NULL;
-	bitmap_zero(graph->entities, MEDIA_ENTITY_ENUM_MAX_ID);
+	bitmap_zero(graph->entities, MEDIA_ENTITY_MAX_LOW_ID);
 
-	if (WARN_ON(media_entity_id(entity) >= MEDIA_ENTITY_ENUM_MAX_ID))
+	if (WARN_ON(media_entity_id(entity) >= MEDIA_ENTITY_MAX_LOW_ID))
 		return;
 
 	__set_bit(media_entity_id(entity), graph->entities);
@@ -381,7 +381,7 @@ media_entity_graph_walk_next(struct media_entity_graph *graph)
 
 		/* Get the entity in the other end of the link . */
 		next = media_entity_other(entity, link);
-		if (WARN_ON(media_entity_id(next) >= MEDIA_ENTITY_ENUM_MAX_ID))
+		if (WARN_ON(media_entity_id(next) >= MEDIA_ENTITY_MAX_LOW_ID))
 			return NULL;
 
 		/* Has the entity already been visited? */
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 44ab153..bb6383b 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -304,7 +304,7 @@ static inline bool is_media_entity_v4l2_subdev(struct media_entity *entity)
 }
 
 #define MEDIA_ENTITY_ENUM_MAX_DEPTH	16
-#define MEDIA_ENTITY_ENUM_MAX_ID	64
+#define MEDIA_ENTITY_MAX_LOW_ID	64
 
 struct media_entity_graph {
 	struct {
@@ -312,7 +312,7 @@ struct media_entity_graph {
 		struct list_head *link;
 	} stack[MEDIA_ENTITY_ENUM_MAX_DEPTH];
 
-	DECLARE_BITMAP(entities, MEDIA_ENTITY_ENUM_MAX_ID);
+	DECLARE_BITMAP(entities, MEDIA_ENTITY_MAX_LOW_ID);
 	int top;
 };
 
-- 
2.1.0.231.g7484e3b

[RFC 2/9] media: Introduce low_id for media entities

[Sakari Ailus]

A low ID is a unique number specific to a media entity. The number is
guaranteed to be under MEDIA_ENTITY_MAX_LOW_ID.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 14 ++++++++++++++
 include/media/media-device.h |  2 ++
 include/media/media-entity.h |  3 +++
 3 files changed, 19 insertions(+)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 1312e93..dfc5e4a 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -20,6 +20,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#include <linux/bitmap.h>
 #include <linux/compat.h>
 #include <linux/export.h>
 #include <linux/ioctl.h>
@@ -543,6 +544,8 @@ int __must_check __media_device_register(struct media_device *mdev,
 	if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0))
 		return -EINVAL;
 
+	bitmap_zero(mdev->entity_low_id, MEDIA_ENTITY_MAX_LOW_ID);
+
 	INIT_LIST_HEAD(&mdev->entities);
 	INIT_LIST_HEAD(&mdev->interfaces);
 	INIT_LIST_HEAD(&mdev->pads);
@@ -628,6 +631,15 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 	INIT_LIST_HEAD(&entity->links);
 
 	spin_lock(&mdev->lock);
+	entity->low_id = find_first_zero_bit(mdev->entity_low_id,
+					     MEDIA_ENTITY_MAX_LOW_ID);
+	if (entity->low_id == MEDIA_ENTITY_MAX_LOW_ID) {
+		spin_unlock(&mdev->lock);
+		return -ENOSPC;
+	}
+
+	__set_bit(entity->low_id, mdev->entity_low_id);
+
 	/* Initialize media_gobj embedded at the entity */
 	media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
 
@@ -660,6 +672,8 @@ void media_device_unregister_entity(struct media_entity *entity)
 
 	spin_lock(&mdev->lock);
 
+	__clear_bit(entity->low_id, mdev->entity_low_id);
+
 	/* Remove interface links with this entity on it */
 	list_for_each_entry_safe(link, tmp, &mdev->links, graph_obj.list) {
 		if (media_type(link->gobj1) == MEDIA_GRAPH_ENTITY
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 1b12774..732163f 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -47,6 +47,7 @@ struct device;
  * @pad_id:	Unique ID used on the last pad registered
  * @link_id:	Unique ID used on the last link registered
  * @intf_devnode_id: Unique ID used on the last interface devnode registered
+ * @entity_low_id: Allocated low entity IDs
  * @entities:	List of registered entities
  * @interfaces:	List of registered interfaces
  * @pads:	List of registered pads
@@ -82,6 +83,7 @@ struct media_device {
 	u32 pad_id;
 	u32 link_id;
 	u32 intf_devnode_id;
+	DECLARE_BITMAP(entity_low_id, MEDIA_ENTITY_MAX_LOW_ID);
 
 	struct list_head entities;
 	struct list_head interfaces;
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index bb6383b..2c56027 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -159,6 +159,8 @@ struct media_entity_operations {
  * @num_pads:	Number of sink and source pads.
  * @num_links:	Number of existing links, both enabled and disabled.
  * @num_backlinks: Number of backlinks
+ * @low_id:	An unique low entity specific number. The numbers are
+ *		re-used if entities are unregistered or registered again.
  * @pads:	Pads array with the size defined by @num_pads.
  * @links:	Linked list for the data links.
  * @ops:	Entity operations.
@@ -187,6 +189,7 @@ struct media_entity {
 	u16 num_pads;
 	u16 num_links;
 	u16 num_backlinks;
+	u8 low_id;
 
 	struct media_pad *pads;
 	struct list_head links;
-- 
2.1.0.231.g7484e3b

[RFC 3/9] media: Add an API to manage entity enumerations

[Sakari Ailus]

This is useful in e.g. knowing whether certain operations have already
been performed for an entity. The users include the framework itself (for
graph walking) and a number of drivers.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/media/media-entity.h | 45 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 2c56027..17ec205 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -23,7 +23,7 @@
 #ifndef _MEDIA_ENTITY_H
 #define _MEDIA_ENTITY_H
 
-#include <linux/bitops.h>
+#include <linux/bitmap.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
 #include <linux/media.h>
@@ -309,6 +309,49 @@ static inline bool is_media_entity_v4l2_subdev(struct media_entity *entity)
 #define MEDIA_ENTITY_ENUM_MAX_DEPTH	16
 #define MEDIA_ENTITY_MAX_LOW_ID	64
 
+#define DECLARE_MEDIA_ENTITY_ENUM(name)		\
+	DECLARE_BITMAP(name, MEDIA_ENTITY_MAX_LOW_ID)
+
+static inline void media_entity_enum_init(unsigned long *e)
+{
+	bitmap_zero(e, MEDIA_ENTITY_MAX_LOW_ID);
+}
+
+static inline void media_entity_enum_set(unsigned long *e,
+					 struct media_entity *entity)
+{
+	__set_bit(entity->low_id, e);
+}
+
+static inline void media_entity_enum_clear(unsigned long *e,
+					   struct media_entity *entity)
+{
+	__clear_bit(entity->low_id, e);
+}
+
+static inline bool media_entity_enum_test(unsigned long *e,
+					  struct media_entity *entity)
+{
+	return test_bit(entity->low_id, e);
+}
+
+static inline bool media_entity_enum_test_and_set(unsigned long *e,
+						  struct media_entity *entity)
+{
+	return __test_and_set_bit(entity->low_id, e);
+}
+
+static inline bool media_entity_enum_empty(unsigned long *e)
+{
+	return bitmap_empty(e, MEDIA_ENTITY_MAX_LOW_ID);
+}
+
+static inline bool media_entity_enum_intersects(unsigned long *e,
+						unsigned long *f)
+{
+	return bitmap_intersects(e, f, MEDIA_ENTITY_MAX_LOW_ID);
+}
+
 struct media_entity_graph {
 	struct {
 		struct media_entity *entity;
-- 
2.1.0.231.g7484e3b

[RFC 4/9] media: Use media entity enum API for managing low IDs

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-device.c | 15 +++++++--------
 include/media/media-device.h |  2 +-
 include/media/media-entity.h | 12 ++++++++++++
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index dfc5e4a..43d0760 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -20,7 +20,6 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
-#include <linux/bitmap.h>
 #include <linux/compat.h>
 #include <linux/export.h>
 #include <linux/ioctl.h>
@@ -544,7 +543,7 @@ int __must_check __media_device_register(struct media_device *mdev,
 	if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0))
 		return -EINVAL;
 
-	bitmap_zero(mdev->entity_low_id, MEDIA_ENTITY_MAX_LOW_ID);
+	media_entity_enum_init(mdev->entity_low_id);
 
 	INIT_LIST_HEAD(&mdev->entities);
 	INIT_LIST_HEAD(&mdev->interfaces);
@@ -618,6 +617,7 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 					      struct media_entity *entity)
 {
 	unsigned int i;
+	int id;
 
 	if (entity->function == MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN ||
 	    entity->function == MEDIA_ENT_F_UNKNOWN)
@@ -631,14 +631,13 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 	INIT_LIST_HEAD(&entity->links);
 
 	spin_lock(&mdev->lock);
-	entity->low_id = find_first_zero_bit(mdev->entity_low_id,
-					     MEDIA_ENTITY_MAX_LOW_ID);
-	if (entity->low_id == MEDIA_ENTITY_MAX_LOW_ID) {
+	id = media_entity_enum_get_next(mdev->entity_low_id);
+	if (id < 0) {
 		spin_unlock(&mdev->lock);
-		return -ENOSPC;
+		return id;
 	}
 
-	__set_bit(entity->low_id, mdev->entity_low_id);
+	entity->id = id;
 
 	/* Initialize media_gobj embedded at the entity */
 	media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
@@ -672,7 +671,7 @@ void media_device_unregister_entity(struct media_entity *entity)
 
 	spin_lock(&mdev->lock);
 
-	__clear_bit(entity->low_id, mdev->entity_low_id);
+	media_entity_enum_clear(mdev->entity_low_id, entity);
 
 	/* Remove interface links with this entity on it */
 	list_for_each_entry_safe(link, tmp, &mdev->links, graph_obj.list) {
diff --git a/include/media/media-device.h b/include/media/media-device.h
index 732163f..b074ff8 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -83,7 +83,7 @@ struct media_device {
 	u32 pad_id;
 	u32 link_id;
 	u32 intf_devnode_id;
-	DECLARE_BITMAP(entity_low_id, MEDIA_ENTITY_MAX_LOW_ID);
+	DECLARE_MEDIA_ENTITY_ENUM(entity_low_id);
 
 	struct list_head entities;
 	struct list_head interfaces;
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 17ec205..95b1061 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -352,6 +352,18 @@ static inline bool media_entity_enum_intersects(unsigned long *e,
 	return bitmap_intersects(e, f, MEDIA_ENTITY_MAX_LOW_ID);
 }
 
+static inline int media_entity_enum_get_next(unsigned long *e)
+{
+	unsigned int id = find_first_zero_bit(e, MEDIA_ENTITY_MAX_LOW_ID);
+
+	if (id == MEDIA_ENTITY_MAX_LOW_ID)
+		return -ENOSPC;
+
+	__set_bit(id, e);
+
+	return id;
+}
+
 struct media_entity_graph {
 	struct {
 		struct media_entity *entity;
-- 
2.1.0.231.g7484e3b

[RFC 5/9] media: Use entity enums in graph walk

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-entity.c | 10 +++++-----
 include/media/media-entity.h |  2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 5526e8c..a4d6e1b 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -332,12 +332,12 @@ void media_entity_graph_walk_start(struct media_entity_graph *graph,
 {
 	graph->top = 0;
 	graph->stack[graph->top].entity = NULL;
-	bitmap_zero(graph->entities, MEDIA_ENTITY_MAX_LOW_ID);
+	media_entity_enum_init(graph->entities);
 
-	if (WARN_ON(media_entity_id(entity) >= MEDIA_ENTITY_MAX_LOW_ID))
+	if (WARN_ON(entity->low_id >= MEDIA_ENTITY_MAX_LOW_ID))
 		return;
 
-	__set_bit(media_entity_id(entity), graph->entities);
+	media_entity_enum_set(graph->entities, entity);
 	stack_push(graph, entity);
 }
 EXPORT_SYMBOL_GPL(media_entity_graph_walk_start);
@@ -381,11 +381,11 @@ media_entity_graph_walk_next(struct media_entity_graph *graph)
 
 		/* Get the entity in the other end of the link . */
 		next = media_entity_other(entity, link);
-		if (WARN_ON(media_entity_id(next) >= MEDIA_ENTITY_MAX_LOW_ID))
+		if (WARN_ON(entity->low_id >= MEDIA_ENTITY_MAX_LOW_ID))
 			return NULL;
 
 		/* Has the entity already been visited? */
-		if (__test_and_set_bit(media_entity_id(next), graph->entities)) {
+		if (media_entity_enum_test_and_set(graph->entities, next)) {
 			link_top(graph) = link_top(graph)->next;
 			continue;
 		}
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 95b1061..849ec4a 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -370,7 +370,7 @@ struct media_entity_graph {
 		struct list_head *link;
 	} stack[MEDIA_ENTITY_ENUM_MAX_DEPTH];
 
-	DECLARE_BITMAP(entities, MEDIA_ENTITY_MAX_LOW_ID);
+	DECLARE_MEDIA_ENTITY_ENUM(entities);
 	int top;
 };
 
-- 
2.1.0.231.g7484e3b

[RFC 6/9] media: entity: Remove useless WARN_ON()'s

[Sakari Ailus]

The checks for entity ID not reaching too high value the framework could
not handle are now present in the entity registration. It's quite
far-fetched this condition could happen inside the framework, so remove
the WARN_ON()'s.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/media-entity.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index a4d6e1b..e4f0f4a 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -334,9 +334,6 @@ void media_entity_graph_walk_start(struct media_entity_graph *graph,
 	graph->stack[graph->top].entity = NULL;
 	media_entity_enum_init(graph->entities);
 
-	if (WARN_ON(entity->low_id >= MEDIA_ENTITY_MAX_LOW_ID))
-		return;
-
 	media_entity_enum_set(graph->entities, entity);
 	stack_push(graph, entity);
 }
@@ -381,8 +378,6 @@ media_entity_graph_walk_next(struct media_entity_graph *graph)
 
 		/* Get the entity in the other end of the link . */
 		next = media_entity_other(entity, link);
-		if (WARN_ON(entity->low_id >= MEDIA_ENTITY_MAX_LOW_ID))
-			return NULL;
 
 		/* Has the entity already been visited? */
 		if (media_entity_enum_test_and_set(graph->entities, next)) {
-- 
2.1.0.231.g7484e3b

[RFC 7/9] omap3isp: Use media entity enumeration API

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c      | 15 +++++++--------
 drivers/media/platform/omap3isp/isp.h      |  5 +++--
 drivers/media/platform/omap3isp/ispccdc.c  |  2 +-
 drivers/media/platform/omap3isp/ispvideo.c | 11 +++++------
 drivers/media/platform/omap3isp/ispvideo.h |  4 ++--
 5 files changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index cb8ac90..c1ee2e3 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -881,7 +881,7 @@ static int isp_pipeline_enable(struct isp_pipeline *pipe,
 	 * starting entities if the pipeline won't start anyway (those entities
 	 * would then likely fail to stop, making the problem worse).
 	 */
-	if (pipe->entities & isp->crashed)
+	if (media_entity_enum_intersects(pipe->entities, isp->crashed))
 		return -EIO;
 
 	spin_lock_irqsave(&pipe->lock, flags);
@@ -974,7 +974,6 @@ static int isp_pipeline_disable(struct isp_pipeline *pipe)
 	struct v4l2_subdev *subdev;
 	int failure = 0;
 	int ret;
-	u32 id;
 
 	/*
 	 * We need to stop all the modules after CCDC first or they'll
@@ -1026,10 +1025,9 @@ static int isp_pipeline_disable(struct isp_pipeline *pipe)
 		if (ret) {
 			dev_info(isp->dev, "Unable to stop %s\n", subdev->name);
 			isp->stop_failure = true;
-			if (subdev == &isp->isp_prev.subdev) {
-				id = media_entity_id(&subdev->entity);
-				isp->crashed |= 1U << id;
-			}
+			if (subdev == &isp->isp_prev.subdev)
+				media_entity_enum_set(isp->crashed,
+						      &subdev->entity);
 			failure = -ETIMEDOUT;
 		}
 	}
@@ -1235,7 +1233,7 @@ static int isp_reset(struct isp_device *isp)
 	}
 
 	isp->stop_failure = false;
-	isp->crashed = 0;
+	media_entity_enum_init(isp->crashed);
 	return 0;
 }
 
@@ -1646,7 +1644,8 @@ static void __omap3isp_put(struct isp_device *isp, bool save_ctx)
 		/* Reset the ISP if an entity has failed to stop. This is the
 		 * only way to recover from such conditions.
 		 */
-		if (isp->crashed || isp->stop_failure)
+		if (!media_entity_enum_empty(isp->crashed) ||
+		    isp->stop_failure)
 			isp_reset(isp);
 		isp_disable_clocks(isp);
 	}
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 5acc2e6..7006c94 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -17,6 +17,7 @@
 #ifndef OMAP3_ISP_CORE_H
 #define OMAP3_ISP_CORE_H
 
+#include <media/media-entity.h>
 #include <media/v4l2-async.h>
 #include <media/v4l2-device.h>
 #include <linux/clk-provider.h>
@@ -152,7 +153,7 @@ struct isp_xclk {
  * @stat_lock: Spinlock for handling statistics
  * @isp_mutex: Mutex for serializing requests to ISP.
  * @stop_failure: Indicates that an entity failed to stop.
- * @crashed: Bitmask of crashed entities (indexed by entity ID)
+ * @crashed: Crashed entities
  * @has_context: Context has been saved at least once and can be restored.
  * @ref_count: Reference count for handling multiple ISP requests.
  * @cam_ick: Pointer to camera interface clock structure.
@@ -194,7 +195,7 @@ struct isp_device {
 	spinlock_t stat_lock;	/* common lock for statistic drivers */
 	struct mutex isp_mutex;	/* For handling ref_count field */
 	bool stop_failure;
-	u32 crashed;
+	DECLARE_MEDIA_ENTITY_ENUM(crashed);
 	int has_context;
 	int ref_count;
 	unsigned int autoidle;
diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c
index f0e530c..6cde922 100644
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1608,7 +1608,7 @@ static int ccdc_isr_buffer(struct isp_ccdc_device *ccdc)
 	/* Wait for the CCDC to become idle. */
 	if (ccdc_sbl_wait_idle(ccdc, 1000)) {
 		dev_info(isp->dev, "CCDC won't become idle!\n");
-		isp->crashed |= 1U << media_entity_id(&ccdc->subdev.entity);
+		media_entity_enum_set(isp->crashed, &ccdc->subdev.entity);
 		omap3isp_pipeline_cancel_stream(pipe);
 		return 0;
 	}
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 52843ac..5937ada 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -234,7 +234,7 @@ static int isp_video_get_graph_data(struct isp_video *video,
 	while ((entity = media_entity_graph_walk_next(&graph))) {
 		struct isp_video *__video;
 
-		pipe->entities |= 1 << media_entity_id(entity);
+		media_entity_enum_set(pipe->entities, entity);
 
 		if (far_end != NULL)
 			continue;
@@ -890,7 +890,6 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
 	struct v4l2_ext_control ctrl;
 	unsigned int i;
 	int ret;
-	u32 id;
 
 	/* Memory-to-memory pipelines have no external subdev. */
 	if (pipe->input != NULL)
@@ -898,7 +897,7 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
 
 	for (i = 0; i < ARRAY_SIZE(ents); i++) {
 		/* Is the entity part of the pipeline? */
-		if (!(pipe->entities & (1 << media_entity_id(ents[i]))))
+		if (!media_entity_enum_test(pipe->entities, ents[i]))
 			continue;
 
 		/* ISP entities have always sink pad == 0. Find source. */
@@ -950,8 +949,8 @@ static int isp_video_check_external_subdevs(struct isp_video *video,
 
 	pipe->external_rate = ctrl.value64;
 
-	id = media_entity_id(&isp->isp_ccdc.subdev.entity);
-	if (pipe->entities & (1 << id)) {
+	if (media_entity_enum_test(pipe->entities,
+				   &isp->isp_ccdc.subdev.entity)) {
 		unsigned int rate = UINT_MAX;
 		/*
 		 * Check that maximum allowed CCDC pixel rate isn't
@@ -1017,7 +1016,7 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	pipe = video->video.entity.pipe
 	     ? to_isp_pipeline(&video->video.entity) : &video->pipe;
 
-	pipe->entities = 0;
+	media_entity_enum_init(pipe->entities);
 
 	/* TODO: Implement PM QoS */
 	pipe->l3_ick = clk_get_rate(video->isp->clock[ISP_CLK_L3_ICK]);
diff --git a/drivers/media/platform/omap3isp/ispvideo.h b/drivers/media/platform/omap3isp/ispvideo.h
index 4071dd7..7f88765 100644
--- a/drivers/media/platform/omap3isp/ispvideo.h
+++ b/drivers/media/platform/omap3isp/ispvideo.h
@@ -80,7 +80,7 @@ enum isp_pipeline_state {
  * struct isp_pipeline - An ISP hardware pipeline
  * @field: The field being processed by the pipeline
  * @error: A hardware error occurred during capture
- * @entities: Bitmask of entities in the pipeline (indexed by entity ID)
+ * @entities: Entities in the pipeline
  */
 struct isp_pipeline {
 	struct media_pipeline pipe;
@@ -89,7 +89,7 @@ struct isp_pipeline {
 	enum isp_pipeline_stream_state stream_state;
 	struct isp_video *input;
 	struct isp_video *output;
-	u32 entities;
+	DECLARE_MEDIA_ENTITY_ENUM(entities);
 	unsigned long l3_ick;
 	unsigned int max_rate;
 	enum v4l2_field field;
-- 
2.1.0.231.g7484e3b

[RFC 8/9] vsp1: Use media entity enumeration API

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/vsp1/vsp1_video.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index f741582..026d6462 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -311,7 +311,7 @@ static int vsp1_pipeline_validate_branch(struct vsp1_pipeline *pipe,
 					 struct vsp1_rwpf *output)
 {
 	struct vsp1_entity *entity;
-	unsigned int entities = 0;
+	DECLARE_MEDIA_ENTITY_ENUM(entities);
 	struct media_pad *pad;
 	bool bru_found = false;
 
@@ -351,11 +351,10 @@ static int vsp1_pipeline_validate_branch(struct vsp1_pipeline *pipe,
 			break;
 
 		/* Ensure the branch has no loop. */
-		if (entities & (1 << media_entity_id(&entity->subdev.entity)))
+		if (media_entity_enum_test_and_set(entities,
+						   &entity->subdev.entity))
 			return -EPIPE;
 
-		entities |= 1 << media_entity_id(&entity->subdev.entity);
-
 		/* UDS can't be chained. */
 		if (entity->type == VSP1_ENTITY_UDS) {
 			if (pipe->uds)
-- 
2.1.0.231.g7484e3b

[RFC 9/9] staging: omap4iss: Use media entity enum API

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/staging/media/omap4iss/iss.c       | 7 ++++---
 drivers/staging/media/omap4iss/iss.h       | 4 ++--
 drivers/staging/media/omap4iss/iss_video.c | 4 ++--
 drivers/staging/media/omap4iss/iss_video.h | 4 ++--
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index 076ddd4..9f310e7 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -606,7 +606,7 @@ static int iss_pipeline_disable(struct iss_pipeline *pipe,
 			 * crashed. Mark it as such, the ISS will be reset when
 			 * applications will release it.
 			 */
-			iss->crashed |= 1U << media_entity_id(&subdev->entity);
+			media_entity_enum_set(iss->crashed, &subdev->entity);
 			failure = -ETIMEDOUT;
 		}
 	}
@@ -641,7 +641,7 @@ static int iss_pipeline_enable(struct iss_pipeline *pipe,
 	 * pipeline won't start anyway (those entities would then likely fail to
 	 * stop, making the problem worse).
 	 */
-	if (pipe->entities & iss->crashed)
+	if (media_entity_enum_intersects(pipe->entities, iss->crashed))
 		return -EIO;
 
 	spin_lock_irqsave(&pipe->lock, flags);
@@ -761,7 +761,8 @@ static int iss_reset(struct iss_device *iss)
 		return -ETIMEDOUT;
 	}
 
-	iss->crashed = 0;
+	media_entity_enum_init(iss->crashed);
+
 	return 0;
 }
 
diff --git a/drivers/staging/media/omap4iss/iss.h b/drivers/staging/media/omap4iss/iss.h
index 35df8b4..a124555 100644
--- a/drivers/staging/media/omap4iss/iss.h
+++ b/drivers/staging/media/omap4iss/iss.h
@@ -82,7 +82,7 @@ struct iss_reg {
 /*
  * struct iss_device - ISS device structure.
  * @syscon: Regmap for the syscon register space
- * @crashed: Bitmask of crashed entities (indexed by entity ID)
+ * @crashed: Crashed entities
  */
 struct iss_device {
 	struct v4l2_device v4l2_dev;
@@ -101,7 +101,7 @@ struct iss_device {
 	u64 raw_dmamask;
 
 	struct mutex iss_mutex;	/* For handling ref_count field */
-	unsigned int crashed;
+	DECLARE_MEDIA_ENTITY_ENUM(crashed);
 	int has_context;
 	int ref_count;
 
diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c
index cbe5783..385b668 100644
--- a/drivers/staging/media/omap4iss/iss_video.c
+++ b/drivers/staging/media/omap4iss/iss_video.c
@@ -771,7 +771,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	pipe->external = NULL;
 	pipe->external_rate = 0;
 	pipe->external_bpp = 0;
-	pipe->entities = 0;
+	media_entity_enum_init(pipe->entities);
 
 	if (video->iss->pdata->set_constraints)
 		video->iss->pdata->set_constraints(video->iss, true);
@@ -783,7 +783,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	entity = &video->video.entity;
 	media_entity_graph_walk_start(&graph, entity);
 	while ((entity = media_entity_graph_walk_next(&graph)))
-		pipe->entities |= 1 << media_entity_id(entity);
+		media_entity_enum_set(pipe->entities, entity);
 
 	/* Verify that the currently configured format matches the output of
 	 * the connected subdev.
diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
index f11fce2..d415594 100644
--- a/drivers/staging/media/omap4iss/iss_video.h
+++ b/drivers/staging/media/omap4iss/iss_video.h
@@ -77,7 +77,7 @@ enum iss_pipeline_state {
 
 /*
  * struct iss_pipeline - An OMAP4 ISS hardware pipeline
- * @entities: Bitmask of entities in the pipeline (indexed by entity ID)
+ * @entities: Entities in the pipeline
  * @error: A hardware error occurred during capture
  */
 struct iss_pipeline {
@@ -87,7 +87,7 @@ struct iss_pipeline {
 	enum iss_pipeline_stream_state stream_state;
 	struct iss_video *input;
 	struct iss_video *output;
-	unsigned int entities;
+	DECLARE_MEDIA_ENTITY_ENUM(entities);
 	atomic_t frame_number;
 	bool do_propagation; /* of frame number */
 	bool error;
-- 
2.1.0.231.g7484e3b

Re: [RFC 0/9] Unrestricted media entity ID range support

[Mauro Carvalho Chehab]

Hi Sakari,

On Fri, 11 Sep 2015 13:09:03 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> wrote:

> Hi all,
> 
> This patchset adds an API for managing entity enumerations, i.e.
> storing a bit of information per entity. The entity ID is no longer
> limited to small integers (e.g. 31 or 63), but
> MEDIA_ENTITY_MAX_LOW_ID. The drivers are also converted to use that
> instead of a fixed number.
> 
> If the number of entities in a real use case grows beyond the
> capabilities of the approach, the algorithm may be changed. But most
> importantly, the API is used to perform the same operation everywhere
> it's done.
> 
> I'm sending this for review only, the code itself is untested.
> 
> I haven't entirely made up my mind on the fourth patch. It could be
> dropped, as it uses the API for a somewhat different purpose.
> 

Sorry for not reviewing this earlier, but I'm traveling this week to
China, and I was having some troubles with the Internet. Btw, I don't
have my notebook here (just a chromebook without the media tree).
So, please consider this as just a preliminary review.

I won't be comment this series patch by patch, because it is really
painful to do it while here with this Internet.

Also, I want to discuss the patch series as a hole.

>From what we've agreed last week, we won't be using a separate ID
range for the entity ID, but this patch series is actually adding
it, and, IMHO, using a confusing nomenclature: instead of calling the
entity ID range as "entity_id" at the media_device struct, you're
now calling it "low_id". That sounds confusing to me. If you think
we should keep a separate range for entities, calling it as 
"entity_id" is clearer.

Also, you said at the low_id comment that if an entity is
unregistered and then re-registered, it would preserve the same
entity ID. That doesn't seem easy to implement, as we would need
to track those previously-used ID. On the other hand, if we just
re-use a previously released ID for some other entity, this can
be a problem, as userspace may not be aware of such changes and
might be asking the Kernel to do the wrong thing.

So, I can't see how non-monotonically incremented numbers would
work here.

Finally, the changes you did still rely on having the ID limited
to a well-defined, hardcoded number (MEDIA_ENTITY_MAX_LOW_ID).

I can see this working only if:

- We keep a separate range ID for entities (so, having a minimum
  of two ranges);

- the entity maximum ID is defined by the driver (as the number
  of entities is actually dependent on the hardware);

- some other mechanism would be available for drivers that
  would support dynamic entity creation.

So, I don't see how this would solve the problems that we
discussed at the last week IRC chats.

Am I missing something?

Regards,
Mauro

PS.: sparse already complains on two places at the media-entity where
bitmaps are declared at the stack. With max entities equal to 64,
that's not an issue, but if we change to a higher number, those will
need to be dynamically allocated, in order to avoid stack overflows.
I didn't see any patches touching that.

Re: [RFC 0/9] Unrestricted media entity ID range support

[Sakari Ailus]

Hi Mauro,

Mauro Carvalho Chehab wrote:
> Hi Sakari,
>
> On Fri, 11 Sep 2015 13:09:03 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
>> Hi all,
>>
>> This patchset adds an API for managing entity enumerations, i.e.
>> storing a bit of information per entity. The entity ID is no longer
>> limited to small integers (e.g. 31 or 63), but
>> MEDIA_ENTITY_MAX_LOW_ID. The drivers are also converted to use that
>> instead of a fixed number.
>>
>> If the number of entities in a real use case grows beyond the
>> capabilities of the approach, the algorithm may be changed. But most
>> importantly, the API is used to perform the same operation everywhere
>> it's done.
>>
>> I'm sending this for review only, the code itself is untested.
>>
>> I haven't entirely made up my mind on the fourth patch. It could be
>> dropped, as it uses the API for a somewhat different purpose.
>>
>
> Sorry for not reviewing this earlier, but I'm traveling this week to
> China, and I was having some troubles with the Internet. Btw, I don't
> have my notebook here (just a chromebook without the media tree).
> So, please consider this as just a preliminary review.
>
> I won't be comment this series patch by patch, because it is really
> painful to do it while here with this Internet.
>
> Also, I want to discuss the patch series as a hole.
>
>  From what we've agreed last week, we won't be using a separate ID
> range for the entity ID, but this patch series is actually adding
> it, and, IMHO, using a confusing nomenclature: instead of calling the
> entity ID range as "entity_id" at the media_device struct, you're
> now calling it "low_id". That sounds confusing to me. If you think
> we should keep a separate range for entities, calling it as
> "entity_id" is clearer.

It's *not* the entity ID. It's a number used internally to keep track of 
the entities, and only used for this purpose, nothing else. If you look 
at the patch, the number of places where low_id is used is very limited. 
Individual drivers do not and must not access the low_id field.

The underlying algorithm for keeping track of entities does not change, 
but that could be changed later on without affecting the users of the 
alrogithm. --- See patch 3.

There are two main reasons for this:

1. No need to implement a new algorithm which would be less efficient 
but could cope in cases we do not have yet; this can be done later on, 
as patch 3 adds an API to access the information without making 
assumptions on the implementation.

2. It does solve the problem. The graph object IDs of the entities can 
be large without adversely affecting the functionality of existing drivers.

>
> Also, you said at the low_id comment that if an entity is
> unregistered and then re-registered, it would preserve the same

I never claimed that, and the patchset does not implement that either.

If an entity is unregistered and registered again, from the MC point of 
view it is not the same entity. We do not keep track of entities that 
are not registered with MC, do we?

> entity ID. That doesn't seem easy to implement, as we would need
> to track those previously-used ID. On the other hand, if we just
> re-use a previously released ID for some other entity, this can
> be a problem, as userspace may not be aware of such changes and
> might be asking the Kernel to do the wrong thing.

Let's not do that then. This is why we have graph object IDs.

>
> So, I can't see how non-monotonically incremented numbers would
> work here.
>
> Finally, the changes you did still rely on having the ID limited
> to a well-defined, hardcoded number (MEDIA_ENTITY_MAX_LOW_ID).
>
> I can see this working only if:
>
> - We keep a separate range ID for entities (so, having a minimum
>    of two ranges);
>
> - the entity maximum ID is defined by the driver (as the number
>    of entities is actually dependent on the hardware);

The case is that we do not have a driver requiring more entities. Once 
we do, we can implement a new algorithm for the purpose. Memory 
allocation will be required, but that's a separate matter from the graph 
object ID of the entities, which is the problem this patchset was 
intended to solve.

>
> - some other mechanism would be available for drivers that
>    would support dynamic entity creation.
>
> So, I don't see how this would solve the problems that we
> discussed at the last week IRC chats.
>
> Am I missing something?

This set indeed solves a problem, and that problem was unrestricting the 
graph object ID of the entities. There are other problems remaining 
before entities can be e.g. unregistered one by one, but they are 
separate problems.

>
> Regards,
> Mauro
>
> PS.: sparse already complains on two places at the media-entity where
> bitmaps are declared at the stack. With max entities equal to 64,
> that's not an issue, but if we change to a higher number, those will
> need to be dynamically allocated, in order to avoid stack overflows.
> I didn't see any patches touching that.

I agree. The aim of the set was not to increase the number of concurrent 
entities. That is a separate problem which can be solved later on once 
we have a use case for it.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC 0/9] Unrestricted media entity ID range support

[Mauro Carvalho Chehab]

Em Sun, 20 Sep 2015 10:26:09 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> Mauro Carvalho Chehab wrote:
> > Hi Sakari,
> >
> > On Fri, 11 Sep 2015 13:09:03 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> >> Hi all,
> >>
> >> This patchset adds an API for managing entity enumerations, i.e.
> >> storing a bit of information per entity. The entity ID is no longer
> >> limited to small integers (e.g. 31 or 63), but
> >> MEDIA_ENTITY_MAX_LOW_ID. The drivers are also converted to use that
> >> instead of a fixed number.
> >>
> >> If the number of entities in a real use case grows beyond the
> >> capabilities of the approach, the algorithm may be changed. But most
> >> importantly, the API is used to perform the same operation everywhere
> >> it's done.
> >>
> >> I'm sending this for review only, the code itself is untested.
> >>
> >> I haven't entirely made up my mind on the fourth patch. It could be
> >> dropped, as it uses the API for a somewhat different purpose.
> >>
> >
> > Sorry for not reviewing this earlier, but I'm traveling this week to
> > China, and I was having some troubles with the Internet. Btw, I don't
> > have my notebook here (just a chromebook without the media tree).
> > So, please consider this as just a preliminary review.
> >
> > I won't be comment this series patch by patch, because it is really
> > painful to do it while here with this Internet.
> >
> > Also, I want to discuss the patch series as a hole.
> >
> >  From what we've agreed last week, we won't be using a separate ID
> > range for the entity ID, but this patch series is actually adding
> > it, and, IMHO, using a confusing nomenclature: instead of calling the
> > entity ID range as "entity_id" at the media_device struct, you're
> > now calling it "low_id". That sounds confusing to me. If you think
> > we should keep a separate range for entities, calling it as
> > "entity_id" is clearer.
> 
> It's *not* the entity ID. It's a number used internally to keep track of 
> the entities, and only used for this purpose, nothing else. If you look 
> at the patch, the number of places where low_id is used is very limited. 
> Individual drivers do not and must not access the low_id field.
> 
> The underlying algorithm for keeping track of entities does not change, 
> but that could be changed later on without affecting the users of the 
> alrogithm. --- See patch 3.
> 
> There are two main reasons for this:
> 
> 1. No need to implement a new algorithm which would be less efficient 
> but could cope in cases we do not have yet; this can be done later on, 
> as patch 3 adds an API to access the information without making 
> assumptions on the implementation.

If this is an internal number used only by the graph traversal
algorithm, then the implementation doesn't seem correct. I mean:
such number should be generated internally when starting the
graph traversal algorithm, and it would be better to store such
graph-traversal internal algorithm data on a separate struct.

> 2. It does solve the problem. The graph object IDs of the entities can 
> be large without adversely affecting the functionality of existing drivers.

Right now, with just those 9 patches, it doesn't ;)

I mean: if I apply your patches after the MC next gen ones and try to use the
graph traversal, it will do the wrong thing for hybrid TV cards, as the number
of entities there are more than 64. Ok, easy to fix after this series by
just changing the value of MEDIA_ENTITY_MAX_LOW_ID, but this will only
work while we don't implement dynamic support.

> >
> > Also, you said at the low_id comment that if an entity is
> > unregistered and then re-registered, it would preserve the same
> 
> I never claimed that, and the patchset does not implement that either.

That's what I understood from this comment:

	Rename the macro as it no longer is a maximum ID that an entity can have,
	but a low ID which is used for internal purposes of enumeration. This is
	the maximum number of concurrent entities that may exist in a media
	device.

If this is the "max number of concurrent entities", you need to reassign
those numbers when entities are removed.

If this is not what you're meaning, then fix the patch description
to let it clearer.

I guess then that "low ID" is actually an upper bound for that
graph-traversal only control number. We should really not use the word "ID"
here, as this is not an ID. it is just some index/control number that will
be granted to be below some upper bound.

> If an entity is unregistered and registered again, from the MC point of 
> view it is not the same entity. We do not keep track of entities that 
> are not registered with MC, do we?
> 
> > entity ID. That doesn't seem easy to implement, as we would need
> > to track those previously-used ID. On the other hand, if we just
> > re-use a previously released ID for some other entity, this can
> > be a problem, as userspace may not be aware of such changes and
> > might be asking the Kernel to do the wrong thing.
> 
> Let's not do that then. This is why we have graph object IDs.

Ok, provided that we better describe it and better represent those
fields.

> >
> > So, I can't see how non-monotonically incremented numbers would
> > work here.
> >
> > Finally, the changes you did still rely on having the ID limited
> > to a well-defined, hardcoded number (MEDIA_ENTITY_MAX_LOW_ID).
> >
> > I can see this working only if:
> >
> > - We keep a separate range ID for entities (so, having a minimum
> >    of two ranges);
> >
> > - the entity maximum ID is defined by the driver (as the number
> >    of entities is actually dependent on the hardware);
> 
> The case is that we do not have a driver requiring more entities. 

We do: the DVB and hybrid TV drivers. A reasonable limit for a
PC consumer device like that would be an upper bound at the order
of 2^10.

> Once 
> we do, we can implement a new algorithm for the purpose. Memory 
> allocation will be required, but that's a separate matter from the graph 
> object ID of the entities, which is the problem this patchset was 
> intended to solve.

The problem this patchset should be solving is exactly the case
of those new drivers that are being properly mapped with the MC
next gen patches ;)

> >
> > - some other mechanism would be available for drivers that
> >    would support dynamic entity creation.
> >
> > So, I don't see how this would solve the problems that we
> > discussed at the last week IRC chats.
> >
> > Am I missing something?
> 
> This set indeed solves a problem, and that problem was unrestricting the 
> graph object ID of the entities. There are other problems remaining 
> before entities can be e.g. unregistered one by one, but they are 
> separate problems.

That's not the way I see it ;) I mean, for the current MC code, this patch
series is not needed, as all drivers right now have less than 64 entities.

This need only starts after/during the MC next gen patches, in order
to address two changes that come on this series:

- having an arbitrary entity ID number that will be a way bigger
  than the current upper bound (the number of entities);

- support more than 64 entities.

So, I would be expecting a patch series that would either:

1) change the graph traversal algorithms to not depend on some
upper bound limit;

	or:

2) dynamically create whatever internal index number and to
dynamically determine the upper bound limit that would be needed for the
graph traversal algorithm to work, allocating those data when the graph 
traversal algorithm starts and freeing them when the graph traversal
stops, and/or when the media device is unregistered.

> 
> >
> > Regards,
> > Mauro
> >
> > PS.: sparse already complains on two places at the media-entity where
> > bitmaps are declared at the stack. With max entities equal to 64,
> > that's not an issue, but if we change to a higher number, those will
> > need to be dynamically allocated, in order to avoid stack overflows.
> > I didn't see any patches touching that.
> 
> I agree. The aim of the set was not to increase the number of concurrent 
> entities. That is a separate problem which can be solved later on once 
> we have a use case for it.
> 

I reviewed that code. The problem there is actually unrelated to what
this patch series is trying to solve, as the problem there is due to the
number of PADs (and not on the number of entities). I mean about this
code at media_entity_pipeline_start():

	while ((entity = media_entity_graph_walk_next(&graph))) {
		DECLARE_BITMAP(active, entity->num_pads);
		DECLARE_BITMAP(has_no_links, entity->num_pads);

That limits the max number of pads, as a big number would cause a
Linux stack overflow.

Those bitmaps should likely be moved to struct media_pipeline or
struct media_device and be either dynamically created/removed or 
having them relying on a max number of pads.

I guess the latter is easier.

In any case, this is independent of the problem we're trying to
fix with this RFC patch series (and even independent of the MC
next gen series).

I'll prepare a patch for that on the top of the current master
branch.

Regards,
Mauro

Re: [RFC 0/9] Unrestricted media entity ID range support

[Sakari Ailus]

Hi Mauro,

Mauro Carvalho Chehab wrote:
> Em Sun, 20 Sep 2015 10:26:09 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
>
>> Hi Mauro,
>>
>> Mauro Carvalho Chehab wrote:
>>> Hi Sakari,
>>>
>>> On Fri, 11 Sep 2015 13:09:03 +0300
>>> Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>>>
>>>> Hi all,
>>>>
>>>> This patchset adds an API for managing entity enumerations, i.e.
>>>> storing a bit of information per entity. The entity ID is no longer
>>>> limited to small integers (e.g. 31 or 63), but
>>>> MEDIA_ENTITY_MAX_LOW_ID. The drivers are also converted to use that
>>>> instead of a fixed number.
>>>>
>>>> If the number of entities in a real use case grows beyond the
>>>> capabilities of the approach, the algorithm may be changed. But most
>>>> importantly, the API is used to perform the same operation everywhere
>>>> it's done.
>>>>
>>>> I'm sending this for review only, the code itself is untested.
>>>>
>>>> I haven't entirely made up my mind on the fourth patch. It could be
>>>> dropped, as it uses the API for a somewhat different purpose.
>>>>
>>>
>>> Sorry for not reviewing this earlier, but I'm traveling this week to
>>> China, and I was having some troubles with the Internet. Btw, I don't
>>> have my notebook here (just a chromebook without the media tree).
>>> So, please consider this as just a preliminary review.
>>>
>>> I won't be comment this series patch by patch, because it is really
>>> painful to do it while here with this Internet.
>>>
>>> Also, I want to discuss the patch series as a hole.
>>>
>>>   From what we've agreed last week, we won't be using a separate ID
>>> range for the entity ID, but this patch series is actually adding
>>> it, and, IMHO, using a confusing nomenclature: instead of calling the
>>> entity ID range as "entity_id" at the media_device struct, you're
>>> now calling it "low_id". That sounds confusing to me. If you think
>>> we should keep a separate range for entities, calling it as
>>> "entity_id" is clearer.
>>
>> It's *not* the entity ID. It's a number used internally to keep track of
>> the entities, and only used for this purpose, nothing else. If you look
>> at the patch, the number of places where low_id is used is very limited.
>> Individual drivers do not and must not access the low_id field.
>>
>> The underlying algorithm for keeping track of entities does not change,
>> but that could be changed later on without affecting the users of the
>> alrogithm. --- See patch 3.
>>
>> There are two main reasons for this:
>>
>> 1. No need to implement a new algorithm which would be less efficient
>> but could cope in cases we do not have yet; this can be done later on,
>> as patch 3 adds an API to access the information without making
>> assumptions on the implementation.
>
> If this is an internal number used only by the graph traversal
> algorithm, then the implementation doesn't seem correct. I mean:
> such number should be generated internally when starting the
> graph traversal algorithm, and it would be better to store such
> graph-traversal internal algorithm data on a separate struct.

In that case the number would become enumeration specific. I have no 
problem in that, but if a single entity specific number works for the 
purpose, I think that could be used as well. Using something else than a 
constant value per entity quickly will require memory allocation, and 
graph traversal is performance critical in many cases.

>
>> 2. It does solve the problem. The graph object IDs of the entities can
>> be large without adversely affecting the functionality of existing drivers.
>
> Right now, with just those 9 patches, it doesn't ;)
>
> I mean: if I apply your patches after the MC next gen ones and try to use the
> graph traversal, it will do the wrong thing for hybrid TV cards, as the number
> of entities there are more than 64. Ok, easy to fix after this series by
> just changing the value of MEDIA_ENTITY_MAX_LOW_ID, but this will only
> work while we don't implement dynamic support.

Fair enough. Still, it didn't work before either: the graph traversal 
algorithm depends on entity ID not exceeding MEDIA_ENTITY_ENUM_MAX_ID 
(which this set renames).

I still consider that a separate problem.

>
>>>
>>> Also, you said at the low_id comment that if an entity is
>>> unregistered and then re-registered, it would preserve the same
>>
>> I never claimed that, and the patchset does not implement that either.
>
> That's what I understood from this comment:
>
> 	Rename the macro as it no longer is a maximum ID that an entity can have,
> 	but a low ID which is used for internal purposes of enumeration. This is
> 	the maximum number of concurrent entities that may exist in a media
> 	device.
>
> If this is the "max number of concurrent entities", you need to reassign
> those numbers when entities are removed.
>
> If this is not what you're meaning, then fix the patch description
> to let it clearer.
>
> I guess then that "low ID" is actually an upper bound for that
> graph-traversal only control number. We should really not use the word "ID"
> here, as this is not an ID. it is just some index/control number that will
> be granted to be below some upper bound.

Index sounds good to me, it's indeed clearer. I didn't like "low" 
either, it just happens to be that way right now. With a different 
algorithm, I think we can get rid of the entire field --- but it will 
have to be replaced by something that requires memory allocation.

...

>>>
>>> - some other mechanism would be available for drivers that
>>>     would support dynamic entity creation.
>>>
>>> So, I don't see how this would solve the problems that we
>>> discussed at the last week IRC chats.
>>>
>>> Am I missing something?
>>
>> This set indeed solves a problem, and that problem was unrestricting the
>> graph object ID of the entities. There are other problems remaining
>> before entities can be e.g. unregistered one by one, but they are
>> separate problems.
>
> That's not the way I see it ;) I mean, for the current MC code, this patch
> series is not needed, as all drivers right now have less than 64 entities.

In the last IRC meeting we agreed to have a single ID range for graph 
object IDs. Now that ID will be given to all graph objects, 64 will be 
reached much sooner than it used to, and it probably happens on at least 
some existing drivers as well.

>
> This need only starts after/during the MC next gen patches, in order
> to address two changes that come on this series:
>
> - having an arbitrary entity ID number that will be a way bigger
>    than the current upper bound (the number of entities);

It's indeed *the number of entities*, not entity ID.

>
> - support more than 64 entities.
>
> So, I would be expecting a patch series that would either:
>
> 1) change the graph traversal algorithms to not depend on some
> upper bound limit;
>
> 	or:
>
> 2) dynamically create whatever internal index number and to
> dynamically determine the upper bound limit that would be needed for the
> graph traversal algorithm to work, allocating those data when the graph
> traversal algorithm starts and freeing them when the graph traversal
> stops, and/or when the media device is unregistered.

Even with 4 k entities, the memory required by that algorithm to track 
the entities would be just 512 bytes + 16 * depth. I wonder if a more 
sophisticated algorithm would be able to operate with less or as 
efficiently.

The memory would need to come from elsewhere than from the stack obviously.

>
>>
>>>
>>> Regards,
>>> Mauro
>>>
>>> PS.: sparse already complains on two places at the media-entity where
>>> bitmaps are declared at the stack. With max entities equal to 64,
>>> that's not an issue, but if we change to a higher number, those will
>>> need to be dynamically allocated, in order to avoid stack overflows.
>>> I didn't see any patches touching that.
>>
>> I agree. The aim of the set was not to increase the number of concurrent
>> entities. That is a separate problem which can be solved later on once
>> we have a use case for it.
>>
>
> I reviewed that code. The problem there is actually unrelated to what
> this patch series is trying to solve, as the problem there is due to the
> number of PADs (and not on the number of entities). I mean about this
> code at media_entity_pipeline_start():
>
> 	while ((entity = media_entity_graph_walk_next(&graph))) {
> 		DECLARE_BITMAP(active, entity->num_pads);
> 		DECLARE_BITMAP(has_no_links, entity->num_pads);
>
> That limits the max number of pads, as a big number would cause a
> Linux stack overflow.
>
> Those bitmaps should likely be moved to struct media_pipeline or
> struct media_device and be either dynamically created/removed or
> having them relying on a max number of pads.

How many pads are we looking forwards to have?

I don't think we have a fixed limit as such, but AFAIR there's no driver 
using more than around five at the moment, so there was really no need 
to worry about it.

I guess it'd be good to have a maximum though just to be on the safe side.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC 0/9] Unrestricted media entity ID range support

[Mauro Carvalho Chehab]

Em Wed, 23 Sep 2015 00:56:05 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Mauro,
> 
> Mauro Carvalho Chehab wrote:
> > Em Sun, 20 Sep 2015 10:26:09 +0300
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >
> >> Hi Mauro,
> >>
> >> Mauro Carvalho Chehab wrote:
> >>> Hi Sakari,
> >>>
> >>> On Fri, 11 Sep 2015 13:09:03 +0300
> >>> Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >>>
> >>>> Hi all,
> >>>>
> >>>> This patchset adds an API for managing entity enumerations, i.e.
> >>>> storing a bit of information per entity. The entity ID is no longer
> >>>> limited to small integers (e.g. 31 or 63), but
> >>>> MEDIA_ENTITY_MAX_LOW_ID. The drivers are also converted to use that
> >>>> instead of a fixed number.
> >>>>
> >>>> If the number of entities in a real use case grows beyond the
> >>>> capabilities of the approach, the algorithm may be changed. But most
> >>>> importantly, the API is used to perform the same operation everywhere
> >>>> it's done.
> >>>>
> >>>> I'm sending this for review only, the code itself is untested.
> >>>>
> >>>> I haven't entirely made up my mind on the fourth patch. It could be
> >>>> dropped, as it uses the API for a somewhat different purpose.
> >>>>
> >>>
> >>> Sorry for not reviewing this earlier, but I'm traveling this week to
> >>> China, and I was having some troubles with the Internet. Btw, I don't
> >>> have my notebook here (just a chromebook without the media tree).
> >>> So, please consider this as just a preliminary review.
> >>>
> >>> I won't be comment this series patch by patch, because it is really
> >>> painful to do it while here with this Internet.
> >>>
> >>> Also, I want to discuss the patch series as a hole.
> >>>
> >>>   From what we've agreed last week, we won't be using a separate ID
> >>> range for the entity ID, but this patch series is actually adding
> >>> it, and, IMHO, using a confusing nomenclature: instead of calling the
> >>> entity ID range as "entity_id" at the media_device struct, you're
> >>> now calling it "low_id". That sounds confusing to me. If you think
> >>> we should keep a separate range for entities, calling it as
> >>> "entity_id" is clearer.
> >>
> >> It's *not* the entity ID. It's a number used internally to keep track of
> >> the entities, and only used for this purpose, nothing else. If you look
> >> at the patch, the number of places where low_id is used is very limited.
> >> Individual drivers do not and must not access the low_id field.
> >>
> >> The underlying algorithm for keeping track of entities does not change,
> >> but that could be changed later on without affecting the users of the
> >> alrogithm. --- See patch 3.
> >>
> >> There are two main reasons for this:
> >>
> >> 1. No need to implement a new algorithm which would be less efficient
> >> but could cope in cases we do not have yet; this can be done later on,
> >> as patch 3 adds an API to access the information without making
> >> assumptions on the implementation.
> >
> > If this is an internal number used only by the graph traversal
> > algorithm, then the implementation doesn't seem correct. I mean:
> > such number should be generated internally when starting the
> > graph traversal algorithm, and it would be better to store such
> > graph-traversal internal algorithm data on a separate struct.
> 
> In that case the number would become enumeration specific. I have no 
> problem in that, but if a single entity specific number works for the 
> purpose, I think that could be used as well. Using something else than a 
> constant value per entity quickly will require memory allocation, and 
> graph traversal is performance critical in many cases.

I guess a single loop at graph traversal start would solve it. Yet,
it means an extra O(n) to fill in the numbers, but the algorithm
that fills such numbers will be at least O(n) either here or at the
entity creation.

Perhaps one solution would be to have a "dirty" flag that would be
raised if a new entity would be created (and removed?). If dirty
flag is raised, it would do the count at the beginning of the graph
start.

> >
> >> 2. It does solve the problem. The graph object IDs of the entities can
> >> be large without adversely affecting the functionality of existing drivers.
> >
> > Right now, with just those 9 patches, it doesn't ;)
> >
> > I mean: if I apply your patches after the MC next gen ones and try to use the
> > graph traversal, it will do the wrong thing for hybrid TV cards, as the number
> > of entities there are more than 64. Ok, easy to fix after this series by
> > just changing the value of MEDIA_ENTITY_MAX_LOW_ID, but this will only
> > work while we don't implement dynamic support.
> 
> Fair enough. Still, it didn't work before either: the graph traversal 
> algorithm depends on entity ID not exceeding MEDIA_ENTITY_ENUM_MAX_ID 
> (which this set renames).

Well, for existing drivers that use graph traversal, this number is
not exceeded.

> I still consider that a separate problem.
> 
> >
> >>>
> >>> Also, you said at the low_id comment that if an entity is
> >>> unregistered and then re-registered, it would preserve the same
> >>
> >> I never claimed that, and the patchset does not implement that either.
> >
> > That's what I understood from this comment:
> >
> > 	Rename the macro as it no longer is a maximum ID that an entity can have,
> > 	but a low ID which is used for internal purposes of enumeration. This is
> > 	the maximum number of concurrent entities that may exist in a media
> > 	device.
> >
> > If this is the "max number of concurrent entities", you need to reassign
> > those numbers when entities are removed.
> >
> > If this is not what you're meaning, then fix the patch description
> > to let it clearer.
> >
> > I guess then that "low ID" is actually an upper bound for that
> > graph-traversal only control number. We should really not use the word "ID"
> > here, as this is not an ID. it is just some index/control number that will
> > be granted to be below some upper bound.
> 
> Index sounds good to me, it's indeed clearer. I didn't like "low" 
> either, it just happens to be that way right now.

OK.

> With a different 
> algorithm, I think we can get rid of the entire field --- but it will 
> have to be replaced by something that requires memory allocation.

It is hard to tell how a new algorithm would be without writing the
code for it and do some experimentation ;)

> 
> ...
> 
> >>>
> >>> - some other mechanism would be available for drivers that
> >>>     would support dynamic entity creation.
> >>>
> >>> So, I don't see how this would solve the problems that we
> >>> discussed at the last week IRC chats.
> >>>
> >>> Am I missing something?
> >>
> >> This set indeed solves a problem, and that problem was unrestricting the
> >> graph object ID of the entities. There are other problems remaining
> >> before entities can be e.g. unregistered one by one, but they are
> >> separate problems.
> >
> > That's not the way I see it ;) I mean, for the current MC code, this patch
> > series is not needed, as all drivers right now have less than 64 entities.
> 
> In the last IRC meeting we agreed to have a single ID range for graph 
> object IDs. Now that ID will be given to all graph objects, 64 will be 
> reached much sooner than it used to, and it probably happens on at least 
> some existing drivers as well.

Yes, using a single range will make this an issue. So, yes we need to
apply the patches that fixes the upper bound problem before applying
the patch that moves from 4 ranges into one. So, the order would be:

- the 82 patches
- the graph traversal change to use an "index" for the entity count
  (the equivalent of this series)
- the patch that changes from 4 to 1 range.
> 
> >
> > This need only starts after/during the MC next gen patches, in order
> > to address two changes that come on this series:
> >
> > - having an arbitrary entity ID number that will be a way bigger
> >    than the current upper bound (the number of entities);
> 
> It's indeed *the number of entities*, not entity ID.
> 
> >
> > - support more than 64 entities.
> >
> > So, I would be expecting a patch series that would either:
> >
> > 1) change the graph traversal algorithms to not depend on some
> > upper bound limit;
> >
> > 	or:
> >
> > 2) dynamically create whatever internal index number and to
> > dynamically determine the upper bound limit that would be needed for the
> > graph traversal algorithm to work, allocating those data when the graph
> > traversal algorithm starts and freeing them when the graph traversal
> > stops, and/or when the media device is unregistered.
> 
> Even with 4 k entities, the memory required by that algorithm to track 
> the entities would be just 512 bytes + 16 * depth. I wonder if a more 
> sophisticated algorithm would be able to operate with less or as 
> efficiently.

Hard to tell without seeing the code ;) 

The current algorithm is not bad, but I never really needed to look at
other alternatives for it.

Yet, with dynamic entity creation/removal, we'll need to add some extra
code to re-numerate the new entities and re-calculate the upper bound,
as entities are created/removed. So, it might justify the need or a better
algorithm, in order to avoid adding too much penalty to the performance.

> 
> The memory would need to come from elsewhere than from the stack obviously.

Yes.

> 
> >
> >>
> >>>
> >>> Regards,
> >>> Mauro
> >>>
> >>> PS.: sparse already complains on two places at the media-entity where
> >>> bitmaps are declared at the stack. With max entities equal to 64,
> >>> that's not an issue, but if we change to a higher number, those will
> >>> need to be dynamically allocated, in order to avoid stack overflows.
> >>> I didn't see any patches touching that.
> >>
> >> I agree. The aim of the set was not to increase the number of concurrent
> >> entities. That is a separate problem which can be solved later on once
> >> we have a use case for it.
> >>
> >
> > I reviewed that code. The problem there is actually unrelated to what
> > this patch series is trying to solve, as the problem there is due to the
> > number of PADs (and not on the number of entities). I mean about this
> > code at media_entity_pipeline_start():
> >
> > 	while ((entity = media_entity_graph_walk_next(&graph))) {
> > 		DECLARE_BITMAP(active, entity->num_pads);
> > 		DECLARE_BITMAP(has_no_links, entity->num_pads);
> >
> > That limits the max number of pads, as a big number would cause a
> > Linux stack overflow.
> >
> > Those bitmaps should likely be moved to struct media_pipeline or
> > struct media_device and be either dynamically created/removed or
> > having them relying on a max number of pads.
> 
> How many pads are we looking forwards to have?
> 
> I don't think we have a fixed limit as such, but AFAIR there's no driver 
> using more than around five at the moment, so there was really no need 
> to worry about it.
> 
> I guess it'd be good to have a maximum though just to be on the safe side.

DVB drivers use currently 256 pads. At the patch I wrote earlier today:

	https://patchwork.linuxtv.org/patch/31512/

(not sure why it was not at patchwork yet... Had to re-send it now)

I used the next power of two as the max limit, so 512 pads. I opted to
just add the two bitmaps used at the graph traversal logic as part of
the private fields of struct media_device. I don't expect any performance
penalty with that.

One thing I forgot to add there was a test if a driver is using more than
the maximum number of PADs. We should add such test on a version 2 of the
patch.

Regards,
Mauro



>