[RFC PATCH 0/4] drivers/base: Generic framework for tracking internal interfaces

[RFC PATCH 0/4] drivers/base: Generic framework for tracking internal interfaces

[Andrzej Hajda]

Generic framework for tracking internal interfaces
==================================================

Summary
-------

interface_tracker is a generic framework which allows to track appearance
and disappearance of different interfaces provided by kernel/driver code inside
the kernel. Examples of such interfaces: clocks, phys, regulators, drm_panel...
Interface is specified by a pair of object pointer and interface type. Object
type depends on interface type, for example interface type drm_panel determines
that object is a device_node. Object pointer is used to distinguish different
interfaces of the same type and should identify object the interface is bound to.
For example it could be DT node, struct device,...

The framework provides two functions for interface providers which should be
called just after interface becomes available or just before interface
removal. Interface consumers can register callback which will be called
when requested interface changes its state, if during callback registration
the interface is already up, notification will be sent also.

The framework allows nesting calls, for example it is possible to signal one
interface in tracker callback of another interface. It is done by putting
every request into the queue. If the queue is empty before adding
the request, the queue will be processed after, if there is already another
request in the queue it means the queue is already processed by different
thread so no additional action is required. With this pattern only spinlock
is necessary to protect the queue. However in case of removal of either
callback or interface caller should be sure that his request has been
processed so framework waits until the queue becomes empty, it is done using
completion mechanism.

The framework functions should not be called in atomic context. Callbacks
should be aware that they can be called in any time between registration and
de-registration, so they should use synchronization mechanisms carefully.
Callbacks should also avoid to perform time consuming tasks, the framework
serializes them, so it could stall other callbacks.

Use cases
---------

The interface is very generic, there are many situations it can be useful:

1. Replacement for subsystem specific methods of publishing/unpublishing
interfaces. Currently many frameworks allows only querying for presence of given
interface. In such cases client can defer probing or implement interface
polling, which is usually subobtimal. Additionally subsystems often do not
provide methods to signal to the consumers that they are about to be destroyed.

2. Monitoring for different interfaces provided by the same object. An example
should explain it better. Lets assume in device tree drm crtc device node have
video link to another node, so it knows only that there is something connected
to its RGB output. It can be a RGB panel (drm_panel framework), it can be an
image enhancer (SoC specific framework) or it can be some signal converter
(drm_encoder, drm_bridge, drm_encoder_slave...). Driver have only phandle to
another node. Currently it is difficult to handle such situations in a generic
way. interface_tracker should make it simple: crtc should monitor all supported
interface types that appears at the device_node pointed by the phandle.

Potential use cases
-------------------

Points mentioned above were the reasons for writing this framework. During
development I have realized that this framework can be also useful for other
tasks.

3. Replacement for deferred probing - if for some reason driver do not wants to
defer but it requires given resources it can use interface_tracker. It should be
possible to create an helper which will wait for appearance of all interfaces
from a given list, and 'continue' probe only when all resources becomes
available.

4. Replacement or helper for subsystem specific solutions:
- V4L2 subdev async registration,
- component framework.
Both frameworks solves a problem of tracking sub-components (un-)registration
by master device, it should be possible to do the same with interface_tracker
framework. Some additional helpers can be convienent to make the implementation
easier.

5. Cure the situation with sysfs 'unbind' property. Currently many drivers are
providers of different resources/interfaces: regulators, clocks, phys,
V4L2 subdevs, ... They are usually protected from module unloading by getting
module reference, but there is no protection from driver unbinding using sysfs
method: echo 'device' >/sys/bus/.../drivers/.../unbind. After unbind consumer
stays with the pointer to non-existing object, next time it tries to use it
system usually crashes. interface_tracker do not add any protection, but it adds
a way to signal to the consumer that given resource will disappear. It allows
to handle such situations more gently.

Potential issues/extensions
---------------------------

1. Scalability - the framework serializes all tasks and callbacks. In case there
are no many users it should not be a problem. If the number of users grows there
are different options to consider:
- run callbacks in parallel, I guess async_schedule_domain can be helpfull,
- partition trackers, for example per interface types - different interface
  types will use different internal queues/lists.

2. Complication of code - asynchronous programming usually seems to be more
complicated. Adding some helpers could make it less painfull.

3. Object comparison - currently object pointers are compared by value, it could
be desirable to allow also other ways of comparison, for example string
comparison. It is not a problem to extend the framework.

TODO
----

1. Testing - the patchset have not been tested yet with multiple users.
2. Add tracker support in other frameworks - currently there is only drm_panel.
   I plan also to add something more complicated, for example use it in
   exynos_drm to track components. If there is positive feedback I can try
   to add also other frameworks.
3. devm_* registration.
4. Helpers - as the situation 'wait for number interfaces before continue'
   seems to be quite common, some helper to easy handling it could be useful.

Final remarks
-------------

Primarily I have planned notifications for DRM panels. Next I have realized
something similar would be necessary for drm_bridge. Discussions with other
developers showed to me that it could be useful in many other areas. I am not
sure if other developers agree with adding it to things like regulators, clocks,
phys, but I will be glad if it can be used at least with drm_panel.

Regards
Andrzej


Andrzej Hajda (4):
  drivers/base: add interface tracker framework
  drm/panel: add interface tracker support
  drm/exynos/dpi: add interface tracker support
  drm/panel/ld9040: do not power off panel on removal

 drivers/base/Makefile                   |   2 +-
 drivers/base/interface_tracker.c        | 307 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/drm_panel.c             |   5 +
 drivers/gpu/drm/exynos/exynos_drm_dpi.c |  58 ++++--
 drivers/gpu/drm/panel/panel-ld9040.c    |   1 -
 include/linux/interface_tracker.h       |  29 +++
 6 files changed, 389 insertions(+), 13 deletions(-)
 create mode 100644 drivers/base/interface_tracker.c
 create mode 100644 include/linux/interface_tracker.h

-- 
1.8.3.2

[RFC PATCH 1/4] drivers/base: add interface tracker framework

[Andrzej Hajda]

interface_tracker is a generic framework which allows to track appearance
and disappearance of different interfaces provided by kernel/driver code inside
the kernel. Examples of such interfaces: clocks, phys, regulators, drm_panel...
Interface is specified by a pair of object pointer and interface type. Object
type depends on interface type, for example interface type drm_panel determines
that object is a device_node. Object pointer is used to distinguish different
interfaces of the same type and should identify object the interface is bound to.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/base/Makefile             |   2 +-
 drivers/base/interface_tracker.c  | 307 ++++++++++++++++++++++++++++++++++++++
 include/linux/interface_tracker.h |  27 ++++
 3 files changed, 335 insertions(+), 1 deletion(-)
 create mode 100644 drivers/base/interface_tracker.c
 create mode 100644 include/linux/interface_tracker.h

diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 04b314e..5a45c41 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -4,7 +4,7 @@ obj-y			:= component.o core.o bus.o dd.o syscore.o \
 			   driver.o class.o platform.o \
 			   cpu.o firmware.o init.o map.o devres.o \
 			   attribute_container.o transport_class.o \
-			   topology.o container.o
+			   topology.o container.o interface_tracker.o
 obj-$(CONFIG_DEVTMPFS)	+= devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y			+= power/
diff --git a/drivers/base/interface_tracker.c b/drivers/base/interface_tracker.c
new file mode 100644
index 0000000..3d36cba
--- /dev/null
+++ b/drivers/base/interface_tracker.c
@@ -0,0 +1,307 @@
+/*
+ * Generic framework for tracking kernel interfaces
+ *
+ * Copyright (c) 2014 Samsung Electronics Co., Ltd
+ * Andrzej Hajda <a.hajda@samsung.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.
+ *
+ * interface_tracker is a generic framework which allows to track appearance
+ * and disappearance of different interfaces provided by kernel/driver code
+ * inside the kernel. Examples of such interfaces: clocks, phys, regulators,
+ * drm_panel...
+ * Interface is specified by a pair of object pointer and interface type. Object
+ * type depends on interface type, for example interface type drm_panel
+ * determines that object is a device_node. Object pointer is used
+ * to distinguish different interfaces of the same type and should identify
+ * object the interface is bound to. For example it could be DT node,
+ * struct device...
+ *
+ * The framework provides two functions for interface providers which should be
+ * called just after interface becomes available or just before interface
+ * removal. Interface consumers can register callback which will be called when
+ * the requested interface changes its state, if during callback registration
+ * the interface is already up, notification will be sent also.
+ *
+ * The framework allows nesting calls, for example it is possible to signal one
+ * interface in tracker callback of another interface. It is done by putting
+ * every request into the queue. If the queue is empty before adding
+ * the request, the queue will be processed after, if there is already another
+ * request in the queue it means the queue is already processed by different
+ * thread so no additional action is required. With this pattern only spinlock
+ * is necessary to protect the queue. However in case of removal of either
+ * callback or interface caller should be sure that his request has been
+ * processed so framework waits until the queue becomes empty, it is done using
+ * completion mechanism.
+ * The framework functions should not be called in atomic context. Callbacks
+ * should be aware that they can be called in any time between registration and
+ * unregistration, so they should use synchronization mechanisms carefully.
+ * Callbacks should also avoid to perform time consuming tasks, the framework
+ * serializes them, so it could stall other callbacks.
+ */
+
+#include <linux/completion.h>
+#include <linux/interface_tracker.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+enum interface_tracker_task_id {
+	interface_tracker_task_register,
+	interface_tracker_task_unregister,
+	interface_tracker_task_ifup,
+	interface_tracker_task_ifdown,
+};
+
+struct interface_tracker_task {
+	struct list_head list;
+	enum interface_tracker_task_id task_id;
+
+	const void *object;
+	unsigned long type;
+	union {
+		struct interface_tracker_block *itb;
+		void *data;
+	};
+};
+
+struct interface_tracker_node {
+	struct list_head list;
+	struct list_head itb_head;
+	const void *object;
+	unsigned long type;
+	void *data;
+	bool ifup;
+};
+
+static LIST_HEAD(interface_tracker_queue);
+static DEFINE_SPINLOCK(interface_tracker_queue_lock);
+static DECLARE_COMPLETION(interface_tracker_queue_completion);
+
+static LIST_HEAD(interface_tracker_nodes);
+
+static struct interface_tracker_node *
+interface_tracker_get_node(const void *object, unsigned long type, bool create)
+{
+	struct interface_tracker_node *node;
+
+	list_for_each_entry(node, &interface_tracker_nodes, list) {
+		if (node->type == type && node->object == object)
+			return node;
+	}
+
+	if (!create)
+		return NULL;
+
+	node = kmalloc(sizeof(*node), GFP_KERNEL);
+	node->object = object;
+	node->type = type;
+	node->ifup = false;
+	INIT_LIST_HEAD(&node->itb_head);
+	list_add(&node->list, &interface_tracker_nodes);
+
+	return node;
+}
+
+static void interface_tracker_process_task(struct interface_tracker_task *task)
+{
+	struct interface_tracker_block *itb;
+	struct interface_tracker_node *node;
+
+	switch (task->task_id) {
+	case interface_tracker_task_register:
+		node = interface_tracker_get_node(task->object, task->type,
+						  true);
+		list_add_tail(&task->itb->list, &node->itb_head);
+
+		if (node->ifup)
+			task->itb->callback(task->itb, task->object, task->type,
+					    true, node->data);
+		return;
+	case interface_tracker_task_unregister:
+		node = interface_tracker_get_node(task->object, task->type,
+						  false);
+		if (WARN_ON(!node))
+			return;
+
+		list_for_each_entry(itb, &node->itb_head, list) {
+			if (itb != task->itb)
+				continue;
+			list_del(&itb->list);
+			if (list_empty(&node->itb_head)) {
+				list_del(&node->list);
+				kfree(node);
+			}
+			return;
+		}
+
+		WARN_ON(true);
+
+		return;
+	case interface_tracker_task_ifup:
+		node = interface_tracker_get_node(task->object, task->type,
+						  true);
+
+		if (WARN_ON(node->ifup))
+			return;
+
+		node->ifup = true;
+		node->data = task->data;
+		list_for_each_entry(itb, &node->itb_head, list)
+			itb->callback(itb, task->object, task->type, true,
+				      node->data);
+		return;
+	case interface_tracker_task_ifdown:
+		node = interface_tracker_get_node(task->object, task->type,
+						  false);
+
+		if (WARN_ON(!node))
+			return;
+
+		WARN_ON(!node->ifup);
+
+		if (list_empty(&node->itb_head)) {
+			list_del(&node->list);
+			kfree(node);
+			return;
+		}
+
+		node->ifup = false;
+		node->data = task->data;
+
+		list_for_each_entry(itb, &node->itb_head, list)
+			itb->callback(itb, task->object, task->type, false,
+				      node->data);
+	}
+}
+
+static int interface_tracker_process_queue(void)
+{
+	struct interface_tracker_task *task, *ptask = NULL;
+	unsigned long flags;
+	bool empty;
+
+	/* Queue non-emptiness is used as a sentinel to prevent processing
+	 * by multiple threads, so we cannot delete entry from the queue
+	 * until it is processed.
+	 */
+	while (true) {
+		spin_lock_irqsave(&interface_tracker_queue_lock, flags);
+
+		if (ptask)
+			list_del(&ptask->list);
+		task = list_first_entry(&interface_tracker_queue,
+					struct interface_tracker_task, list);
+
+		empty = list_empty(&interface_tracker_queue);
+		if (empty)
+			complete_all(&interface_tracker_queue_completion);
+
+		spin_unlock_irqrestore(&interface_tracker_queue_lock, flags);
+
+		if (ptask)
+			kfree(ptask);
+
+		if (empty)
+			break;
+
+		interface_tracker_process_task(task);
+		ptask = task;
+	}
+
+	return 0;
+}
+
+static int interface_tracker_add_task(struct interface_tracker_task *task,
+				      bool sync)
+{
+	unsigned long flags;
+	int ret = 0;
+	bool empty;
+
+	spin_lock_irqsave(&interface_tracker_queue_lock, flags);
+
+	empty = list_empty(&interface_tracker_queue);
+	if (empty)
+		reinit_completion(&interface_tracker_queue_completion);
+
+	list_add(&task->list, &interface_tracker_queue);
+
+	spin_unlock_irqrestore(&interface_tracker_queue_lock, flags);
+
+	if (empty) {
+		ret = interface_tracker_process_queue();
+	}else if (sync) {
+		wait_for_completion(&interface_tracker_queue_completion);
+	}
+
+	return ret;
+}
+
+int interface_tracker_register(const void *object, unsigned long type,
+			       struct interface_tracker_block *itb)
+{
+	struct interface_tracker_task *task;
+
+	task = kmalloc(sizeof(*task), GFP_KERNEL);
+	if (!task)
+		return -ENOMEM;
+
+	task->task_id = interface_tracker_task_register;
+	task->object = object;
+	task->type = type;
+	task->itb = itb;
+
+	return interface_tracker_add_task(task, false);
+}
+
+int interface_tracker_unregister(const void *object, unsigned long type,
+				 struct interface_tracker_block *itb)
+{
+	struct interface_tracker_task *task;
+
+	task = kmalloc(sizeof(*task), GFP_KERNEL);
+	if (!task)
+		return -ENOMEM;
+
+	task->task_id = interface_tracker_task_unregister;
+	task->object = object;
+	task->type = type;
+	task->itb = itb;
+
+	return interface_tracker_add_task(task, true);
+}
+
+int interface_tracker_ifup(const void *object, unsigned long type, void *data)
+{
+	struct interface_tracker_task *task;
+
+	task = kmalloc(sizeof(*task), GFP_KERNEL);
+	if (!task)
+		return -ENOMEM;
+
+	task->task_id = interface_tracker_task_ifup;
+	task->object = object;
+	task->type = type;
+	task->data = data;
+
+	return interface_tracker_add_task(task, false);
+}
+
+int interface_tracker_ifdown(const void *object, unsigned long type, void *data)
+{
+	struct interface_tracker_task *task;
+
+	task = kmalloc(sizeof(*task), GFP_KERNEL);
+	if (!task)
+		return -ENOMEM;
+
+	task->task_id = interface_tracker_task_ifdown;
+	task->object = object;
+	task->type = type;
+	task->data = data;
+
+	return interface_tracker_add_task(task, true);
+}
diff --git a/include/linux/interface_tracker.h b/include/linux/interface_tracker.h
new file mode 100644
index 0000000..e1eff00
--- /dev/null
+++ b/include/linux/interface_tracker.h
@@ -0,0 +1,27 @@
+#ifndef INTERFACE_TRACKER_H
+#define INTERFACE_TRACKER_H
+
+#include <linux/types.h>
+
+struct list_head;
+struct interface_tracker_block;
+
+typedef void (*interface_tracker_fn_t)(struct interface_tracker_block *itb,
+				       const void *object, unsigned long type,
+				       bool on, void *data);
+
+struct interface_tracker_block {
+	interface_tracker_fn_t callback;
+	struct list_head list;
+};
+
+extern int interface_tracker_register(const void *object, unsigned long type,
+				      struct interface_tracker_block *itb);
+extern int interface_tracker_unregister(const void *object, unsigned long type,
+					struct interface_tracker_block *itb);
+extern int interface_tracker_ifup(const void *object, unsigned long type,
+				  void *data);
+extern int interface_tracker_ifdown(const void *object, unsigned long type,
+				    void *data);
+
+#endif /* INTERFACE_TRACKER_H */
-- 
1.8.3.2

[RFC PATCH 2/4] drm/panel: add interface tracker support

[Andrzej Hajda]

drm_panel framework allows only query for presence of given panel.
It also does not protect panel module from unloading and does not
provide solution for driver unbinding. interface_tracker
should solve both issues.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
---
 drivers/gpu/drm/drm_panel.c       | 5 +++++
 include/linux/interface_tracker.h | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 2ef988e..72a3c5c 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -22,6 +22,7 @@
  */
 
 #include <linux/err.h>
+#include <linux/interface_tracker.h>
 #include <linux/module.h>
 
 #include <drm/drm_crtc.h>
@@ -41,6 +42,8 @@ int drm_panel_add(struct drm_panel *panel)
 	mutex_lock(&panel_lock);
 	list_add_tail(&panel->list, &panel_list);
 	mutex_unlock(&panel_lock);
+	interface_tracker_ifup(panel->dev->of_node,
+			       INTERFACE_TRACKER_TYPE_DRM_PANEL, panel);
 
 	return 0;
 }
@@ -48,6 +51,8 @@ EXPORT_SYMBOL(drm_panel_add);
 
 void drm_panel_remove(struct drm_panel *panel)
 {
+	interface_tracker_ifdown(panel->dev->of_node,
+				 INTERFACE_TRACKER_TYPE_DRM_PANEL, panel);
 	mutex_lock(&panel_lock);
 	list_del_init(&panel->list);
 	mutex_unlock(&panel_lock);
diff --git a/include/linux/interface_tracker.h b/include/linux/interface_tracker.h
index e1eff00..0a08cba 100644
--- a/include/linux/interface_tracker.h
+++ b/include/linux/interface_tracker.h
@@ -6,6 +6,8 @@
 struct list_head;
 struct interface_tracker_block;
 
+#define INTERFACE_TRACKER_TYPE_DRM_PANEL 1
+
 typedef void (*interface_tracker_fn_t)(struct interface_tracker_block *itb,
 				       const void *object, unsigned long type,
 				       bool on, void *data);
-- 
1.8.3.2

[RFC PATCH 3/4] drm/exynos/dpi: add interface tracker support

[Andrzej Hajda]

exynos_dpi uses connector polling for tracking panel presence,
this solution introduces unnecessary 10s delay before panel activation.
Moreover it is unsafe, module unloading or driver unbinding can
cause system crash. interface_tracker support solves both problems.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_dpi.c | 58 ++++++++++++++++++++++++++-------
 1 file changed, 47 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_dpi.c b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
index 2b09c7c..4c6682f 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_dpi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_dpi.c
@@ -14,6 +14,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_panel.h>
 
+#include <linux/interface_tracker.h>
 #include <linux/regulator/consumer.h>
 
 #include <video/of_videomode.h>
@@ -21,6 +22,8 @@
 
 #include "exynos_drm_drv.h"
 
+static void exynos_dpi_dpms(struct exynos_drm_display *display, int mode);
+
 struct exynos_dpi {
 	struct device *dev;
 	struct device_node *panel_node;
@@ -28,6 +31,7 @@ struct exynos_dpi {
 	struct drm_panel *panel;
 	struct drm_connector connector;
 	struct drm_encoder *encoder;
+	struct interface_tracker_block itb;
 
 	struct videomode *vm;
 	int dpms_mode;
@@ -41,15 +45,9 @@ exynos_dpi_detect(struct drm_connector *connector, bool force)
 	struct exynos_dpi *ctx = connector_to_dpi(connector);
 
 	/* panels supported only by boot-loader are always connected */
-	if (!ctx->panel_node)
+	if (ctx->vm)
 		return connector_status_connected;
 
-	if (!ctx->panel) {
-		ctx->panel = of_drm_find_panel(ctx->panel_node);
-		if (ctx->panel)
-			drm_panel_attach(ctx->panel, &ctx->connector);
-	}
-
 	if (ctx->panel)
 		return connector_status_connected;
 
@@ -114,6 +112,28 @@ static struct drm_connector_helper_funcs exynos_dpi_connector_helper_funcs = {
 	.best_encoder = exynos_dpi_best_encoder,
 };
 
+void exynos_dpi_notifier(struct interface_tracker_block *itb,
+			 const void *object, unsigned long type, bool on,
+			 void *data)
+{
+	struct exynos_dpi *ctx = container_of(itb, struct exynos_dpi, itb);
+
+	mutex_lock(&ctx->connector.dev->mode_config.mutex);
+	if (on) {
+		ctx->panel = data;
+		drm_panel_attach(ctx->panel, &ctx->connector);
+	} else {
+		struct exynos_drm_display *display;
+
+		display = platform_get_drvdata(to_platform_device(ctx->dev));
+		exynos_dpi_dpms(display, DRM_MODE_DPMS_OFF);
+		drm_panel_detach(ctx->panel);
+		ctx->panel = NULL;
+	}
+	mutex_unlock(&ctx->connector.dev->mode_config.mutex);
+	drm_helper_hpd_irq_event(ctx->connector.dev);
+}
+
 static int exynos_dpi_create_connector(struct exynos_drm_display *display,
 				       struct drm_encoder *encoder)
 {
@@ -123,10 +143,7 @@ static int exynos_dpi_create_connector(struct exynos_drm_display *display,
 
 	ctx->encoder = encoder;
 
-	if (ctx->panel_node)
-		connector->polled = DRM_CONNECTOR_POLL_CONNECT;
-	else
-		connector->polled = DRM_CONNECTOR_POLL_HPD;
+	connector->polled = DRM_CONNECTOR_POLL_HPD;
 
 	ret = drm_connector_init(encoder->dev, connector,
 				 &exynos_dpi_connector_funcs,
@@ -140,9 +157,27 @@ static int exynos_dpi_create_connector(struct exynos_drm_display *display,
 	drm_sysfs_connector_add(connector);
 	drm_mode_connector_attach_encoder(connector, encoder);
 
+	if (ctx->panel_node) {
+		ctx->itb.callback = exynos_dpi_notifier;
+		interface_tracker_register(ctx->panel_node,
+					   INTERFACE_TRACKER_TYPE_DRM_PANEL,
+					   &ctx->itb);
+	}
+
 	return 0;
 }
 
+static void exynos_dpi_display_remove(struct exynos_drm_display *display)
+{
+	struct exynos_dpi *ctx = display->ctx;
+
+	if (ctx->panel_node) {
+		interface_tracker_unregister(ctx->panel_node,
+					     INTERFACE_TRACKER_TYPE_DRM_PANEL,
+					     &ctx->itb);
+	}
+}
+
 static void exynos_dpi_poweron(struct exynos_dpi *ctx)
 {
 	if (ctx->panel)
@@ -178,6 +213,7 @@ static void exynos_dpi_dpms(struct exynos_drm_display *display, int mode)
 
 static struct exynos_drm_display_ops exynos_dpi_display_ops = {
 	.create_connector = exynos_dpi_create_connector,
+	.remove = exynos_dpi_display_remove,
 	.dpms = exynos_dpi_dpms
 };
 
-- 
1.8.3.2

[RFC PATCH 4/4] drm/panel/ld9040: do not power off panel on removal

[Andrzej Hajda]

Panel is powered off already by connector during drm_panel_remove call.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/panel/panel-ld9040.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-ld9040.c b/drivers/gpu/drm/panel/panel-ld9040.c
index 1f1f837..1def4b0 100644
--- a/drivers/gpu/drm/panel/panel-ld9040.c
+++ b/drivers/gpu/drm/panel/panel-ld9040.c
@@ -348,7 +348,6 @@ static int ld9040_remove(struct spi_device *spi)
 {
 	struct ld9040 *ctx = spi_get_drvdata(spi);
 
-	ld9040_power_off(ctx);
 	drm_panel_remove(&ctx->panel);
 
 	return 0;
-- 
1.8.3.2

Re: [RFC PATCH 0/4] drivers/base: Generic framework for tracking internal interfaces

[Greg Kroah-Hartman]

On Wed, Apr 30, 2014 at 04:02:50PM +0200, Andrzej Hajda wrote:
> Generic framework for tracking internal interfaces
> ==================================================
> 
> Summary
> -------
> 
> interface_tracker is a generic framework which allows to track appearance
> and disappearance of different interfaces provided by kernel/driver code inside
> the kernel. Examples of such interfaces: clocks, phys, regulators, drm_panel...
> Interface is specified by a pair of object pointer and interface type. Object
> type depends on interface type, for example interface type drm_panel determines
> that object is a device_node. Object pointer is used to distinguish different
> interfaces of the same type and should identify object the interface is bound to.
> For example it could be DT node, struct device,...
> 
> The framework provides two functions for interface providers which should be
> called just after interface becomes available or just before interface
> removal. Interface consumers can register callback which will be called
> when requested interface changes its state, if during callback registration
> the interface is already up, notification will be sent also.
> 
> The framework allows nesting calls, for example it is possible to signal one
> interface in tracker callback of another interface. It is done by putting
> every request into the queue. If the queue is empty before adding
> the request, the queue will be processed after, if there is already another
> request in the queue it means the queue is already processed by different
> thread so no additional action is required. With this pattern only spinlock
> is necessary to protect the queue. However in case of removal of either
> callback or interface caller should be sure that his request has been
> processed so framework waits until the queue becomes empty, it is done using
> completion mechanism.
> 
> The framework functions should not be called in atomic context. Callbacks
> should be aware that they can be called in any time between registration and
> de-registration, so they should use synchronization mechanisms carefully.
> Callbacks should also avoid to perform time consuming tasks, the framework
> serializes them, so it could stall other callbacks.
> 
> Use cases
> ---------
> 
> The interface is very generic, there are many situations it can be useful:
> 
> 1. Replacement for subsystem specific methods of publishing/unpublishing
> interfaces. Currently many frameworks allows only querying for presence of given
> interface. In such cases client can defer probing or implement interface
> polling, which is usually subobtimal. Additionally subsystems often do not
> provide methods to signal to the consumers that they are about to be destroyed.
> 
> 2. Monitoring for different interfaces provided by the same object. An example
> should explain it better. Lets assume in device tree drm crtc device node have
> video link to another node, so it knows only that there is something connected
> to its RGB output. It can be a RGB panel (drm_panel framework), it can be an
> image enhancer (SoC specific framework) or it can be some signal converter
> (drm_encoder, drm_bridge, drm_encoder_slave...). Driver have only phandle to
> another node. Currently it is difficult to handle such situations in a generic
> way. interface_tracker should make it simple: crtc should monitor all supported
> interface types that appears at the device_node pointed by the phandle.
> 
> Potential use cases
> -------------------
> 
> Points mentioned above were the reasons for writing this framework. During
> development I have realized that this framework can be also useful for other
> tasks.
> 
> 3. Replacement for deferred probing - if for some reason driver do not wants to
> defer but it requires given resources it can use interface_tracker. It should be
> possible to create an helper which will wait for appearance of all interfaces
> from a given list, and 'continue' probe only when all resources becomes
> available.
> 
> 4. Replacement or helper for subsystem specific solutions:
> - V4L2 subdev async registration,
> - component framework.
> Both frameworks solves a problem of tracking sub-components (un-)registration
> by master device, it should be possible to do the same with interface_tracker
> framework. Some additional helpers can be convienent to make the implementation
> easier.
> 
> 5. Cure the situation with sysfs 'unbind' property. Currently many drivers are
> providers of different resources/interfaces: regulators, clocks, phys,
> V4L2 subdevs, ... They are usually protected from module unloading by getting
> module reference, but there is no protection from driver unbinding using sysfs
> method: echo 'device' >/sys/bus/.../drivers/.../unbind. After unbind consumer
> stays with the pointer to non-existing object, next time it tries to use it
> system usually crashes. interface_tracker do not add any protection, but it adds
> a way to signal to the consumer that given resource will disappear. It allows
> to handle such situations more gently.
> 
> Potential issues/extensions
> ---------------------------
> 
> 1. Scalability - the framework serializes all tasks and callbacks. In case there
> are no many users it should not be a problem. If the number of users grows there
> are different options to consider:
> - run callbacks in parallel, I guess async_schedule_domain can be helpfull,
> - partition trackers, for example per interface types - different interface
>   types will use different internal queues/lists.
> 
> 2. Complication of code - asynchronous programming usually seems to be more
> complicated. Adding some helpers could make it less painfull.
> 
> 3. Object comparison - currently object pointers are compared by value, it could
> be desirable to allow also other ways of comparison, for example string
> comparison. It is not a problem to extend the framework.
> 
> TODO
> ----
> 
> 1. Testing - the patchset have not been tested yet with multiple users.

That's not good :)

What's wrong with the existing container code in the driver core, or the
component code?  Shouldn't either of those work properly for you?  Or
the managed token interface being proposed, how does that tie in here?

> 2. Add tracker support in other frameworks - currently there is only drm_panel.
>    I plan also to add something more complicated, for example use it in
>    exynos_drm to track components. If there is positive feedback I can try
>    to add also other frameworks.
> 3. devm_* registration.
> 4. Helpers - as the situation 'wait for number interfaces before continue'
>    seems to be quite common, some helper to easy handling it could be useful.
> 
> Final remarks
> -------------
> 
> Primarily I have planned notifications for DRM panels. Next I have realized
> something similar would be necessary for drm_bridge. Discussions with other
> developers showed to me that it could be useful in many other areas. I am not
> sure if other developers agree with adding it to things like regulators, clocks,
> phys, but I will be glad if it can be used at least with drm_panel.

Shouldn't the component code handle your DRM panels today?

thanks,

greg k-h

Re: [RFC PATCH 0/4] drivers/base: Generic framework for tracking internal interfaces

[Andrzej Hajda]

Hi Greg,

Thanks for comments. I CC Laurent, I hope it could be interesting for 
him also.

Greg Kroah-Hartman wrote, On 30.04.2014 17:49:
> On Wed, Apr 30, 2014 at 04:02:50PM +0200, Andrzej Hajda wrote:
>> Generic framework for tracking internal interfaces
>> ==================================================
>>
>> Summary
>> -------
>>
>> interface_tracker is a generic framework which allows to track appearance
>> and disappearance of different interfaces provided by kernel/driver code inside
>> the kernel. Examples of such interfaces: clocks, phys, regulators, drm_panel...
>> Interface is specified by a pair of object pointer and interface type. Object
>> type depends on interface type, for example interface type drm_panel determines
>> that object is a device_node. Object pointer is used to distinguish different
>> interfaces of the same type and should identify object the interface is bound to.
>> For example it could be DT node, struct device,...
>>
>> The framework provides two functions for interface providers which should be
>> called just after interface becomes available or just before interface
>> removal. Interface consumers can register callback which will be called
>> when requested interface changes its state, if during callback registration
>> the interface is already up, notification will be sent also.
>>
>> The framework allows nesting calls, for example it is possible to signal one
>> interface in tracker callback of another interface. It is done by putting
>> every request into the queue. If the queue is empty before adding
>> the request, the queue will be processed after, if there is already another
>> request in the queue it means the queue is already processed by different
>> thread so no additional action is required. With this pattern only spinlock
>> is necessary to protect the queue. However in case of removal of either
>> callback or interface caller should be sure that his request has been
>> processed so framework waits until the queue becomes empty, it is done using
>> completion mechanism.
>>
>> The framework functions should not be called in atomic context. Callbacks
>> should be aware that they can be called in any time between registration and
>> de-registration, so they should use synchronization mechanisms carefully.
>> Callbacks should also avoid to perform time consuming tasks, the framework
>> serializes them, so it could stall other callbacks.
>>
>> Use cases
>> ---------
>>
>> The interface is very generic, there are many situations it can be useful:
>>
>> 1. Replacement for subsystem specific methods of publishing/unpublishing
>> interfaces. Currently many frameworks allows only querying for presence of given
>> interface. In such cases client can defer probing or implement interface
>> polling, which is usually subobtimal. Additionally subsystems often do not
>> provide methods to signal to the consumers that they are about to be destroyed.
>>
>> 2. Monitoring for different interfaces provided by the same object. An example
>> should explain it better. Lets assume in device tree drm crtc device node have
>> video link to another node, so it knows only that there is something connected
>> to its RGB output. It can be a RGB panel (drm_panel framework), it can be an
>> image enhancer (SoC specific framework) or it can be some signal converter
>> (drm_encoder, drm_bridge, drm_encoder_slave...). Driver have only phandle to
>> another node. Currently it is difficult to handle such situations in a generic
>> way. interface_tracker should make it simple: crtc should monitor all supported
>> interface types that appears at the device_node pointed by the phandle.
>>
>> Potential use cases
>> -------------------
>>
>> Points mentioned above were the reasons for writing this framework. During
>> development I have realized that this framework can be also useful for other
>> tasks.
>>
>> 3. Replacement for deferred probing - if for some reason driver do not wants to
>> defer but it requires given resources it can use interface_tracker. It should be
>> possible to create an helper which will wait for appearance of all interfaces
>> from a given list, and 'continue' probe only when all resources becomes
>> available.
>>
>> 4. Replacement or helper for subsystem specific solutions:
>> - V4L2 subdev async registration,
>> - component framework.
>> Both frameworks solves a problem of tracking sub-components (un-)registration
>> by master device, it should be possible to do the same with interface_tracker
>> framework. Some additional helpers can be convienent to make the implementation
>> easier.
>>
>> 5. Cure the situation with sysfs 'unbind' property. Currently many drivers are
>> providers of different resources/interfaces: regulators, clocks, phys,
>> V4L2 subdevs, ... They are usually protected from module unloading by getting
>> module reference, but there is no protection from driver unbinding using sysfs
>> method: echo 'device' >/sys/bus/.../drivers/.../unbind. After unbind consumer
>> stays with the pointer to non-existing object, next time it tries to use it
>> system usually crashes. interface_tracker do not add any protection, but it adds
>> a way to signal to the consumer that given resource will disappear. It allows
>> to handle such situations more gently.
>>
>> Potential issues/extensions
>> ---------------------------
>>
>> 1. Scalability - the framework serializes all tasks and callbacks. In case there
>> are no many users it should not be a problem. If the number of users grows there
>> are different options to consider:
>> - run callbacks in parallel, I guess async_schedule_domain can be helpfull,
>> - partition trackers, for example per interface types - different interface
>>    types will use different internal queues/lists.
>>
>> 2. Complication of code - asynchronous programming usually seems to be more
>> complicated. Adding some helpers could make it less painfull.
>>
>> 3. Object comparison - currently object pointers are compared by value, it could
>> be desirable to allow also other ways of comparison, for example string
>> comparison. It is not a problem to extend the framework.
>>
>> TODO
>> ----
>>
>> 1. Testing - the patchset have not been tested yet with multiple users.
>
> That's not good :)

I would treat this series as a real RFC, with code which can be easily 
tested by volunteers but as for RFC I am asking for comments if the idea 
is OK and the patchset have chances to be accepted.
Anyway, I have performed some basic tests and it works :)

>
> What's wrong with the existing container code in the driver core, or the
> component code?  Shouldn't either of those work properly for you?  Or
> the managed token interface being proposed, how does that tie in here?

Container code seems to solve very specific problem, not really relevant 
to this patchset. The component code would solve a very specific part of 
problems this framework solves as described in use cases above. On the 
other side problems which components solves are
solvable with this framework.

Managed token interface is about sharing 'named' resources it is not 
connected with interface tracking.

>
>> 2. Add tracker support in other frameworks - currently there is only drm_panel.
>>     I plan also to add something more complicated, for example use it in
>>     exynos_drm to track components. If there is positive feedback I can try
>>     to add also other frameworks.
>> 3. devm_* registration.
>> 4. Helpers - as the situation 'wait for number interfaces before continue'
>>     seems to be quite common, some helper to easy handling it could be useful.
>>
>> Final remarks
>> -------------
>>
>> Primarily I have planned notifications for DRM panels. Next I have realized
>> something similar would be necessary for drm_bridge. Discussions with other
>> developers showed to me that it could be useful in many other areas. I am not
>> sure if other developers agree with adding it to things like regulators, clocks,
>> phys, but I will be glad if it can be used at least with drm_panel.
>
> Shouldn't the component code handle your DRM panels today?

I should correct my last sentence to this:
I will be glad if it can be used at least with video interfaces, ie. 
drm_panel, video encoders/bridges, image enhancers.

The main problem with component framework is that componentization 
significantly changes every driver and changes it in a way which is not 
compatible with traditional drivers, so devices which are intended to 
work with different DRM masters are hard to componentize if some of DRMs 
are componentized and some not.

If you look at interface_tracker framework it achieves the same goal
as componentization without even touching panel drivers, look at 2nd 
patch of the patchset - it adds interface_tracker support for all panels 
with 7 lines of code in drm_panel framework and of course the
change is backward compatible.
Adding interface_tracker support to the 'master' requires more work:
1. registration of the callback.
2. unregistration of the callback.
3. the callback code.
But it is still much less than in case of components.

And interface_tracker framework allows much more, as I described in "Use 
cases" and "Potential use cases" sections.

Regards
Andrzej

>
> thanks,
>
> greg k-h
>

Re: [RFC PATCH 0/4] drivers/base: Generic framework for tracking internal interfaces

[Russell King - ARM Linux]

On Wed, Apr 30, 2014 at 11:42:09PM +0200, Andrzej Hajda wrote:
> The main problem with component framework is that componentization  
> significantly changes every driver and changes it in a way which is not  
> compatible with traditional drivers, so devices which are intended to  
> work with different DRM masters are hard to componentize if some of DRMs  
> are componentized and some not.

Many of the problems which the component helpers are designed to solve
are those where you need the drm_device structure (or snd_card, or whatever
subsystem specific card/device representation structure) pre-created in
order to initialise the components.

In the case of DRM, you can't initialise encoders or connectors without
their drm_device structure pre-existing - because these components are
attached to the drm_device.

Your solution to that is to delay those calls, but the DRM subsystem is
not designed to cope like that - it's designed such that when the
connector or encoder initialisation functions are called, it is assumed
that the driver is initialising its state. (I've raised this point before
but you've just fobbed it off in the past.)

Another issue here is that the order of initialisation matters greatly.
Take CRTCs for example.  In DRM, the order of attachment of CRTCs defines
their identity, changing the order changes their identity, and changes
how they are bound to their respective connectors.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [RFC PATCH 0/4] drivers/base: Generic framework for tracking internal interfaces

[Andrzej Hajda]

Russell King - ARM Linux wrote, On 01.05.2014 00:28:
> On Wed, Apr 30, 2014 at 11:42:09PM +0200, Andrzej Hajda wrote:
>> The main problem with component framework is that componentization
>> significantly changes every driver and changes it in a way which is not
>> compatible with traditional drivers, so devices which are intended to
>> work with different DRM masters are hard to componentize if some of DRMs
>> are componentized and some not.
> Many of the problems which the component helpers are designed to solve
> are those where you need the drm_device structure (or snd_card, or whatever
> subsystem specific card/device representation structure) pre-created in
> order to initialise the components.
>
> In the case of DRM, you can't initialise encoders or connectors without
> their drm_device structure pre-existing - because these components are
> attached to the drm_device.
>
> Your solution to that is to delay those calls, but the DRM subsystem is
> not designed to cope like that - it's designed such that when the
> connector or encoder initialisation functions are called, it is assumed
> that the driver is initialising its state. (I've raised this point before
> but you've just fobbed it off in the past.)
>
> Another issue here is that the order of initialisation matters greatly.
> Take CRTCs for example.  In DRM, the order of attachment of CRTCs defines
> their identity, changing the order changes their identity, and changes
> how they are bound to their respective connectors.
>
The two problems you show here are not a real problems in this framework:
1. making real device initialization during drm initialization - 
decision is left
to driver developer what should be done in probe, what should be done in
'bind', I guess this is also true for components, at least the framework 
allows it.
2. initialization order - if you put initialization into components 
'bind' function,
master can choose any order of calls to 'bind'.

Anyway you can implement the same behaviour as components with
interface_tracker. Just simple proof of concept, how to convert 
componentized
drivers to interface_tracker:
Components:
1. you can reuse component_ops
2. You replace calls of component_add and component_del with calls
to interface_tracker_ifup(dev, INTERFACE_TRACKER_TYPE_COMPONENT, 
&specific_component_ops),
or interface_tracker_ifdown.
Thats all for components.

Master:
1. you register callback for tracking all components.
2. in the callback you check if all components are up, if yes you do the
same as in component framework initialization, to simplify it
helper function can be added.

I guess it should work the same way, if there is interest in it I can 
develop the
helper next week, I hope.

What is the benefit of interface_tracker:
1. interface_tracker is more generic - it can track not only components.
2. you put component initialization code into helper function - sounds 
like mid-layer removal,
developer can choose different helper if it suits better.

So from component point of view interface_tracker can be treated as kind 
of extensions
of the component framework.

I hope I have answerer all your concerns.

I have holidays till Sunday and I am not sure if I will be able to 
answer next emails before
Monday.

Regards
Andrzej

Re: [RFC PATCH 0/4] drivers/base: Generic framework for tracking internal interfaces

[Russell King - ARM Linux]

On Thu, May 01, 2014 at 09:04:19AM +0200, Andrzej Hajda wrote:
> 2. You replace calls of component_add and component_del with calls
> to interface_tracker_ifup(dev, INTERFACE_TRACKER_TYPE_COMPONENT,  
> &specific_component_ops),
> or interface_tracker_ifdown.
> Thats all for components.

How does the master get to decide which components are for it?  As
I see it, all masters see all components of a particular "type".
What if you have a system with two masters each of which are bound
to a set of unique components but some of the components are of a
the same type?

How does the master know what "type" to use?

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [RFC PATCH 0/4] drivers/base: Generic framework for tracking internal interfaces

[Andrzej Hajda]

On 05/01/2014 11:11 AM, Russell King - ARM Linux wrote:
> On Thu, May 01, 2014 at 09:04:19AM +0200, Andrzej Hajda wrote:
>> 2. You replace calls of component_add and component_del with calls
>> to interface_tracker_ifup(dev, INTERFACE_TRACKER_TYPE_COMPONENT,  
>> &specific_component_ops),
>> or interface_tracker_ifdown.
>> Thats all for components.
> How does the master get to decide which components are for it?

This is not a part of the framework. The master can construct
the list of its components anyhow. Some examples:
1. List of device tree video interface port nodes pointed by
master's remote-endpoint phandles.
2. List of device nodes pointed by supernode phandles.
3. Nodes pointed by other phandles in master's node.
4. Devices compatible with the list of drivers.

This is for create list of objects. As interface type it should
use the types of interface it expects at given nodes and is able to handle.

Small issue:
If the master creates list of devices and for particular interface
type expects DT node as the object, translation is easy: dev->of_node.
But if the situation is reverse kernel does not provide generic helper
for translating of_node to device, however kernel have everything to
provide such function if necessary.
Other solution is to use only DT nodes for object identification,
it will narrow the framework usage to DT architectures, but seems to be
more flexible anyway - we can have more than one interface of given type per
device, we can distinguish them by port node.

>   As
> I see it, all masters see all components of a particular "type".
> What if you have a system with two masters each of which are bound
> to a set of unique components but some of the components are of a
> the same type?

The master receives notifications only about interfaces he has
registered callback for. For example:
interface_tracker_register(node, INTERFACE_TRACKER_TYPE_DRM_PANEL, cb);

means that 'cb' callback will be called only if panel identifed by node
is up or down.
If the driver expect that at the 'node' there could be also component it
can also
listen for components:
interface_tracker_register(node, INTERFACE_TRACKER_TYPE_COMPONENT, cb);

Now 'cb' will be called if component or panel appears/disappears at node
'node'.

so callback function can look like:

void cb_func(struct interface_tracker_block *itb, const void *object,
unsigned long type, bool on,
                      void *data)
{
    struct priv_struct *priv = container_of(itb, struct priv_struct, itb);
    switch(type) {
    case INTERFACE_TRACKER_TYPE_DRM_PANEL:
        handle_drm_panel(priv, data, on);
        break;
    case INTERFACE_TRACKER_TYPE_DRM_COMPONENT:
        handle_drm_component(priv, data, object, on);
        break;
    }
}

where handlers can look like:

void handle_drm_panel(struct priv_struct *priv, struct drm_panel *panel,
bool on);
void handle_drm_component(struct priv_struct *priv, struct component_ops
*ops, struct device *dev, bool on);

>
> How does the master know what "type" to use?
>

It should use type which it expects at the given node.

Regards
Andrzej