[PATCH v2 00/22] Unrestricted media entity ID range support

[PATCH v2 00/22] Unrestricted media entity ID range support

[Sakari Ailus]

Hi,

This is the second version of the unrestricted media entity ID range
support patchset.

v1 of the set can be found here:

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

What has changed since v1:

- Updated documentation in Documentation/media-framework.txt (last patch).

- omap3isp: Moved entity enum cleanup from isp_video_streamon to
  isp_video_streamoff. This fixes memory-to-memory operation.

- Make media entity graph enumration API changes to davinci_vpfe staging
  driver as well.

- Document structs media_entity_enum, media_entity_graph and
  media_pipeline using KernelDoc.

-- 
Kind regards,
Sakari

[PATCH v2 01/22] 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 v2 02/22] 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 v2 03/22] 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 | 136 ++++++++++++++++++++++++++++++++++++++++---
 3 files changed, 181 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..5a0339a 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,30 @@ 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 - An enumeration of media entities.
+ *
+ * @__e:	Pre-allocated space reserved for media entities if the total
+ *		number of entities  does not exceed MEDIA_ENTITY_ENUM_MAX_ID.
+ * @e:		Bit mask in which each bit represents one entity at struct
+ *		media_entity->internal_idx.
+ * @idx_max:	Number of bits in the enum.
+ */
+struct media_entity_enum {
+	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);
+	unsigned long *e;
+	int idx_max;
+};
+
 struct media_pipeline {
 };
 
@@ -307,15 +331,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

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

[Mauro Carvalho Chehab]

Hi Sakari,

Em Sun, 29 Nov 2015 21:20:04 +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.

Please use better names on the vars. See below for details.

> 
> 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 | 136 ++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 181 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.
> + */

We're using kernel-doc macros only at the header files. Please
move those macros to there.

> +int __media_entity_enum_init(struct media_entity_enum *e, int idx_max)

using "e" as a var is not nice, specially on a public header.

I would use "ent_enum" instead for media_entity_enum pointers. This
applies everywhere on this patch series.

> +{
> +	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;

This is crap! I can't tell what the above code is doing,
as the var names don't give any clue!

> +	}
> +
> +	bitmap_zero(e->e, idx_max);

Ok, here, there's a hint that one of the "e" is a bitmap, while
the other is a struct... Weird, and very easy to do wrong things!

Please name the bitmap as "ent_enum_bmap" or something like that.

> +	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..5a0339a 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,30 @@ 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 - An enumeration of media entities.
> + *
> + * @__e:	Pre-allocated space reserved for media entities if the total
> + *		number of entities  does not exceed MEDIA_ENTITY_ENUM_MAX_ID.
> + * @e:		Bit mask in which each bit represents one entity at struct
> + *		media_entity->internal_idx.
> + * @idx_max:	Number of bits in the enum.
> + */
> +struct media_entity_enum {
> +	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);
> +	unsigned long *e;

As mentioned before, don't use single letter names, specially inside
publicly used structures. There's no good reason for that, and the
code become really obscure.

Also, just declare one bitmap. having a "pre-allocated" hardcoded
one here is very confusing.

> +	int idx_max;
> +};
> +
>  struct media_pipeline {
>  };
>  
> @@ -307,15 +331,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)

Huh, this is even uglier! The first media_entity_enum "e", and the
second "f"...

For me, "f" is reserved for a "float" type ;) Seriously, the conventional
usage for single-letter vars is to use them only for vars that don't
deserve a name, like temporary integer vars and loop indexes (i, j, k),
temporary pointers (p), temporary float values (f), etc.

All the rest should have more than one letter. Failing to do that makes
the code very hard to be read by other people.

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

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

[Sakari Ailus]

Hi Mauro,

Thanks for the comments!

Mauro Carvalho Chehab wrote:
> Hi Sakari,
> 
> Em Sun, 29 Nov 2015 21:20:04 +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.
> 
> Please use better names on the vars. See below for details.
> 
>>
>> 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 | 136 ++++++++++++++++++++++++++++++++++++++++---
>>  3 files changed, 181 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.
>> + */
> 
> We're using kernel-doc macros only at the header files. Please
> move those macros to there.

Oops. Will fix.

> 
>> +int __media_entity_enum_init(struct media_entity_enum *e, int idx_max)
> 
> using "e" as a var is not nice, specially on a public header.
> 
> I would use "ent_enum" instead for media_entity_enum pointers. This
> applies everywhere on this patch series.

I'm fine with that suggestion, I'll change to use ent_enum instead
(where there's no more specific purpose for the variable / field).

> 
>> +{
>> +	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;
> 
> This is crap! I can't tell what the above code is doing,
> as the var names don't give any clue!

In retrospect, the names could perhaps have been more descriptive.
There's no need to be angry at the letter "e" however, it's just doing
its job...

> 
>> +	}
>> +
>> +	bitmap_zero(e->e, idx_max);
> 
> Ok, here, there's a hint that one of the "e" is a bitmap, while
> the other is a struct... Weird, and very easy to do wrong things!
> 
> Please name the bitmap as "ent_enum_bmap" or something like that.

Fine with that.

> 
>> +	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..5a0339a 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,30 @@ 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 - An enumeration of media entities.
>> + *
>> + * @__e:	Pre-allocated space reserved for media entities if the total
>> + *		number of entities  does not exceed MEDIA_ENTITY_ENUM_MAX_ID.
>> + * @e:		Bit mask in which each bit represents one entity at struct
>> + *		media_entity->internal_idx.
>> + * @idx_max:	Number of bits in the enum.
>> + */
>> +struct media_entity_enum {
>> +	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);
>> +	unsigned long *e;
> 
> As mentioned before, don't use single letter names, specially inside
> publicly used structures. There's no good reason for that, and the
> code become really obscure.
> 
> Also, just declare one bitmap. having a "pre-allocated" hardcoded
> one here is very confusing.

I prefer to keep it as allocating a few bytes of memory is silly. In the
vast majority of the cases the pre-allocated array will be sufficient.

I have no strong opinion about this though. I agree it'd simplify the
code to remove it.

> 
>> +	int idx_max;
>> +};
>> +
>>  struct media_pipeline {
>>  };
>>  
>> @@ -307,15 +331,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)
> 
> Huh, this is even uglier! The first media_entity_enum "e", and the
> second "f"...
> 
> For me, "f" is reserved for a "float" type ;) Seriously, the conventional
> usage for single-letter vars is to use them only for vars that don't
> deserve a name, like temporary integer vars and loop indexes (i, j, k),
> temporary pointers (p), temporary float values (f), etc.
> 
> All the rest should have more than one letter. Failing to do that makes
> the code very hard to be read by other people.

Frankly, if the function is to find an intersection between two sets,
why would someone really care what those sets would be called? This is a
tiny function.

If you however really disdain the letter "f" as well, I can opt for
using ent_enum[12] instead. :-)

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

-- 
Regards,

Sakari Ailus
sakari.ailus@iki.fi

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

[Mauro Carvalho Chehab]

Em Mon, 14 Dec 2015 00:12:08 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> Thanks for the comments!
> 
> Mauro Carvalho Chehab wrote:
> > Hi Sakari,
> > 
> > Em Sun, 29 Nov 2015 21:20:04 +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.
> > 
> > Please use better names on the vars. See below for details.
> > 
> >>
> >> 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 | 136 ++++++++++++++++++++++++++++++++++++++++---
> >>  3 files changed, 181 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.
> >> + */
> > 
> > We're using kernel-doc macros only at the header files. Please
> > move those macros to there.
> 
> Oops. Will fix.
> 
> > 
> >> +int __media_entity_enum_init(struct media_entity_enum *e, int idx_max)
> > 
> > using "e" as a var is not nice, specially on a public header.
> > 
> > I would use "ent_enum" instead for media_entity_enum pointers. This
> > applies everywhere on this patch series.
> 
> I'm fine with that suggestion, I'll change to use ent_enum instead
> (where there's no more specific purpose for the variable / field).

Thanks!
> 
> > 
> >> +{
> >> +	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;
> > 
> > This is crap! I can't tell what the above code is doing,
> > as the var names don't give any clue!
> 
> In retrospect, the names could perhaps have been more descriptive.
> There's no need to be angry at the letter "e" however, it's just doing
> its job...

I'm not angry with the letter "e" (or with you). Just the above code
seems too obscure for a poor reviewer ;)

> 
> > 
> >> +	}
> >> +
> >> +	bitmap_zero(e->e, idx_max);
> > 
> > Ok, here, there's a hint that one of the "e" is a bitmap, while
> > the other is a struct... Weird, and very easy to do wrong things!
> > 
> > Please name the bitmap as "ent_enum_bmap" or something like that.
> 
> Fine with that.
> 
> > 
> >> +	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..5a0339a 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,30 @@ 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 - An enumeration of media entities.
> >> + *
> >> + * @__e:	Pre-allocated space reserved for media entities if the total
> >> + *		number of entities  does not exceed MEDIA_ENTITY_ENUM_MAX_ID.
> >> + * @e:		Bit mask in which each bit represents one entity at struct
> >> + *		media_entity->internal_idx.
> >> + * @idx_max:	Number of bits in the enum.
> >> + */
> >> +struct media_entity_enum {
> >> +	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);
> >> +	unsigned long *e;
> > 
> > As mentioned before, don't use single letter names, specially inside
> > publicly used structures. There's no good reason for that, and the
> > code become really obscure.
> > 
> > Also, just declare one bitmap. having a "pre-allocated" hardcoded
> > one here is very confusing.
> 
> I prefer to keep it as allocating a few bytes of memory is silly. In the
> vast majority of the cases the pre-allocated array will be sufficient.

"vast majority" actually depends on the driver/subsystem. The fact that
right now most drivers work fine with < 64 entities may not be
true in the future.

> 
> I have no strong opinion about this though. I agree it'd simplify the
> code to remove it.

IMHO, a simpler code is a good reason to do that.

> 
> > 
> >> +	int idx_max;
> >> +};
> >> +
> >>  struct media_pipeline {
> >>  };
> >>  
> >> @@ -307,15 +331,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)
> > 
> > Huh, this is even uglier! The first media_entity_enum "e", and the
> > second "f"...
> > 
> > For me, "f" is reserved for a "float" type ;) Seriously, the conventional
> > usage for single-letter vars is to use them only for vars that don't
> > deserve a name, like temporary integer vars and loop indexes (i, j, k),
> > temporary pointers (p), temporary float values (f), etc.
> > 
> > All the rest should have more than one letter. Failing to do that makes
> > the code very hard to be read by other people.
> 
> Frankly, if the function is to find an intersection between two sets,
> why would someone really care what those sets would be called? This is a
> tiny function.

That looks ugly from the documentation standpoint.

> If you however really disdain the letter "f" as well, I can opt for
> using ent_enum[12] instead. :-)

Naming as ent_enum1 and ent_enum2 sounds better.

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

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

[Sakari Ailus]

Hi Mauro,

On Sun, Dec 13, 2015 at 10:54:57PM -0200, Mauro Carvalho Chehab wrote:
> > >> +	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..5a0339a 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,30 @@ 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 - An enumeration of media entities.
> > >> + *
> > >> + * @__e:	Pre-allocated space reserved for media entities if the total
> > >> + *		number of entities  does not exceed MEDIA_ENTITY_ENUM_MAX_ID.
> > >> + * @e:		Bit mask in which each bit represents one entity at struct
> > >> + *		media_entity->internal_idx.
> > >> + * @idx_max:	Number of bits in the enum.
> > >> + */
> > >> +struct media_entity_enum {
> > >> +	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);
> > >> +	unsigned long *e;
> > > 
> > > As mentioned before, don't use single letter names, specially inside
> > > publicly used structures. There's no good reason for that, and the
> > > code become really obscure.
> > > 
> > > Also, just declare one bitmap. having a "pre-allocated" hardcoded
> > > one here is very confusing.
> > 
> > I prefer to keep it as allocating a few bytes of memory is silly. In the
> > vast majority of the cases the pre-allocated array will be sufficient.
> 
> "vast majority" actually depends on the driver/subsystem. The fact that
> right now most drivers work fine with < 64 entities may not be
> true in the future.

One reason the pre-allocated bitmap shouldn't be removed yet is that by
doing that, i.e. allocating memory in media_entity_enum_init(), the user
must check the return code. The further patches in the set make drivers do
that add __must_check modifier to the prototype.

I will thus add another patch near the top to remove the pre-allocated
bitmap.

-- 
Kind regards,

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

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

[Mauro Carvalho Chehab]

Em Wed, 16 Dec 2015 01:16:30 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> On Sun, Dec 13, 2015 at 10:54:57PM -0200, Mauro Carvalho Chehab wrote:
> > > >> +	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..5a0339a 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,30 @@ 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 - An enumeration of media entities.
> > > >> + *
> > > >> + * @__e:	Pre-allocated space reserved for media entities if the total
> > > >> + *		number of entities  does not exceed MEDIA_ENTITY_ENUM_MAX_ID.
> > > >> + * @e:		Bit mask in which each bit represents one entity at struct
> > > >> + *		media_entity->internal_idx.
> > > >> + * @idx_max:	Number of bits in the enum.
> > > >> + */
> > > >> +struct media_entity_enum {
> > > >> +	DECLARE_BITMAP(__e, MEDIA_ENTITY_ENUM_MAX_ID);
> > > >> +	unsigned long *e;
> > > > 
> > > > As mentioned before, don't use single letter names, specially inside
> > > > publicly used structures. There's no good reason for that, and the
> > > > code become really obscure.
> > > > 
> > > > Also, just declare one bitmap. having a "pre-allocated" hardcoded
> > > > one here is very confusing.
> > > 
> > > I prefer to keep it as allocating a few bytes of memory is silly. In the
> > > vast majority of the cases the pre-allocated array will be sufficient.
> > 
> > "vast majority" actually depends on the driver/subsystem. The fact that
> > right now most drivers work fine with < 64 entities may not be
> > true in the future.
> 
> One reason the pre-allocated bitmap shouldn't be removed yet is that by
> doing that, i.e. allocating memory in media_entity_enum_init(), the user
> must check the return code. The further patches in the set make drivers do
> that add __must_check modifier to the prototype.
> 
> I will thus add another patch near the top to remove the pre-allocated
> bitmap.

Works for me.

Regards,
Mauro

[PATCH v2 04/22] 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 5a0339a..2601bb0 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -95,6 +95,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 {
 };
 
@@ -437,16 +447,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

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

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:05 +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>
> ---
>  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 5a0339a..2601bb0 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -95,6 +95,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];

The best would be to use a pointer here, and allocate the exact
size only when using it.

> +
> +	DECLARE_BITMAP(entities, MEDIA_ENTITY_ENUM_MAX_ID);

Same here.

Ok, this patch is just moving things around, but some patch in
the series should be doing dynamic allocation of those.

> +	int top;
> +};
> +
>  struct media_pipeline {
>  };
>  
> @@ -437,16 +447,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)
>  

[PATCH v2 05/22] media: Add KernelDoc documentation for struct media_entity_graph

[Sakari Ailus]

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

diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 2601bb0..8fd888f 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -95,6 +95,14 @@ struct media_entity_enum {
 	int idx_max;
 };
 
+/*
+ * struct media_entity_graph - Media graph traversal state
+ *
+ * @stack.entity:	Media entity in the stack
+ * @stack.link:		Link through which the entity was reached
+ * @entities:		Visited entities
+ * @top:		The top of the stack
+ */
 struct media_entity_graph {
 	struct {
 		struct media_entity *entity;
-- 
2.1.4

Re: [PATCH v2 05/22] media: Add KernelDoc documentation for struct media_entity_graph

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:06 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/media/media-entity.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 2601bb0..8fd888f 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -95,6 +95,14 @@ struct media_entity_enum {
>  	int idx_max;
>  };
>  
> +/*
> + * struct media_entity_graph - Media graph traversal state
> + *
> + * @stack.entity:	Media entity in the stack
> + * @stack.link:		Link through which the entity was reached
> + * @entities:		Visited entities

The above is ok, but I guess you could get something better at the
documentation, for example using, instead:
	@stack.@entity

This requires some testing with ./scripts/kernel-doc, though.

> + * @top:		The top of the stack
> + */
>  struct media_entity_graph {
>  	struct {
>  		struct media_entity *entity;

[PATCH v2 06/22] 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>
Reviewed-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/media-entity.c | 16 ++++++++--------
 include/media/media-entity.h |  6 ++++++
 2 files changed, 14 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 8fd888f..4b5ca39 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -113,7 +113,13 @@ struct media_entity_graph {
 	int top;
 };
 
+/*
+ * struct media_pipeline - Media pipeline related information
+ *
+ * @graph:	Media graph walk during pipeline start / stop
+ */
 struct media_pipeline {
+	struct media_entity_graph graph;
 };
 
 /**
-- 
2.1.4

[PATCH v2 07/22] 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 4b5ca39..f0652e2 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -506,8 +506,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

Re: [PATCH v2 07/22] media: Amend media graph walk API by init and cleanup functions

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:08 +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.

Looks ok to me.


> 
> 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 4b5ca39..f0652e2 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -506,8 +506,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,

Re: [PATCH v2 07/22] media: Amend media graph walk API by init and cleanup functions

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:08 +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.

looks ok to me.

> 
> 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 4b5ca39..f0652e2 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -506,8 +506,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,

[PATCH v2 08/22] 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

Re: [PATCH v2 08/22] media: Use the new media_entity_graph_walk_start()

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:09 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

Please add a description here.

The patch itself looks ok.

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

[PATCH v2 09/22] 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

Re: [PATCH v2 09/22] v4l: omap3isp: Use the new media_entity_graph_walk_start() interface

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:10 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

description missing. Otherwise looks good.

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

[PATCH v2 10/22] v4l: exynos4-is: Use the new media_entity_graph_walk_start() interface

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Javier Martinez Canillas <javier@osg.samsung.com>
Cc: Kamil Debski <k.debski@samsung.com>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.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

Re: [PATCH v2 10/22] v4l: exynos4-is: Use the new media_entity_graph_walk_start() interface

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:11 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

description missing. Otherwise looks ok.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Javier Martinez Canillas <javier@osg.samsung.com>
> Cc: Kamil Debski <k.debski@samsung.com>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.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

[PATCH v2 11/22] v4l: xilinx: Use the new media_entity_graph_walk_start() interface

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Hyun Kwon <hyun.kwon@xilinx.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

Re: [PATCH v2 11/22] v4l: xilinx: Use the new media_entity_graph_walk_start() interface

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:12 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

description is missing. Patch looks ok, except for that.

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Hyun Kwon <hyun.kwon@xilinx.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;

[PATCH v2 12/22] 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

Re: [PATCH v2 12/22] v4l: vsp1: Use the new media_entity_graph_walk_start() interface

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:13 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

Same as the other similar patches: description is missing.

I would prefer if you could merge those patches that do the same
thing on different drivers. Less emails to write ;)

Won't be replying anymore to patches that are ok but misses
descriptions.

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

[PATCH v2 13/22] 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 f0652e2..8122736 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -109,8 +109,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;
 };
 
 /*
-- 
2.1.4

Re: [PATCH v2 13/22] media: Use entity enums in graph walk

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:14 +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.

Looks OK to me, and explains one of the things I pointed on a previous
patch.

> 
> 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 f0652e2..8122736 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -109,8 +109,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;
>  };
>  
>  /*

[PATCH v2 14/22] 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 |  4 +++-
 2 files changed, 14 insertions(+), 7 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 8122736..85c2656 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -116,9 +116,11 @@ struct media_entity_graph {
 /*
  * struct media_pipeline - Media pipeline related information
  *
- * @graph:	Media graph walk during pipeline start / stop
+ * @streaming_count:	Streaming start count - streaming stop count
+ * @graph:		Media graph walk during pipeline start / stop
  */
 struct media_pipeline {
+	int streaming_count;
 	struct media_entity_graph graph;
 };
 
-- 
2.1.4

Re: [PATCH v2 14/22] media: Keep using the same graph walk object for a given pipeline

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:15 +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.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-entity.c | 17 +++++++++++------
>  include/media/media-entity.h |  4 +++-
>  2 files changed, 14 insertions(+), 7 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);

Cant it be replaced by a WARN_ON()?

>  	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 8122736..85c2656 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -116,9 +116,11 @@ struct media_entity_graph {
>  /*
>   * struct media_pipeline - Media pipeline related information
>   *
> - * @graph:	Media graph walk during pipeline start / stop
> + * @streaming_count:	Streaming start count - streaming stop count
> + * @graph:		Media graph walk during pipeline start / stop
>   */
>  struct media_pipeline {
> +	int streaming_count;
>  	struct media_entity_graph graph;
>  };
>  

[PATCH v2 15/22] 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..9358740 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,7 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
 	}
 
 	mutex_unlock(&video->stream_lock);
+
 	return 0;
 
 err_set_stream:
@@ -1120,7 +1122,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;
 }
 
@@ -1172,6 +1178,8 @@ isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
 	/* TODO: Implement PM QoS */
 	media_entity_pipeline_stop(&video->video.entity);
 
+	media_entity_enum_cleanup(&pipe->entities);
+
 done:
 	mutex_unlock(&video->stream_lock);
 	return 0;
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

Re: [PATCH v2 15/22] v4l: omap3isp: Use media entity enumeration API

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:16 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

-ENODESCRIPTION!

> 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..9358740 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,7 @@ isp_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>  	}
>  
>  	mutex_unlock(&video->stream_lock);
> +
>  	return 0;
>  
>  err_set_stream:
> @@ -1120,7 +1122,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;
>  }
>  
> @@ -1172,6 +1178,8 @@ isp_video_streamoff(struct file *file, void *fh, enum v4l2_buf_type type)
>  	/* TODO: Implement PM QoS */
>  	media_entity_pipeline_stop(&video->video.entity);
>  
> +	media_entity_enum_cleanup(&pipe->entities);
> +
>  done:
>  	mutex_unlock(&video->stream_lock);
>  	return 0;
> 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;

Please don't use "entities" for a media_entity_enum type.
Call it as ent_enum or something else that let the type
clearer.


>  	unsigned long l3_ick;
>  	unsigned int max_rate;
>  	enum v4l2_field field;

[PATCH v2 16/22] 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

Re: [PATCH v2 16/22] v4l: vsp1: Use media entity enumeration API

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:17 +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/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;

Again, entities is a bad name ;) we're reserving this name to
mean media_v2_entity. Using it with some other type makes
thinks confusing.

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

Just put it on a single line. This kind of line breakage doesn't look nice ;)

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

[PATCH v2 17/22] staging: v4l: 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@linux.intel.com>
---
 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 v2 18/22] 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

Re: [PATCH v2 18/22] staging: v4l: omap4iss: Use media entity enum API

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:19 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

Same note as previous patches... no description, better name for the
entity_enum vars.

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

[PATCH v2 19/22] 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 v2 20/22] staging: v4l: davinci_vpbe: Use the new media_entity_graph_walk_start() interface

[Sakari Ailus]

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Prabhakar Lad <prabhakar.lad@ti.com>
---
 drivers/staging/media/davinci_vpfe/vpfe_video.c | 37 ++++++++++++++++++-------
 drivers/staging/media/davinci_vpfe/vpfe_video.h |  1 +
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/media/davinci_vpfe/vpfe_video.c b/drivers/staging/media/davinci_vpfe/vpfe_video.c
index 2dbf14b..1bacd19 100644
--- a/drivers/staging/media/davinci_vpfe/vpfe_video.c
+++ b/drivers/staging/media/davinci_vpfe/vpfe_video.c
@@ -127,13 +127,14 @@ __vpfe_video_get_format(struct vpfe_video_device *video,
 }
 
 /* make a note of pipeline details */
-static void vpfe_prepare_pipeline(struct vpfe_video_device *video)
+static int vpfe_prepare_pipeline(struct vpfe_video_device *video)
 {
+	struct media_entity_graph graph;
 	struct media_entity *entity = &video->video_dev.entity;
 	struct media_device *mdev = entity->graph_obj.mdev;
 	struct vpfe_pipeline *pipe = &video->pipe;
 	struct vpfe_video_device *far_end = NULL;
-	struct media_entity_graph graph;
+	int ret;
 
 	pipe->input_num = 0;
 	pipe->output_num = 0;
@@ -144,6 +145,11 @@ static void vpfe_prepare_pipeline(struct vpfe_video_device *video)
 		pipe->outputs[pipe->output_num++] = video;
 
 	mutex_lock(&mdev->graph_mutex);
+	ret = media_entity_graph_walk_init(&graph, entity->graph_obj.mdev);
+	if (ret) {
+		mutex_unlock(&video->lock);
+		return -ENOMEM;
+	}
 	media_entity_graph_walk_start(&graph, entity);
 	while ((entity = media_entity_graph_walk_next(&graph))) {
 		if (entity == &video->video_dev.entity)
@@ -156,7 +162,10 @@ static void vpfe_prepare_pipeline(struct vpfe_video_device *video)
 		else
 			pipe->outputs[pipe->output_num++] = far_end;
 	}
+	media_entity_graph_walk_cleanup(&graph);
 	mutex_unlock(&mdev->graph_mutex);
+
+	return 0;
 }
 
 /* update pipe state selected by user */
@@ -165,7 +174,9 @@ static int vpfe_update_pipe_state(struct vpfe_video_device *video)
 	struct vpfe_pipeline *pipe = &video->pipe;
 	int ret;
 
-	vpfe_prepare_pipeline(video);
+	ret = vpfe_prepare_pipeline(video);
+	if (ret)
+		return ret;
 
 	/* Find out if there is any input video
 	  if yes, it is single shot.
@@ -276,11 +287,10 @@ static int vpfe_video_validate_pipeline(struct vpfe_pipeline *pipe)
  */
 static int vpfe_pipeline_enable(struct vpfe_pipeline *pipe)
 {
-	struct media_entity_graph graph;
 	struct media_entity *entity;
 	struct v4l2_subdev *subdev;
 	struct media_device *mdev;
-	int ret = 0;
+	int ret;
 
 	if (pipe->state == VPFE_PIPELINE_STREAM_CONTINUOUS)
 		entity = vpfe_get_input_entity(pipe->outputs[0]);
@@ -289,8 +299,12 @@ static int vpfe_pipeline_enable(struct vpfe_pipeline *pipe)
 
 	mdev = entity->graph_obj.mdev;
 	mutex_lock(&mdev->graph_mutex);
-	media_entity_graph_walk_start(&graph, entity);
-	while ((entity = media_entity_graph_walk_next(&graph))) {
+	ret = media_entity_graph_walk_init(&pipe->graph,
+					   entity->graph_obj.mdev);
+	if (ret)
+		goto out;
+	media_entity_graph_walk_start(&pipe->graph, entity);
+	while ((entity = media_entity_graph_walk_next(&pipe->graph))) {
 
 		if (!is_media_entity_v4l2_subdev(entity))
 			continue;
@@ -299,6 +313,9 @@ static int vpfe_pipeline_enable(struct vpfe_pipeline *pipe)
 		if (ret < 0 && ret != -ENOIOCTLCMD)
 			break;
 	}
+out:
+	if (ret)
+		media_entity_graph_walk_cleanup(&pipe->graph);
 	mutex_unlock(&mdev->graph_mutex);
 	return ret;
 }
@@ -316,7 +333,6 @@ static int vpfe_pipeline_enable(struct vpfe_pipeline *pipe)
  */
 static int vpfe_pipeline_disable(struct vpfe_pipeline *pipe)
 {
-	struct media_entity_graph graph;
 	struct media_entity *entity;
 	struct v4l2_subdev *subdev;
 	struct media_device *mdev;
@@ -329,9 +345,9 @@ static int vpfe_pipeline_disable(struct vpfe_pipeline *pipe)
 
 	mdev = entity->graph_obj.mdev;
 	mutex_lock(&mdev->graph_mutex);
-	media_entity_graph_walk_start(&graph, entity);
+	media_entity_graph_walk_start(&pipe->graph, entity);
 
-	while ((entity = media_entity_graph_walk_next(&graph))) {
+	while ((entity = media_entity_graph_walk_next(&pipe->graph))) {
 
 		if (!is_media_entity_v4l2_subdev(entity))
 			continue;
@@ -342,6 +358,7 @@ static int vpfe_pipeline_disable(struct vpfe_pipeline *pipe)
 	}
 	mutex_unlock(&mdev->graph_mutex);
 
+	media_entity_graph_walk_cleanup(&pipe->graph);
 	return ret ? -ETIMEDOUT : 0;
 }
 
diff --git a/drivers/staging/media/davinci_vpfe/vpfe_video.h b/drivers/staging/media/davinci_vpfe/vpfe_video.h
index 1b1b6c4..81f7698 100644
--- a/drivers/staging/media/davinci_vpfe/vpfe_video.h
+++ b/drivers/staging/media/davinci_vpfe/vpfe_video.h
@@ -51,6 +51,7 @@ enum vpfe_video_state {
 struct vpfe_pipeline {
 	/* media pipeline */
 	struct media_pipeline		*pipe;
+	struct media_entity_graph	graph;
 	/* state of the pipeline, continuous,
 	 * single-shot or stopped
 	 */
-- 
2.1.4

[PATCH v2 21/22] 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 85c2656..145d339 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -72,14 +72,14 @@ 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 - An enumeration of media entities.
  *
@@ -90,7 +90,7 @@ struct media_gobj {
  * @idx_max:	Number of bits in the enum.
  */
 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

[PATCH v2 22/22] media: Update media graph walk documentation for the changed API

[Sakari Ailus]

media_entity_graph_walk_init() and media_entity_graph_walk_cleanup() are
now mandatory.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/media-framework.txt | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/Documentation/media-framework.txt b/Documentation/media-framework.txt
index b424de6..738a526 100644
--- a/Documentation/media-framework.txt
+++ b/Documentation/media-framework.txt
@@ -241,13 +241,22 @@ supported by the graph traversal API. To prevent infinite loops, the graph
 traversal code limits the maximum depth to MEDIA_ENTITY_ENUM_MAX_DEPTH,
 currently defined as 16.
 
-Drivers initiate a graph traversal by calling
+The graph traversal must be initialised calling
+
+	media_entity_graph_walk_init(struct media_entity_graph *graph);
+
+The return value of the function must be checked. Should the number of
+graph entities exceed the pre-allocated memory, it will also allocate
+memory for the enumeration.
+
+Once initialised, the graph walk may be started by calling
 
 	media_entity_graph_walk_start(struct media_entity_graph *graph,
 				      struct media_entity *entity);
 
-The graph structure, provided by the caller, is initialized to start graph
-traversal at the given entity.
+The graph structure, provided by the caller, is initialized to start
+graph traversal at the given entity. It is possible to start the graph
+walk multiple times using the same graph struct.
 
 Drivers can then retrieve the next entity by calling
 
@@ -255,8 +264,11 @@ Drivers can then retrieve the next entity by calling
 
 When the graph traversal is complete the function will return NULL.
 
-Graph traversal can be interrupted at any moment. No cleanup function call is
-required and the graph structure can be freed normally.
+Graph traversal can be interrupted at any moment. Once the graph
+structure is no longer needed, the resources that have been allocated
+by media_entity_graph_walk_init() are released using
+
+	media_entity_graph_walk_cleanup(struct media_entity_graph *graph);
 
 Helper functions can be used to find a link between two given pads, or a pad
 connected to another pad through an enabled link
-- 
2.1.4

Re: [PATCH v2 22/22] media: Update media graph walk documentation for the changed API

[Mauro Carvalho Chehab]

Em Sun, 29 Nov 2015 21:20:23 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> media_entity_graph_walk_init() and media_entity_graph_walk_cleanup() are
> now mandatory.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/media-framework.txt | 22 +++++++++++++++++-----

This patch needs to be reworked, as the media framework documentation
got moved to kernel-doc.

>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/media-framework.txt b/Documentation/media-framework.txt
> index b424de6..738a526 100644
> --- a/Documentation/media-framework.txt
> +++ b/Documentation/media-framework.txt
> @@ -241,13 +241,22 @@ supported by the graph traversal API. To prevent infinite loops, the graph
>  traversal code limits the maximum depth to MEDIA_ENTITY_ENUM_MAX_DEPTH,
>  currently defined as 16.
>  
> -Drivers initiate a graph traversal by calling
> +The graph traversal must be initialised calling
> +
> +	media_entity_graph_walk_init(struct media_entity_graph *graph);
> +
> +The return value of the function must be checked. Should the number of
> +graph entities exceed the pre-allocated memory, it will also allocate
> +memory for the enumeration.
> +
> +Once initialised, the graph walk may be started by calling
>  
>  	media_entity_graph_walk_start(struct media_entity_graph *graph,
>  				      struct media_entity *entity);
>  
> -The graph structure, provided by the caller, is initialized to start graph
> -traversal at the given entity.
> +The graph structure, provided by the caller, is initialized to start
> +graph traversal at the given entity. It is possible to start the graph
> +walk multiple times using the same graph struct.
>  
>  Drivers can then retrieve the next entity by calling
>  
> @@ -255,8 +264,11 @@ Drivers can then retrieve the next entity by calling
>  
>  When the graph traversal is complete the function will return NULL.
>  
> -Graph traversal can be interrupted at any moment. No cleanup function call is
> -required and the graph structure can be freed normally.
> +Graph traversal can be interrupted at any moment. Once the graph
> +structure is no longer needed, the resources that have been allocated
> +by media_entity_graph_walk_init() are released using
> +
> +	media_entity_graph_walk_cleanup(struct media_entity_graph *graph);
>  
>  Helper functions can be used to find a link between two given pads, or a pad
>  connected to another pad through an enabled link