[PATCH 00/19] Unrestricted media entity ID range support

[PATCH 00/19] Unrestricted media entity ID range support

[Sakari Ailus]

Hi,

This is an update to my previous RFC patchset here:

<URL:http://www.spinics.net/lists/linux-media/msg93481.html>

In addition to the problems the patchset previously resolved and features
it implemented:

- unrestricted media entity ID support and
- API to manage entity enumerations

this set now solves one additional problem:

- unrestricted number of media entities.

The set also paves way to dynamic media entity registration but does not
implement it yet. In a completely dynamic system there's a lot more
mandatory error handling and difficult corner cases that one has to deal
with. There are cases where entity enumeration or graph walk may not fail
such as stopping streaming or power count calculation in link
enabling/disabling. This can be resolved by allocating entity enumerations
at a safe time and keeping them around until when they're needed and
releasing when they're no longer needed.

Keeping around entity enumerations together with dynamic entity
enumeration allocation brings up another problem: how do you ensure an
entity enumeration is large enough for all the entities that can be
encountered?

These additional problems are left for the future. This set does not
intend to address them.

The set has been tested with the omap3isp driver while the exynos4-is,
xilinx, vsp1 and omap4iss driver have been compile tested only. I'd
appreciate if others who have access to the hardware tested them.

What's changed is that the media entity enumerations are now dynamically
allocated if the number of entities in an enumeration exceeds a certain
constant value (which is now 64). The implication is that now error
handling is mandatory in entity enum initialisation (and thus graph walk
as well) as memory allocation may fail. Enumerations are allocated and
re-used in a few occasions in file handle open/close and pipeline
start/stop in order to guarantee successful graph walk, for instance.

The set also contains a fix for the omap4iss power management code. I kept
it in the set since another omap4iss driver patch depends on it.

Reviews would be very welcome.

-- 
Kind regards,
Sakari

[PATCH 01/19] media: Enforce single entity->pipe in a pipeline

[Sakari Ailus]

If a different entity->pipe in a pipeline was encountered, a warning was
issued but the execution continued as if nothing had happened. Return an
error instead right there.

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

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 66b8db0..d11f440 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -431,7 +431,12 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 		DECLARE_BITMAP(has_no_links, MEDIA_ENTITY_MAX_PADS);
 
 		entity->stream_count++;
-		WARN_ON(entity->pipe && entity->pipe != pipe);
+
+		if (WARN_ON(entity->pipe && entity->pipe != pipe)) {
+			ret = -EBUSY;
+			goto error;
+		}
+
 		entity->pipe = pipe;
 
 		/* Already streaming --- no need to check. */
-- 
2.1.4

[PATCH 02/19] media: Introduce internal index for media entities

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

The internal index can be used internally by the framework in order to keep
track of entities for a purpose or another. The internal index is constant
while it's registered to a media device, but the same index may be re-used
once the entity having that index is unregistered.

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

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index c181758..ebb84cb 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -22,6 +22,7 @@
 
 #include <linux/compat.h>
 #include <linux/export.h>
+#include <linux/idr.h>
 #include <linux/ioctl.h>
 #include <linux/media.h>
 #include <linux/types.h>
@@ -546,6 +547,7 @@ void media_device_init(struct media_device *mdev)
 	INIT_LIST_HEAD(&mdev->links);
 	spin_lock_init(&mdev->lock);
 	mutex_init(&mdev->graph_mutex);
+	ida_init(&mdev->entity_internal_idx);
 
 	dev_dbg(mdev->dev, "Media device initialized\n");
 }
@@ -558,6 +560,8 @@ EXPORT_SYMBOL_GPL(media_device_init);
  */
 void media_device_cleanup(struct media_device *mdev)
 {
+	ida_destroy(&mdev->entity_internal_idx);
+	mdev->entity_internal_idx_max = 0;
 	mutex_destroy(&mdev->graph_mutex);
 }
 EXPORT_SYMBOL_GPL(media_device_cleanup);
@@ -658,6 +662,17 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 	INIT_LIST_HEAD(&entity->links);
 
 	spin_lock(&mdev->lock);
+
+	entity->internal_idx = ida_simple_get(&mdev->entity_internal_idx, 1, 0,
+					      GFP_KERNEL);
+	if (entity->internal_idx < 0) {
+		spin_unlock(&mdev->lock);
+		return entity->internal_idx;
+	}
+
+	mdev->entity_internal_idx_max =
+		max(mdev->entity_internal_idx_max, entity->internal_idx);
+
 	/* Initialize media_gobj embedded at the entity */
 	media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
 
@@ -690,6 +705,8 @@ void media_device_unregister_entity(struct media_entity *entity)
 
 	spin_lock(&mdev->lock);
 
+	ida_simple_remove(&mdev->entity_internal_idx, entity->internal_idx);
+
 	/* 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 a2c7570..c0e1764 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -30,6 +30,7 @@
 #include <media/media-devnode.h>
 #include <media/media-entity.h>
 
+struct ida;
 struct device;
 
 /**
@@ -47,6 +48,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_internal_idx: Allocated internal entity indices
  * @entities:	List of registered entities
  * @interfaces:	List of registered interfaces
  * @pads:	List of registered pads
@@ -82,6 +84,8 @@ struct media_device {
 	u32 pad_id;
 	u32 link_id;
 	u32 intf_devnode_id;
+	struct ida entity_internal_idx;
+	int entity_internal_idx_max;
 
 	struct list_head entities;
 	struct list_head interfaces;
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index eebdd24..d3d3a39 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
+ * @internal_idx: An unique internal 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;
+	int internal_idx;
 
 	struct media_pad *pads;
 	struct list_head links;
-- 
2.1.4

[PATCH 03/19] media: Add an API to manage entity enumerations

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

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>
---
 drivers/media/media-entity.c |  39 +++++++++++++
 include/media/media-device.h |  14 +++++
 include/media/media-entity.h | 128 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 173 insertions(+), 8 deletions(-)

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index d11f440..fceaf44 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -213,6 +213,45 @@ void media_gobj_remove(struct media_gobj *gobj)
 }
 
 /**
+ * __media_entity_enum_init - Initialise an entity enumeration
+ *
+ * @e: Entity enumeration to be initialised
+ * @idx_max: Maximum number of entities in the enumeration
+ *
+ * Returns zero on success or a negative error code.
+ */
+int __media_entity_enum_init(struct media_entity_enum *e, int idx_max)
+{
+	if (idx_max > MEDIA_ENTITY_ENUM_MAX_ID) {
+		e->e = kcalloc(DIV_ROUND_UP(idx_max, BITS_PER_LONG),
+			       sizeof(long), GFP_KERNEL);
+		if (!e->e)
+			return -ENOMEM;
+	} else {
+		e->e = e->__e;
+	}
+
+	bitmap_zero(e->e, idx_max);
+	e->idx_max = idx_max;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(__media_entity_enum_init);
+
+/**
+ * media_entity_enum_cleanup - Release resources of an entity enumeration
+ *
+ * @e: Entity enumeration to be released
+ */
+void media_entity_enum_cleanup(struct media_entity_enum *e)
+{
+	if (e->e != e->__e)
+		kfree(e->e);
+	e->e = NULL;
+}
+EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);
+
+/**
  * media_entity_init - Initialize a media entity
  *
  * @num_pads: Total number of sink and source pads.
diff --git a/include/media/media-device.h b/include/media/media-device.h
index c0e1764..2d46c66 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -110,6 +110,20 @@ struct media_device {
 /* media_devnode to media_device */
 #define to_media_device(node) container_of(node, struct media_device, devnode)
 
+/**
+ * media_entity_enum_init - Initialise an entity enumeration
+ *
+ * @e: Entity enumeration to be initialised
+ * @mdev: The related media device
+ *
+ * Returns zero on success or a negative error code.
+ */
+static inline __must_check int media_entity_enum_init(
+	struct media_entity_enum *e, struct media_device *mdev)
+{
+	return __media_entity_enum_init(e, mdev->entity_internal_idx_max + 1);
+}
+
 void media_device_init(struct media_device *mdev);
 void media_device_cleanup(struct media_device *mdev);
 int __must_check __media_device_register(struct media_device *mdev,
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index d3d3a39..fc54192 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>
@@ -71,6 +71,22 @@ struct media_gobj {
 	struct list_head	list;
 };
 
+#define MEDIA_ENTITY_ENUM_MAX_DEPTH	16
+#define MEDIA_ENTITY_ENUM_MAX_ID	64
+
+/*
+ * The number of pads can't be bigger than the number of entities,
+ * as the worse-case scenario is to have one entity linked up to
+ * MEDIA_ENTITY_ENUM_MAX_ID - 1 entities.
+ */
+#define MEDIA_ENTITY_MAX_PADS		(MEDIA_ENTITY_ENUM_MAX_ID - 1)
+
+struct media_entity_enum {
+	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);
+	unsigned long *e;
+	int idx_max;
+};
+
 struct media_pipeline {
 };
 
@@ -307,15 +323,111 @@ 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
+int __media_entity_enum_init(struct media_entity_enum *e, int idx_max);
+void media_entity_enum_cleanup(struct media_entity_enum *e);
 
-/*
- * The number of pads can't be bigger than the number of entities,
- * as the worse-case scenario is to have one entity linked up to
- * MEDIA_ENTITY_ENUM_MAX_ID - 1 entities.
+/**
+ * media_entity_enum_zero - Clear the entire enum
+ *
+ * @e: Entity enumeration to be cleared
  */
-#define MEDIA_ENTITY_MAX_PADS		(MEDIA_ENTITY_ENUM_MAX_ID - 1)
+static inline void media_entity_enum_zero(struct media_entity_enum *e)
+{
+	bitmap_zero(e->e, e->idx_max);
+}
+
+/**
+ * media_entity_enum_set - Mark a single entity in the enum
+ *
+ * @e: Entity enumeration
+ * @entity: Entity to be marked
+ */
+static inline void media_entity_enum_set(struct media_entity_enum *e,
+					 struct media_entity *entity)
+{
+	if (WARN_ON(entity->internal_idx >= e->idx_max))
+		return;
+
+	__set_bit(entity->internal_idx, e->e);
+}
+
+/**
+ * media_entity_enum_clear - Unmark a single entity in the enum
+ *
+ * @e: Entity enumeration
+ * @entity: Entity to be unmarked
+ */
+static inline void media_entity_enum_clear(struct media_entity_enum *e,
+					   struct media_entity *entity)
+{
+	if (WARN_ON(entity->internal_idx >= e->idx_max))
+		return;
+
+	__clear_bit(entity->internal_idx, e->e);
+}
+
+/**
+ * media_entity_enum_test - Test whether the entity is marked
+ *
+ * @e: Entity enumeration
+ * @entity: Entity to be tested
+ *
+ * Returns true if the entity was marked.
+ */
+static inline bool media_entity_enum_test(struct media_entity_enum *e,
+					  struct media_entity *entity)
+{
+	if (WARN_ON(entity->internal_idx >= e->idx_max))
+		return true;
+
+	return test_bit(entity->internal_idx, e->e);
+}
+
+/**
+ * media_entity_enum_test - Test whether the entity is marked, and mark it
+ *
+ * @e: Entity enumeration
+ * @entity: Entity to be tested
+ *
+ * Returns true if the entity was marked, and mark it before doing so.
+ */
+static inline bool media_entity_enum_test_and_set(struct media_entity_enum *e,
+						  struct media_entity *entity)
+{
+	if (WARN_ON(entity->internal_idx >= e->idx_max))
+		return true;
+
+	return __test_and_set_bit(entity->internal_idx, e->e);
+}
+
+/**
+ * media_entity_enum_test - Test whether the entire enum is empty
+ *
+ * @e: Entity enumeration
+ * @entity: Entity to be tested
+ *
+ * Returns true if the entity was marked.
+ */
+static inline bool media_entity_enum_empty(struct media_entity_enum *e)
+{
+	return bitmap_empty(e->e, e->idx_max);
+}
+
+/**
+ * media_entity_enum_intersects - Test whether two enums intersect
+ *
+ * @e: First entity enumeration
+ * @f: Second entity enumeration
+ *
+ * Returns true if entity enumerations e and f intersect, otherwise false.
+ */
+static inline bool media_entity_enum_intersects(struct media_entity_enum *e,
+						struct media_entity_enum *f)
+{
+	WARN_ON(e->idx_max != f->idx_max);
+
+	return bitmap_intersects(e->e, f->e, min(e->idx_max, f->idx_max));
+}
 
 struct media_entity_graph {
 	struct {
-- 
2.1.4

[PATCH 04/19] media: Move struct media_entity_graph definition up

[Sakari Ailus]

It will be needed in struct media_pipeline shortly.

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

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index fc54192..dde9a5f 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -87,6 +87,16 @@ struct media_entity_enum {
 	int idx_max;
 };
 
+struct media_entity_graph {
+	struct {
+		struct media_entity *entity;
+		struct list_head *link;
+	} stack[MEDIA_ENTITY_ENUM_MAX_DEPTH];
+
+	DECLARE_BITMAP(entities, MEDIA_ENTITY_ENUM_MAX_ID);
+	int top;
+};
+
 struct media_pipeline {
 };
 
@@ -429,16 +439,6 @@ static inline bool media_entity_enum_intersects(struct media_entity_enum *e,
 	return bitmap_intersects(e->e, f->e, min(e->idx_max, f->idx_max));
 }
 
-struct media_entity_graph {
-	struct {
-		struct media_entity *entity;
-		struct list_head *link;
-	} stack[MEDIA_ENTITY_ENUM_MAX_DEPTH];
-
-	DECLARE_BITMAP(entities, MEDIA_ENTITY_ENUM_MAX_ID);
-	int top;
-};
-
 #define gobj_to_entity(gobj) \
 		container_of(gobj, struct media_entity, graph_obj)
 
-- 
2.1.4

[PATCH 05/19] media: Move media graph state for streamon/off to the pipeline

[Sakari Ailus]

The struct media_entity_graph was allocated in the stack, limiting the
number of entities that could be reasonably allocated. Instead, move the
struct to struct media_pipeline which is typically allocated using
kmalloc() instead.

The intent is to keep the enumeration around for later use for the
duration of the streaming. As streaming is eventually stopped, an
unfortunate memory allocation failure would prevent stopping the
streaming. As no memory will need to be allocated, the problem is avoided
altogether.

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

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index fceaf44..667ab32 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -456,16 +456,16 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 					     struct media_pipeline *pipe)
 {
 	struct media_device *mdev = entity->graph_obj.mdev;
-	struct media_entity_graph graph;
+	struct media_entity_graph *graph = &pipe->graph;
 	struct media_entity *entity_err = entity;
 	struct media_link *link;
 	int ret;
 
 	mutex_lock(&mdev->graph_mutex);
 
-	media_entity_graph_walk_start(&graph, entity);
+	media_entity_graph_walk_start(graph, entity);
 
-	while ((entity = media_entity_graph_walk_next(&graph))) {
+	while ((entity = media_entity_graph_walk_next(graph))) {
 		DECLARE_BITMAP(active, MEDIA_ENTITY_MAX_PADS);
 		DECLARE_BITMAP(has_no_links, MEDIA_ENTITY_MAX_PADS);
 
@@ -546,9 +546,9 @@ error:
 	 * Link validation on graph failed. We revert what we did and
 	 * return the error.
 	 */
-	media_entity_graph_walk_start(&graph, entity_err);
+	media_entity_graph_walk_start(graph, entity_err);
 
-	while ((entity_err = media_entity_graph_walk_next(&graph))) {
+	while ((entity_err = media_entity_graph_walk_next(graph))) {
 		entity_err->stream_count--;
 		if (entity_err->stream_count == 0)
 			entity_err->pipe = NULL;
@@ -582,13 +582,13 @@ EXPORT_SYMBOL_GPL(media_entity_pipeline_start);
 void media_entity_pipeline_stop(struct media_entity *entity)
 {
 	struct media_device *mdev = entity->graph_obj.mdev;
-	struct media_entity_graph graph;
+	struct media_entity_graph *graph = &entity->pipe->graph;
 
 	mutex_lock(&mdev->graph_mutex);
 
-	media_entity_graph_walk_start(&graph, entity);
+	media_entity_graph_walk_start(graph, entity);
 
-	while ((entity = media_entity_graph_walk_next(&graph))) {
+	while ((entity = media_entity_graph_walk_next(graph))) {
 		entity->stream_count--;
 		if (entity->stream_count == 0)
 			entity->pipe = NULL;
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index dde9a5f..b2864cb 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -98,6 +98,8 @@ struct media_entity_graph {
 };
 
 struct media_pipeline {
+	/* For walking the graph in pipeline start / stop */
+	struct media_entity_graph graph;
 };
 
 /**
-- 
2.1.4

[PATCH 06/19] media: Amend media graph walk API by init and cleanup functions

[Sakari Ailus]

Add media_entity_graph_walk_init() and media_entity_graph_walk_cleanup()
functions in order to dynamically allocate memory for the graph. This is
not done in media_entity_graph_walk_start() as there are situations where
e.g. correct error handling, that itself may not fail, requires successful
graph walk.

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

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 667ab32..bf3c31f 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -353,14 +353,44 @@ static struct media_entity *stack_pop(struct media_entity_graph *graph)
 #define stack_top(en)	((en)->stack[(en)->top].entity)
 
 /**
+ * media_entity_graph_walk_init - Allocate resources for graph walk
+ * @graph: Media graph structure that will be used to walk the graph
+ * @mdev: Media device
+ *
+ * Reserve resources for graph walk in media device's current
+ * state. The memory must be released using
+ * media_entity_graph_walk_free().
+ *
+ * Returns error on failure, zero on success.
+ */
+__must_check int media_entity_graph_walk_init(
+	struct media_entity_graph *graph, struct media_device *mdev)
+{
+	return 0;
+}
+EXPORT_SYMBOL_GPL(media_entity_graph_walk_init);
+
+/**
+ * media_entity_graph_walk_cleanup - Release resources related to graph walking
+ * @graph: Media graph structure that was used to walk the graph
+ */
+void media_entity_graph_walk_cleanup(struct media_entity_graph *graph)
+{
+}
+EXPORT_SYMBOL_GPL(media_entity_graph_walk_cleanup);
+
+/**
  * media_entity_graph_walk_start - Start walking the media graph at a given entity
  * @graph: Media graph structure that will be used to walk the graph
  * @entity: Starting entity
  *
- * This function initializes the graph traversal structure to walk the entities
- * graph starting at the given entity. The traversal structure must not be
- * modified by the caller during graph traversal. When done the structure can
- * safely be freed.
+ * Before using this function, media_entity_graph_walk_init() must be
+ * used to allocate resources used for walking the graph. This
+ * function initializes the graph traversal structure to walk the
+ * entities graph starting at the given entity. The traversal
+ * structure must not be modified by the caller during graph
+ * traversal. After the graph walk, the resources must be released
+ * using media_entity_graph_walk_cleanup().
  */
 void media_entity_graph_walk_start(struct media_entity_graph *graph,
 				   struct media_entity *entity)
@@ -377,7 +407,6 @@ void media_entity_graph_walk_start(struct media_entity_graph *graph,
 }
 EXPORT_SYMBOL_GPL(media_entity_graph_walk_start);
 
-
 /**
  * media_entity_graph_walk_next - Get the next entity in the graph
  * @graph: Media graph structure
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index b2864cb..6e12b53 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -486,8 +486,11 @@ struct media_pad *media_entity_remote_pad(struct media_pad *pad);
 struct media_entity *media_entity_get(struct media_entity *entity);
 void media_entity_put(struct media_entity *entity);
 
+__must_check int media_entity_graph_walk_init(
+	struct media_entity_graph *graph, struct media_device *mdev);
+void media_entity_graph_walk_cleanup(struct media_entity_graph *graph);
 void media_entity_graph_walk_start(struct media_entity_graph *graph,
-		struct media_entity *entity);
+				   struct media_entity *entity);
 struct media_entity *
 media_entity_graph_walk_next(struct media_entity_graph *graph);
 __must_check int media_entity_pipeline_start(struct media_entity *entity,
-- 
2.1.4

[PATCH 07/19] media: Use the new media_entity_graph_walk_start()

[Sakari Ailus]

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

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index bf3c31f..4161dc7 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -492,7 +492,13 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 
 	mutex_lock(&mdev->graph_mutex);
 
-	media_entity_graph_walk_start(graph, entity);
+	ret = media_entity_graph_walk_init(&pipe->graph, mdev);
+	if (ret) {
+		mutex_unlock(&mdev->graph_mutex);
+		return ret;
+	}
+
+	media_entity_graph_walk_start(&pipe->graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(graph))) {
 		DECLARE_BITMAP(active, MEDIA_ENTITY_MAX_PADS);
@@ -590,6 +596,8 @@ error:
 			break;
 	}
 
+	media_entity_graph_walk_cleanup(graph);
+
 	mutex_unlock(&mdev->graph_mutex);
 
 	return ret;
@@ -623,6 +631,8 @@ void media_entity_pipeline_stop(struct media_entity *entity)
 			entity->pipe = NULL;
 	}
 
+	media_entity_graph_walk_cleanup(graph);
+
 	mutex_unlock(&mdev->graph_mutex);
 }
 EXPORT_SYMBOL_GPL(media_entity_pipeline_stop);
-- 
2.1.4

[PATCH 08/19] v4l: omap3isp: Use the new media_entity_graph_walk_start() interface

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/omap3isp/isp.c      | 63 ++++++++++++++++++------------
 drivers/media/platform/omap3isp/isp.h      |  4 +-
 drivers/media/platform/omap3isp/ispvideo.c | 19 ++++++++-
 drivers/media/platform/omap3isp/ispvideo.h |  1 +
 4 files changed, 60 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 0d1249f..4a01a36 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -683,14 +683,14 @@ static irqreturn_t isp_isr(int irq, void *_isp)
  *
  * Return the total number of users of all video device nodes in the pipeline.
  */
-static int isp_pipeline_pm_use_count(struct media_entity *entity)
+static int isp_pipeline_pm_use_count(struct media_entity *entity,
+	struct media_entity_graph *graph)
 {
-	struct media_entity_graph graph;
 	int use = 0;
 
-	media_entity_graph_walk_start(&graph, entity);
+	media_entity_graph_walk_start(graph, entity);
 
-	while ((entity = media_entity_graph_walk_next(&graph))) {
+	while ((entity = media_entity_graph_walk_next(graph))) {
 		if (is_media_entity_v4l2_io(entity))
 			use += entity->use_count;
 	}
@@ -742,27 +742,27 @@ static int isp_pipeline_pm_power_one(struct media_entity *entity, int change)
  *
  * Return 0 on success or a negative error code on failure.
  */
-static int isp_pipeline_pm_power(struct media_entity *entity, int change)
+static int isp_pipeline_pm_power(struct media_entity *entity, int change,
+	struct media_entity_graph *graph)
 {
-	struct media_entity_graph graph;
 	struct media_entity *first = entity;
 	int ret = 0;
 
 	if (!change)
 		return 0;
 
-	media_entity_graph_walk_start(&graph, entity);
+	media_entity_graph_walk_start(graph, entity);
 
-	while (!ret && (entity = media_entity_graph_walk_next(&graph)))
+	while (!ret && (entity = media_entity_graph_walk_next(graph)))
 		if (is_media_entity_v4l2_subdev(entity))
 			ret = isp_pipeline_pm_power_one(entity, change);
 
 	if (!ret)
-		return 0;
+		return ret;
 
-	media_entity_graph_walk_start(&graph, first);
+	media_entity_graph_walk_start(graph, first);
 
-	while ((first = media_entity_graph_walk_next(&graph))
+	while ((first = media_entity_graph_walk_next(graph))
 	       && first != entity)
 		if (is_media_entity_v4l2_subdev(first))
 			isp_pipeline_pm_power_one(first, -change);
@@ -782,7 +782,8 @@ static int isp_pipeline_pm_power(struct media_entity *entity, int change)
  * off is assumed to never fail. No failure can occur when the use parameter is
  * set to 0.
  */
-int omap3isp_pipeline_pm_use(struct media_entity *entity, int use)
+int omap3isp_pipeline_pm_use(struct media_entity *entity, int use,
+			     struct media_entity_graph *graph)
 {
 	int change = use ? 1 : -1;
 	int ret;
@@ -794,7 +795,7 @@ int omap3isp_pipeline_pm_use(struct media_entity *entity, int use)
 	WARN_ON(entity->use_count < 0);
 
 	/* Apply power change to connected non-nodes. */
-	ret = isp_pipeline_pm_power(entity, change);
+	ret = isp_pipeline_pm_power(entity, change, graph);
 	if (ret < 0)
 		entity->use_count -= change;
 
@@ -820,35 +821,49 @@ int omap3isp_pipeline_pm_use(struct media_entity *entity, int use)
 static int isp_pipeline_link_notify(struct media_link *link, u32 flags,
 				    unsigned int notification)
 {
+	struct media_entity_graph *graph =
+		&container_of(link->graph_obj.mdev, struct isp_device,
+			      media_dev)->pm_count_graph;
 	struct media_entity *source = link->source->entity;
 	struct media_entity *sink = link->sink->entity;
-	int source_use = isp_pipeline_pm_use_count(source);
-	int sink_use = isp_pipeline_pm_use_count(sink);
-	int ret;
+	int source_use;
+	int sink_use;
+	int ret = 0;
+
+	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH) {
+		ret = media_entity_graph_walk_init(graph,
+						   link->graph_obj.mdev);
+		if (ret)
+			return ret;
+	}
+
+	source_use = isp_pipeline_pm_use_count(source, graph);
+	sink_use = isp_pipeline_pm_use_count(sink, graph);
 
 	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
 	    !(flags & MEDIA_LNK_FL_ENABLED)) {
 		/* Powering off entities is assumed to never fail. */
-		isp_pipeline_pm_power(source, -sink_use);
-		isp_pipeline_pm_power(sink, -source_use);
+		isp_pipeline_pm_power(source, -sink_use, graph);
+		isp_pipeline_pm_power(sink, -source_use, graph);
 		return 0;
 	}
 
 	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
 		(flags & MEDIA_LNK_FL_ENABLED)) {
 
-		ret = isp_pipeline_pm_power(source, sink_use);
+		ret = isp_pipeline_pm_power(source, sink_use, graph);
 		if (ret < 0)
 			return ret;
 
-		ret = isp_pipeline_pm_power(sink, source_use);
+		ret = isp_pipeline_pm_power(sink, source_use, graph);
 		if (ret < 0)
-			isp_pipeline_pm_power(source, -sink_use);
-
-		return ret;
+			isp_pipeline_pm_power(source, -sink_use, graph);
 	}
 
-	return 0;
+	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH)
+		media_entity_graph_walk_cleanup(graph);
+
+	return ret;
 }
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 5acc2e6..b6f81f2 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -176,6 +176,7 @@ struct isp_device {
 	struct v4l2_device v4l2_dev;
 	struct v4l2_async_notifier notifier;
 	struct media_device media_dev;
+	struct media_entity_graph pm_count_graph;
 	struct device *dev;
 	u32 revision;
 
@@ -265,7 +266,8 @@ void omap3isp_subclk_enable(struct isp_device *isp,
 void omap3isp_subclk_disable(struct isp_device *isp,
 			     enum isp_subclk_resource res);
 
-int omap3isp_pipeline_pm_use(struct media_entity *entity, int use);
+int omap3isp_pipeline_pm_use(struct media_entity *entity, int use,
+			     struct media_entity_graph *graph);
 
 int omap3isp_register_entities(struct platform_device *pdev,
 			       struct v4l2_device *v4l2_dev);
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 52843ac..e68ec2f 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -227,8 +227,15 @@ static int isp_video_get_graph_data(struct isp_video *video,
 	struct media_entity *entity = &video->video.entity;
 	struct media_device *mdev = entity->graph_obj.mdev;
 	struct isp_video *far_end = NULL;
+	int ret;
 
 	mutex_lock(&mdev->graph_mutex);
+	ret = media_entity_graph_walk_init(&graph, entity->graph_obj.mdev);
+	if (ret) {
+		mutex_unlock(&mdev->graph_mutex);
+		return ret;
+	}
+
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
@@ -252,6 +259,8 @@ static int isp_video_get_graph_data(struct isp_video *video,
 
 	mutex_unlock(&mdev->graph_mutex);
 
+	media_entity_graph_walk_cleanup(&graph);
+
 	if (video->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
 		pipe->input = far_end;
 		pipe->output = video;
@@ -1242,7 +1251,12 @@ static int isp_video_open(struct file *file)
 		goto done;
 	}
 
-	ret = omap3isp_pipeline_pm_use(&video->video.entity, 1);
+	ret = media_entity_graph_walk_init(&handle->graph,
+					   &video->isp->media_dev);
+	if (ret)
+		goto done;
+
+	ret = omap3isp_pipeline_pm_use(&video->video.entity, 1, &handle->graph);
 	if (ret < 0) {
 		omap3isp_put(video->isp);
 		goto done;
@@ -1273,6 +1287,7 @@ static int isp_video_open(struct file *file)
 done:
 	if (ret < 0) {
 		v4l2_fh_del(&handle->vfh);
+		media_entity_graph_walk_cleanup(&handle->graph);
 		kfree(handle);
 	}
 
@@ -1292,7 +1307,7 @@ static int isp_video_release(struct file *file)
 	vb2_queue_release(&handle->queue);
 	mutex_unlock(&video->queue_lock);
 
-	omap3isp_pipeline_pm_use(&video->video.entity, 0);
+	omap3isp_pipeline_pm_use(&video->video.entity, 0, &handle->graph);
 
 	/* Release the file handle. */
 	v4l2_fh_del(vfh);
diff --git a/drivers/media/platform/omap3isp/ispvideo.h b/drivers/media/platform/omap3isp/ispvideo.h
index 4071dd7..6c498ea 100644
--- a/drivers/media/platform/omap3isp/ispvideo.h
+++ b/drivers/media/platform/omap3isp/ispvideo.h
@@ -189,6 +189,7 @@ struct isp_video_fh {
 	struct vb2_queue queue;
 	struct v4l2_format format;
 	struct v4l2_fract timeperframe;
+	struct media_entity_graph graph;
 };
 
 #define to_isp_video_fh(fh)	container_of(fh, struct isp_video_fh, vfh)
-- 
2.1.4

[PATCH 09/19] v4l: exynos4-is: Use the new media_entity_graph_walk_start() interface

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/exynos4-is/media-dev.c | 31 +++++++++++++++++----------
 drivers/media/platform/exynos4-is/media-dev.h |  1 +
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index d55b4f3..704040c 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -1046,10 +1046,10 @@ static int __fimc_md_modify_pipeline(struct media_entity *entity, bool enable)
 }
 
 /* Locking: called with entity->graph_obj.mdev->graph_mutex mutex held. */
-static int __fimc_md_modify_pipelines(struct media_entity *entity, bool enable)
+static int __fimc_md_modify_pipelines(struct media_entity *entity, bool enable,
+				      struct media_entity_graph *graph)
 {
 	struct media_entity *entity_err = entity;
-	struct media_entity_graph graph;
 	int ret;
 
 	/*
@@ -1058,9 +1058,9 @@ static int __fimc_md_modify_pipelines(struct media_entity *entity, bool enable)
 	 * through active links. This is needed as we cannot power on/off the
 	 * subdevs in random order.
 	 */
-	media_entity_graph_walk_start(&graph, entity);
+	media_entity_graph_walk_start(graph, entity);
 
-	while ((entity = media_entity_graph_walk_next(&graph))) {
+	while ((entity = media_entity_graph_walk_next(graph))) {
 		if (!is_media_entity_v4l2_io(entity))
 			continue;
 
@@ -1071,10 +1071,11 @@ static int __fimc_md_modify_pipelines(struct media_entity *entity, bool enable)
 	}
 
 	return 0;
- err:
-	media_entity_graph_walk_start(&graph, entity_err);
 
-	while ((entity_err = media_entity_graph_walk_next(&graph))) {
+err:
+	media_entity_graph_walk_start(graph, entity_err);
+
+	while ((entity_err = media_entity_graph_walk_next(graph))) {
 		if (!is_media_entity_v4l2_io(entity_err))
 			continue;
 
@@ -1090,21 +1091,29 @@ static int __fimc_md_modify_pipelines(struct media_entity *entity, bool enable)
 static int fimc_md_link_notify(struct media_link *link, unsigned int flags,
 				unsigned int notification)
 {
+	struct media_entity_graph *graph =
+		&container_of(link->graph_obj.mdev, struct fimc_md,
+			      media_dev)->link_setup_graph;
 	struct media_entity *sink = link->sink->entity;
 	int ret = 0;
 
 	/* Before link disconnection */
 	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH) {
+		ret = media_entity_graph_walk_init(graph,
+						   link->graph_obj.mdev);
+		if (ret)
+			return ret;
 		if (!(flags & MEDIA_LNK_FL_ENABLED))
-			ret = __fimc_md_modify_pipelines(sink, false);
+			ret = __fimc_md_modify_pipelines(sink, false, graph);
 #if 0
 		else
 			/* TODO: Link state change validation */
 #endif
 	/* After link activation */
-	} else if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
-		   (link->flags & MEDIA_LNK_FL_ENABLED)) {
-		ret = __fimc_md_modify_pipelines(sink, true);
+	} else if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH) {
+		if (link->flags & MEDIA_LNK_FL_ENABLED)
+			ret = __fimc_md_modify_pipelines(sink, true, graph);
+		media_entity_graph_walk_cleanup(graph);
 	}
 
 	return ret ? -EPIPE : 0;
diff --git a/drivers/media/platform/exynos4-is/media-dev.h b/drivers/media/platform/exynos4-is/media-dev.h
index 9a69913..e80c55d 100644
--- a/drivers/media/platform/exynos4-is/media-dev.h
+++ b/drivers/media/platform/exynos4-is/media-dev.h
@@ -154,6 +154,7 @@ struct fimc_md {
 	bool user_subdev_api;
 	spinlock_t slock;
 	struct list_head pipelines;
+	struct media_entity_graph link_setup_graph;
 };
 
 static inline
-- 
2.1.4

[PATCH 10/19] v4l: xilinx: Use the new media_entity_graph_walk_start() interface

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/platform/xilinx/xilinx-dma.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/platform/xilinx/xilinx-dma.c b/drivers/media/platform/xilinx/xilinx-dma.c
index bc244a0..0a19824 100644
--- a/drivers/media/platform/xilinx/xilinx-dma.c
+++ b/drivers/media/platform/xilinx/xilinx-dma.c
@@ -182,10 +182,17 @@ static int xvip_pipeline_validate(struct xvip_pipeline *pipe,
 	struct media_device *mdev = entity->graph_obj.mdev;
 	unsigned int num_inputs = 0;
 	unsigned int num_outputs = 0;
+	int ret;
 
 	mutex_lock(&mdev->graph_mutex);
 
 	/* Walk the graph to locate the video nodes. */
+	ret = media_entity_graph_walk_init(&graph, entity->graph_obj.mdev);
+	if (ret) {
+		mutex_unlock(&mdev->graph_mutex);
+		return ret;
+	}
+
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
@@ -206,6 +213,8 @@ static int xvip_pipeline_validate(struct xvip_pipeline *pipe,
 
 	mutex_unlock(&mdev->graph_mutex);
 
+	media_entity_graph_walk_cleanup(&graph);
+
 	/* We need exactly one output and zero or one input. */
 	if (num_outputs != 1 || num_inputs > 1)
 		return -EPIPE;
-- 
2.1.4

[PATCH 11/19] v4l: vsp1: Use the new media_entity_graph_walk_start() interface

[Sakari Ailus]

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

diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index f741582..ce10d86 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -415,6 +415,12 @@ static int vsp1_pipeline_validate(struct vsp1_pipeline *pipe,
 	mutex_lock(&mdev->graph_mutex);
 
 	/* Walk the graph to locate the entities and video nodes. */
+	ret = media_entity_graph_walk_init(&graph, mdev);
+	if (ret) {
+		mutex_unlock(&mdev->graph_mutex);
+		return ret;
+	}
+
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
@@ -448,6 +454,8 @@ static int vsp1_pipeline_validate(struct vsp1_pipeline *pipe,
 
 	mutex_unlock(&mdev->graph_mutex);
 
+	media_entity_graph_walk_cleanup(&graph);
+
 	/* We need one output and at least one input. */
 	if (pipe->num_inputs == 0 || !pipe->output) {
 		ret = -EPIPE;
-- 
2.1.4

[PATCH 12/19] media: Use entity enums in graph walk

[Sakari Ailus]

This will also mean that the necessary graph related data structures will
be allocated dynamically, removing the need for maximum ID checks.

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

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 4161dc7..7429c03 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -366,7 +366,7 @@ static struct media_entity *stack_pop(struct media_entity_graph *graph)
 __must_check int media_entity_graph_walk_init(
 	struct media_entity_graph *graph, struct media_device *mdev)
 {
-	return 0;
+	return media_entity_enum_init(&graph->entities, mdev);
 }
 EXPORT_SYMBOL_GPL(media_entity_graph_walk_init);
 
@@ -376,6 +376,7 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_init);
  */
 void media_entity_graph_walk_cleanup(struct media_entity_graph *graph)
 {
+	media_entity_enum_cleanup(&graph->entities);
 }
 EXPORT_SYMBOL_GPL(media_entity_graph_walk_cleanup);
 
@@ -395,14 +396,11 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_cleanup);
 void media_entity_graph_walk_start(struct media_entity_graph *graph,
 				   struct media_entity *entity)
 {
+	media_entity_enum_zero(&graph->entities);
+	media_entity_enum_set(&graph->entities, entity);
+
 	graph->top = 0;
 	graph->stack[graph->top].entity = NULL;
-	bitmap_zero(graph->entities, MEDIA_ENTITY_ENUM_MAX_ID);
-
-	if (WARN_ON(media_entity_id(entity) >= MEDIA_ENTITY_ENUM_MAX_ID))
-		return;
-
-	__set_bit(media_entity_id(entity), graph->entities);
 	stack_push(graph, entity);
 }
 EXPORT_SYMBOL_GPL(media_entity_graph_walk_start);
@@ -445,11 +443,9 @@ 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))
-			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 6e12b53..21fd07b 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -93,8 +93,8 @@ struct media_entity_graph {
 		struct list_head *link;
 	} stack[MEDIA_ENTITY_ENUM_MAX_DEPTH];
 
-	DECLARE_BITMAP(entities, MEDIA_ENTITY_ENUM_MAX_ID);
 	int top;
+	struct media_entity_enum entities;
 };
 
 struct media_pipeline {
-- 
2.1.4

[PATCH 13/19] media: Keep using the same graph walk object for a given pipeline

[Sakari Ailus]

Initialise a given graph walk object once, and then keep using it whilst
the same pipeline is running. Once the pipeline is stopped, release the
graph walk object.

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

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 7429c03..137aa09d 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -488,10 +488,10 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
 
 	mutex_lock(&mdev->graph_mutex);
 
-	ret = media_entity_graph_walk_init(&pipe->graph, mdev);
-	if (ret) {
-		mutex_unlock(&mdev->graph_mutex);
-		return ret;
+	if (!pipe->streaming_count++) {
+		ret = media_entity_graph_walk_init(&pipe->graph, mdev);
+		if (ret)
+			goto error_graph_walk_start;
 	}
 
 	media_entity_graph_walk_start(&pipe->graph, entity);
@@ -592,7 +592,9 @@ error:
 			break;
 	}
 
-	media_entity_graph_walk_cleanup(graph);
+error_graph_walk_start:
+	if (!--pipe->streaming_count)
+		media_entity_graph_walk_cleanup(graph);
 
 	mutex_unlock(&mdev->graph_mutex);
 
@@ -616,9 +618,11 @@ void media_entity_pipeline_stop(struct media_entity *entity)
 {
 	struct media_device *mdev = entity->graph_obj.mdev;
 	struct media_entity_graph *graph = &entity->pipe->graph;
+	struct media_pipeline *pipe = entity->pipe;
 
 	mutex_lock(&mdev->graph_mutex);
 
+	BUG_ON(!pipe->streaming_count);
 	media_entity_graph_walk_start(graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(graph))) {
@@ -627,7 +631,8 @@ void media_entity_pipeline_stop(struct media_entity *entity)
 			entity->pipe = NULL;
 	}
 
-	media_entity_graph_walk_cleanup(graph);
+	if (!--pipe->streaming_count)
+		media_entity_graph_walk_cleanup(graph);
 
 	mutex_unlock(&mdev->graph_mutex);
 }
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 21fd07b..cc01e08 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -98,6 +98,7 @@ struct media_entity_graph {
 };
 
 struct media_pipeline {
+	int streaming_count;
 	/* For walking the graph in pipeline start / stop */
 	struct media_entity_graph graph;
 };
-- 
2.1.4

[PATCH 14/19] v4l: omap3isp: Use media entity enumeration API

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

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

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 4a01a36..61c128e 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -896,7 +896,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);
@@ -989,7 +989,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
@@ -1041,10 +1040,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;
 		}
 	}
@@ -1250,7 +1248,7 @@ static int isp_reset(struct isp_device *isp)
 	}
 
 	isp->stop_failure = false;
-	isp->crashed = 0;
+	media_entity_enum_zero(&isp->crashed);
 	return 0;
 }
 
@@ -1661,7 +1659,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);
 	}
@@ -2201,6 +2200,8 @@ static int isp_remove(struct platform_device *pdev)
 	isp_detach_iommu(isp);
 	__omap3isp_put(isp, false);
 
+	media_entity_enum_cleanup(&isp->crashed);
+
 	return 0;
 }
 
@@ -2348,6 +2349,10 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 	struct isp_bus_cfg *bus;
 	int ret;
 
+	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
+	if (ret)
+		return ret;
+
 	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
 		/* Only try to link entities whose interface was set on bound */
 		if (sd->host_priv) {
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index b6f81f2..6a1288d 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.
@@ -195,7 +196,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;
+	struct 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..80cf550 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 e68ec2f..579332b 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -241,7 +241,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;
@@ -899,7 +899,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)
@@ -907,7 +906,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. */
@@ -959,8 +958,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
@@ -1026,7 +1025,9 @@ 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;
+	ret = media_entity_enum_init(&pipe->entities, &video->isp->media_dev);
+	if (ret)
+		goto err_enum_init;
 
 	/* TODO: Implement PM QoS */
 	pipe->l3_ick = clk_get_rate(video->isp->clock[ISP_CLK_L3_ICK]);
@@ -1100,6 +1101,9 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	}
 
 	mutex_unlock(&video->stream_lock);
+
+	media_entity_enum_cleanup(&pipe->entities);
+
 	return 0;
 
 err_set_stream:
@@ -1120,7 +1124,11 @@ err_pipeline_start:
 	INIT_LIST_HEAD(&video->dmaqueue);
 	video->queue = NULL;
 
+	media_entity_enum_cleanup(&pipe->entities);
+
+err_enum_init:
 	mutex_unlock(&video->stream_lock);
+
 	return ret;
 }
 
diff --git a/drivers/media/platform/omap3isp/ispvideo.h b/drivers/media/platform/omap3isp/ispvideo.h
index 6c498ea..9f08492 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;
+	struct media_entity_enum entities;
 	unsigned long l3_ick;
 	unsigned int max_rate;
 	enum v4l2_field field;
-- 
2.1.4

[PATCH 15/19] v4l: vsp1: Use media entity enumeration API

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

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

diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index ce10d86..b3fd2be 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -311,24 +311,35 @@ static int vsp1_pipeline_validate_branch(struct vsp1_pipeline *pipe,
 					 struct vsp1_rwpf *output)
 {
 	struct vsp1_entity *entity;
-	unsigned int entities = 0;
+	struct media_entity_enum entities;
 	struct media_pad *pad;
+	int rval;
 	bool bru_found = false;
 
 	input->location.left = 0;
 	input->location.top = 0;
 
+	rval = media_entity_enum_init(
+		&entities, input->entity.pads[RWPF_PAD_SOURCE].graph_obj.mdev);
+	if (rval)
+		return rval;
+
 	pad = media_entity_remote_pad(&input->entity.pads[RWPF_PAD_SOURCE]);
 
 	while (1) {
-		if (pad == NULL)
-			return -EPIPE;
+		if (pad == NULL) {
+			rval = -EPIPE;
+			goto out;
+		}
 
 		/* We've reached a video node, that shouldn't have happened. */
-		if (!is_media_entity_v4l2_subdev(pad->entity))
-			return -EPIPE;
+		if (!is_media_entity_v4l2_subdev(pad->entity)) {
+			rval = -EPIPE;
+			goto out;
+		}
 
-		entity = to_vsp1_entity(media_entity_to_v4l2_subdev(pad->entity));
+		entity = to_vsp1_entity(
+			media_entity_to_v4l2_subdev(pad->entity));
 
 		/* A BRU is present in the pipeline, store the compose rectangle
 		 * location in the input RPF for use when configuring the RPF.
@@ -351,15 +362,18 @@ 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)))
-			return -EPIPE;
-
-		entities |= 1 << media_entity_id(&entity->subdev.entity);
+		if (media_entity_enum_test_and_set(&entities,
+						   &entity->subdev.entity)) {
+			rval = -EPIPE;
+			goto out;
+		}
 
 		/* UDS can't be chained. */
 		if (entity->type == VSP1_ENTITY_UDS) {
-			if (pipe->uds)
-				return -EPIPE;
+			if (pipe->uds) {
+				rval = -EPIPE;
+				goto out;
+			}
 
 			pipe->uds = entity;
 			pipe->uds_input = bru_found ? pipe->bru
@@ -377,9 +391,12 @@ static int vsp1_pipeline_validate_branch(struct vsp1_pipeline *pipe,
 
 	/* The last entity must be the output WPF. */
 	if (entity != &output->entity)
-		return -EPIPE;
+		rval = -EPIPE;
 
-	return 0;
+out:
+	media_entity_enum_cleanup(&entities);
+
+	return rval;
 }
 
 static void __vsp1_pipeline_cleanup(struct vsp1_pipeline *pipe)
-- 
2.1.4

[PATCH 16/19] staging: omap4iss: Fix sub-device power management code

[Sakari Ailus]

The same bug was present in the omap4iss driver as was in the omap3isp
driver. The code got copied to the omap4iss driver while broken. Fix the
omap4iss driver as well.

Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
---
 drivers/staging/media/omap4iss/iss.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index 076ddd4..c097fd5 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -533,14 +533,14 @@ static int iss_pipeline_link_notify(struct media_link *link, u32 flags,
 	int ret;
 
 	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
-	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
+	    !(flags & MEDIA_LNK_FL_ENABLED)) {
 		/* Powering off entities is assumed to never fail. */
 		iss_pipeline_pm_power(source, -sink_use);
 		iss_pipeline_pm_power(sink, -source_use);
 		return 0;
 	}
 
-	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
+	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
 		(flags & MEDIA_LNK_FL_ENABLED)) {
 		ret = iss_pipeline_pm_power(source, sink_use);
 		if (ret < 0)
-- 
2.1.4

[PATCH 17/19] staging: v4l: omap4iss: Use media entity enum API

[Sakari Ailus]

From: Sakari Ailus <sakari.ailus@linux.intel.com>

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

diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index c097fd5..7b0561f 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_zero(&iss->crashed);
+
 	return 0;
 }
 
@@ -1090,7 +1091,7 @@ void omap4iss_put(struct iss_device *iss)
 		 * be worth investigating whether resetting the ISP only can't
 		 * fix the problem in some cases.
 		 */
-		if (iss->crashed)
+		if (!media_entity_enum_empty(&iss->crashed))
 			iss_reset(iss);
 		iss_disable_clocks(iss);
 	}
@@ -1490,6 +1491,10 @@ static int iss_probe(struct platform_device *pdev)
 	if (ret < 0)
 		goto error_modules;
 
+	ret = media_entity_enum_init(&iss->crashed, &iss->media_dev);
+	if (ret)
+		goto error_entities;
+
 	ret = iss_create_pads_links(iss);
 	if (ret < 0)
 		goto error_entities;
@@ -1500,6 +1505,7 @@ static int iss_probe(struct platform_device *pdev)
 
 error_entities:
 	iss_unregister_entities(iss);
+	media_entity_enum_cleanup(&iss->crashed);
 error_modules:
 	iss_cleanup_modules(iss);
 error_iss:
@@ -1517,6 +1523,7 @@ static int iss_remove(struct platform_device *pdev)
 	struct iss_device *iss = platform_get_drvdata(pdev);
 
 	iss_unregister_entities(iss);
+	media_entity_enum_cleanup(&iss->crashed);
 	iss_cleanup_modules(iss);
 
 	return 0;
diff --git a/drivers/staging/media/omap4iss/iss.h b/drivers/staging/media/omap4iss/iss.h
index 35df8b4..5dd0d99 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;
+	struct 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..b56f999 100644
--- a/drivers/staging/media/omap4iss/iss_video.c
+++ b/drivers/staging/media/omap4iss/iss_video.c
@@ -761,6 +761,10 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	if (type != video->type)
 		return -EINVAL;
 
+	ret = media_entity_enum_init(&pipe->entities, entity->graph_obj.mdev);
+	if (ret)
+		return ret;
+
 	mutex_lock(&video->stream_lock);
 
 	/* Start streaming on the pipeline. No link touching an entity in the
@@ -771,7 +775,6 @@ 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;
 
 	if (video->iss->pdata->set_constraints)
 		video->iss->pdata->set_constraints(video->iss, true);
@@ -783,7 +786,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.
@@ -854,6 +857,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	}
 
 	mutex_unlock(&video->stream_lock);
+
 	return 0;
 
 err_omap4iss_set_stream:
@@ -866,6 +870,9 @@ err_media_entity_pipeline_start:
 	video->queue = NULL;
 
 	mutex_unlock(&video->stream_lock);
+
+	media_entity_enum_cleanup(&pipe->entities);
+
 	return ret;
 }
 
@@ -903,6 +910,8 @@ iss_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 	vb2_streamoff(&vfh->queue, type);
 	video->queue = NULL;
 
+	media_entity_enum_cleanup(&pipe->entities);
+
 	if (video->iss->pdata->set_constraints)
 		video->iss->pdata->set_constraints(video->iss, false);
 	media_entity_pipeline_stop(&video->video.entity);
diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
index f11fce2..b5d3a96 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;
+	struct media_entity_enum entities;
 	atomic_t frame_number;
 	bool do_propagation; /* of frame number */
 	bool error;
-- 
2.1.4

[PATCH 18/19] staging: v4l: omap4iss: Use the new media_entity_graph_walk_start() interface

[Sakari Ailus]

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

diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index 7b0561f..ab5cba4 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -389,14 +389,14 @@ static irqreturn_t iss_isr(int irq, void *_iss)
  *
  * Return the total number of users of all video device nodes in the pipeline.
  */
-static int iss_pipeline_pm_use_count(struct media_entity *entity)
+static int iss_pipeline_pm_use_count(struct media_entity *entity,
+				     struct media_entity_graph *graph)
 {
-	struct media_entity_graph graph;
 	int use = 0;
 
-	media_entity_graph_walk_start(&graph, entity);
+	media_entity_graph_walk_start(graph, entity);
 
-	while ((entity = media_entity_graph_walk_next(&graph))) {
+	while ((entity = media_entity_graph_walk_next(graph))) {
 		if (is_media_entity_v4l2_io(entity))
 			use += entity->use_count;
 	}
@@ -449,27 +449,27 @@ static int iss_pipeline_pm_power_one(struct media_entity *entity, int change)
  *
  * Return 0 on success or a negative error code on failure.
  */
-static int iss_pipeline_pm_power(struct media_entity *entity, int change)
+static int iss_pipeline_pm_power(struct media_entity *entity, int change,
+				 struct media_entity_graph *graph)
 {
-	struct media_entity_graph graph;
 	struct media_entity *first = entity;
 	int ret = 0;
 
 	if (!change)
 		return 0;
 
-	media_entity_graph_walk_start(&graph, entity);
+	media_entity_graph_walk_start(graph, entity);
 
-	while (!ret && (entity = media_entity_graph_walk_next(&graph)))
+	while (!ret && (entity = media_entity_graph_walk_next(graph)))
 		if (is_media_entity_v4l2_subdev(entity))
 			ret = iss_pipeline_pm_power_one(entity, change);
 
 	if (!ret)
 		return 0;
 
-	media_entity_graph_walk_start(&graph, first);
+	media_entity_graph_walk_start(graph, first);
 
-	while ((first = media_entity_graph_walk_next(&graph))
+	while ((first = media_entity_graph_walk_next(graph))
 	       && first != entity)
 		if (is_media_entity_v4l2_subdev(first))
 			iss_pipeline_pm_power_one(first, -change);
@@ -489,7 +489,8 @@ static int iss_pipeline_pm_power(struct media_entity *entity, int change)
  * off is assumed to never fail. No failure can occur when the use parameter is
  * set to 0.
  */
-int omap4iss_pipeline_pm_use(struct media_entity *entity, int use)
+int omap4iss_pipeline_pm_use(struct media_entity *entity, int use,
+			     struct media_entity_graph *graph)
 {
 	int change = use ? 1 : -1;
 	int ret;
@@ -501,7 +502,7 @@ int omap4iss_pipeline_pm_use(struct media_entity *entity, int use)
 	WARN_ON(entity->use_count < 0);
 
 	/* Apply power change to connected non-nodes. */
-	ret = iss_pipeline_pm_power(entity, change);
+	ret = iss_pipeline_pm_power(entity, change, graph);
 	if (ret < 0)
 		entity->use_count -= change;
 
@@ -526,34 +527,48 @@ int omap4iss_pipeline_pm_use(struct media_entity *entity, int use)
 static int iss_pipeline_link_notify(struct media_link *link, u32 flags,
 				    unsigned int notification)
 {
+	struct media_entity_graph *graph =
+		&container_of(link->graph_obj.mdev, struct iss_device,
+			      media_dev)->pm_count_graph;
 	struct media_entity *source = link->source->entity;
 	struct media_entity *sink = link->sink->entity;
-	int source_use = iss_pipeline_pm_use_count(source);
-	int sink_use = iss_pipeline_pm_use_count(sink);
+	int source_use;
+	int sink_use;
 	int ret;
 
+	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH) {
+		ret = media_entity_graph_walk_init(graph,
+						   link->graph_obj.mdev);
+		if (ret)
+			return ret;
+	}
+
+	source_use = iss_pipeline_pm_use_count(source, graph);
+	sink_use = iss_pipeline_pm_use_count(sink, graph);
+
 	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
 	    !(flags & MEDIA_LNK_FL_ENABLED)) {
 		/* Powering off entities is assumed to never fail. */
-		iss_pipeline_pm_power(source, -sink_use);
-		iss_pipeline_pm_power(sink, -source_use);
+		iss_pipeline_pm_power(source, -sink_use, graph);
+		iss_pipeline_pm_power(sink, -source_use, graph);
 		return 0;
 	}
 
 	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
 		(flags & MEDIA_LNK_FL_ENABLED)) {
-		ret = iss_pipeline_pm_power(source, sink_use);
+		ret = iss_pipeline_pm_power(source, sink_use, graph);
 		if (ret < 0)
 			return ret;
 
-		ret = iss_pipeline_pm_power(sink, source_use);
+		ret = iss_pipeline_pm_power(sink, source_use, graph);
 		if (ret < 0)
-			iss_pipeline_pm_power(source, -sink_use);
-
-		return ret;
+			iss_pipeline_pm_power(source, -sink_use, graph);
 	}
 
-	return 0;
+	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH)
+		media_entity_graph_walk_cleanup(graph);
+
+	return ret;
 }
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/staging/media/omap4iss/iss.h b/drivers/staging/media/omap4iss/iss.h
index 5dd0d99..ee7dc08 100644
--- a/drivers/staging/media/omap4iss/iss.h
+++ b/drivers/staging/media/omap4iss/iss.h
@@ -87,6 +87,7 @@ struct iss_reg {
 struct iss_device {
 	struct v4l2_device v4l2_dev;
 	struct media_device media_dev;
+	struct media_entity_graph pm_count_graph;
 	struct device *dev;
 	u32 revision;
 
@@ -151,7 +152,8 @@ void omap4iss_isp_subclk_enable(struct iss_device *iss,
 void omap4iss_isp_subclk_disable(struct iss_device *iss,
 				 enum iss_isp_subclk_resource res);
 
-int omap4iss_pipeline_pm_use(struct media_entity *entity, int use);
+int omap4iss_pipeline_pm_use(struct media_entity *entity, int use,
+			     struct media_entity_graph *graph);
 
 int omap4iss_register_entities(struct platform_device *pdev,
 			       struct v4l2_device *v4l2_dev);
diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c
index b56f999..6cfb65e 100644
--- a/drivers/staging/media/omap4iss/iss_video.c
+++ b/drivers/staging/media/omap4iss/iss_video.c
@@ -210,6 +210,12 @@ iss_video_far_end(struct iss_video *video)
 	struct iss_video *far_end = NULL;
 
 	mutex_lock(&mdev->graph_mutex);
+
+	if (media_entity_graph_walk_init(&graph, mdev)) {
+		mutex_unlock(&mdev->graph_mutex);
+		return NULL;
+	}
+
 	media_entity_graph_walk_start(&graph, entity);
 
 	while ((entity = media_entity_graph_walk_next(&graph))) {
@@ -227,6 +233,9 @@ iss_video_far_end(struct iss_video *video)
 	}
 
 	mutex_unlock(&mdev->graph_mutex);
+
+	media_entity_graph_walk_cleanup(&graph);
+
 	return far_end;
 }
 
@@ -751,7 +760,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	struct iss_video_fh *vfh = to_iss_video_fh(fh);
 	struct iss_video *video = video_drvdata(file);
 	struct media_entity_graph graph;
-	struct media_entity *entity;
+	struct media_entity *entity = &video->video.entity;
 	enum iss_pipeline_state state;
 	struct iss_pipeline *pipe;
 	struct iss_video *far_end;
@@ -779,12 +788,14 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	if (video->iss->pdata->set_constraints)
 		video->iss->pdata->set_constraints(video->iss, true);
 
+	ret = media_entity_enum_init(&pipe->entities, &video->iss->media_dev);
+	if (ret)
+		goto err_enum_init;
+
 	ret = media_entity_pipeline_start(&video->video.entity, &pipe->pipe);
 	if (ret < 0)
 		goto err_media_entity_pipeline_start;
 
-	entity = &video->video.entity;
-	media_entity_graph_walk_start(&graph, entity);
 	while ((entity = media_entity_graph_walk_next(&graph)))
 		media_entity_enum_set(&pipe->entities, entity);
 
@@ -858,6 +869,8 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 
 	mutex_unlock(&video->stream_lock);
 
+	media_entity_graph_walk_cleanup(&graph);
+
 	return 0;
 
 err_omap4iss_set_stream:
@@ -869,10 +882,11 @@ err_media_entity_pipeline_start:
 		video->iss->pdata->set_constraints(video->iss, false);
 	video->queue = NULL;
 
-	mutex_unlock(&video->stream_lock);
-
 	media_entity_enum_cleanup(&pipe->entities);
 
+err_enum_init:
+	mutex_unlock(&video->stream_lock);
+
 	return ret;
 }
 
@@ -994,7 +1008,13 @@ static int iss_video_open(struct file *file)
 		goto done;
 	}
 
-	ret = omap4iss_pipeline_pm_use(&video->video.entity, 1);
+	ret = media_entity_graph_walk_init(&handle->graph,
+					   &video->iss->media_dev);
+	if (ret)
+		goto done;
+
+	ret = omap4iss_pipeline_pm_use(&video->video.entity, 1,
+				       &handle->graph);
 	if (ret < 0) {
 		omap4iss_put(video->iss);
 		goto done;
@@ -1033,6 +1053,7 @@ static int iss_video_open(struct file *file)
 done:
 	if (ret < 0) {
 		v4l2_fh_del(&handle->vfh);
+		media_entity_graph_walk_cleanup(&handle->graph);
 		kfree(handle);
 	}
 
@@ -1048,12 +1069,13 @@ static int iss_video_release(struct file *file)
 	/* Disable streaming and free the buffers queue resources. */
 	iss_video_streamoff(file, vfh, video->type);
 
-	omap4iss_pipeline_pm_use(&video->video.entity, 0);
+	omap4iss_pipeline_pm_use(&video->video.entity, 0, &handle->graph);
 
 	/* Release the videobuf2 queue */
 	vb2_queue_release(&handle->queue);
 
 	/* Release the file handle. */
+	media_entity_graph_walk_cleanup(&handle->graph);
 	v4l2_fh_del(vfh);
 	kfree(handle);
 	file->private_data = NULL;
diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
index b5d3a96..2ba4fb9 100644
--- a/drivers/staging/media/omap4iss/iss_video.h
+++ b/drivers/staging/media/omap4iss/iss_video.h
@@ -183,6 +183,7 @@ struct iss_video_fh {
 	struct vb2_queue queue;
 	struct v4l2_format format;
 	struct v4l2_fract timeperframe;
+	struct media_entity_graph graph;
 };
 
 #define to_iss_video_fh(fh)	container_of(fh, struct iss_video_fh, vfh)
-- 
2.1.4

[PATCH 19/19] media: Rename MEDIA_ENTITY_ENUM_MAX_ID as MEDIA_ENTITY_ENUM_STACK_ALLOC

[Sakari Ailus]

The purpose of the macro has changed, rename it accordingly. It is not and
should no longer be used in drivers directly, but only for the purpose for
defining how many bits can be allocated from the stack for entity
enumerations.

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

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index 137aa09d..feca976 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -222,7 +222,7 @@ void media_gobj_remove(struct media_gobj *gobj)
  */
 int __media_entity_enum_init(struct media_entity_enum *e, int idx_max)
 {
-	if (idx_max > MEDIA_ENTITY_ENUM_MAX_ID) {
+	if (idx_max > MEDIA_ENTITY_ENUM_STACK_ALLOC) {
 		e->e = kcalloc(DIV_ROUND_UP(idx_max, BITS_PER_LONG),
 			       sizeof(long), GFP_KERNEL);
 		if (!e->e)
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index cc01e08..5cab165 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -72,17 +72,17 @@ struct media_gobj {
 };
 
 #define MEDIA_ENTITY_ENUM_MAX_DEPTH	16
-#define MEDIA_ENTITY_ENUM_MAX_ID	64
+#define MEDIA_ENTITY_ENUM_STACK_ALLOC	64
 
 /*
  * The number of pads can't be bigger than the number of entities,
  * as the worse-case scenario is to have one entity linked up to
- * MEDIA_ENTITY_ENUM_MAX_ID - 1 entities.
+ * MEDIA_ENTITY_ENUM_STACK_ALLOC - 1 entities.
  */
-#define MEDIA_ENTITY_MAX_PADS		(MEDIA_ENTITY_ENUM_MAX_ID - 1)
+#define MEDIA_ENTITY_MAX_PADS		(MEDIA_ENTITY_ENUM_STACK_ALLOC - 1)
 
 struct media_entity_enum {
-	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);
+	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_STACK_ALLOC);
 	unsigned long *e;
 	int idx_max;
 };
-- 
2.1.4

Re: [PATCH 01/19] media: Enforce single entity->pipe in a pipeline

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:32 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> If a different entity->pipe in a pipeline was encountered, a warning was
> issued but the execution continued as if nothing had happened. Return an
> error instead right there.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

> ---
>  drivers/media/media-entity.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 66b8db0..d11f440 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -431,7 +431,12 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  		DECLARE_BITMAP(has_no_links, MEDIA_ENTITY_MAX_PADS);
>  
>  		entity->stream_count++;
> -		WARN_ON(entity->pipe && entity->pipe != pipe);
> +
> +		if (WARN_ON(entity->pipe && entity->pipe != pipe)) {
> +			ret = -EBUSY;
> +			goto error;
> +		}
> +
>  		entity->pipe = pipe;
>  
>  		/* Already streaming --- no need to check. */


-- 

Cheers,
Mauro

Re: [PATCH 02/19] media: Introduce internal index for media entities

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:33 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> The internal index can be used internally by the framework in order to keep
> track of entities for a purpose or another. The internal index is constant
> while it's registered to a media device, but the same index may be re-used
> once the entity having that index is unregistered.

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

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-device.c | 17 +++++++++++++++++
>  include/media/media-device.h |  4 ++++
>  include/media/media-entity.h |  3 +++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index c181758..ebb84cb 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -22,6 +22,7 @@
>  
>  #include <linux/compat.h>
>  #include <linux/export.h>
> +#include <linux/idr.h>
>  #include <linux/ioctl.h>
>  #include <linux/media.h>
>  #include <linux/types.h>
> @@ -546,6 +547,7 @@ void media_device_init(struct media_device *mdev)
>  	INIT_LIST_HEAD(&mdev->links);
>  	spin_lock_init(&mdev->lock);
>  	mutex_init(&mdev->graph_mutex);
> +	ida_init(&mdev->entity_internal_idx);
>  
>  	dev_dbg(mdev->dev, "Media device initialized\n");
>  }
> @@ -558,6 +560,8 @@ EXPORT_SYMBOL_GPL(media_device_init);
>   */
>  void media_device_cleanup(struct media_device *mdev)
>  {
> +	ida_destroy(&mdev->entity_internal_idx);
> +	mdev->entity_internal_idx_max = 0;
>  	mutex_destroy(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_device_cleanup);
> @@ -658,6 +662,17 @@ int __must_check media_device_register_entity(struct media_device *mdev,
>  	INIT_LIST_HEAD(&entity->links);
>  
>  	spin_lock(&mdev->lock);
> +
> +	entity->internal_idx = ida_simple_get(&mdev->entity_internal_idx, 1, 0,
> +					      GFP_KERNEL);
> +	if (entity->internal_idx < 0) {
> +		spin_unlock(&mdev->lock);
> +		return entity->internal_idx;
> +	}
> +
> +	mdev->entity_internal_idx_max =
> +		max(mdev->entity_internal_idx_max, entity->internal_idx);
> +
>  	/* Initialize media_gobj embedded at the entity */
>  	media_gobj_init(mdev, MEDIA_GRAPH_ENTITY, &entity->graph_obj);
>  
> @@ -690,6 +705,8 @@ void media_device_unregister_entity(struct media_entity *entity)
>  
>  	spin_lock(&mdev->lock);
>  
> +	ida_simple_remove(&mdev->entity_internal_idx, entity->internal_idx);
> +
>  	/* 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 a2c7570..c0e1764 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -30,6 +30,7 @@
>  #include <media/media-devnode.h>
>  #include <media/media-entity.h>
>  
> +struct ida;
>  struct device;
>  
>  /**
> @@ -47,6 +48,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_internal_idx: Allocated internal entity indices
>   * @entities:	List of registered entities
>   * @interfaces:	List of registered interfaces
>   * @pads:	List of registered pads
> @@ -82,6 +84,8 @@ struct media_device {
>  	u32 pad_id;
>  	u32 link_id;
>  	u32 intf_devnode_id;
> +	struct ida entity_internal_idx;
> +	int entity_internal_idx_max;
>  
>  	struct list_head entities;
>  	struct list_head interfaces;
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index eebdd24..d3d3a39 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
> + * @internal_idx: An unique internal 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;
> +	int internal_idx;
>  
>  	struct media_pad *pads;
>  	struct list_head links;


-- 

Cheers,
Mauro

Re: [PATCH 04/19] media: Move struct media_entity_graph definition up

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:35 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> It will be needed in struct media_pipeline shortly.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
(but see below)

> ---
>  include/media/media-entity.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index fc54192..dde9a5f 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -87,6 +87,16 @@ struct media_entity_enum {
>  	int idx_max;
>  };
>  
> +struct media_entity_graph {

Not a problem on this patch itself, but since you're touching this
struct, it would be nice to take the opportunity and document it ;)

Regards,
Mauro

> +	struct {
> +		struct media_entity *entity;
> +		struct list_head *link;
> +	} stack[MEDIA_ENTITY_ENUM_MAX_DEPTH];
> +
> +	DECLARE_BITMAP(entities, MEDIA_ENTITY_ENUM_MAX_ID);
> +	int top;
> +};
> +
>  struct media_pipeline {
>  };
>  
> @@ -429,16 +439,6 @@ static inline bool media_entity_enum_intersects(struct media_entity_enum *e,
>  	return bitmap_intersects(e->e, f->e, min(e->idx_max, f->idx_max));
>  }
>  
> -struct media_entity_graph {
> -	struct {
> -		struct media_entity *entity;
> -		struct list_head *link;
> -	} stack[MEDIA_ENTITY_ENUM_MAX_DEPTH];
> -
> -	DECLARE_BITMAP(entities, MEDIA_ENTITY_ENUM_MAX_ID);
> -	int top;
> -};
> -
>  #define gobj_to_entity(gobj) \
>  		container_of(gobj, struct media_entity, graph_obj)
>  


-- 

Cheers,
Mauro

Re: [PATCH 05/19] media: Move media graph state for streamon/off to the pipeline

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:36 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> The struct media_entity_graph was allocated in the stack, limiting the
> number of entities that could be reasonably allocated. Instead, move the
> struct to struct media_pipeline which is typically allocated using
> kmalloc() instead.
> 
> The intent is to keep the enumeration around for later use for the
> duration of the streaming. As streaming is eventually stopped, an
> unfortunate memory allocation failure would prevent stopping the
> streaming. As no memory will need to be allocated, the problem is avoided
> altogether.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-entity.c | 16 ++++++++--------
>  include/media/media-entity.h |  2 ++
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index fceaf44..667ab32 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -456,16 +456,16 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  					     struct media_pipeline *pipe)
>  {
>  	struct media_device *mdev = entity->graph_obj.mdev;
> -	struct media_entity_graph graph;
> +	struct media_entity_graph *graph = &pipe->graph;
>  	struct media_entity *entity_err = entity;
>  	struct media_link *link;
>  	int ret;
>  
>  	mutex_lock(&mdev->graph_mutex);
>  
> -	media_entity_graph_walk_start(&graph, entity);
> +	media_entity_graph_walk_start(graph, entity);
>  
> -	while ((entity = media_entity_graph_walk_next(&graph))) {
> +	while ((entity = media_entity_graph_walk_next(graph))) {
>  		DECLARE_BITMAP(active, MEDIA_ENTITY_MAX_PADS);
>  		DECLARE_BITMAP(has_no_links, MEDIA_ENTITY_MAX_PADS);
>  
> @@ -546,9 +546,9 @@ error:
>  	 * Link validation on graph failed. We revert what we did and
>  	 * return the error.
>  	 */
> -	media_entity_graph_walk_start(&graph, entity_err);
> +	media_entity_graph_walk_start(graph, entity_err);
>  
> -	while ((entity_err = media_entity_graph_walk_next(&graph))) {
> +	while ((entity_err = media_entity_graph_walk_next(graph))) {
>  		entity_err->stream_count--;
>  		if (entity_err->stream_count == 0)
>  			entity_err->pipe = NULL;
> @@ -582,13 +582,13 @@ EXPORT_SYMBOL_GPL(media_entity_pipeline_start);
>  void media_entity_pipeline_stop(struct media_entity *entity)
>  {
>  	struct media_device *mdev = entity->graph_obj.mdev;
> -	struct media_entity_graph graph;
> +	struct media_entity_graph *graph = &entity->pipe->graph;
>  
>  	mutex_lock(&mdev->graph_mutex);
>  
> -	media_entity_graph_walk_start(&graph, entity);
> +	media_entity_graph_walk_start(graph, entity);
>  
> -	while ((entity = media_entity_graph_walk_next(&graph))) {
> +	while ((entity = media_entity_graph_walk_next(graph))) {
>  		entity->stream_count--;
>  		if (entity->stream_count == 0)
>  			entity->pipe = NULL;
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index dde9a5f..b2864cb 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -98,6 +98,8 @@ struct media_entity_graph {
>  };
>  
>  struct media_pipeline {
> +	/* For walking the graph in pipeline start / stop */
> +	struct media_entity_graph graph;
>  };

Please use the kernel-doc format for documenting struct.

After this change:

Reviewed-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>  
>  /**


-- 

Cheers,
Mauro

Re: [PATCH 07/19] media: Use the new media_entity_graph_walk_start()

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:38 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Please add some documentation at the body for all patches.

Btw, IMHO, it would be best to fold this patch and the following ones
that are related to media_entity_graph_walk_init() altogether, as it
makes easier to review if all places were covered.

> ---
>  drivers/media/media-entity.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index bf3c31f..4161dc7 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -492,7 +492,13 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  
>  	mutex_lock(&mdev->graph_mutex);
>  
> -	media_entity_graph_walk_start(graph, entity);
> +	ret = media_entity_graph_walk_init(&pipe->graph, mdev);
> +	if (ret) {
> +		mutex_unlock(&mdev->graph_mutex);
> +		return ret;
> +	}
> +
> +	media_entity_graph_walk_start(&pipe->graph, entity);
>  
>  	while ((entity = media_entity_graph_walk_next(graph))) {
>  		DECLARE_BITMAP(active, MEDIA_ENTITY_MAX_PADS);
> @@ -590,6 +596,8 @@ error:
>  			break;
>  	}
>  
> +	media_entity_graph_walk_cleanup(graph);
> +
>  	mutex_unlock(&mdev->graph_mutex);
>  
>  	return ret;
> @@ -623,6 +631,8 @@ void media_entity_pipeline_stop(struct media_entity *entity)
>  			entity->pipe = NULL;
>  	}
>  
> +	media_entity_graph_walk_cleanup(graph);
> +
>  	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_entity_pipeline_stop);


-- 

Cheers,
Mauro

Re: [PATCH 12/19] media: Use entity enums in graph walk

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:43 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> This will also mean that the necessary graph related data structures will
> be allocated dynamically, removing the need for maximum ID checks.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

> ---
>  drivers/media/media-entity.c | 16 ++++++----------
>  include/media/media-entity.h |  2 +-
>  2 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 4161dc7..7429c03 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -366,7 +366,7 @@ static struct media_entity *stack_pop(struct media_entity_graph *graph)
>  __must_check int media_entity_graph_walk_init(
>  	struct media_entity_graph *graph, struct media_device *mdev)
>  {
> -	return 0;
> +	return media_entity_enum_init(&graph->entities, mdev);
>  }
>  EXPORT_SYMBOL_GPL(media_entity_graph_walk_init);
>  
> @@ -376,6 +376,7 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_init);
>   */
>  void media_entity_graph_walk_cleanup(struct media_entity_graph *graph)
>  {
> +	media_entity_enum_cleanup(&graph->entities);
>  }
>  EXPORT_SYMBOL_GPL(media_entity_graph_walk_cleanup);
>  
> @@ -395,14 +396,11 @@ EXPORT_SYMBOL_GPL(media_entity_graph_walk_cleanup);
>  void media_entity_graph_walk_start(struct media_entity_graph *graph,
>  				   struct media_entity *entity)
>  {
> +	media_entity_enum_zero(&graph->entities);
> +	media_entity_enum_set(&graph->entities, entity);
> +
>  	graph->top = 0;
>  	graph->stack[graph->top].entity = NULL;
> -	bitmap_zero(graph->entities, MEDIA_ENTITY_ENUM_MAX_ID);
> -
> -	if (WARN_ON(media_entity_id(entity) >= MEDIA_ENTITY_ENUM_MAX_ID))
> -		return;
> -
> -	__set_bit(media_entity_id(entity), graph->entities);
>  	stack_push(graph, entity);
>  }
>  EXPORT_SYMBOL_GPL(media_entity_graph_walk_start);
> @@ -445,11 +443,9 @@ 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))
> -			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 6e12b53..21fd07b 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -93,8 +93,8 @@ struct media_entity_graph {
>  		struct list_head *link;
>  	} stack[MEDIA_ENTITY_ENUM_MAX_DEPTH];
>  
> -	DECLARE_BITMAP(entities, MEDIA_ENTITY_ENUM_MAX_ID);
>  	int top;
> +	struct media_entity_enum entities;
>  };
>  
>  struct media_pipeline {


-- 

Cheers,
Mauro

Re: [PATCH 13/19] media: Keep using the same graph walk object for a given pipeline

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:44 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Initialise a given graph walk object once, and then keep using it whilst
> the same pipeline is running. Once the pipeline is stopped, release the
> graph walk object.

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

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-entity.c | 17 +++++++++++------
>  include/media/media-entity.h |  1 +
>  2 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 7429c03..137aa09d 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -488,10 +488,10 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
>  
>  	mutex_lock(&mdev->graph_mutex);
>  
> -	ret = media_entity_graph_walk_init(&pipe->graph, mdev);
> -	if (ret) {
> -		mutex_unlock(&mdev->graph_mutex);
> -		return ret;
> +	if (!pipe->streaming_count++) {
> +		ret = media_entity_graph_walk_init(&pipe->graph, mdev);
> +		if (ret)
> +			goto error_graph_walk_start;
>  	}
>  
>  	media_entity_graph_walk_start(&pipe->graph, entity);
> @@ -592,7 +592,9 @@ error:
>  			break;
>  	}
>  
> -	media_entity_graph_walk_cleanup(graph);
> +error_graph_walk_start:
> +	if (!--pipe->streaming_count)
> +		media_entity_graph_walk_cleanup(graph);
>  
>  	mutex_unlock(&mdev->graph_mutex);
>  
> @@ -616,9 +618,11 @@ void media_entity_pipeline_stop(struct media_entity *entity)
>  {
>  	struct media_device *mdev = entity->graph_obj.mdev;
>  	struct media_entity_graph *graph = &entity->pipe->graph;
> +	struct media_pipeline *pipe = entity->pipe;
>  
>  	mutex_lock(&mdev->graph_mutex);
>  
> +	BUG_ON(!pipe->streaming_count);
>  	media_entity_graph_walk_start(graph, entity);
>  
>  	while ((entity = media_entity_graph_walk_next(graph))) {
> @@ -627,7 +631,8 @@ void media_entity_pipeline_stop(struct media_entity *entity)
>  			entity->pipe = NULL;
>  	}
>  
> -	media_entity_graph_walk_cleanup(graph);
> +	if (!--pipe->streaming_count)
> +		media_entity_graph_walk_cleanup(graph);
>  
>  	mutex_unlock(&mdev->graph_mutex);
>  }
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 21fd07b..cc01e08 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -98,6 +98,7 @@ struct media_entity_graph {
>  };
>  
>  struct media_pipeline {
> +	int streaming_count;
>  	/* For walking the graph in pipeline start / stop */
>  	struct media_entity_graph graph;
>  };


-- 

Cheers,
Mauro

Re: [PATCH 14/19] v4l: omap3isp: Use media entity enumeration API

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:45 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/omap3isp/isp.c      | 21 +++++++++++++--------
>  drivers/media/platform/omap3isp/isp.h      |  5 +++--
>  drivers/media/platform/omap3isp/ispccdc.c  |  2 +-
>  drivers/media/platform/omap3isp/ispvideo.c | 20 ++++++++++++++------
>  drivers/media/platform/omap3isp/ispvideo.h |  4 ++--
>  5 files changed, 33 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index 4a01a36..61c128e 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -896,7 +896,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;

If the size of entities/crashed enums is different, it should be
returning an error, I guess, as this would be a driver's problem, and the
graph traversal on OMAP3 would likely be wrong.

>  
>  	spin_lock_irqsave(&pipe->lock, flags);
> @@ -989,7 +989,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
> @@ -1041,10 +1040,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;
>  		}
>  	}
> @@ -1250,7 +1248,7 @@ static int isp_reset(struct isp_device *isp)
>  	}
>  
>  	isp->stop_failure = false;
> -	isp->crashed = 0;
> +	media_entity_enum_zero(&isp->crashed);
>  	return 0;
>  }
>  
> @@ -1661,7 +1659,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);
>  	}
> @@ -2201,6 +2200,8 @@ static int isp_remove(struct platform_device *pdev)
>  	isp_detach_iommu(isp);
>  	__omap3isp_put(isp, false);
>  
> +	media_entity_enum_cleanup(&isp->crashed);
> +
>  	return 0;
>  }
>  
> @@ -2348,6 +2349,10 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
>  	struct isp_bus_cfg *bus;
>  	int ret;
>  
> +	ret = media_entity_enum_init(&isp->crashed, &isp->media_dev);
> +	if (ret)
> +		return ret;
> +
>  	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
>  		/* Only try to link entities whose interface was set on bound */
>  		if (sd->host_priv) {
> diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
> index b6f81f2..6a1288d 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.
> @@ -195,7 +196,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;
> +	struct 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..80cf550 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 e68ec2f..579332b 100644
> --- a/drivers/media/platform/omap3isp/ispvideo.c
> +++ b/drivers/media/platform/omap3isp/ispvideo.c
> @@ -241,7 +241,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;
> @@ -899,7 +899,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)
> @@ -907,7 +906,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. */
> @@ -959,8 +958,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
> @@ -1026,7 +1025,9 @@ 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;
> +	ret = media_entity_enum_init(&pipe->entities, &video->isp->media_dev);
> +	if (ret)
> +		goto err_enum_init;
>  
>  	/* TODO: Implement PM QoS */
>  	pipe->l3_ick = clk_get_rate(video->isp->clock[ISP_CLK_L3_ICK]);
> @@ -1100,6 +1101,9 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>  	}
>  
>  	mutex_unlock(&video->stream_lock);
> +
> +	media_entity_enum_cleanup(&pipe->entities);
> +
>  	return 0;
>  
>  err_set_stream:
> @@ -1120,7 +1124,11 @@ err_pipeline_start:
>  	INIT_LIST_HEAD(&video->dmaqueue);
>  	video->queue = NULL;
>  
> +	media_entity_enum_cleanup(&pipe->entities);
> +
> +err_enum_init:
>  	mutex_unlock(&video->stream_lock);
> +
>  	return ret;
>  }
>  
> diff --git a/drivers/media/platform/omap3isp/ispvideo.h b/drivers/media/platform/omap3isp/ispvideo.h
> index 6c498ea..9f08492 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;
> +	struct media_entity_enum entities;
>  	unsigned long l3_ick;
>  	unsigned int max_rate;
>  	enum v4l2_field field;


-- 

Cheers,
Mauro

Re: [PATCH 15/19] v4l: vsp1: Use media entity enumeration API

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:46 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> From: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 45 ++++++++++++++++++++++----------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
> index ce10d86..b3fd2be 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -311,24 +311,35 @@ static int vsp1_pipeline_validate_branch(struct vsp1_pipeline *pipe,
>  					 struct vsp1_rwpf *output)
>  {
>  	struct vsp1_entity *entity;
> -	unsigned int entities = 0;
> +	struct media_entity_enum entities;
>  	struct media_pad *pad;
> +	int rval;
>  	bool bru_found = false;
>  
>  	input->location.left = 0;
>  	input->location.top = 0;
>  
> +	rval = media_entity_enum_init(
> +		&entities, input->entity.pads[RWPF_PAD_SOURCE].graph_obj.mdev);
> +	if (rval)
> +		return rval;
> +
>  	pad = media_entity_remote_pad(&input->entity.pads[RWPF_PAD_SOURCE]);
>  
>  	while (1) {
> -		if (pad == NULL)
> -			return -EPIPE;
> +		if (pad == NULL) {
> +			rval = -EPIPE;
> +			goto out;
> +		}
>  
>  		/* We've reached a video node, that shouldn't have happened. */
> -		if (!is_media_entity_v4l2_subdev(pad->entity))
> -			return -EPIPE;
> +		if (!is_media_entity_v4l2_subdev(pad->entity)) {
> +			rval = -EPIPE;
> +			goto out;
> +		}
>  
> -		entity = to_vsp1_entity(media_entity_to_v4l2_subdev(pad->entity));
> +		entity = to_vsp1_entity(
> +			media_entity_to_v4l2_subdev(pad->entity));
>  
>  		/* A BRU is present in the pipeline, store the compose rectangle
>  		 * location in the input RPF for use when configuring the RPF.
> @@ -351,15 +362,18 @@ 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)))
> -			return -EPIPE;
> -
> -		entities |= 1 << media_entity_id(&entity->subdev.entity);
> +		if (media_entity_enum_test_and_set(&entities,
> +						   &entity->subdev.entity)) {
> +			rval = -EPIPE;
> +			goto out;
> +		}
>  
>  		/* UDS can't be chained. */
>  		if (entity->type == VSP1_ENTITY_UDS) {
> -			if (pipe->uds)
> -				return -EPIPE;
> +			if (pipe->uds) {
> +				rval = -EPIPE;
> +				goto out;
> +			}
>  
>  			pipe->uds = entity;
>  			pipe->uds_input = bru_found ? pipe->bru
> @@ -377,9 +391,12 @@ static int vsp1_pipeline_validate_branch(struct vsp1_pipeline *pipe,
>  
>  	/* The last entity must be the output WPF. */
>  	if (entity != &output->entity)
> -		return -EPIPE;
> +		rval = -EPIPE;
>  
> -	return 0;
> +out:
> +	media_entity_enum_cleanup(&entities);
> +
> +	return rval;
>  }
>  
>  static void __vsp1_pipeline_cleanup(struct vsp1_pipeline *pipe)


-- 

Cheers,
Mauro

Re: [PATCH 16/19] staging: omap4iss: Fix sub-device power management code

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:47 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> The same bug was present in the omap4iss driver as was in the omap3isp
> driver. The code got copied to the omap4iss driver while broken. Fix the
> omap4iss driver as well.

Looks ok.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
>  drivers/staging/media/omap4iss/iss.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index 076ddd4..c097fd5 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -533,14 +533,14 @@ static int iss_pipeline_link_notify(struct media_link *link, u32 flags,
>  	int ret;
>  
>  	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> -	    !(link->flags & MEDIA_LNK_FL_ENABLED)) {
> +	    !(flags & MEDIA_LNK_FL_ENABLED)) {
>  		/* Powering off entities is assumed to never fail. */
>  		iss_pipeline_pm_power(source, -sink_use);
>  		iss_pipeline_pm_power(sink, -source_use);
>  		return 0;
>  	}
>  
> -	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
>  		(flags & MEDIA_LNK_FL_ENABLED)) {
>  		ret = iss_pipeline_pm_power(source, sink_use);
>  		if (ret < 0)


-- 

Cheers,
Mauro

Re: [PATCH 17/19] staging: v4l: omap4iss: Use media entity enum API

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:48 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Where are the comments?

> ---
>  drivers/staging/media/omap4iss/iss.c       | 15 +++++++++++----
>  drivers/staging/media/omap4iss/iss.h       |  4 ++--
>  drivers/staging/media/omap4iss/iss_video.c | 13 +++++++++++--
>  drivers/staging/media/omap4iss/iss_video.h |  4 ++--
>  4 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index c097fd5..7b0561f 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;

Should return error if the enum sizes are different.

>  
>  	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_zero(&iss->crashed);
> +
>  	return 0;
>  }
>  
> @@ -1090,7 +1091,7 @@ void omap4iss_put(struct iss_device *iss)
>  		 * be worth investigating whether resetting the ISP only can't
>  		 * fix the problem in some cases.
>  		 */
> -		if (iss->crashed)
> +		if (!media_entity_enum_empty(&iss->crashed))
>  			iss_reset(iss);
>  		iss_disable_clocks(iss);
>  	}
> @@ -1490,6 +1491,10 @@ static int iss_probe(struct platform_device *pdev)
>  	if (ret < 0)
>  		goto error_modules;
>  
> +	ret = media_entity_enum_init(&iss->crashed, &iss->media_dev);
> +	if (ret)
> +		goto error_entities;
> +
>  	ret = iss_create_pads_links(iss);
>  	if (ret < 0)
>  		goto error_entities;
> @@ -1500,6 +1505,7 @@ static int iss_probe(struct platform_device *pdev)
>  
>  error_entities:
>  	iss_unregister_entities(iss);
> +	media_entity_enum_cleanup(&iss->crashed);
>  error_modules:
>  	iss_cleanup_modules(iss);
>  error_iss:
> @@ -1517,6 +1523,7 @@ static int iss_remove(struct platform_device *pdev)
>  	struct iss_device *iss = platform_get_drvdata(pdev);
>  
>  	iss_unregister_entities(iss);
> +	media_entity_enum_cleanup(&iss->crashed);
>  	iss_cleanup_modules(iss);
>  
>  	return 0;
> diff --git a/drivers/staging/media/omap4iss/iss.h b/drivers/staging/media/omap4iss/iss.h
> index 35df8b4..5dd0d99 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;
> +	struct 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..b56f999 100644
> --- a/drivers/staging/media/omap4iss/iss_video.c
> +++ b/drivers/staging/media/omap4iss/iss_video.c
> @@ -761,6 +761,10 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>  	if (type != video->type)
>  		return -EINVAL;
>  
> +	ret = media_entity_enum_init(&pipe->entities, entity->graph_obj.mdev);
> +	if (ret)
> +		return ret;
> +
>  	mutex_lock(&video->stream_lock);
>  
>  	/* Start streaming on the pipeline. No link touching an entity in the
> @@ -771,7 +775,6 @@ 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;
>  
>  	if (video->iss->pdata->set_constraints)
>  		video->iss->pdata->set_constraints(video->iss, true);
> @@ -783,7 +786,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.
> @@ -854,6 +857,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>  	}
>  
>  	mutex_unlock(&video->stream_lock);
> +
>  	return 0;
>  
>  err_omap4iss_set_stream:
> @@ -866,6 +870,9 @@ err_media_entity_pipeline_start:
>  	video->queue = NULL;
>  
>  	mutex_unlock(&video->stream_lock);
> +
> +	media_entity_enum_cleanup(&pipe->entities);
> +
>  	return ret;
>  }
>  
> @@ -903,6 +910,8 @@ iss_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
>  	vb2_streamoff(&vfh->queue, type);
>  	video->queue = NULL;
>  
> +	media_entity_enum_cleanup(&pipe->entities);
> +
>  	if (video->iss->pdata->set_constraints)
>  		video->iss->pdata->set_constraints(video->iss, false);
>  	media_entity_pipeline_stop(&video->video.entity);
> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
> index f11fce2..b5d3a96 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;
> +	struct media_entity_enum entities;
>  	atomic_t frame_number;
>  	bool do_propagation; /* of frame number */
>  	bool error;


-- 

Cheers,
Mauro

Re: [PATCH 18/19] staging: v4l: omap4iss: Use the new media_entity_graph_walk_start() interface

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:49 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Comments?

> ---
>  drivers/staging/media/omap4iss/iss.c       | 59 +++++++++++++++++++-----------
>  drivers/staging/media/omap4iss/iss.h       |  4 +-
>  drivers/staging/media/omap4iss/iss_video.c | 36 ++++++++++++++----
>  drivers/staging/media/omap4iss/iss_video.h |  1 +
>  4 files changed, 70 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index 7b0561f..ab5cba4 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -389,14 +389,14 @@ static irqreturn_t iss_isr(int irq, void *_iss)
>   *
>   * Return the total number of users of all video device nodes in the pipeline.
>   */
> -static int iss_pipeline_pm_use_count(struct media_entity *entity)
> +static int iss_pipeline_pm_use_count(struct media_entity *entity,
> +				     struct media_entity_graph *graph)
>  {
> -	struct media_entity_graph graph;
>  	int use = 0;
>  
> -	media_entity_graph_walk_start(&graph, entity);
> +	media_entity_graph_walk_start(graph, entity);
>  
> -	while ((entity = media_entity_graph_walk_next(&graph))) {
> +	while ((entity = media_entity_graph_walk_next(graph))) {
>  		if (is_media_entity_v4l2_io(entity))
>  			use += entity->use_count;
>  	}
> @@ -449,27 +449,27 @@ static int iss_pipeline_pm_power_one(struct media_entity *entity, int change)
>   *
>   * Return 0 on success or a negative error code on failure.
>   */
> -static int iss_pipeline_pm_power(struct media_entity *entity, int change)
> +static int iss_pipeline_pm_power(struct media_entity *entity, int change,
> +				 struct media_entity_graph *graph)
>  {
> -	struct media_entity_graph graph;
>  	struct media_entity *first = entity;
>  	int ret = 0;
>  
>  	if (!change)
>  		return 0;
>  
> -	media_entity_graph_walk_start(&graph, entity);
> +	media_entity_graph_walk_start(graph, entity);
>  
> -	while (!ret && (entity = media_entity_graph_walk_next(&graph)))
> +	while (!ret && (entity = media_entity_graph_walk_next(graph)))
>  		if (is_media_entity_v4l2_subdev(entity))
>  			ret = iss_pipeline_pm_power_one(entity, change);
>  
>  	if (!ret)
>  		return 0;
>  
> -	media_entity_graph_walk_start(&graph, first);
> +	media_entity_graph_walk_start(graph, first);
>  
> -	while ((first = media_entity_graph_walk_next(&graph))
> +	while ((first = media_entity_graph_walk_next(graph))
>  	       && first != entity)
>  		if (is_media_entity_v4l2_subdev(first))
>  			iss_pipeline_pm_power_one(first, -change);
> @@ -489,7 +489,8 @@ static int iss_pipeline_pm_power(struct media_entity *entity, int change)
>   * off is assumed to never fail. No failure can occur when the use parameter is
>   * set to 0.
>   */
> -int omap4iss_pipeline_pm_use(struct media_entity *entity, int use)
> +int omap4iss_pipeline_pm_use(struct media_entity *entity, int use,
> +			     struct media_entity_graph *graph)
>  {
>  	int change = use ? 1 : -1;
>  	int ret;
> @@ -501,7 +502,7 @@ int omap4iss_pipeline_pm_use(struct media_entity *entity, int use)
>  	WARN_ON(entity->use_count < 0);
>  
>  	/* Apply power change to connected non-nodes. */
> -	ret = iss_pipeline_pm_power(entity, change);
> +	ret = iss_pipeline_pm_power(entity, change, graph);
>  	if (ret < 0)
>  		entity->use_count -= change;
>  
> @@ -526,34 +527,48 @@ int omap4iss_pipeline_pm_use(struct media_entity *entity, int use)
>  static int iss_pipeline_link_notify(struct media_link *link, u32 flags,
>  				    unsigned int notification)
>  {
> +	struct media_entity_graph *graph =
> +		&container_of(link->graph_obj.mdev, struct iss_device,
> +			      media_dev)->pm_count_graph;
>  	struct media_entity *source = link->source->entity;
>  	struct media_entity *sink = link->sink->entity;
> -	int source_use = iss_pipeline_pm_use_count(source);
> -	int sink_use = iss_pipeline_pm_use_count(sink);
> +	int source_use;
> +	int sink_use;
>  	int ret;
>  
> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH) {
> +		ret = media_entity_graph_walk_init(graph,
> +						   link->graph_obj.mdev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	source_use = iss_pipeline_pm_use_count(source, graph);
> +	sink_use = iss_pipeline_pm_use_count(sink, graph);
> +
>  	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH &&
>  	    !(flags & MEDIA_LNK_FL_ENABLED)) {
>  		/* Powering off entities is assumed to never fail. */
> -		iss_pipeline_pm_power(source, -sink_use);
> -		iss_pipeline_pm_power(sink, -source_use);
> +		iss_pipeline_pm_power(source, -sink_use, graph);
> +		iss_pipeline_pm_power(sink, -source_use, graph);
>  		return 0;
>  	}
>  
>  	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
>  		(flags & MEDIA_LNK_FL_ENABLED)) {
> -		ret = iss_pipeline_pm_power(source, sink_use);
> +		ret = iss_pipeline_pm_power(source, sink_use, graph);
>  		if (ret < 0)
>  			return ret;
>  
> -		ret = iss_pipeline_pm_power(sink, source_use);
> +		ret = iss_pipeline_pm_power(sink, source_use, graph);
>  		if (ret < 0)
> -			iss_pipeline_pm_power(source, -sink_use);
> -
> -		return ret;
> +			iss_pipeline_pm_power(source, -sink_use, graph);
>  	}
>  
> -	return 0;
> +	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH)
> +		media_entity_graph_walk_cleanup(graph);
> +
> +	return ret;
>  }
>  
>  /* -----------------------------------------------------------------------------
> diff --git a/drivers/staging/media/omap4iss/iss.h b/drivers/staging/media/omap4iss/iss.h
> index 5dd0d99..ee7dc08 100644
> --- a/drivers/staging/media/omap4iss/iss.h
> +++ b/drivers/staging/media/omap4iss/iss.h
> @@ -87,6 +87,7 @@ struct iss_reg {
>  struct iss_device {
>  	struct v4l2_device v4l2_dev;
>  	struct media_device media_dev;
> +	struct media_entity_graph pm_count_graph;
>  	struct device *dev;
>  	u32 revision;
>  
> @@ -151,7 +152,8 @@ void omap4iss_isp_subclk_enable(struct iss_device *iss,
>  void omap4iss_isp_subclk_disable(struct iss_device *iss,
>  				 enum iss_isp_subclk_resource res);
>  
> -int omap4iss_pipeline_pm_use(struct media_entity *entity, int use);
> +int omap4iss_pipeline_pm_use(struct media_entity *entity, int use,
> +			     struct media_entity_graph *graph);
>  
>  int omap4iss_register_entities(struct platform_device *pdev,
>  			       struct v4l2_device *v4l2_dev);
> diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c
> index b56f999..6cfb65e 100644
> --- a/drivers/staging/media/omap4iss/iss_video.c
> +++ b/drivers/staging/media/omap4iss/iss_video.c
> @@ -210,6 +210,12 @@ iss_video_far_end(struct iss_video *video)
>  	struct iss_video *far_end = NULL;
>  
>  	mutex_lock(&mdev->graph_mutex);
> +
> +	if (media_entity_graph_walk_init(&graph, mdev)) {
> +		mutex_unlock(&mdev->graph_mutex);
> +		return NULL;
> +	}
> +
>  	media_entity_graph_walk_start(&graph, entity);
>  
>  	while ((entity = media_entity_graph_walk_next(&graph))) {
> @@ -227,6 +233,9 @@ iss_video_far_end(struct iss_video *video)
>  	}
>  
>  	mutex_unlock(&mdev->graph_mutex);
> +
> +	media_entity_graph_walk_cleanup(&graph);
> +
>  	return far_end;
>  }
>  
> @@ -751,7 +760,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>  	struct iss_video_fh *vfh = to_iss_video_fh(fh);
>  	struct iss_video *video = video_drvdata(file);
>  	struct media_entity_graph graph;
> -	struct media_entity *entity;
> +	struct media_entity *entity = &video->video.entity;
>  	enum iss_pipeline_state state;
>  	struct iss_pipeline *pipe;
>  	struct iss_video *far_end;
> @@ -779,12 +788,14 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>  	if (video->iss->pdata->set_constraints)
>  		video->iss->pdata->set_constraints(video->iss, true);
>  
> +	ret = media_entity_enum_init(&pipe->entities, &video->iss->media_dev);
> +	if (ret)
> +		goto err_enum_init;
> +
>  	ret = media_entity_pipeline_start(&video->video.entity, &pipe->pipe);
>  	if (ret < 0)
>  		goto err_media_entity_pipeline_start;
>  
> -	entity = &video->video.entity;
> -	media_entity_graph_walk_start(&graph, entity);
>  	while ((entity = media_entity_graph_walk_next(&graph)))
>  		media_entity_enum_set(&pipe->entities, entity);
>  
> @@ -858,6 +869,8 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>  
>  	mutex_unlock(&video->stream_lock);
>  
> +	media_entity_graph_walk_cleanup(&graph);
> +
>  	return 0;
>  
>  err_omap4iss_set_stream:
> @@ -869,10 +882,11 @@ err_media_entity_pipeline_start:
>  		video->iss->pdata->set_constraints(video->iss, false);
>  	video->queue = NULL;
>  
> -	mutex_unlock(&video->stream_lock);
> -
>  	media_entity_enum_cleanup(&pipe->entities);
>  
> +err_enum_init:
> +	mutex_unlock(&video->stream_lock);
> +
>  	return ret;
>  }
>  
> @@ -994,7 +1008,13 @@ static int iss_video_open(struct file *file)
>  		goto done;
>  	}
>  
> -	ret = omap4iss_pipeline_pm_use(&video->video.entity, 1);
> +	ret = media_entity_graph_walk_init(&handle->graph,
> +					   &video->iss->media_dev);
> +	if (ret)
> +		goto done;
> +
> +	ret = omap4iss_pipeline_pm_use(&video->video.entity, 1,
> +				       &handle->graph);
>  	if (ret < 0) {
>  		omap4iss_put(video->iss);
>  		goto done;
> @@ -1033,6 +1053,7 @@ static int iss_video_open(struct file *file)
>  done:
>  	if (ret < 0) {
>  		v4l2_fh_del(&handle->vfh);
> +		media_entity_graph_walk_cleanup(&handle->graph);
>  		kfree(handle);
>  	}
>  
> @@ -1048,12 +1069,13 @@ static int iss_video_release(struct file *file)
>  	/* Disable streaming and free the buffers queue resources. */
>  	iss_video_streamoff(file, vfh, video->type);
>  
> -	omap4iss_pipeline_pm_use(&video->video.entity, 0);
> +	omap4iss_pipeline_pm_use(&video->video.entity, 0, &handle->graph);
>  
>  	/* Release the videobuf2 queue */
>  	vb2_queue_release(&handle->queue);
>  
>  	/* Release the file handle. */
> +	media_entity_graph_walk_cleanup(&handle->graph);
>  	v4l2_fh_del(vfh);
>  	kfree(handle);
>  	file->private_data = NULL;
> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
> index b5d3a96..2ba4fb9 100644
> --- a/drivers/staging/media/omap4iss/iss_video.h
> +++ b/drivers/staging/media/omap4iss/iss_video.h
> @@ -183,6 +183,7 @@ struct iss_video_fh {
>  	struct vb2_queue queue;
>  	struct v4l2_format format;
>  	struct v4l2_fract timeperframe;
> +	struct media_entity_graph graph;
>  };
>  
>  #define to_iss_video_fh(fh)	container_of(fh, struct iss_video_fh, vfh)


-- 

Cheers,
Mauro

Re: [PATCH 03/19] media: Add an API to manage entity enumerations

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:34 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> 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>
> ---
>  drivers/media/media-entity.c |  39 +++++++++++++
>  include/media/media-device.h |  14 +++++
>  include/media/media-entity.h | 128 ++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 173 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index d11f440..fceaf44 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -213,6 +213,45 @@ void media_gobj_remove(struct media_gobj *gobj)
>  }
>  
>  /**
> + * __media_entity_enum_init - Initialise an entity enumeration
> + *
> + * @e: Entity enumeration to be initialised
> + * @idx_max: Maximum number of entities in the enumeration
> + *
> + * Returns zero on success or a negative error code.
> + */
> +int __media_entity_enum_init(struct media_entity_enum *e, int idx_max)
> +{
> +	if (idx_max > MEDIA_ENTITY_ENUM_MAX_ID) {
> +		e->e = kcalloc(DIV_ROUND_UP(idx_max, BITS_PER_LONG),
> +			       sizeof(long), GFP_KERNEL);

That looks wrong to me when the graph size increases. 

If e->e is not null, you need first to free the previously allocated
map before allocing a new one.

> +		if (!e->e)
> +			return -ENOMEM;
> +	} else {
> +		e->e = e->__e;
> +	}
> +
> +	bitmap_zero(e->e, idx_max);
> +	e->idx_max = idx_max;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(__media_entity_enum_init);
> +
> +/**
> + * media_entity_enum_cleanup - Release resources of an entity enumeration
> + *
> + * @e: Entity enumeration to be released
> + */
> +void media_entity_enum_cleanup(struct media_entity_enum *e)
> +{
> +	if (e->e != e->__e)
> +		kfree(e->e);
> +	e->e = NULL;
> +}
> +EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);
> +
> +/**
>   * media_entity_init - Initialize a media entity
>   *
>   * @num_pads: Total number of sink and source pads.
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index c0e1764..2d46c66 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -110,6 +110,20 @@ struct media_device {
>  /* media_devnode to media_device */
>  #define to_media_device(node) container_of(node, struct media_device, devnode)
>  
> +/**
> + * media_entity_enum_init - Initialise an entity enumeration
> + *
> + * @e: Entity enumeration to be initialised
> + * @mdev: The related media device
> + *
> + * Returns zero on success or a negative error code.
> + */
> +static inline __must_check int media_entity_enum_init(
> +	struct media_entity_enum *e, struct media_device *mdev)
> +{
> +	return __media_entity_enum_init(e, mdev->entity_internal_idx_max + 1);
> +}
> +
>  void media_device_init(struct media_device *mdev);
>  void media_device_cleanup(struct media_device *mdev);
>  int __must_check __media_device_register(struct media_device *mdev,
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index d3d3a39..fc54192 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>
> @@ -71,6 +71,22 @@ struct media_gobj {
>  	struct list_head	list;
>  };
>  
> +#define MEDIA_ENTITY_ENUM_MAX_DEPTH	16
> +#define MEDIA_ENTITY_ENUM_MAX_ID	64
> +
> +/*
> + * The number of pads can't be bigger than the number of entities,
> + * as the worse-case scenario is to have one entity linked up to
> + * MEDIA_ENTITY_ENUM_MAX_ID - 1 entities.
> + */
> +#define MEDIA_ENTITY_MAX_PADS		(MEDIA_ENTITY_ENUM_MAX_ID - 1)
> +
> +struct media_entity_enum {
> +	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);

I don't think it makes sense to keep MEDIA_ENTITY_ENUM_MAX_ID. 
Instead, let's just dynamically allocate the bitmap.

> +	unsigned long *e;

Btw, "__e" and "e" are very bad names. Please use a better name
here.

> +	int idx_max;
> +};
> +
>  struct media_pipeline {
>  };
>  
> @@ -307,15 +323,111 @@ 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
> +int __media_entity_enum_init(struct media_entity_enum *e, int idx_max);
> +void media_entity_enum_cleanup(struct media_entity_enum *e);
>  
> -/*
> - * The number of pads can't be bigger than the number of entities,
> - * as the worse-case scenario is to have one entity linked up to
> - * MEDIA_ENTITY_ENUM_MAX_ID - 1 entities.
> +/**
> + * media_entity_enum_zero - Clear the entire enum
> + *
> + * @e: Entity enumeration to be cleared
>   */
> -#define MEDIA_ENTITY_MAX_PADS		(MEDIA_ENTITY_ENUM_MAX_ID - 1)
> +static inline void media_entity_enum_zero(struct media_entity_enum *e)
> +{
> +	bitmap_zero(e->e, e->idx_max);
> +}
> +
> +/**
> + * media_entity_enum_set - Mark a single entity in the enum
> + *
> + * @e: Entity enumeration
> + * @entity: Entity to be marked
> + */
> +static inline void media_entity_enum_set(struct media_entity_enum *e,
> +					 struct media_entity *entity)
> +{
> +	if (WARN_ON(entity->internal_idx >= e->idx_max))
> +		return;
> +
> +	__set_bit(entity->internal_idx, e->e);
> +}
> +
> +/**
> + * media_entity_enum_clear - Unmark a single entity in the enum
> + *
> + * @e: Entity enumeration
> + * @entity: Entity to be unmarked
> + */
> +static inline void media_entity_enum_clear(struct media_entity_enum *e,
> +					   struct media_entity *entity)
> +{
> +	if (WARN_ON(entity->internal_idx >= e->idx_max))
> +		return;
> +
> +	__clear_bit(entity->internal_idx, e->e);
> +}
> +
> +/**
> + * media_entity_enum_test - Test whether the entity is marked
> + *
> + * @e: Entity enumeration
> + * @entity: Entity to be tested
> + *
> + * Returns true if the entity was marked.
> + */
> +static inline bool media_entity_enum_test(struct media_entity_enum *e,
> +					  struct media_entity *entity)
> +{
> +	if (WARN_ON(entity->internal_idx >= e->idx_max))
> +		return true;
> +
> +	return test_bit(entity->internal_idx, e->e);
> +}
> +
> +/**
> + * media_entity_enum_test - Test whether the entity is marked, and mark it
> + *
> + * @e: Entity enumeration
> + * @entity: Entity to be tested
> + *
> + * Returns true if the entity was marked, and mark it before doing so.
> + */
> +static inline bool media_entity_enum_test_and_set(struct media_entity_enum *e,
> +						  struct media_entity *entity)
> +{
> +	if (WARN_ON(entity->internal_idx >= e->idx_max))
> +		return true;
> +
> +	return __test_and_set_bit(entity->internal_idx, e->e);
> +}
> +
> +/**
> + * media_entity_enum_test - Test whether the entire enum is empty
> + *
> + * @e: Entity enumeration
> + * @entity: Entity to be tested
> + *
> + * Returns true if the entity was marked.
> + */
> +static inline bool media_entity_enum_empty(struct media_entity_enum *e)
> +{
> +	return bitmap_empty(e->e, e->idx_max);
> +}
> +
> +/**
> + * media_entity_enum_intersects - Test whether two enums intersect
> + *
> + * @e: First entity enumeration
> + * @f: Second entity enumeration
> + *
> + * Returns true if entity enumerations e and f intersect, otherwise false.
> + */
> +static inline bool media_entity_enum_intersects(struct media_entity_enum *e,
> +						struct media_entity_enum *f)
> +{
> +	WARN_ON(e->idx_max != f->idx_max);

I'm not sure about a WARN_ON() here. The routine will do the right thing,
due to the min() function below:

> +
> +	return bitmap_intersects(e->e, f->e, min(e->idx_max, f->idx_max));

So, the routine will be doing its job.

If having a different size is a problem for some driver/usecase, the
WARN_ON() should be there, not at the core.

> +}
>  
>  struct media_entity_graph {
>  	struct {


-- 

Cheers,
Mauro

Re: [PATCH 06/19] media: Amend media graph walk API by init and cleanup functions

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:37 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Add media_entity_graph_walk_init() and media_entity_graph_walk_cleanup()
> functions in order to dynamically allocate memory for the graph. This is
> not done in media_entity_graph_walk_start() as there are situations where
> e.g. correct error handling, that itself may not fail, requires successful
> graph walk.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

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

> ---
>  drivers/media/media-entity.c | 39 ++++++++++++++++++++++++++++++++++-----
>  include/media/media-entity.h |  5 ++++-
>  2 files changed, 38 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 667ab32..bf3c31f 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -353,14 +353,44 @@ static struct media_entity *stack_pop(struct media_entity_graph *graph)
>  #define stack_top(en)	((en)->stack[(en)->top].entity)
>  
>  /**
> + * media_entity_graph_walk_init - Allocate resources for graph walk
> + * @graph: Media graph structure that will be used to walk the graph
> + * @mdev: Media device
> + *
> + * Reserve resources for graph walk in media device's current
> + * state. The memory must be released using
> + * media_entity_graph_walk_free().
> + *
> + * Returns error on failure, zero on success.
> + */
> +__must_check int media_entity_graph_walk_init(
> +	struct media_entity_graph *graph, struct media_device *mdev)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(media_entity_graph_walk_init);
> +
> +/**
> + * media_entity_graph_walk_cleanup - Release resources related to graph walking
> + * @graph: Media graph structure that was used to walk the graph
> + */
> +void media_entity_graph_walk_cleanup(struct media_entity_graph *graph)
> +{
> +}
> +EXPORT_SYMBOL_GPL(media_entity_graph_walk_cleanup);
> +
> +/**
>   * media_entity_graph_walk_start - Start walking the media graph at a given entity
>   * @graph: Media graph structure that will be used to walk the graph
>   * @entity: Starting entity
>   *
> - * This function initializes the graph traversal structure to walk the entities
> - * graph starting at the given entity. The traversal structure must not be
> - * modified by the caller during graph traversal. When done the structure can
> - * safely be freed.
> + * Before using this function, media_entity_graph_walk_init() must be
> + * used to allocate resources used for walking the graph. This
> + * function initializes the graph traversal structure to walk the
> + * entities graph starting at the given entity. The traversal
> + * structure must not be modified by the caller during graph
> + * traversal. After the graph walk, the resources must be released
> + * using media_entity_graph_walk_cleanup().
>   */
>  void media_entity_graph_walk_start(struct media_entity_graph *graph,
>  				   struct media_entity *entity)
> @@ -377,7 +407,6 @@ void media_entity_graph_walk_start(struct media_entity_graph *graph,
>  }
>  EXPORT_SYMBOL_GPL(media_entity_graph_walk_start);
>  
> -
>  /**
>   * media_entity_graph_walk_next - Get the next entity in the graph
>   * @graph: Media graph structure
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index b2864cb..6e12b53 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -486,8 +486,11 @@ struct media_pad *media_entity_remote_pad(struct media_pad *pad);
>  struct media_entity *media_entity_get(struct media_entity *entity);
>  void media_entity_put(struct media_entity *entity);
>  
> +__must_check int media_entity_graph_walk_init(
> +	struct media_entity_graph *graph, struct media_device *mdev);
> +void media_entity_graph_walk_cleanup(struct media_entity_graph *graph);
>  void media_entity_graph_walk_start(struct media_entity_graph *graph,
> -		struct media_entity *entity);
> +				   struct media_entity *entity);
>  struct media_entity *
>  media_entity_graph_walk_next(struct media_entity_graph *graph);
>  __must_check int media_entity_pipeline_start(struct media_entity *entity,


-- 

Cheers,
Mauro

Re: [PATCH 19/19] media: Rename MEDIA_ENTITY_ENUM_MAX_ID as MEDIA_ENTITY_ENUM_STACK_ALLOC

[Mauro Carvalho Chehab]

Em Tue, 27 Oct 2015 01:01:50 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> The purpose of the macro has changed, rename it accordingly. It is not and
> should no longer be used in drivers directly, but only for the purpose for
> defining how many bits can be allocated from the stack for entity
> enumerations.

See my comments on patch 03/19.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-entity.c | 2 +-
>  include/media/media-entity.h | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> index 137aa09d..feca976 100644
> --- a/drivers/media/media-entity.c
> +++ b/drivers/media/media-entity.c
> @@ -222,7 +222,7 @@ void media_gobj_remove(struct media_gobj *gobj)
>   */
>  int __media_entity_enum_init(struct media_entity_enum *e, int idx_max)
>  {
> -	if (idx_max > MEDIA_ENTITY_ENUM_MAX_ID) {
> +	if (idx_max > MEDIA_ENTITY_ENUM_STACK_ALLOC) {
>  		e->e = kcalloc(DIV_ROUND_UP(idx_max, BITS_PER_LONG),
>  			       sizeof(long), GFP_KERNEL);
>  		if (!e->e)
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index cc01e08..5cab165 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -72,17 +72,17 @@ struct media_gobj {
>  };
>  
>  #define MEDIA_ENTITY_ENUM_MAX_DEPTH	16
> -#define MEDIA_ENTITY_ENUM_MAX_ID	64
> +#define MEDIA_ENTITY_ENUM_STACK_ALLOC	64
>  
>  /*
>   * The number of pads can't be bigger than the number of entities,
>   * as the worse-case scenario is to have one entity linked up to
> - * MEDIA_ENTITY_ENUM_MAX_ID - 1 entities.
> + * MEDIA_ENTITY_ENUM_STACK_ALLOC - 1 entities.
>   */
> -#define MEDIA_ENTITY_MAX_PADS		(MEDIA_ENTITY_ENUM_MAX_ID - 1)
> +#define MEDIA_ENTITY_MAX_PADS		(MEDIA_ENTITY_ENUM_STACK_ALLOC - 1)
>  
>  struct media_entity_enum {
> -	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);
> +	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_STACK_ALLOC);
>  	unsigned long *e;
>  	int idx_max;
>  };


-- 

Cheers,
Mauro

Re: [PATCH 03/19] media: Add an API to manage entity enumerations

[Sakari Ailus]

Hi Mauro,

Many thanks for the thorough review of the set!

On Wed, Oct 28, 2015 at 11:09:31AM +0900, Mauro Carvalho Chehab wrote:
> Em Tue, 27 Oct 2015 01:01:34 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > 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>
> > ---
> >  drivers/media/media-entity.c |  39 +++++++++++++
> >  include/media/media-device.h |  14 +++++
> >  include/media/media-entity.h | 128 ++++++++++++++++++++++++++++++++++++++++---
> >  3 files changed, 173 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index d11f440..fceaf44 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -213,6 +213,45 @@ void media_gobj_remove(struct media_gobj *gobj)
> >  }
> >  
> >  /**
> > + * __media_entity_enum_init - Initialise an entity enumeration
> > + *
> > + * @e: Entity enumeration to be initialised
> > + * @idx_max: Maximum number of entities in the enumeration
> > + *
> > + * Returns zero on success or a negative error code.
> > + */
> > +int __media_entity_enum_init(struct media_entity_enum *e, int idx_max)
> > +{
> > +	if (idx_max > MEDIA_ENTITY_ENUM_MAX_ID) {
> > +		e->e = kcalloc(DIV_ROUND_UP(idx_max, BITS_PER_LONG),
> > +			       sizeof(long), GFP_KERNEL);
> 
> That looks wrong to me when the graph size increases. 
> 
> If e->e is not null, you need first to free the previously allocated
> map before allocing a new one.

Indeed. You'll first have to call media_entity_enum_cleanup().

It's the responsibility of the user of this interface (i.e. driver and the
framework) to ensure that the enum is large enough to contain all the
entities. Allowing dynamic updates to the graph does indeed open a can of
worms and I'd prefer to keep that can sealed for a bit longer.

> 
> > +		if (!e->e)
> > +			return -ENOMEM;
> > +	} else {
> > +		e->e = e->__e;
> > +	}
> > +
> > +	bitmap_zero(e->e, idx_max);
> > +	e->idx_max = idx_max;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(__media_entity_enum_init);
> > +
> > +/**
> > + * media_entity_enum_cleanup - Release resources of an entity enumeration
> > + *
> > + * @e: Entity enumeration to be released
> > + */
> > +void media_entity_enum_cleanup(struct media_entity_enum *e)
> > +{
> > +	if (e->e != e->__e)
> > +		kfree(e->e);
> > +	e->e = NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(media_entity_enum_cleanup);
> > +
> > +/**
> >   * media_entity_init - Initialize a media entity
> >   *
> >   * @num_pads: Total number of sink and source pads.
> > diff --git a/include/media/media-device.h b/include/media/media-device.h
> > index c0e1764..2d46c66 100644
> > --- a/include/media/media-device.h
> > +++ b/include/media/media-device.h
> > @@ -110,6 +110,20 @@ struct media_device {
> >  /* media_devnode to media_device */
> >  #define to_media_device(node) container_of(node, struct media_device, devnode)
> >  
> > +/**
> > + * media_entity_enum_init - Initialise an entity enumeration
> > + *
> > + * @e: Entity enumeration to be initialised
> > + * @mdev: The related media device
> > + *
> > + * Returns zero on success or a negative error code.
> > + */
> > +static inline __must_check int media_entity_enum_init(
> > +	struct media_entity_enum *e, struct media_device *mdev)
> > +{
> > +	return __media_entity_enum_init(e, mdev->entity_internal_idx_max + 1);
> > +}
> > +
> >  void media_device_init(struct media_device *mdev);
> >  void media_device_cleanup(struct media_device *mdev);
> >  int __must_check __media_device_register(struct media_device *mdev,
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index d3d3a39..fc54192 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>
> > @@ -71,6 +71,22 @@ struct media_gobj {
> >  	struct list_head	list;
> >  };
> >  
> > +#define MEDIA_ENTITY_ENUM_MAX_DEPTH	16
> > +#define MEDIA_ENTITY_ENUM_MAX_ID	64
> > +
> > +/*
> > + * The number of pads can't be bigger than the number of entities,
> > + * as the worse-case scenario is to have one entity linked up to
> > + * MEDIA_ENTITY_ENUM_MAX_ID - 1 entities.
> > + */
> > +#define MEDIA_ENTITY_MAX_PADS		(MEDIA_ENTITY_ENUM_MAX_ID - 1)
> > +
> > +struct media_entity_enum {
> > +	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);
> 
> I don't think it makes sense to keep MEDIA_ENTITY_ENUM_MAX_ID. 
> Instead, let's just dynamically allocate the bitmap.

I think it makes sense to allocate from stack (or otherwise statically) if
the need is not much, which means the vast majority of the users. As the
maximum is only 64, this means eight bytes.

Grabbing that eight bytes from stack is certainly faster than calling
kmalloc() in any case.

> 
> > +	unsigned long *e;
> 
> Btw, "__e" and "e" are very bad names. Please use a better name
> here.

Such as... bits? :-)

> > +	int idx_max;
> > +};
> > +
> >  struct media_pipeline {
> >  };
> >  
> > @@ -307,15 +323,111 @@ 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
> > +int __media_entity_enum_init(struct media_entity_enum *e, int idx_max);
> > +void media_entity_enum_cleanup(struct media_entity_enum *e);
> >  
> > -/*
> > - * The number of pads can't be bigger than the number of entities,
> > - * as the worse-case scenario is to have one entity linked up to
> > - * MEDIA_ENTITY_ENUM_MAX_ID - 1 entities.
> > +/**
> > + * media_entity_enum_zero - Clear the entire enum
> > + *
> > + * @e: Entity enumeration to be cleared
> >   */
> > -#define MEDIA_ENTITY_MAX_PADS		(MEDIA_ENTITY_ENUM_MAX_ID - 1)
> > +static inline void media_entity_enum_zero(struct media_entity_enum *e)
> > +{
> > +	bitmap_zero(e->e, e->idx_max);
> > +}
> > +
> > +/**
> > + * media_entity_enum_set - Mark a single entity in the enum
> > + *
> > + * @e: Entity enumeration
> > + * @entity: Entity to be marked
> > + */
> > +static inline void media_entity_enum_set(struct media_entity_enum *e,
> > +					 struct media_entity *entity)
> > +{
> > +	if (WARN_ON(entity->internal_idx >= e->idx_max))
> > +		return;
> > +
> > +	__set_bit(entity->internal_idx, e->e);
> > +}
> > +
> > +/**
> > + * media_entity_enum_clear - Unmark a single entity in the enum
> > + *
> > + * @e: Entity enumeration
> > + * @entity: Entity to be unmarked
> > + */
> > +static inline void media_entity_enum_clear(struct media_entity_enum *e,
> > +					   struct media_entity *entity)
> > +{
> > +	if (WARN_ON(entity->internal_idx >= e->idx_max))
> > +		return;
> > +
> > +	__clear_bit(entity->internal_idx, e->e);
> > +}
> > +
> > +/**
> > + * media_entity_enum_test - Test whether the entity is marked
> > + *
> > + * @e: Entity enumeration
> > + * @entity: Entity to be tested
> > + *
> > + * Returns true if the entity was marked.
> > + */
> > +static inline bool media_entity_enum_test(struct media_entity_enum *e,
> > +					  struct media_entity *entity)
> > +{
> > +	if (WARN_ON(entity->internal_idx >= e->idx_max))
> > +		return true;
> > +
> > +	return test_bit(entity->internal_idx, e->e);
> > +}
> > +
> > +/**
> > + * media_entity_enum_test - Test whether the entity is marked, and mark it
> > + *
> > + * @e: Entity enumeration
> > + * @entity: Entity to be tested
> > + *
> > + * Returns true if the entity was marked, and mark it before doing so.
> > + */
> > +static inline bool media_entity_enum_test_and_set(struct media_entity_enum *e,
> > +						  struct media_entity *entity)
> > +{
> > +	if (WARN_ON(entity->internal_idx >= e->idx_max))
> > +		return true;
> > +
> > +	return __test_and_set_bit(entity->internal_idx, e->e);
> > +}
> > +
> > +/**
> > + * media_entity_enum_test - Test whether the entire enum is empty
> > + *
> > + * @e: Entity enumeration
> > + * @entity: Entity to be tested
> > + *
> > + * Returns true if the entity was marked.
> > + */
> > +static inline bool media_entity_enum_empty(struct media_entity_enum *e)
> > +{
> > +	return bitmap_empty(e->e, e->idx_max);
> > +}
> > +
> > +/**
> > + * media_entity_enum_intersects - Test whether two enums intersect
> > + *
> > + * @e: First entity enumeration
> > + * @f: Second entity enumeration
> > + *
> > + * Returns true if entity enumerations e and f intersect, otherwise false.
> > + */
> > +static inline bool media_entity_enum_intersects(struct media_entity_enum *e,
> > +						struct media_entity_enum *f)
> > +{
> > +	WARN_ON(e->idx_max != f->idx_max);
> 
> I'm not sure about a WARN_ON() here. The routine will do the right thing,
> due to the min() function below:

Yes, it'll do the "right" thing. But is that still what the user wanted?
Very probably not.

> 
> > +
> > +	return bitmap_intersects(e->e, f->e, min(e->idx_max, f->idx_max));
> 
> So, the routine will be doing its job.
> 
> If having a different size is a problem for some driver/usecase, the
> WARN_ON() should be there, not at the core.

I'd like to see first such use case related to entities. Then it could be
removed IMO.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 04/19] media: Move struct media_entity_graph definition up

[Sakari Ailus]

Hi Mauro,

On Wed, Oct 28, 2015 at 09:36:50AM +0900, Mauro Carvalho Chehab wrote:
> Em Tue, 27 Oct 2015 01:01:35 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > It will be needed in struct media_pipeline shortly.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> (but see below)
> 
> > ---
> >  include/media/media-entity.h | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index fc54192..dde9a5f 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -87,6 +87,16 @@ struct media_entity_enum {
> >  	int idx_max;
> >  };
> >  
> > +struct media_entity_graph {
> 
> Not a problem on this patch itself, but since you're touching this
> struct, it would be nice to take the opportunity and document it ;)

I'll document it in a separate patch on top of the set. Would you be fine
with that?

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 05/19] media: Move media graph state for streamon/off to the pipeline

[Sakari Ailus]

Hi Mauro,

On Wed, Oct 28, 2015 at 09:38:47AM +0900, Mauro Carvalho Chehab wrote:
> Em Tue, 27 Oct 2015 01:01:36 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > The struct media_entity_graph was allocated in the stack, limiting the
> > number of entities that could be reasonably allocated. Instead, move the
> > struct to struct media_pipeline which is typically allocated using
> > kmalloc() instead.
> > 
> > The intent is to keep the enumeration around for later use for the
> > duration of the streaming. As streaming is eventually stopped, an
> > unfortunate memory allocation failure would prevent stopping the
> > streaming. As no memory will need to be allocated, the problem is avoided
> > altogether.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/media-entity.c | 16 ++++++++--------
> >  include/media/media-entity.h |  2 ++
> >  2 files changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
> > index fceaf44..667ab32 100644
> > --- a/drivers/media/media-entity.c
> > +++ b/drivers/media/media-entity.c
> > @@ -456,16 +456,16 @@ __must_check int media_entity_pipeline_start(struct media_entity *entity,
> >  					     struct media_pipeline *pipe)
> >  {
> >  	struct media_device *mdev = entity->graph_obj.mdev;
> > -	struct media_entity_graph graph;
> > +	struct media_entity_graph *graph = &pipe->graph;
> >  	struct media_entity *entity_err = entity;
> >  	struct media_link *link;
> >  	int ret;
> >  
> >  	mutex_lock(&mdev->graph_mutex);
> >  
> > -	media_entity_graph_walk_start(&graph, entity);
> > +	media_entity_graph_walk_start(graph, entity);
> >  
> > -	while ((entity = media_entity_graph_walk_next(&graph))) {
> > +	while ((entity = media_entity_graph_walk_next(graph))) {
> >  		DECLARE_BITMAP(active, MEDIA_ENTITY_MAX_PADS);
> >  		DECLARE_BITMAP(has_no_links, MEDIA_ENTITY_MAX_PADS);
> >  
> > @@ -546,9 +546,9 @@ error:
> >  	 * Link validation on graph failed. We revert what we did and
> >  	 * return the error.
> >  	 */
> > -	media_entity_graph_walk_start(&graph, entity_err);
> > +	media_entity_graph_walk_start(graph, entity_err);
> >  
> > -	while ((entity_err = media_entity_graph_walk_next(&graph))) {
> > +	while ((entity_err = media_entity_graph_walk_next(graph))) {
> >  		entity_err->stream_count--;
> >  		if (entity_err->stream_count == 0)
> >  			entity_err->pipe = NULL;
> > @@ -582,13 +582,13 @@ EXPORT_SYMBOL_GPL(media_entity_pipeline_start);
> >  void media_entity_pipeline_stop(struct media_entity *entity)
> >  {
> >  	struct media_device *mdev = entity->graph_obj.mdev;
> > -	struct media_entity_graph graph;
> > +	struct media_entity_graph *graph = &entity->pipe->graph;
> >  
> >  	mutex_lock(&mdev->graph_mutex);
> >  
> > -	media_entity_graph_walk_start(&graph, entity);
> > +	media_entity_graph_walk_start(graph, entity);
> >  
> > -	while ((entity = media_entity_graph_walk_next(&graph))) {
> > +	while ((entity = media_entity_graph_walk_next(graph))) {
> >  		entity->stream_count--;
> >  		if (entity->stream_count == 0)
> >  			entity->pipe = NULL;
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index dde9a5f..b2864cb 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -98,6 +98,8 @@ struct media_entity_graph {
> >  };
> >  
> >  struct media_pipeline {
> > +	/* For walking the graph in pipeline start / stop */
> > +	struct media_entity_graph graph;
> >  };
> 
> Please use the kernel-doc format for documenting struct.

I'll do that.

> 
> After this change:
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Thanks!

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 07/19] media: Use the new media_entity_graph_walk_start()

[Sakari Ailus]

Hi Mauro,

On Wed, Oct 28, 2015 at 09:43:43AM +0900, Mauro Carvalho Chehab wrote:
> Em Tue, 27 Oct 2015 01:01:38 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Please add some documentation at the body for all patches.
> 
> Btw, IMHO, it would be best to fold this patch and the following ones
> that are related to media_entity_graph_walk_init() altogether, as it
> makes easier to review if all places were covered.

I think patches such as the 8th are easier to review as they are.

For the coverage,

$ git grep -l -E '^	*media_entity_graph_walk_start' 
Documentation/media-framework.txt
drivers/media/media-entity.c
drivers/media/platform/exynos4-is/media-dev.c
drivers/media/platform/omap3isp/isp.c
drivers/media/platform/omap3isp/ispvideo.c
drivers/media/platform/vsp1/vsp1_video.c
drivers/media/platform/xilinx/xilinx-dma.c
drivers/staging/media/davinci_vpfe/vpfe_video.c
drivers/staging/media/omap4iss/iss.c
drivers/staging/media/omap4iss/iss_video.c

Which suggests that I'm probably missing a few patches, indeed. I'll take
that into account in the next submission.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 14/19] v4l: omap3isp: Use media entity enumeration API

[Sakari Ailus]

Hi Mauro,

On Wed, Oct 28, 2015 at 10:30:30AM +0900, Mauro Carvalho Chehab wrote:
> Em Tue, 27 Oct 2015 01:01:45 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/platform/omap3isp/isp.c      | 21 +++++++++++++--------
> >  drivers/media/platform/omap3isp/isp.h      |  5 +++--
> >  drivers/media/platform/omap3isp/ispccdc.c  |  2 +-
> >  drivers/media/platform/omap3isp/ispvideo.c | 20 ++++++++++++++------
> >  drivers/media/platform/omap3isp/ispvideo.h |  4 ++--
> >  5 files changed, 33 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> > index 4a01a36..61c128e 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -896,7 +896,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;
> 
> If the size of entities/crashed enums is different, it should be
> returning an error, I guess, as this would be a driver's problem, and the
> graph traversal on OMAP3 would likely be wrong.

They should always have the same size. The omap3isp does not support dynamic
entity (un)registration. Both enums are initialised once all the entities
have been registered.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH 04/19] media: Move struct media_entity_graph definition up

[Sakari Ailus]

On Wed, Nov 04, 2015 at 12:22:38AM +0200, Sakari Ailus wrote:
> Hi Mauro,
> 
> On Wed, Oct 28, 2015 at 09:36:50AM +0900, Mauro Carvalho Chehab wrote:
> > Em Tue, 27 Oct 2015 01:01:35 +0200
> > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> > 
> > > It will be needed in struct media_pipeline shortly.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > Reviewed-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > (but see below)
> > 
> > > ---
> > >  include/media/media-entity.h | 20 ++++++++++----------
> > >  1 file changed, 10 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > > index fc54192..dde9a5f 100644
> > > --- a/include/media/media-entity.h
> > > +++ b/include/media/media-entity.h
> > > @@ -87,6 +87,16 @@ struct media_entity_enum {
> > >  	int idx_max;
> > >  };
> > >  
> > > +struct media_entity_graph {
> > 
> > Not a problem on this patch itself, but since you're touching this
> > struct, it would be nice to take the opportunity and document it ;)
> 
> I'll document it in a separate patch on top of the set. Would you be fine
> with that?

Thinking about this, I'll change the patches to include the documentation.
It's better that way, I agree.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk