[PATCH v2 0/5] Unify MC graph power management code

[PATCH v2 0/5] Unify MC graph power management code

[Sakari Ailus]

Hi,

This set unifies the MC graph power management code for the omap3isp and        
omap4iss drivers. The implementation may also be useful for other drivers       
managing the power state for drivers which provide MC, V4L2 sub-device and      
V4L2 interfaces.                                                                
                                                                                
The original implementation has been in the mainline kernel for quite a         
while. During that time, changes that introduced bugs in the code have          
been made and the fixes written after those bugs were found, ended up           
being applied a lot later to the omap4iss implementation.                       
                                                                                
In the future this should be moved to make use of runtime PM, which             
should be easier when we only have a single implementation.                     
                                                                                
Updates since v1:

- Move the common graph PM code to drivers/media/v4l2-core/v4l2-mc.c and
  include/media/v4l2-mc.h

-- 
Kind regards,
Sakari

[PATCH v2 1/5] media: Use all bits of an enumeration

[Sakari Ailus]

The allocation takes place in longs. Assign the size of the enum
accordingly.

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

diff --git a/drivers/media/media-entity.c b/drivers/media/media-entity.c
index e89d85a..2c579f4 100644
--- a/drivers/media/media-entity.c
+++ b/drivers/media/media-entity.c
@@ -73,8 +73,9 @@ static inline const char *intf_type(struct media_interface *intf)
 __must_check int __media_entity_enum_init(struct media_entity_enum *ent_enum,
 					  int idx_max)
 {
-	ent_enum->bmap = kcalloc(DIV_ROUND_UP(idx_max, BITS_PER_LONG),
-				 sizeof(long), GFP_KERNEL);
+	idx_max = ALIGN(idx_max, BITS_PER_LONG);
+	ent_enum->bmap = kcalloc(idx_max / BITS_PER_LONG, sizeof(long),
+				 GFP_KERNEL);
 	if (!ent_enum->bmap)
 		return -ENOMEM;
 
-- 
2.1.4

[PATCH v2 2/5] media: Always keep a graph walk large enough around

[Sakari Ailus]

Re-create the graph walk object as needed in order to have one large enough
available for all entities in the graph.

This enumeration is used for pipeline power management in the future.

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

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 4d1c13d..52d7809 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -577,6 +577,26 @@ int __must_check media_device_register_entity(struct media_device *mdev,
 
 	spin_unlock(&mdev->lock);
 
+	mutex_lock(&mdev->graph_mutex);
+	if (mdev->entity_internal_idx_max
+	    >= mdev->pm_count_walk.ent_enum.idx_max) {
+		struct media_entity_graph new = { 0 };
+
+		/*
+		 * Initialise the new graph walk before cleaning up
+		 * the old one in order not to spoil the graph walk
+		 * object of the media device if graph walk init fails.
+		 */
+		ret = media_entity_graph_walk_init(&new, mdev);
+		if (ret) {
+			mutex_unlock(&mdev->graph_mutex);
+			return ret;
+		}
+		media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
+		mdev->pm_count_walk = new;
+	}
+	mutex_unlock(&mdev->graph_mutex);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(media_device_register_entity);
@@ -652,6 +672,7 @@ void media_device_cleanup(struct media_device *mdev)
 {
 	ida_destroy(&mdev->entity_internal_idx);
 	mdev->entity_internal_idx_max = 0;
+	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
 	mutex_destroy(&mdev->graph_mutex);
 }
 EXPORT_SYMBOL_GPL(media_device_cleanup);
diff --git a/include/media/media-device.h b/include/media/media-device.h
index d385589..dba3986 100644
--- a/include/media/media-device.h
+++ b/include/media/media-device.h
@@ -323,6 +323,11 @@ struct media_device {
 	spinlock_t lock;
 	/* Serializes graph operations. */
 	struct mutex graph_mutex;
+	/*
+	 * Graph walk for power state walk. Access serialised using
+	 * graph_mutex.
+	 */
+	struct media_entity_graph pm_count_walk;
 
 	int (*link_notify)(struct media_link *link, u32 flags,
 			   unsigned int notification);
-- 
2.1.4

[PATCH v2 3/5] v4l: Add generic pipeline power management code

[Sakari Ailus]

When the Media controller framework was merged, it was decided not to add
pipeline power management code for it was not seen generic. As a result, a
number of drivers have copied the same piece of code, with same bugfixes
done to them at different points of time (or not at all).

Add these functions to V4L2. Their use is optional for drivers.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/Makefile  |   3 +
 drivers/media/v4l2-core/v4l2-mc.c | 201 ++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-mc.h           |  66 +++++++++++++
 3 files changed, 270 insertions(+)
 create mode 100644 drivers/media/v4l2-core/v4l2-mc.c
 create mode 100644 include/media/v4l2-mc.h

diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
index 1dc8bba..f7d31745 100644
--- a/drivers/media/v4l2-core/Makefile
+++ b/drivers/media/v4l2-core/Makefile
@@ -20,6 +20,9 @@ endif
 obj-$(CONFIG_VIDEO_V4L2) += videodev.o
 obj-$(CONFIG_VIDEO_V4L2) += v4l2-common.o
 obj-$(CONFIG_VIDEO_V4L2) += v4l2-dv-timings.o
+ifneq ($(CONFIG_MEDIA_CONTROLLER),)
+  obj-$(CONFIG_VIDEO_V4L2) += v4l2-mc.o
+endif
 
 obj-$(CONFIG_VIDEO_TUNER) += tuner.o
 
diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
new file mode 100644
index 0000000..4c70cb4
--- /dev/null
+++ b/drivers/media/v4l2-core/v4l2-mc.c
@@ -0,0 +1,201 @@
+/*
+ * V4L2 Media controller support
+ *
+ * Copyright (C) 2006--2010 Nokia Corporation
+ * Copyright (c) 2016 Intel Corporation.
+ *
+ * Author: Sakari Ailus <sakari.ailus@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <media/media-device.h>
+#include <media/media-entity.h>
+#include <media/v4l2-subdev.h>
+#include <linux/module.h>
+
+/* -----------------------------------------------------------------------------
+ * Pipeline power management
+ *
+ * Entities must be powered up when part of a pipeline that contains at least
+ * one open video device node.
+ *
+ * To achieve this use the entity use_count field to track the number of users.
+ * For entities corresponding to video device nodes the use_count field stores
+ * the users count of the node. For entities corresponding to subdevs the
+ * use_count field stores the total number of users of all video device nodes
+ * in the pipeline.
+ *
+ * The v4l2_pipeline_pm_use() function must be called in the open() and
+ * close() handlers of video device nodes. It increments or decrements the use
+ * count of all subdev entities in the pipeline.
+ *
+ * To react to link management on powered pipelines, the link setup notification
+ * callback updates the use count of all entities in the source and sink sides
+ * of the link.
+ */
+
+/*
+ * pipeline_pm_use_count - Count the number of users of a pipeline
+ * @entity: The entity
+ *
+ * Return the total number of users of all video device nodes in the pipeline.
+ */
+static int pipeline_pm_use_count(struct media_entity *entity,
+	struct media_entity_graph *graph)
+{
+	int use = 0;
+
+	media_entity_graph_walk_start(graph, entity);
+
+	while ((entity = media_entity_graph_walk_next(graph))) {
+		if (is_media_entity_v4l2_io(entity))
+			use += entity->use_count;
+	}
+
+	return use;
+}
+
+/*
+ * pipeline_pm_power_one - Apply power change to an entity
+ * @entity: The entity
+ * @change: Use count change
+ *
+ * Change the entity use count by @change. If the entity is a subdev update its
+ * power state by calling the core::s_power operation when the use count goes
+ * from 0 to != 0 or from != 0 to 0.
+ *
+ * Return 0 on success or a negative error code on failure.
+ */
+static int pipeline_pm_power_one(struct media_entity *entity, int change)
+{
+	struct v4l2_subdev *subdev;
+	int ret;
+
+	subdev = is_media_entity_v4l2_subdev(entity)
+	       ? media_entity_to_v4l2_subdev(entity) : NULL;
+
+	if (entity->use_count == 0 && change > 0 && subdev != NULL) {
+		ret = v4l2_subdev_call(subdev, core, s_power, 1);
+		if (ret < 0 && ret != -ENOIOCTLCMD)
+			return ret;
+	}
+
+	entity->use_count += change;
+	WARN_ON(entity->use_count < 0);
+
+	if (entity->use_count == 0 && change < 0 && subdev != NULL)
+		v4l2_subdev_call(subdev, core, s_power, 0);
+
+	return 0;
+}
+
+/*
+ * pipeline_pm_power - Apply power change to all entities in a pipeline
+ * @entity: The entity
+ * @change: Use count change
+ *
+ * Walk the pipeline to update the use count and the power state of all non-node
+ * entities.
+ *
+ * Return 0 on success or a negative error code on failure.
+ */
+static int pipeline_pm_power(struct media_entity *entity, int change,
+	struct media_entity_graph *graph)
+{
+	struct media_entity *first = entity;
+	int ret = 0;
+
+	if (!change)
+		return 0;
+
+	media_entity_graph_walk_start(graph, entity);
+
+	while (!ret && (entity = media_entity_graph_walk_next(graph)))
+		if (is_media_entity_v4l2_subdev(entity))
+			ret = pipeline_pm_power_one(entity, change);
+
+	if (!ret)
+		return ret;
+
+	media_entity_graph_walk_start(graph, first);
+
+	while ((first = media_entity_graph_walk_next(graph))
+	       && first != entity)
+		if (is_media_entity_v4l2_subdev(first))
+			pipeline_pm_power_one(first, -change);
+
+	return ret;
+}
+
+int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
+{
+	struct media_device *mdev = entity->graph_obj.mdev;
+	int change = use ? 1 : -1;
+	int ret;
+
+	mutex_lock(&mdev->graph_mutex);
+
+	/* Apply use count to node. */
+	entity->use_count += change;
+	WARN_ON(entity->use_count < 0);
+
+	/* Apply power change to connected non-nodes. */
+	ret = pipeline_pm_power(entity, change, &mdev->pm_count_walk);
+	if (ret < 0)
+		entity->use_count -= change;
+
+	mutex_unlock(&mdev->graph_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_pipeline_pm_use);
+
+int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
+			      unsigned int notification)
+{
+	struct media_entity_graph *graph = &link->graph_obj.mdev->pm_count_walk;
+	struct media_entity *source = link->source->entity;
+	struct media_entity *sink = link->sink->entity;
+	int source_use;
+	int sink_use;
+	int ret = 0;
+
+	source_use = pipeline_pm_use_count(source, graph);
+	sink_use = 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. */
+		pipeline_pm_power(source, -sink_use, graph);
+		pipeline_pm_power(sink, -source_use, graph);
+		return 0;
+	}
+
+	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
+		(flags & MEDIA_LNK_FL_ENABLED)) {
+
+		ret = pipeline_pm_power(source, sink_use, graph);
+		if (ret < 0)
+			return ret;
+
+		ret = pipeline_pm_power(sink, source_use, graph);
+		if (ret < 0)
+			pipeline_pm_power(source, -sink_use, graph);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_pipeline_link_notify);
+
+MODULE_AUTHOR("Sakari Ailus <sakari.ailus@linux.intel.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("V4L2 Media controller support");
diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
new file mode 100644
index 0000000..9e353e2
--- /dev/null
+++ b/include/media/v4l2-mc.h
@@ -0,0 +1,66 @@
+/*
+ * V4L2 Media controller support
+ *
+ * Copyright (C) 2006--2010 Nokia Corporation
+ * Copyright (c) 2016 Intel Corporation.
+ *
+ * Author: Sakari Ailus <sakari.ailus@linux.intel.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License version
+ * 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __V4L2_MC_H__
+#define __V4L2_MC_H__
+
+#include <linux/types.h>
+
+struct media_entity;
+
+/**
+ * v4l2_pipeline_pm_use - Update the use count of an entity
+ * @entity: The entity
+ * @use: Use (1) or stop using (0) the entity
+ *
+ * Update the use count of all entities in the pipeline and power entities on or
+ * off accordingly.
+ *
+ * This function is intended to be called in video node open (use ==
+ * 1) and release (use == 0). It uses struct media_entity.use_count to
+ * track the power status. The use of this function should be paired
+ * with v4l2_pipeline_link_notify().
+ *
+ * Return 0 on success or a negative error code on failure. Powering entities
+ * off is assumed to never fail. No failure can occur when the use parameter is
+ * set to 0.
+ */
+int v4l2_pipeline_pm_use(struct media_entity *entity, int use);
+
+
+/**
+ * v4l2_pipeline_link_notify - Link management notification callback
+ * @link: The link
+ * @flags: New link flags that will be applied
+ * @notification: The link's state change notification type (MEDIA_DEV_NOTIFY_*)
+ *
+ * React to link management on powered pipelines by updating the use count of
+ * all entities in the source and sink sides of the link. Entities are powered
+ * on or off accordingly. The use of this function should be paired
+ * with v4l2_pipeline_pm_use().
+ *
+ * Return 0 on success or a negative error code on failure. Powering entities
+ * off is assumed to never fail. This function will not fail for disconnection
+ * events.
+ */
+int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
+			      unsigned int notification);
+
+
+#endif /* __V4L2_MC_H__ */
-- 
2.1.4

[PATCH v2 4/5] v4l: omap3isp: Use V4L2 graph PM operations

[Sakari Ailus]

Power on devices represented by entities in the graph through the pipeline
state using V4L2 graph PM operations instead of what was in the omap3isp
driver.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/omap3isp/isp.c      | 213 +----------------------------
 drivers/media/platform/omap3isp/isp.h      |   4 -
 drivers/media/platform/omap3isp/ispvideo.c |  13 +-
 drivers/media/platform/omap3isp/ispvideo.h |   1 -
 4 files changed, 6 insertions(+), 225 deletions(-)

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index 67efa9e..8be579c 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -64,6 +64,7 @@
 
 #include <media/v4l2-common.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-mc.h>
 #include <media/v4l2-of.h>
 
 #include "isp.h"
@@ -657,216 +658,6 @@ static irqreturn_t isp_isr(int irq, void *_isp)
 }
 
 /* -----------------------------------------------------------------------------
- * Pipeline power management
- *
- * Entities must be powered up when part of a pipeline that contains at least
- * one open video device node.
- *
- * To achieve this use the entity use_count field to track the number of users.
- * For entities corresponding to video device nodes the use_count field stores
- * the users count of the node. For entities corresponding to subdevs the
- * use_count field stores the total number of users of all video device nodes
- * in the pipeline.
- *
- * The omap3isp_pipeline_pm_use() function must be called in the open() and
- * close() handlers of video device nodes. It increments or decrements the use
- * count of all subdev entities in the pipeline.
- *
- * To react to link management on powered pipelines, the link setup notification
- * callback updates the use count of all entities in the source and sink sides
- * of the link.
- */
-
-/*
- * isp_pipeline_pm_use_count - Count the number of users of a pipeline
- * @entity: The entity
- *
- * 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,
-	struct media_entity_graph *graph)
-{
-	int use = 0;
-
-	media_entity_graph_walk_start(graph, entity);
-
-	while ((entity = media_entity_graph_walk_next(graph))) {
-		if (is_media_entity_v4l2_io(entity))
-			use += entity->use_count;
-	}
-
-	return use;
-}
-
-/*
- * isp_pipeline_pm_power_one - Apply power change to an entity
- * @entity: The entity
- * @change: Use count change
- *
- * Change the entity use count by @change. If the entity is a subdev update its
- * power state by calling the core::s_power operation when the use count goes
- * from 0 to != 0 or from != 0 to 0.
- *
- * Return 0 on success or a negative error code on failure.
- */
-static int isp_pipeline_pm_power_one(struct media_entity *entity, int change)
-{
-	struct v4l2_subdev *subdev;
-	int ret;
-
-	subdev = is_media_entity_v4l2_subdev(entity)
-	       ? media_entity_to_v4l2_subdev(entity) : NULL;
-
-	if (entity->use_count == 0 && change > 0 && subdev != NULL) {
-		ret = v4l2_subdev_call(subdev, core, s_power, 1);
-		if (ret < 0 && ret != -ENOIOCTLCMD)
-			return ret;
-	}
-
-	entity->use_count += change;
-	WARN_ON(entity->use_count < 0);
-
-	if (entity->use_count == 0 && change < 0 && subdev != NULL)
-		v4l2_subdev_call(subdev, core, s_power, 0);
-
-	return 0;
-}
-
-/*
- * isp_pipeline_pm_power - Apply power change to all entities in a pipeline
- * @entity: The entity
- * @change: Use count change
- *
- * Walk the pipeline to update the use count and the power state of all non-node
- * entities.
- *
- * Return 0 on success or a negative error code on failure.
- */
-static int isp_pipeline_pm_power(struct media_entity *entity, int change,
-	struct media_entity_graph *graph)
-{
-	struct media_entity *first = entity;
-	int ret = 0;
-
-	if (!change)
-		return 0;
-
-	media_entity_graph_walk_start(graph, entity);
-
-	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 ret;
-
-	media_entity_graph_walk_start(graph, first);
-
-	while ((first = media_entity_graph_walk_next(graph))
-	       && first != entity)
-		if (is_media_entity_v4l2_subdev(first))
-			isp_pipeline_pm_power_one(first, -change);
-
-	return ret;
-}
-
-/*
- * omap3isp_pipeline_pm_use - Update the use count of an entity
- * @entity: The entity
- * @use: Use (1) or stop using (0) the entity
- *
- * Update the use count of all entities in the pipeline and power entities on or
- * off accordingly.
- *
- * Return 0 on success or a negative error code on failure. Powering entities
- * 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,
-			     struct media_entity_graph *graph)
-{
-	int change = use ? 1 : -1;
-	int ret;
-
-	mutex_lock(&entity->graph_obj.mdev->graph_mutex);
-
-	/* Apply use count to node. */
-	entity->use_count += change;
-	WARN_ON(entity->use_count < 0);
-
-	/* Apply power change to connected non-nodes. */
-	ret = isp_pipeline_pm_power(entity, change, graph);
-	if (ret < 0)
-		entity->use_count -= change;
-
-	mutex_unlock(&entity->graph_obj.mdev->graph_mutex);
-
-	return ret;
-}
-
-/*
- * isp_pipeline_link_notify - Link management notification callback
- * @link: The link
- * @flags: New link flags that will be applied
- * @notification: The link's state change notification type (MEDIA_DEV_NOTIFY_*)
- *
- * React to link management on powered pipelines by updating the use count of
- * all entities in the source and sink sides of the link. Entities are powered
- * on or off accordingly.
- *
- * Return 0 on success or a negative error code on failure. Powering entities
- * off is assumed to never fail. This function will not fail for disconnection
- * events.
- */
-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;
-	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, 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, graph);
-		if (ret < 0)
-			return ret;
-
-		ret = isp_pipeline_pm_power(sink, source_use, graph);
-		if (ret < 0)
-			isp_pipeline_pm_power(source, -sink_use, graph);
-	}
-
-	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH)
-		media_entity_graph_walk_cleanup(graph);
-
-	return ret;
-}
-
-/* -----------------------------------------------------------------------------
  * Pipeline stream management
  */
 
@@ -1889,7 +1680,7 @@ static int isp_register_entities(struct isp_device *isp)
 	strlcpy(isp->media_dev.model, "TI OMAP3 ISP",
 		sizeof(isp->media_dev.model));
 	isp->media_dev.hw_revision = isp->revision;
-	isp->media_dev.link_notify = isp_pipeline_link_notify;
+	isp->media_dev.link_notify = v4l2_pipeline_link_notify;
 	media_device_init(&isp->media_dev);
 
 	isp->v4l2_dev.mdev = &isp->media_dev;
diff --git a/drivers/media/platform/omap3isp/isp.h b/drivers/media/platform/omap3isp/isp.h
index 49b7f71..7e6f663 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -177,7 +177,6 @@ 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;
 
@@ -267,9 +266,6 @@ 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,
-			     struct media_entity_graph *graph);
-
 int omap3isp_register_entities(struct platform_device *pdev,
 			       struct v4l2_device *v4l2_dev);
 void omap3isp_unregister_entities(struct platform_device *pdev);
diff --git a/drivers/media/platform/omap3isp/ispvideo.c b/drivers/media/platform/omap3isp/ispvideo.c
index 2aff755..ac76d29 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -22,8 +22,10 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
+
 #include <media/v4l2-dev.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-mc.h>
 #include <media/videobuf2-dma-contig.h>
 
 #include "ispvideo.h"
@@ -1292,12 +1294,7 @@ static int isp_video_open(struct file *file)
 		goto done;
 	}
 
-	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);
+	ret = v4l2_pipeline_pm_use(&video->video.entity, 1);
 	if (ret < 0) {
 		omap3isp_put(video->isp);
 		goto done;
@@ -1328,7 +1325,6 @@ 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);
 	}
 
@@ -1348,8 +1344,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, &handle->graph);
-	media_entity_graph_walk_cleanup(&handle->graph);
+	v4l2_pipeline_pm_use(&video->video.entity, 0);
 
 	/* 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 1564298..6a48d58 100644
--- a/drivers/media/platform/omap3isp/ispvideo.h
+++ b/drivers/media/platform/omap3isp/ispvideo.h
@@ -189,7 +189,6 @@ struct isp_video_fh {
 	struct vb2_queue queue;
 	struct v4l2_format format;
 	struct v4l2_fract timeperframe;
-	struct media_entity_graph graph;
 };
 
 #define to_isp_video_fh(fh)	container_of(fh, struct isp_video_fh, vfh)
-- 
2.1.4

[PATCH v2 5/5] staging: v4l: omap4iss: Use V4L2 graph PM operations

[Sakari Ailus]

Power on devices represented by entities in the graph through the pipeline
state using V4L2 graph PM operations instead of what was in the omap3isp
driver.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/omap4iss/iss.c       | 211 +----------------------------
 drivers/staging/media/omap4iss/iss.h       |   6 +-
 drivers/staging/media/omap4iss/iss_video.c |  15 +-
 drivers/staging/media/omap4iss/iss_video.h |   1 -
 4 files changed, 7 insertions(+), 226 deletions(-)

diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
index 30b473c..fb80d2b 100644
--- a/drivers/staging/media/omap4iss/iss.c
+++ b/drivers/staging/media/omap4iss/iss.c
@@ -363,215 +363,6 @@ static irqreturn_t iss_isr(int irq, void *_iss)
 }
 
 /* -----------------------------------------------------------------------------
- * Pipeline power management
- *
- * Entities must be powered up when part of a pipeline that contains at least
- * one open video device node.
- *
- * To achieve this use the entity use_count field to track the number of users.
- * For entities corresponding to video device nodes the use_count field stores
- * the users count of the node. For entities corresponding to subdevs the
- * use_count field stores the total number of users of all video device nodes
- * in the pipeline.
- *
- * The omap4iss_pipeline_pm_use() function must be called in the open() and
- * close() handlers of video device nodes. It increments or decrements the use
- * count of all subdev entities in the pipeline.
- *
- * To react to link management on powered pipelines, the link setup notification
- * callback updates the use count of all entities in the source and sink sides
- * of the link.
- */
-
-/*
- * iss_pipeline_pm_use_count - Count the number of users of a pipeline
- * @entity: The entity
- *
- * 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,
-				     struct media_entity_graph *graph)
-{
-	int use = 0;
-
-	media_entity_graph_walk_start(graph, entity);
-
-	while ((entity = media_entity_graph_walk_next(graph))) {
-		if (is_media_entity_v4l2_io(entity))
-			use += entity->use_count;
-	}
-
-	return use;
-}
-
-/*
- * iss_pipeline_pm_power_one - Apply power change to an entity
- * @entity: The entity
- * @change: Use count change
- *
- * Change the entity use count by @change. If the entity is a subdev update its
- * power state by calling the core::s_power operation when the use count goes
- * from 0 to != 0 or from != 0 to 0.
- *
- * Return 0 on success or a negative error code on failure.
- */
-static int iss_pipeline_pm_power_one(struct media_entity *entity, int change)
-{
-	struct v4l2_subdev *subdev;
-
-	subdev = is_media_entity_v4l2_subdev(entity)
-	       ? media_entity_to_v4l2_subdev(entity) : NULL;
-
-	if (entity->use_count == 0 && change > 0 && subdev) {
-		int ret;
-
-		ret = v4l2_subdev_call(subdev, core, s_power, 1);
-		if (ret < 0 && ret != -ENOIOCTLCMD)
-			return ret;
-	}
-
-	entity->use_count += change;
-	WARN_ON(entity->use_count < 0);
-
-	if (entity->use_count == 0 && change < 0 && subdev)
-		v4l2_subdev_call(subdev, core, s_power, 0);
-
-	return 0;
-}
-
-/*
- * iss_pipeline_pm_power - Apply power change to all entities in a pipeline
- * @entity: The entity
- * @change: Use count change
- *
- * Walk the pipeline to update the use count and the power state of all non-node
- * entities.
- *
- * Return 0 on success or a negative error code on failure.
- */
-static int iss_pipeline_pm_power(struct media_entity *entity, int change,
-				 struct media_entity_graph *graph)
-{
-	struct media_entity *first = entity;
-	int ret = 0;
-
-	if (!change)
-		return 0;
-
-	media_entity_graph_walk_start(graph, entity);
-
-	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);
-
-	while ((first = media_entity_graph_walk_next(graph)) &&
-	       first != entity)
-		if (is_media_entity_v4l2_subdev(first))
-			iss_pipeline_pm_power_one(first, -change);
-
-	return ret;
-}
-
-/*
- * omap4iss_pipeline_pm_use - Update the use count of an entity
- * @entity: The entity
- * @use: Use (1) or stop using (0) the entity
- *
- * Update the use count of all entities in the pipeline and power entities on or
- * off accordingly.
- *
- * Return 0 on success or a negative error code on failure. Powering entities
- * 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,
-			     struct media_entity_graph *graph)
-{
-	int change = use ? 1 : -1;
-	int ret;
-
-	mutex_lock(&entity->graph_obj.mdev->graph_mutex);
-
-	/* Apply use count to node. */
-	entity->use_count += change;
-	WARN_ON(entity->use_count < 0);
-
-	/* Apply power change to connected non-nodes. */
-	ret = iss_pipeline_pm_power(entity, change, graph);
-	if (ret < 0)
-		entity->use_count -= change;
-
-	mutex_unlock(&entity->graph_obj.mdev->graph_mutex);
-
-	return ret;
-}
-
-/*
- * iss_pipeline_link_notify - Link management notification callback
- * @link: The link
- * @flags: New link flags that will be applied
- *
- * React to link management on powered pipelines by updating the use count of
- * all entities in the source and sink sides of the link. Entities are powered
- * on or off accordingly.
- *
- * Return 0 on success or a negative error code on failure. Powering entities
- * off is assumed to never fail. This function will not fail for disconnection
- * events.
- */
-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;
-	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, 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, graph);
-		if (ret < 0)
-			return ret;
-
-		ret = iss_pipeline_pm_power(sink, source_use, graph);
-		if (ret < 0)
-			iss_pipeline_pm_power(source, -sink_use, graph);
-	}
-
-	if (notification == MEDIA_DEV_NOTIFY_POST_LINK_CH)
-		media_entity_graph_walk_cleanup(graph);
-
-	return ret;
-}
-
-/* -----------------------------------------------------------------------------
  * Pipeline stream management
  */
 
@@ -1197,7 +988,7 @@ static int iss_register_entities(struct iss_device *iss)
 	strlcpy(iss->media_dev.model, "TI OMAP4 ISS",
 		sizeof(iss->media_dev.model));
 	iss->media_dev.hw_revision = iss->revision;
-	iss->media_dev.link_notify = iss_pipeline_link_notify;
+	iss->media_dev.link_notify = v4l2_pipeline_link_notify;
 	ret = media_device_register(&iss->media_dev);
 	if (ret < 0) {
 		dev_err(iss->dev, "Media device registration failed (%d)\n",
diff --git a/drivers/staging/media/omap4iss/iss.h b/drivers/staging/media/omap4iss/iss.h
index 05f08a3..760ee27 100644
--- a/drivers/staging/media/omap4iss/iss.h
+++ b/drivers/staging/media/omap4iss/iss.h
@@ -15,6 +15,8 @@
 #define _OMAP4_ISS_H_
 
 #include <media/v4l2-device.h>
+#include <media/v4l2-mc.h>
+
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/platform_device.h>
@@ -87,7 +89,6 @@ 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;
 
@@ -152,9 +153,6 @@ 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,
-			     struct media_entity_graph *graph);
-
 int omap4iss_register_entities(struct platform_device *pdev,
 			       struct v4l2_device *v4l2_dev);
 void omap4iss_unregister_entities(struct platform_device *pdev);
diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c
index 058233a..f54349b 100644
--- a/drivers/staging/media/omap4iss/iss_video.c
+++ b/drivers/staging/media/omap4iss/iss_video.c
@@ -19,8 +19,10 @@
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/module.h>
+
 #include <media/v4l2-dev.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-mc.h>
 
 #include "iss_video.h"
 #include "iss.h"
@@ -1009,13 +1011,7 @@ static int iss_video_open(struct file *file)
 		goto done;
 	}
 
-	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);
+	ret = v4l2_pipeline_pm_use(&video->video.entity, 1);
 	if (ret < 0) {
 		omap4iss_put(video->iss);
 		goto done;
@@ -1054,7 +1050,6 @@ 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);
 	}
 
@@ -1070,13 +1065,11 @@ 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, &handle->graph);
+	v4l2_pipeline_pm_use(&video->video.entity, 0);
 
 	/* 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 34588b7..c8bd295 100644
--- a/drivers/staging/media/omap4iss/iss_video.h
+++ b/drivers/staging/media/omap4iss/iss_video.h
@@ -183,7 +183,6 @@ 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

Re: [PATCH v2 2/5] media: Always keep a graph walk large enough around

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3384 bytes --]

Hi Sakari,

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.5-rc1 next-20160127]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Sakari-Ailus/Unify-MC-graph-power-management-code/20160127-215417
base:   git://linuxtv.org/media_tree.git master
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/linux/init.h:1: warning: no structured comments found
   kernel/sys.c:1: warning: no structured comments found
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
   drivers/dma-buf/reservation.c:1: warning: no structured comments found
   include/linux/reservation.h:1: warning: no structured comments found
>> include/media/media-device.h:334: warning: No description found for parameter 'pm_count_walk'
>> include/media/media-device.h:334: warning: No description found for parameter 'pm_count_walk'
   include/linux/spi/spi.h:540: warning: No description found for parameter 'max_transfer_size'

vim +/pm_count_walk +334 include/media/media-device.h

57cf79b7 Mauro Carvalho Chehab 2015-08-21  318  	struct list_head interfaces;
9155d859 Mauro Carvalho Chehab 2015-08-23  319  	struct list_head pads;
9155d859 Mauro Carvalho Chehab 2015-08-23  320  	struct list_head links;
53e269c1 Laurent Pinchart      2009-12-09  321  
a08fad1e Mauro Carvalho Chehab 2015-12-09  322  	/* Protects the graph objects creation/removal */
53e269c1 Laurent Pinchart      2009-12-09  323  	spinlock_t lock;
503c3d82 Laurent Pinchart      2010-03-07  324  	/* Serializes graph operations. */
503c3d82 Laurent Pinchart      2010-03-07  325  	struct mutex graph_mutex;
12c5791e Sakari Ailus          2016-01-27  326  	/*
12c5791e Sakari Ailus          2016-01-27  327  	 * Graph walk for power state walk. Access serialised using
12c5791e Sakari Ailus          2016-01-27  328  	 * graph_mutex.
12c5791e Sakari Ailus          2016-01-27  329  	 */
12c5791e Sakari Ailus          2016-01-27  330  	struct media_entity_graph pm_count_walk;
97548ed4 Laurent Pinchart      2009-12-09  331  
813f5c0a Sylwester Nawrocki    2013-05-31  332  	int (*link_notify)(struct media_link *link, u32 flags,
813f5c0a Sylwester Nawrocki    2013-05-31  333  			   unsigned int notification);
176fb0d1 Laurent Pinchart      2009-12-09 @334  };
176fb0d1 Laurent Pinchart      2009-12-09  335  
e576d60b Shuah Khan            2015-06-05  336  #ifdef CONFIG_MEDIA_CONTROLLER
e576d60b Shuah Khan            2015-06-05  337  
813f5c0a Sylwester Nawrocki    2013-05-31  338  /* Supported link_notify @notification values. */
813f5c0a Sylwester Nawrocki    2013-05-31  339  #define MEDIA_DEV_NOTIFY_PRE_LINK_CH	0
813f5c0a Sylwester Nawrocki    2013-05-31  340  #define MEDIA_DEV_NOTIFY_POST_LINK_CH	1
813f5c0a Sylwester Nawrocki    2013-05-31  341  
176fb0d1 Laurent Pinchart      2009-12-09  342  /* media_devnode to media_device */

:::::: The code at line 334 was first introduced by commit
:::::: 176fb0d108f7495ccf9aa127e1342a1a0d87e004 [media] media: Media device

:::::: TO: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
:::::: CC: Mauro Carvalho Chehab <mchehab@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6230 bytes --]

Re: [PATCH v2 2/5] media: Always keep a graph walk large enough around

[Mauro Carvalho Chehab]

Hi Sakari,

Em Wed, 27 Jan 2016 16:47:55 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Re-create the graph walk object as needed in order to have one large enough
> available for all entities in the graph.
> 
> This enumeration is used for pipeline power management in the future.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/media-device.c | 21 +++++++++++++++++++++
>  include/media/media-device.h |  5 +++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 4d1c13d..52d7809 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -577,6 +577,26 @@ int __must_check media_device_register_entity(struct media_device *mdev,
>  
>  	spin_unlock(&mdev->lock);
>  
> +	mutex_lock(&mdev->graph_mutex);
> +	if (mdev->entity_internal_idx_max
> +	    >= mdev->pm_count_walk.ent_enum.idx_max) {
> +		struct media_entity_graph new = { 0 };
> +
> +		/*
> +		 * Initialise the new graph walk before cleaning up
> +		 * the old one in order not to spoil the graph walk
> +		 * object of the media device if graph walk init fails.
> +		 */
> +		ret = media_entity_graph_walk_init(&new, mdev);
> +		if (ret) {
> +			mutex_unlock(&mdev->graph_mutex);
> +			return ret;
> +		}
> +		media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> +		mdev->pm_count_walk = new;
> +	}
> +	mutex_unlock(&mdev->graph_mutex);
> +

I don't like the idea of creating a new graph init and destroying the
previous one every time. In principle, this needs to be done only
when trying to start the graph - or just before registering the
MC devnode, if the driver needs/wants to optimize it.

As kbuildtest also didn't like this patch, I'm not applying it
for now.

>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(media_device_register_entity);
> @@ -652,6 +672,7 @@ void media_device_cleanup(struct media_device *mdev)
>  {
>  	ida_destroy(&mdev->entity_internal_idx);
>  	mdev->entity_internal_idx_max = 0;
> +	media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
>  	mutex_destroy(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_device_cleanup);
> diff --git a/include/media/media-device.h b/include/media/media-device.h
> index d385589..dba3986 100644
> --- a/include/media/media-device.h
> +++ b/include/media/media-device.h
> @@ -323,6 +323,11 @@ struct media_device {
>  	spinlock_t lock;
>  	/* Serializes graph operations. */
>  	struct mutex graph_mutex;
> +	/*
> +	 * Graph walk for power state walk. Access serialised using
> +	 * graph_mutex.
> +	 */
> +	struct media_entity_graph pm_count_walk;
>  
>  	int (*link_notify)(struct media_link *link, u32 flags,
>  			   unsigned int notification);


-- 
Thanks,
Mauro

Re: [PATCH v2 3/5] v4l: Add generic pipeline power management code

[Mauro Carvalho Chehab]

Em Wed, 27 Jan 2016 16:47:56 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> When the Media controller framework was merged, it was decided not to add
> pipeline power management code for it was not seen generic. As a result, a
> number of drivers have copied the same piece of code, with same bugfixes
> done to them at different points of time (or not at all).
> 
> Add these functions to V4L2. Their use is optional for drivers.
> 

This patch has a trivial conflict with another patch, already merged, that
created this. So, I had to rebase, as follows...
yet, there are some issues that I'll point on a separate e-mail.


From: Sakari Ailus <sakari.ailus@iki.fi>
Date:   Wed Jan 27 12:47:56 2016 -0200

[media] v4l: Add generic pipeline power management code
    
When the Media controller framework was merged, it was decided not to add
pipeline power management code for it was not seen generic. As a result, a
number of drivers have copied the same piece of code, with same bugfixes
done to them at different points of time (or not at all).
    
Add these functions to V4L2. Their use is optional for drivers.
    
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
index a7f41b323522..f6d3463cd14c 100644
--- a/drivers/media/v4l2-core/v4l2-mc.c
+++ b/drivers/media/v4l2-core/v4l2-mc.c
@@ -256,3 +256,177 @@ int v4l2_mc_create_media_graph(struct media_device *mdev)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_mc_create_media_graph);
+
+/* -----------------------------------------------------------------------------
+ * Pipeline power management
+ *
+ * Entities must be powered up when part of a pipeline that contains at least
+ * one open video device node.
+ *
+ * To achieve this use the entity use_count field to track the number of users.
+ * For entities corresponding to video device nodes the use_count field stores
+ * the users count of the node. For entities corresponding to subdevs the
+ * use_count field stores the total number of users of all video device nodes
+ * in the pipeline.
+ *
+ * The v4l2_pipeline_pm_use() function must be called in the open() and
+ * close() handlers of video device nodes. It increments or decrements the use
+ * count of all subdev entities in the pipeline.
+ *
+ * To react to link management on powered pipelines, the link setup notification
+ * callback updates the use count of all entities in the source and sink sides
+ * of the link.
+ */
+
+/*
+ * pipeline_pm_use_count - Count the number of users of a pipeline
+ * @entity: The entity
+ *
+ * Return the total number of users of all video device nodes in the pipeline.
+ */
+static int pipeline_pm_use_count(struct media_entity *entity,
+	struct media_entity_graph *graph)
+{
+	int use = 0;
+
+	media_entity_graph_walk_start(graph, entity);
+
+	while ((entity = media_entity_graph_walk_next(graph))) {
+		if (is_media_entity_v4l2_io(entity))
+			use += entity->use_count;
+	}
+
+	return use;
+}
+
+/*
+ * pipeline_pm_power_one - Apply power change to an entity
+ * @entity: The entity
+ * @change: Use count change
+ *
+ * Change the entity use count by @change. If the entity is a subdev update its
+ * power state by calling the core::s_power operation when the use count goes
+ * from 0 to != 0 or from != 0 to 0.
+ *
+ * Return 0 on success or a negative error code on failure.
+ */
+static int pipeline_pm_power_one(struct media_entity *entity, int change)
+{
+	struct v4l2_subdev *subdev;
+	int ret;
+
+	subdev = is_media_entity_v4l2_subdev(entity)
+	       ? media_entity_to_v4l2_subdev(entity) : NULL;
+
+	if (entity->use_count == 0 && change > 0 && subdev != NULL) {
+		ret = v4l2_subdev_call(subdev, core, s_power, 1);
+		if (ret < 0 && ret != -ENOIOCTLCMD)
+			return ret;
+	}
+
+	entity->use_count += change;
+	WARN_ON(entity->use_count < 0);
+
+	if (entity->use_count == 0 && change < 0 && subdev != NULL)
+		v4l2_subdev_call(subdev, core, s_power, 0);
+
+	return 0;
+}
+
+/*
+ * pipeline_pm_power - Apply power change to all entities in a pipeline
+ * @entity: The entity
+ * @change: Use count change
+ *
+ * Walk the pipeline to update the use count and the power state of all non-node
+ * entities.
+ *
+ * Return 0 on success or a negative error code on failure.
+ */
+static int pipeline_pm_power(struct media_entity *entity, int change,
+	struct media_entity_graph *graph)
+{
+	struct media_entity *first = entity;
+	int ret = 0;
+
+	if (!change)
+		return 0;
+
+	media_entity_graph_walk_start(graph, entity);
+
+	while (!ret && (entity = media_entity_graph_walk_next(graph)))
+		if (is_media_entity_v4l2_subdev(entity))
+			ret = pipeline_pm_power_one(entity, change);
+
+	if (!ret)
+		return ret;
+
+	media_entity_graph_walk_start(graph, first);
+
+	while ((first = media_entity_graph_walk_next(graph))
+	       && first != entity)
+		if (is_media_entity_v4l2_subdev(first))
+			pipeline_pm_power_one(first, -change);
+
+	return ret;
+}
+
+int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
+{
+	struct media_device *mdev = entity->graph_obj.mdev;
+	int change = use ? 1 : -1;
+	int ret;
+
+	mutex_lock(&mdev->graph_mutex);
+
+	/* Apply use count to node. */
+	entity->use_count += change;
+	WARN_ON(entity->use_count < 0);
+
+	/* Apply power change to connected non-nodes. */
+	ret = pipeline_pm_power(entity, change, &mdev->pm_count_walk);
+	if (ret < 0)
+		entity->use_count -= change;
+
+	mutex_unlock(&mdev->graph_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_pipeline_pm_use);
+
+int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
+			      unsigned int notification)
+{
+	struct media_entity_graph *graph = &link->graph_obj.mdev->pm_count_walk;
+	struct media_entity *source = link->source->entity;
+	struct media_entity *sink = link->sink->entity;
+	int source_use;
+	int sink_use;
+	int ret = 0;
+
+	source_use = pipeline_pm_use_count(source, graph);
+	sink_use = 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. */
+		pipeline_pm_power(source, -sink_use, graph);
+		pipeline_pm_power(sink, -source_use, graph);
+		return 0;
+	}
+
+	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
+		(flags & MEDIA_LNK_FL_ENABLED)) {
+
+		ret = pipeline_pm_power(source, sink_use, graph);
+		if (ret < 0)
+			return ret;
+
+		ret = pipeline_pm_power(sink, source_use, graph);
+		if (ret < 0)
+			pipeline_pm_power(source, -sink_use, graph);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_pipeline_link_notify);
diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
index 79d84bb3573c..13252f8cd1aa 100644
--- a/include/media/v4l2-mc.h
+++ b/include/media/v4l2-mc.h
@@ -18,6 +18,12 @@
 #define _V4L2_MC_H
 
 #include <media/media-device.h>
+#include <linux/types.h>
+
+/* We don't need to include the headers for those */
+struct pci_dev;
+struct usb_device;
+struct media_entity;
 
 /**
  * enum tuner_pad_index - tuner pad index for MEDIA_ENT_F_TUNER
@@ -95,9 +101,6 @@ enum demod_pad_index {
 	DEMOD_NUM_PADS
 };
 
-/* We don't need to include pci.h or usb.h here */
-struct pci_dev;
-struct usb_device;
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 /**
@@ -144,6 +147,44 @@ struct media_device *__v4l2_mc_usb_media_device_init(struct usb_device *udev,
 						     const char *board_name,
 						     const char *driver_name);
 
+/**
+ * v4l2_pipeline_pm_use - Update the use count of an entity
+ * @entity: The entity
+ * @use: Use (1) or stop using (0) the entity
+ *
+ * Update the use count of all entities in the pipeline and power entities on or
+ * off accordingly.
+ *
+ * This function is intended to be called in video node open (use ==
+ * 1) and release (use == 0). It uses struct media_entity.use_count to
+ * track the power status. The use of this function should be paired
+ * with v4l2_pipeline_link_notify().
+ *
+ * Return 0 on success or a negative error code on failure. Powering entities
+ * off is assumed to never fail. No failure can occur when the use parameter is
+ * set to 0.
+ */
+int v4l2_pipeline_pm_use(struct media_entity *entity, int use);
+
+
+/**
+ * v4l2_pipeline_link_notify - Link management notification callback
+ * @link: The link
+ * @flags: New link flags that will be applied
+ * @notification: The link's state change notification type (MEDIA_DEV_NOTIFY_*)
+ *
+ * React to link management on powered pipelines by updating the use count of
+ * all entities in the source and sink sides of the link. Entities are powered
+ * on or off accordingly. The use of this function should be paired
+ * with v4l2_pipeline_pm_use().
+ *
+ * Return 0 on success or a negative error code on failure. Powering entities
+ * off is assumed to never fail. This function will not fail for disconnection
+ * events.
+ */
+int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
+			      unsigned int notification);
+
 #else
 static inline int v4l2_mc_create_media_graph(struct media_device *mdev)
 {
@@ -164,7 +205,8 @@ struct media_device *__v4l2_mc_usb_media_device_init(struct usb_device *udev,
 {
 	return NULL;
 }
-#endif
+
+#endif /* _V4L2_MC_H */
 
 #define v4l2_mc_usb_media_device_init(udev, name) \
 	__v4l2_mc_usb_media_device_init(udev, name, KBUILD_MODNAME)




-- 
Thanks,
Mauro

Re: [PATCH v2 3/5] v4l: Add generic pipeline power management code

[Mauro Carvalho Chehab]

Em Fri, 19 Feb 2016 12:06:58 -0200
Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu:

> Em Wed, 27 Jan 2016 16:47:56 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > When the Media controller framework was merged, it was decided not to add
> > pipeline power management code for it was not seen generic. As a result, a
> > number of drivers have copied the same piece of code, with same bugfixes
> > done to them at different points of time (or not at all).
> > 
> > Add these functions to V4L2. Their use is optional for drivers.
> > 
> 
> This patch has a trivial conflict with another patch, already merged, that
> created this. So, I had to rebase, as follows...
> yet, there are some issues that I'll point on a separate e-mail.
> 
> 
> From: Sakari Ailus <sakari.ailus@iki.fi>
> Date:   Wed Jan 27 12:47:56 2016 -0200
> 
> [media] v4l: Add generic pipeline power management code
>     
> When the Media controller framework was merged, it was decided not to add
> pipeline power management code for it was not seen generic. As a result, a
> number of drivers have copied the same piece of code, with same bugfixes
> done to them at different points of time (or not at all).
>     
> Add these functions to V4L2. Their use is optional for drivers.
>     
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> index a7f41b323522..f6d3463cd14c 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -256,3 +256,177 @@ int v4l2_mc_create_media_graph(struct media_device *mdev)
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_mc_create_media_graph);
> +
> +/* -----------------------------------------------------------------------------
> + * Pipeline power management
> + *
> + * Entities must be powered up when part of a pipeline that contains at least
> + * one open video device node.
> + *
> + * To achieve this use the entity use_count field to track the number of users.
> + * For entities corresponding to video device nodes the use_count field stores
> + * the users count of the node. For entities corresponding to subdevs the
> + * use_count field stores the total number of users of all video device nodes
> + * in the pipeline.
> + *
> + * The v4l2_pipeline_pm_use() function must be called in the open() and
> + * close() handlers of video device nodes. It increments or decrements the use
> + * count of all subdev entities in the pipeline.
> + *
> + * To react to link management on powered pipelines, the link setup notification
> + * callback updates the use count of all entities in the source and sink sides
> + * of the link.
> + */
> +
> +/*
> + * pipeline_pm_use_count - Count the number of users of a pipeline
> + * @entity: The entity
> + *
> + * Return the total number of users of all video device nodes in the pipeline.
> + */
> +static int pipeline_pm_use_count(struct media_entity *entity,
> +	struct media_entity_graph *graph)
> +{
> +	int use = 0;
> +
> +	media_entity_graph_walk_start(graph, entity);
> +
> +	while ((entity = media_entity_graph_walk_next(graph))) {
> +		if (is_media_entity_v4l2_io(entity))
> +			use += entity->use_count;
> +	}
> +
> +	return use;
> +}
> +
> +/*
> + * pipeline_pm_power_one - Apply power change to an entity
> + * @entity: The entity
> + * @change: Use count change
> + *
> + * Change the entity use count by @change. If the entity is a subdev update its
> + * power state by calling the core::s_power operation when the use count goes
> + * from 0 to != 0 or from != 0 to 0.
> + *
> + * Return 0 on success or a negative error code on failure.
> + */
> +static int pipeline_pm_power_one(struct media_entity *entity, int change)
> +{
> +	struct v4l2_subdev *subdev;
> +	int ret;
> +
> +	subdev = is_media_entity_v4l2_subdev(entity)
> +	       ? media_entity_to_v4l2_subdev(entity) : NULL;
> +
> +	if (entity->use_count == 0 && change > 0 && subdev != NULL) {
> +		ret = v4l2_subdev_call(subdev, core, s_power, 1);
> +		if (ret < 0 && ret != -ENOIOCTLCMD)
> +			return ret;
> +	}
> +
> +	entity->use_count += change;
> +	WARN_ON(entity->use_count < 0);
> +
> +	if (entity->use_count == 0 && change < 0 && subdev != NULL)
> +		v4l2_subdev_call(subdev, core, s_power, 0);
> +
> +	return 0;
> +}
> +
> +/*
> + * pipeline_pm_power - Apply power change to all entities in a pipeline
> + * @entity: The entity
> + * @change: Use count change
> + *
> + * Walk the pipeline to update the use count and the power state of all non-node
> + * entities.
> + *
> + * Return 0 on success or a negative error code on failure.
> + */
> +static int pipeline_pm_power(struct media_entity *entity, int change,
> +	struct media_entity_graph *graph)
> +{
> +	struct media_entity *first = entity;
> +	int ret = 0;
> +
> +	if (!change)
> +		return 0;
> +
> +	media_entity_graph_walk_start(graph, entity);
> +
> +	while (!ret && (entity = media_entity_graph_walk_next(graph)))
> +		if (is_media_entity_v4l2_subdev(entity))
> +			ret = pipeline_pm_power_one(entity, change);
> +
> +	if (!ret)
> +		return ret;
> +
> +	media_entity_graph_walk_start(graph, first);
> +
> +	while ((first = media_entity_graph_walk_next(graph))
> +	       && first != entity)
> +		if (is_media_entity_v4l2_subdev(first))
> +			pipeline_pm_power_one(first, -change);
> +
> +	return ret;
> +}
> +
> +int v4l2_pipeline_pm_use(struct media_entity *entity, int use)
> +{
> +	struct media_device *mdev = entity->graph_obj.mdev;
> +	int change = use ? 1 : -1;
> +	int ret;
> +
> +	mutex_lock(&mdev->graph_mutex);
> +
> +	/* Apply use count to node. */
> +	entity->use_count += change;
> +	WARN_ON(entity->use_count < 0);
> +
> +	/* Apply power change to connected non-nodes. */
> +	ret = pipeline_pm_power(entity, change, &mdev->pm_count_walk);
> +	if (ret < 0)
> +		entity->use_count -= change;
> +
> +	mutex_unlock(&mdev->graph_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_pipeline_pm_use);
> +
> +int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> +			      unsigned int notification)
> +{
> +	struct media_entity_graph *graph = &link->graph_obj.mdev->pm_count_walk;
> +	struct media_entity *source = link->source->entity;
> +	struct media_entity *sink = link->sink->entity;
> +	int source_use;
> +	int sink_use;
> +	int ret = 0;
> +
> +	source_use = pipeline_pm_use_count(source, graph);
> +	sink_use = 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. */
> +		pipeline_pm_power(source, -sink_use, graph);
> +		pipeline_pm_power(sink, -source_use, graph);
> +		return 0;
> +	}
> +
> +	if (notification == MEDIA_DEV_NOTIFY_PRE_LINK_CH &&
> +		(flags & MEDIA_LNK_FL_ENABLED)) {
> +
> +		ret = pipeline_pm_power(source, sink_use, graph);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = pipeline_pm_power(sink, source_use, graph);
> +		if (ret < 0)
> +			pipeline_pm_power(source, -sink_use, graph);
> +	}
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_pipeline_link_notify);
> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> index 79d84bb3573c..13252f8cd1aa 100644
> --- a/include/media/v4l2-mc.h
> +++ b/include/media/v4l2-mc.h
> @@ -18,6 +18,12 @@
>  #define _V4L2_MC_H
>  
>  #include <media/media-device.h>
> +#include <linux/types.h>
> +
> +/* We don't need to include the headers for those */
> +struct pci_dev;
> +struct usb_device;
> +struct media_entity;
>  
>  /**
>   * enum tuner_pad_index - tuner pad index for MEDIA_ENT_F_TUNER
> @@ -95,9 +101,6 @@ enum demod_pad_index {
>  	DEMOD_NUM_PADS
>  };
>  
> -/* We don't need to include pci.h or usb.h here */
> -struct pci_dev;
> -struct usb_device;
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  /**
> @@ -144,6 +147,44 @@ struct media_device *__v4l2_mc_usb_media_device_init(struct usb_device *udev,
>  						     const char *board_name,
>  						     const char *driver_name);
>  
> +/**
> + * v4l2_pipeline_pm_use - Update the use count of an entity
> + * @entity: The entity
> + * @use: Use (1) or stop using (0) the entity
> + *
> + * Update the use count of all entities in the pipeline and power entities on or
> + * off accordingly.
> + *
> + * This function is intended to be called in video node open (use ==
> + * 1) and release (use == 0). It uses struct media_entity.use_count to
> + * track the power status. The use of this function should be paired
> + * with v4l2_pipeline_link_notify().
> + *
> + * Return 0 on success or a negative error code on failure. Powering entities
> + * off is assumed to never fail. No failure can occur when the use parameter is
> + * set to 0.
> + */
> +int v4l2_pipeline_pm_use(struct media_entity *entity, int use);
> +
> +
> +/**
> + * v4l2_pipeline_link_notify - Link management notification callback
> + * @link: The link
> + * @flags: New link flags that will be applied
> + * @notification: The link's state change notification type (MEDIA_DEV_NOTIFY_*)
> + *
> + * React to link management on powered pipelines by updating the use count of
> + * all entities in the source and sink sides of the link. Entities are powered
> + * on or off accordingly. The use of this function should be paired
> + * with v4l2_pipeline_pm_use().
> + *
> + * Return 0 on success or a negative error code on failure. Powering entities
> + * off is assumed to never fail. This function will not fail for disconnection
> + * events.
> + */
> +int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> +			      unsigned int notification);
> +
>  #else
>  static inline int v4l2_mc_create_media_graph(struct media_device *mdev)
>  {
> @@ -164,7 +205,8 @@ struct media_device *__v4l2_mc_usb_media_device_init(struct usb_device *udev,
>  {
>  	return NULL;
>  }
> -#endif
> +
> +#endif /* _V4L2_MC_H */
>  
>  #define v4l2_mc_usb_media_device_init(udev, name) \
>  	__v4l2_mc_usb_media_device_init(udev, name, KBUILD_MODNAME)

The patch doesn't build. It fails with the following errors:

drivers/media/v4l2-core/v4l2-mc.c: In function 'pipeline_pm_power_one':
drivers/media/v4l2-core/v4l2-mc.c:319:11: error: implicit declaration of function 'media_entity_to_v4l2_subdev' [-Werror=implicit-function-declaration]
         ? media_entity_to_v4l2_subdev(entity) : NULL;
           ^
drivers/media/v4l2-core/v4l2-mc.c:319:47: warning: pointer/integer type mismatch in conditional expression
         ? media_entity_to_v4l2_subdev(entity) : NULL;
                                               ^
drivers/media/v4l2-core/v4l2-mc.c:322:9: error: implicit declaration of function 'v4l2_subdev_call' [-Werror=implicit-function-declaration]
   ret = v4l2_subdev_call(subdev, core, s_power, 1);
         ^
drivers/media/v4l2-core/v4l2-mc.c:322:34: error: 'core' undeclared (first use in this function)
   ret = v4l2_subdev_call(subdev, core, s_power, 1);
                                  ^
drivers/media/v4l2-core/v4l2-mc.c:322:34: note: each undeclared identifier is reported only once for each function it appears in
drivers/media/v4l2-core/v4l2-mc.c:322:40: error: 's_power' undeclared (first use in this function)
   ret = v4l2_subdev_call(subdev, core, s_power, 1);
                                        ^
drivers/media/v4l2-core/v4l2-mc.c: In function 'v4l2_pipeline_pm_use':
drivers/media/v4l2-core/v4l2-mc.c:387:47: error: 'struct media_device' has no member named 'pm_count_walk'
  ret = pipeline_pm_power(entity, change, &mdev->pm_count_walk);
                                               ^
drivers/media/v4l2-core/v4l2-mc.c: In function 'v4l2_pipeline_link_notify':
drivers/media/v4l2-core/v4l2-mc.c:400:58: error: 'struct media_device' has no member named 'pm_count_walk'
  struct media_entity_graph *graph = &link->graph_obj.mdev->pm_count_walk;
                                                          ^
cc1: some warnings being treated as errors
scripts/Makefile.build:258: recipe for target 'drivers/media/v4l2-core/v4l2-mc.o' failed
make[2]: *** [drivers/media/v4l2-core/v4l2-mc.o] Error 1
scripts/Makefile.build:407: recipe for target 'drivers/media/v4l2-core' failed
make[1]: *** [drivers/media/v4l2-core] Error 2
make[1]: *** Waiting for unfinished jobs....
Makefile:1391: recipe for target '_module_drivers/media' failed
make: *** [_module_drivers/media] Error 2

Adding an include <media/v4l2-subdev.h> doesn't solve it either:

drivers/media/v4l2-core/v4l2-mc.c:45 v4l2_mc_pci_media_device_init() warn: should this be a bitwise op?
drivers/media/v4l2-core/v4l2-mc.c: In function 'v4l2_pipeline_pm_use':
drivers/media/v4l2-core/v4l2-mc.c:388:47: error: 'struct media_device' has no member named 'pm_count_walk'
  ret = pipeline_pm_power(entity, change, &mdev->pm_count_walk);
                                               ^
drivers/media/v4l2-core/v4l2-mc.c: In function 'v4l2_pipeline_link_notify':
drivers/media/v4l2-core/v4l2-mc.c:401:58: error: 'struct media_device' has no member named 'pm_count_walk'
  struct media_entity_graph *graph = &link->graph_obj.mdev->pm_count_walk;
                                                          ^
scripts/Makefile.build:258: recipe for target 'drivers/media/v4l2-core/v4l2-mc.o' failed
make[2]: *** [drivers/media/v4l2-core/v4l2-mc.o] Error 1
scripts/Makefile.build:407: recipe for target 'drivers/media/v4l2-core' failed
make[1]: *** [drivers/media/v4l2-core] Error 2
Makefile:1391: recipe for target '_module_drivers/media' failed
make: *** [_module_drivers/media] Error 2


So, please fix the above issues and test if it will compile when
V4L2 subdev and/or PM is not selected and resubmit the patches.

Thanks!
Mauro
-- 
Thanks,
Mauro

Re: [PATCH v2 2/5] media: Always keep a graph walk large enough around

[Sakari Ailus]

Hi Mauro,

On Fri, Feb 19, 2016 at 12:03:41PM -0200, Mauro Carvalho Chehab wrote:
> Hi Sakari,
> 
> Em Wed, 27 Jan 2016 16:47:55 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > Re-create the graph walk object as needed in order to have one large enough
> > available for all entities in the graph.
> > 
> > This enumeration is used for pipeline power management in the future.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/media-device.c | 21 +++++++++++++++++++++
> >  include/media/media-device.h |  5 +++++
> >  2 files changed, 26 insertions(+)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index 4d1c13d..52d7809 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -577,6 +577,26 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> >  
> >  	spin_unlock(&mdev->lock);
> >  
> > +	mutex_lock(&mdev->graph_mutex);
> > +	if (mdev->entity_internal_idx_max
> > +	    >= mdev->pm_count_walk.ent_enum.idx_max) {
> > +		struct media_entity_graph new = { 0 };
> > +
> > +		/*
> > +		 * Initialise the new graph walk before cleaning up
> > +		 * the old one in order not to spoil the graph walk
> > +		 * object of the media device if graph walk init fails.
> > +		 */
> > +		ret = media_entity_graph_walk_init(&new, mdev);
> > +		if (ret) {
> > +			mutex_unlock(&mdev->graph_mutex);
> > +			return ret;
> > +		}
> > +		media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> > +		mdev->pm_count_walk = new;
> > +	}
> > +	mutex_unlock(&mdev->graph_mutex);
> > +
> 
> I don't like the idea of creating a new graph init and destroying the
> previous one every time. In principle, this needs to be done only
> when trying to start the graph - or just before registering the
> MC devnode, if the driver needs/wants to optimize it.

It's not every time --- with the previous patch, that's every 32 or 64
additional entity, depending on how many bits the unsigned long is.

> 
> As kbuildtest also didn't like this patch, I'm not applying it
> for now.

For missing KernelDoc documentation for a struct field.

Other fields in the struct don't have KernelDoc documentation either, and I
didn't feel it'd fit well for this patch. I can add a patch to change the
field documentation to the set if you like.

-- 
Kind regards,

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

Re: [PATCH v2 3/5] v4l: Add generic pipeline power management code

[Sakari Ailus]

Hi Mauro,

Thanks for the review. I'll fix the issues and resend.

This and the further patches depend on the second patch, so please only
apply them in the same order.

-- 
Regards,

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

Re: [PATCH v2 2/5] media: Always keep a graph walk large enough around

[Mauro Carvalho Chehab]

Em Fri, 19 Feb 2016 16:40:46 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> On Fri, Feb 19, 2016 at 12:03:41PM -0200, Mauro Carvalho Chehab wrote:
> > Hi Sakari,
> > 
> > Em Wed, 27 Jan 2016 16:47:55 +0200
> > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> >   
> > > Re-create the graph walk object as needed in order to have one large enough
> > > available for all entities in the graph.
> > > 
> > > This enumeration is used for pipeline power management in the future.
> > > 
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/media/media-device.c | 21 +++++++++++++++++++++
> > >  include/media/media-device.h |  5 +++++
> > >  2 files changed, 26 insertions(+)
> > > 
> > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > > index 4d1c13d..52d7809 100644
> > > --- a/drivers/media/media-device.c
> > > +++ b/drivers/media/media-device.c
> > > @@ -577,6 +577,26 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> > >  
> > >  	spin_unlock(&mdev->lock);
> > >  
> > > +	mutex_lock(&mdev->graph_mutex);
> > > +	if (mdev->entity_internal_idx_max
> > > +	    >= mdev->pm_count_walk.ent_enum.idx_max) {
> > > +		struct media_entity_graph new = { 0 };
> > > +
> > > +		/*
> > > +		 * Initialise the new graph walk before cleaning up
> > > +		 * the old one in order not to spoil the graph walk
> > > +		 * object of the media device if graph walk init fails.
> > > +		 */
> > > +		ret = media_entity_graph_walk_init(&new, mdev);
> > > +		if (ret) {
> > > +			mutex_unlock(&mdev->graph_mutex);
> > > +			return ret;
> > > +		}
> > > +		media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> > > +		mdev->pm_count_walk = new;
> > > +	}
> > > +	mutex_unlock(&mdev->graph_mutex);
> > > +  
> > 
> > I don't like the idea of creating a new graph init and destroying the
> > previous one every time. In principle, this needs to be done only
> > when trying to start the graph - or just before registering the
> > MC devnode, if the driver needs/wants to optimize it.  
> 
> It's not every time --- with the previous patch, that's every 32 or 64
> additional entity, depending on how many bits the unsigned long is.

Still it will be called a lot of times for DVB devices. Why doing that,
if such alloc can be done only after finishing registering all devices?

> 
> > 
> > As kbuildtest also didn't like this patch, I'm not applying it
> > for now.  
> 
> For missing KernelDoc documentation for a struct field.
> 
> Other fields in the struct don't have KernelDoc documentation either, and I
> didn't feel it'd fit well for this patch. I can add a patch to change the
> field documentation to the set if you like.

Ok, it could be done on a separate patch. Feel free to submit it.

Thanks!
Mauro

Re: [PATCH v2 3/5] v4l: Add generic pipeline power management code

[Mauro Carvalho Chehab]

Em Fri, 19 Feb 2016 16:49:49 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro,
> 
> Thanks for the review. I'll fix the issues and resend.
> 
> This and the further patches depend on the second patch, so please only
> apply them in the same order.

Yes, I noticed. That's why I marked patches 4 and 5 as changes requested.

Regards,
Mauro

Re: [PATCH v2 2/5] media: Always keep a graph walk large enough around

[Sakari Ailus]

Hi Mauro,

On Fri, Feb 19, 2016 at 02:14:23PM -0200, Mauro Carvalho Chehab wrote:
> Em Fri, 19 Feb 2016 16:40:46 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
> > Hi Mauro,
> > 
> > On Fri, Feb 19, 2016 at 12:03:41PM -0200, Mauro Carvalho Chehab wrote:
> > > Hi Sakari,
> > > 
> > > Em Wed, 27 Jan 2016 16:47:55 +0200
> > > Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> > >   
> > > > Re-create the graph walk object as needed in order to have one large enough
> > > > available for all entities in the graph.
> > > > 
> > > > This enumeration is used for pipeline power management in the future.
> > > > 
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/media/media-device.c | 21 +++++++++++++++++++++
> > > >  include/media/media-device.h |  5 +++++
> > > >  2 files changed, 26 insertions(+)
> > > > 
> > > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > > > index 4d1c13d..52d7809 100644
> > > > --- a/drivers/media/media-device.c
> > > > +++ b/drivers/media/media-device.c
> > > > @@ -577,6 +577,26 @@ int __must_check media_device_register_entity(struct media_device *mdev,
> > > >  
> > > >  	spin_unlock(&mdev->lock);
> > > >  
> > > > +	mutex_lock(&mdev->graph_mutex);
> > > > +	if (mdev->entity_internal_idx_max
> > > > +	    >= mdev->pm_count_walk.ent_enum.idx_max) {
> > > > +		struct media_entity_graph new = { 0 };
> > > > +
> > > > +		/*
> > > > +		 * Initialise the new graph walk before cleaning up
> > > > +		 * the old one in order not to spoil the graph walk
> > > > +		 * object of the media device if graph walk init fails.
> > > > +		 */
> > > > +		ret = media_entity_graph_walk_init(&new, mdev);
> > > > +		if (ret) {
> > > > +			mutex_unlock(&mdev->graph_mutex);
> > > > +			return ret;
> > > > +		}
> > > > +		media_entity_graph_walk_cleanup(&mdev->pm_count_walk);
> > > > +		mdev->pm_count_walk = new;
> > > > +	}
> > > > +	mutex_unlock(&mdev->graph_mutex);
> > > > +  
> > > 
> > > I don't like the idea of creating a new graph init and destroying the
> > > previous one every time. In principle, this needs to be done only
> > > when trying to start the graph - or just before registering the
> > > MC devnode, if the driver needs/wants to optimize it.  
> > 
> > It's not every time --- with the previous patch, that's every 32 or 64
> > additional entity, depending on how many bits the unsigned long is.
> 
> Still it will be called a lot of times for DVB devices. Why doing that,
> if such alloc can be done only after finishing registering all devices?

The intent is to prepare for being able to dynamically allocate and remove
entities, and still not allocate excessive amounts of memory for the graph.
With this set, there's a guarantee that the graph walk will always be
successful even if entities were added to or removed from the graph.

That's important as there are operations that may not fail such as
decrementing the use count of an entity (and possibly other entities as well
as a result; video nodes in practice).

I'd still like to claim that this will not have noticeable adverse effect on
the time it takes to register the necessary entities.

-- 
Kind regards,

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

Re: [PATCH v2 2/5] media: Always keep a graph walk large enough around

[Sakari Ailus]

On Fri, Feb 19, 2016 at 02:14:23PM -0200, Mauro Carvalho Chehab wrote:
...
> > > As kbuildtest also didn't like this patch, I'm not applying it
> > > for now.  
> > 
> > For missing KernelDoc documentation for a struct field.
> > 
> > Other fields in the struct don't have KernelDoc documentation either, and I
> > didn't feel it'd fit well for this patch. I can add a patch to change the
> > field documentation to the set if you like.
> 
> Ok, it could be done on a separate patch. Feel free to submit it.

I noticed the struct had KernelDoc comments but I missed them.

I'll update the patch accordingly. If you still think it'd be a good idea to
move the graph initialisation elsewhere, let me know. In the meantime I'll
send v3.

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