[PATCH 1/5] gpu: host1x: Call ->remove() only when a device is bound

[PATCH 1/5] gpu: host1x: Call ->remove() only when a device is bound

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

When a driver's ->probe() function fails, the host1x bus must not call
its ->remove() function because the driver will already have cleaned up
in the error handling path in ->probe().

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/host1x/bus.c | 9 +++++++--
 include/linux/host1x.h   | 2 ++
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index aaf54859adb0..e4182e68e29c 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -116,7 +116,10 @@ static void host1x_subdev_register(struct host1x_device *device,
 	if (list_empty(&device->subdevs)) {
 		err = device->driver->probe(device);
 		if (err < 0)
-			dev_err(&device->dev, "probe failed: %d\n", err);
+			dev_err(&device->dev, "probe failed for %ps: %d\n",
+				device->driver, err);
+		else
+			device->bound = true;
 	}
 }
 
@@ -130,10 +133,12 @@ static void __host1x_subdev_unregister(struct host1x_device *device,
 	 * If all subdevices have been activated, we're about to remove the
 	 * first active subdevice, so unload the driver first.
 	 */
-	if (list_empty(&device->subdevs)) {
+	if (list_empty(&device->subdevs) && device->bound) {
 		err = device->driver->remove(device);
 		if (err < 0)
 			dev_err(&device->dev, "remove failed: %d\n", err);
+
+		device->bound = false;
 	}
 
 	/*
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index bb9840fd1e18..7890b553d12e 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -272,6 +272,8 @@ struct host1x_device {
 
 	struct mutex clients_lock;
 	struct list_head clients;
+
+	bool bound;
 };
 
 static inline struct host1x_device *to_host1x_device(struct device *dev)
-- 
2.1.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] gpu: host1x: Call host1x_device_add() under lock

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Instead of locking within host1x_device_add(), call it under the lock to
make the locking more consistent.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/host1x/bus.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index e4182e68e29c..769116dba797 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -323,9 +323,7 @@ static int host1x_device_add(struct host1x *host1x,
 		return err;
 	}
 
-	mutex_lock(&host1x->devices_lock);
 	list_add_tail(&device->list, &host1x->devices);
-	mutex_unlock(&host1x->devices_lock);
 
 	mutex_lock(&clients_lock);
 
@@ -414,11 +412,11 @@ static void host1x_attach_driver(struct host1x *host1x,
 		}
 	}
 
-	mutex_unlock(&host1x->devices_lock);
-
 	err = host1x_device_add(host1x, driver);
 	if (err < 0)
 		dev_err(host1x->dev, "failed to allocate device: %d\n", err);
+
+	mutex_unlock(&host1x->devices_lock);
 }
 
 static void host1x_detach_driver(struct host1x *host1x,
-- 
2.1.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] gpu: host1x: Factor out __host1x_device_del()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

This function is needed in several places, so factor it out.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/host1x/bus.c | 93 +++++++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index 769116dba797..0b52f0ea8871 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -279,10 +279,59 @@ void host1x_bus_exit(void)
 	bus_unregister(&host1x_bus_type);
 }
 
+static void __host1x_device_del(struct host1x_device *device)
+{
+	struct host1x_subdev *subdev, *sd;
+	struct host1x_client *client, *cl;
+
+	mutex_lock(&device->subdevs_lock);
+
+	/* unregister subdevices */
+	list_for_each_entry_safe(subdev, sd, &device->active, list) {
+		/*
+		 * host1x_subdev_unregister() will remove the client from
+		 * any lists, so we'll need to manually add it back to the
+		 * list of idle clients.
+		 *
+		 * XXX: Alternatively, perhaps don't remove the client from
+		 * any lists in host1x_subdev_unregister() and instead do
+		 * that explicitly from host1x_unregister_client()?
+		 */
+		client = subdev->client;
+
+		__host1x_subdev_unregister(device, subdev);
+
+		/* add the client to the list of idle clients */
+		mutex_lock(&clients_lock);
+		list_add_tail(&client->list, &clients);
+		mutex_unlock(&clients_lock);
+	}
+
+	/* remove subdevices */
+	list_for_each_entry_safe(subdev, sd, &device->subdevs, list)
+		host1x_subdev_del(subdev);
+
+	mutex_unlock(&device->subdevs_lock);
+
+	/* move clients to idle list */
+	mutex_lock(&clients_lock);
+	mutex_lock(&device->clients_lock);
+
+	list_for_each_entry_safe(client, cl, &device->clients, list)
+		list_move_tail(&client->list, &clients);
+
+	mutex_unlock(&device->clients_lock);
+	mutex_unlock(&clients_lock);
+
+	/* finally remove the device */
+	list_del_init(&device->list);
+}
+
 static void host1x_device_release(struct device *dev)
 {
 	struct host1x_device *device = to_host1x_device(dev);
 
+	__host1x_device_del(device);
 	kfree(device);
 }
 
@@ -350,50 +399,6 @@ static int host1x_device_add(struct host1x *host1x,
 static void host1x_device_del(struct host1x *host1x,
 			      struct host1x_device *device)
 {
-	struct host1x_subdev *subdev, *sd;
-	struct host1x_client *client, *cl;
-
-	mutex_lock(&device->subdevs_lock);
-
-	/* unregister subdevices */
-	list_for_each_entry_safe(subdev, sd, &device->active, list) {
-		/*
-		 * host1x_subdev_unregister() will remove the client from
-		 * any lists, so we'll need to manually add it back to the
-		 * list of idle clients.
-		 *
-		 * XXX: Alternatively, perhaps don't remove the client from
-		 * any lists in host1x_subdev_unregister() and instead do
-		 * that explicitly from host1x_unregister_client()?
-		 */
-		client = subdev->client;
-
-		__host1x_subdev_unregister(device, subdev);
-
-		/* add the client to the list of idle clients */
-		mutex_lock(&clients_lock);
-		list_add_tail(&client->list, &clients);
-		mutex_unlock(&clients_lock);
-	}
-
-	/* remove subdevices */
-	list_for_each_entry_safe(subdev, sd, &device->subdevs, list)
-		host1x_subdev_del(subdev);
-
-	mutex_unlock(&device->subdevs_lock);
-
-	/* move clients to idle list */
-	mutex_lock(&clients_lock);
-	mutex_lock(&device->clients_lock);
-
-	list_for_each_entry_safe(client, cl, &device->clients, list)
-		list_move_tail(&client->list, &clients);
-
-	mutex_unlock(&device->clients_lock);
-	mutex_unlock(&clients_lock);
-
-	/* finally remove the device */
-	list_del_init(&device->list);
 	device_unregister(&device->dev);
 }
 
-- 
2.1.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/5] gpu: host1x: Provide a proper struct bus_type

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Previously the struct bus_type exported by the host1x infrastructure was
only a very basic skeleton. Turn that implementation into a more full-
fledged bus to support proper probe ordering and power management.

Note that the bus infrastructure needs to be available before any of the
drivers can be registered, so the bus needs to be registered before the
host1x module. Otherwise drivers could be registered before the module
is loaded and trigger a BUG_ON() in driver_register(). To ensure the bus
infrastructure is always there, always build the code into the kernel
when enabled and register it with a postcore initcall.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/gpu/drm/tegra/drm.c |   4 +-
 drivers/gpu/host1x/Makefile |   3 +-
 drivers/gpu/host1x/bus.c    | 115 +++++++++++++++++++++++++++++++-------------
 drivers/gpu/host1x/bus.h    |   3 --
 drivers/gpu/host1x/dev.c    |   9 +---
 include/linux/host1x.h      |  18 +++++--
 6 files changed, 102 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index e549afeece1f..272c2dca3536 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -908,7 +908,9 @@ static const struct of_device_id host1x_drm_subdevs[] = {
 };
 
 static struct host1x_driver host1x_drm_driver = {
-	.name = "drm",
+	.driver = {
+		.name = "drm",
+	},
 	.probe = host1x_drm_probe,
 	.remove = host1x_drm_remove,
 	.subdevs = host1x_drm_subdevs,
diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
index c1189f004441..a3e667a1b6f5 100644
--- a/drivers/gpu/host1x/Makefile
+++ b/drivers/gpu/host1x/Makefile
@@ -1,5 +1,4 @@
 host1x-y = \
-	bus.o \
 	syncpt.o \
 	dev.o \
 	intr.o \
@@ -13,3 +12,5 @@ host1x-y = \
 	hw/host1x04.o
 
 obj-$(CONFIG_TEGRA_HOST1X) += host1x.o
+
+obj-y += bus.o
diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
index 0b52f0ea8871..28630a5e9397 100644
--- a/drivers/gpu/host1x/bus.c
+++ b/drivers/gpu/host1x/bus.c
@@ -72,13 +72,14 @@ static void host1x_subdev_del(struct host1x_subdev *subdev)
 /**
  * host1x_device_parse_dt() - scan device tree and add matching subdevices
  */
-static int host1x_device_parse_dt(struct host1x_device *device)
+static int host1x_device_parse_dt(struct host1x_device *device,
+				  struct host1x_driver *driver)
 {
 	struct device_node *np;
 	int err;
 
 	for_each_child_of_node(device->dev.parent->of_node, np) {
-		if (of_match_node(device->driver->subdevs, np) &&
+		if (of_match_node(driver->subdevs, np) &&
 		    of_device_is_available(np)) {
 			err = host1x_subdev_add(device, np);
 			if (err < 0)
@@ -109,17 +110,12 @@ static void host1x_subdev_register(struct host1x_device *device,
 	mutex_unlock(&device->clients_lock);
 	mutex_unlock(&device->subdevs_lock);
 
-	/*
-	 * When all subdevices have been registered, the composite device is
-	 * ready to be probed.
-	 */
 	if (list_empty(&device->subdevs)) {
-		err = device->driver->probe(device);
+		err = device_add(&device->dev);
 		if (err < 0)
-			dev_err(&device->dev, "probe failed for %ps: %d\n",
-				device->driver, err);
+			dev_err(&device->dev, "failed to add: %d\n", err);
 		else
-			device->bound = true;
+			device->registered = true;
 	}
 }
 
@@ -127,18 +123,16 @@ static void __host1x_subdev_unregister(struct host1x_device *device,
 				       struct host1x_subdev *subdev)
 {
 	struct host1x_client *client = subdev->client;
-	int err;
 
 	/*
 	 * If all subdevices have been activated, we're about to remove the
 	 * first active subdevice, so unload the driver first.
 	 */
-	if (list_empty(&device->subdevs) && device->bound) {
-		err = device->driver->remove(device);
-		if (err < 0)
-			dev_err(&device->dev, "remove failed: %d\n", err);
-
-		device->bound = false;
+	if (list_empty(&device->subdevs)) {
+		if (device->registered) {
+			device->registered = false;
+			device_del(&device->dev);
+		}
 	}
 
 	/*
@@ -265,19 +259,65 @@ static int host1x_del_client(struct host1x *host1x,
 	return -ENODEV;
 }
 
+static int host1x_device_match(struct device *dev, struct device_driver *drv)
+{
+	return strcmp(dev_name(dev), drv->name) == 0;
+}
+
+static int host1x_device_probe(struct device *dev)
+{
+	struct host1x_driver *driver = to_host1x_driver(dev->driver);
+	struct host1x_device *device = to_host1x_device(dev);
+
+	if (driver->probe)
+		return driver->probe(device);
+
+	return 0;
+}
+
+static int host1x_device_remove(struct device *dev)
+{
+	struct host1x_driver *driver = to_host1x_driver(dev->driver);
+	struct host1x_device *device = to_host1x_device(dev);
+
+	if (driver->remove)
+		return driver->remove(device);
+
+	return 0;
+}
+
+static void host1x_device_shutdown(struct device *dev)
+{
+	struct host1x_driver *driver = to_host1x_driver(dev->driver);
+	struct host1x_device *device = to_host1x_device(dev);
+
+	if (driver->shutdown)
+		driver->shutdown(device);
+}
+
+static const struct dev_pm_ops host1x_device_pm_ops = {
+	.suspend = pm_generic_suspend,
+	.resume = pm_generic_resume,
+	.freeze = pm_generic_freeze,
+	.thaw = pm_generic_thaw,
+	.poweroff = pm_generic_poweroff,
+	.restore = pm_generic_restore,
+};
+
 static struct bus_type host1x_bus_type = {
 	.name = "host1x",
+	.match = host1x_device_match,
+	.probe = host1x_device_probe,
+	.remove = host1x_device_remove,
+	.shutdown = host1x_device_shutdown,
+	.pm = &host1x_device_pm_ops,
 };
 
-int host1x_bus_init(void)
+static int host1x_bus_init(void)
 {
 	return bus_register(&host1x_bus_type);
 }
-
-void host1x_bus_exit(void)
-{
-	bus_unregister(&host1x_bus_type);
-}
+postcore_initcall(host1x_bus_init);
 
 static void __host1x_device_del(struct host1x_device *device)
 {
@@ -347,6 +387,8 @@ static int host1x_device_add(struct host1x *host1x,
 	if (!device)
 		return -ENOMEM;
 
+	device_initialize(&device->dev);
+
 	mutex_init(&device->subdevs_lock);
 	INIT_LIST_HEAD(&device->subdevs);
 	INIT_LIST_HEAD(&device->active);
@@ -357,18 +399,14 @@ static int host1x_device_add(struct host1x *host1x,
 
 	device->dev.coherent_dma_mask = host1x->dev->coherent_dma_mask;
 	device->dev.dma_mask = &device->dev.coherent_dma_mask;
+	dev_set_name(&device->dev, "%s", driver->driver.name);
 	device->dev.release = host1x_device_release;
-	dev_set_name(&device->dev, "%s", driver->name);
 	device->dev.bus = &host1x_bus_type;
 	device->dev.parent = host1x->dev;
 
-	err = device_register(&device->dev);
-	if (err < 0)
-		return err;
-
-	err = host1x_device_parse_dt(device);
+	err = host1x_device_parse_dt(device, driver);
 	if (err < 0) {
-		device_unregister(&device->dev);
+		kfree(device);
 		return err;
 	}
 
@@ -399,7 +437,12 @@ static int host1x_device_add(struct host1x *host1x,
 static void host1x_device_del(struct host1x *host1x,
 			      struct host1x_device *device)
 {
-	device_unregister(&device->dev);
+	if (device->registered) {
+		device->registered = false;
+		device_del(&device->dev);
+	}
+
+	put_device(&device->dev);
 }
 
 static void host1x_attach_driver(struct host1x *host1x,
@@ -474,7 +517,8 @@ int host1x_unregister(struct host1x *host1x)
 	return 0;
 }
 
-int host1x_driver_register(struct host1x_driver *driver)
+int host1x_driver_register_full(struct host1x_driver *driver,
+				struct module *owner)
 {
 	struct host1x *host1x;
 
@@ -491,9 +535,12 @@ int host1x_driver_register(struct host1x_driver *driver)
 
 	mutex_unlock(&devices_lock);
 
-	return 0;
+	driver->driver.bus = &host1x_bus_type;
+	driver->driver.owner = owner;
+
+	return driver_register(&driver->driver);
 }
-EXPORT_SYMBOL(host1x_driver_register);
+EXPORT_SYMBOL(host1x_driver_register_full);
 
 void host1x_driver_unregister(struct host1x_driver *driver)
 {
diff --git a/drivers/gpu/host1x/bus.h b/drivers/gpu/host1x/bus.h
index 4099e99212c8..c7d976e8ead7 100644
--- a/drivers/gpu/host1x/bus.h
+++ b/drivers/gpu/host1x/bus.h
@@ -20,9 +20,6 @@
 
 struct host1x;
 
-int host1x_bus_init(void);
-void host1x_bus_exit(void);
-
 int host1x_register(struct host1x *host1x);
 int host1x_unregister(struct host1x *host1x);
 
diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
index 2529908d304b..18b36410347d 100644
--- a/drivers/gpu/host1x/dev.c
+++ b/drivers/gpu/host1x/dev.c
@@ -216,13 +216,9 @@ static int __init tegra_host1x_init(void)
 {
 	int err;
 
-	err = host1x_bus_init();
-	if (err < 0)
-		return err;
-
 	err = platform_driver_register(&tegra_host1x_driver);
 	if (err < 0)
-		goto unregister_bus;
+		return err;
 
 	err = platform_driver_register(&tegra_mipi_driver);
 	if (err < 0)
@@ -232,8 +228,6 @@ static int __init tegra_host1x_init(void)
 
 unregister_host1x:
 	platform_driver_unregister(&tegra_host1x_driver);
-unregister_bus:
-	host1x_bus_exit();
 	return err;
 }
 module_init(tegra_host1x_init);
@@ -242,7 +236,6 @@ static void __exit tegra_host1x_exit(void)
 {
 	platform_driver_unregister(&tegra_mipi_driver);
 	platform_driver_unregister(&tegra_host1x_driver);
-	host1x_bus_exit();
 }
 module_exit(tegra_host1x_exit);
 
diff --git a/include/linux/host1x.h b/include/linux/host1x.h
index 7890b553d12e..464f33814a94 100644
--- a/include/linux/host1x.h
+++ b/include/linux/host1x.h
@@ -250,17 +250,29 @@ void host1x_job_unpin(struct host1x_job *job);
 struct host1x_device;
 
 struct host1x_driver {
+	struct device_driver driver;
+
 	const struct of_device_id *subdevs;
 	struct list_head list;
-	const char *name;
 
 	int (*probe)(struct host1x_device *device);
 	int (*remove)(struct host1x_device *device);
+	void (*shutdown)(struct host1x_device *device);
 };
 
-int host1x_driver_register(struct host1x_driver *driver);
+static inline struct host1x_driver *
+to_host1x_driver(struct device_driver *driver)
+{
+	return container_of(driver, struct host1x_driver, driver);
+}
+
+int host1x_driver_register_full(struct host1x_driver *driver,
+				struct module *owner);
 void host1x_driver_unregister(struct host1x_driver *driver);
 
+#define host1x_driver_register(driver) \
+	host1x_driver_register_full(driver, THIS_MODULE)
+
 struct host1x_device {
 	struct host1x_driver *driver;
 	struct list_head list;
@@ -273,7 +285,7 @@ struct host1x_device {
 	struct mutex clients_lock;
 	struct list_head clients;
 
-	bool bound;
+	bool registered;
 };
 
 static inline struct host1x_device *to_host1x_device(struct device *dev)
-- 
2.1.3

[PATCH 5/5] drm/tegra: Add minimal power management

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

For now only disable the KMS hotplug polling helper logic upon suspend
and re-enable it on resume.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/drm.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 272c2dca3536..16c44b9abbd8 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -889,6 +889,30 @@ static int host1x_drm_remove(struct host1x_device *dev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int host1x_drm_suspend(struct device *dev)
+{
+	struct drm_device *drm = dev_get_drvdata(dev);
+
+	drm_kms_helper_poll_disable(drm);
+
+	return 0;
+}
+
+static int host1x_drm_resume(struct device *dev)
+{
+	struct drm_device *drm = dev_get_drvdata(dev);
+
+	drm_kms_helper_poll_enable(drm);
+
+	return 0;
+}
+#endif
+
+static const struct dev_pm_ops host1x_drm_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(host1x_drm_suspend, host1x_drm_resume)
+};
+
 static const struct of_device_id host1x_drm_subdevs[] = {
 	{ .compatible = "nvidia,tegra20-dc", },
 	{ .compatible = "nvidia,tegra20-hdmi", },
@@ -910,6 +934,7 @@ static const struct of_device_id host1x_drm_subdevs[] = {
 static struct host1x_driver host1x_drm_driver = {
 	.driver = {
 		.name = "drm",
+		.pm = &host1x_drm_pm_ops,
 	},
 	.probe = host1x_drm_probe,
 	.remove = host1x_drm_remove,
-- 
2.1.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] gpu: host1x: Provide a proper struct bus_type

[Mark Zhang]

On 12/19/2014 11:24 PM, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Previously the struct bus_type exported by the host1x infrastructure was
> only a very basic skeleton. Turn that implementation into a more full-
> fledged bus to support proper probe ordering and power management.
> 
> Note that the bus infrastructure needs to be available before any of the
> drivers can be registered, so the bus needs to be registered before the
> host1x module. Otherwise drivers could be registered before the module
> is loaded and trigger a BUG_ON() in driver_register(). To ensure the bus
> infrastructure is always there, always build the code into the kernel
> when enabled and register it with a postcore initcall.
> 

So this means there is no chance that host1x can be built as a kernel
module, right? I'm fine with that, just asking.

> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
[...]
> diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
> index c1189f004441..a3e667a1b6f5 100644
> --- a/drivers/gpu/host1x/Makefile
> +++ b/drivers/gpu/host1x/Makefile
> @@ -1,5 +1,4 @@
>  host1x-y = \
> -	bus.o \
>  	syncpt.o \
>  	dev.o \
>  	intr.o \
> @@ -13,3 +12,5 @@ host1x-y = \
>  	hw/host1x04.o
>  
>  obj-$(CONFIG_TEGRA_HOST1X) += host1x.o
> +
> +obj-y += bus.o

I didn't get it, why we need to do this?

> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index 0b52f0ea8871..28630a5e9397 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -72,13 +72,14 @@ static void host1x_subdev_del(struct host1x_subdev *subdev)
[...]
>  
>  static inline struct host1x_device *to_host1x_device(struct device *dev)
> 

The change looks good to me. Just one thing I feel not comfortable:
"struct host1x_device" is not a real device, it represents the drm
device actually. The real tegra host1x device is represented by "struct
host1x". But the name "host1x_device" makes people confusing, I mean, it
will make people thinking it's the real "tegra host1x" device then bring
the reading difficulty. Why don't we change this to something like
"drm_device" or "tegra_drm_device"?

Mark

Re: [PATCH 5/5] drm/tegra: Add minimal power management

[Mark Zhang]

+1

Mark
On 12/19/2014 11:24 PM, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> For now only disable the KMS hotplug polling helper logic upon suspend
> and re-enable it on resume.
> 
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/gpu/drm/tegra/drm.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 272c2dca3536..16c44b9abbd8 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -889,6 +889,30 @@ static int host1x_drm_remove(struct host1x_device *dev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int host1x_drm_suspend(struct device *dev)
> +{
> +	struct drm_device *drm = dev_get_drvdata(dev);
> +
> +	drm_kms_helper_poll_disable(drm);
> +
> +	return 0;
> +}
> +
> +static int host1x_drm_resume(struct device *dev)
> +{
> +	struct drm_device *drm = dev_get_drvdata(dev);
> +
> +	drm_kms_helper_poll_enable(drm);
> +
> +	return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops host1x_drm_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(host1x_drm_suspend, host1x_drm_resume)
> +};
> +
>  static const struct of_device_id host1x_drm_subdevs[] = {
>  	{ .compatible = "nvidia,tegra20-dc", },
>  	{ .compatible = "nvidia,tegra20-hdmi", },
> @@ -910,6 +934,7 @@ static const struct of_device_id host1x_drm_subdevs[] = {
>  static struct host1x_driver host1x_drm_driver = {
>  	.driver = {
>  		.name = "drm",
> +		.pm = &host1x_drm_pm_ops,
>  	},
>  	.probe = host1x_drm_probe,
>  	.remove = host1x_drm_remove,
> 

Re: [PATCH 4/5] gpu: host1x: Provide a proper struct bus_type

[Thierry Reding]

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

On Tue, Dec 23, 2014 at 03:30:20PM +0800, Mark Zhang wrote:
> On 12/19/2014 11:24 PM, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > Previously the struct bus_type exported by the host1x infrastructure was
> > only a very basic skeleton. Turn that implementation into a more full-
> > fledged bus to support proper probe ordering and power management.
> > 
> > Note that the bus infrastructure needs to be available before any of the
> > drivers can be registered, so the bus needs to be registered before the
> > host1x module. Otherwise drivers could be registered before the module
> > is loaded and trigger a BUG_ON() in driver_register(). To ensure the bus
> > infrastructure is always there, always build the code into the kernel
> > when enabled and register it with a postcore initcall.
> > 
> 
> So this means there is no chance that host1x can be built as a kernel
> module, right? I'm fine with that, just asking.

No, it means that not all of host1x can be built as a module. The host1x
bus infrastructure will always be built-in when TEGRA_HOST1X is enabled.

> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> [...]
> > diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
> > index c1189f004441..a3e667a1b6f5 100644
> > --- a/drivers/gpu/host1x/Makefile
> > +++ b/drivers/gpu/host1x/Makefile
> > @@ -1,5 +1,4 @@
> >  host1x-y = \
> > -	bus.o \
> >  	syncpt.o \
> >  	dev.o \
> >  	intr.o \
> > @@ -13,3 +12,5 @@ host1x-y = \
> >  	hw/host1x04.o
> >  
> >  obj-$(CONFIG_TEGRA_HOST1X) += host1x.o
> > +
> > +obj-y += bus.o
> 
> I didn't get it, why we need to do this?

If CONFIG_TEGRA_HOST1X=m, then obj-$(CONFIG_TEGRA_HOST1X) builds the
bus.o into a module. But we want it to always be built-in. The build
system will descend into the drivers/gpu/host1x directory only if the
TEGRA_HOST1X symbol is selected (either =y or =m), therefore obj-y here
will result in bus.o being built-in whether the rest of host1x is built
as a module or built-in.

> > diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> > index 0b52f0ea8871..28630a5e9397 100644
> > --- a/drivers/gpu/host1x/bus.c
> > +++ b/drivers/gpu/host1x/bus.c
> > @@ -72,13 +72,14 @@ static void host1x_subdev_del(struct host1x_subdev *subdev)
> [...]
> >  
> >  static inline struct host1x_device *to_host1x_device(struct device *dev)
> > 
> 
> The change looks good to me. Just one thing I feel not comfortable:
> "struct host1x_device" is not a real device, it represents the drm
> device actually. The real tegra host1x device is represented by "struct
> host1x". But the name "host1x_device" makes people confusing, I mean, it
> will make people thinking it's the real "tegra host1x" device then bring
> the reading difficulty.

The reason behind that name is that host1x provides a bus (real to some
degree, but also virtual). host1x is the device that provides the bus
whereas a host1x_device is a "device" on the "host1x" bus. That's just
like an i2c_client is a "client" on the "I2C" bus. Or an spi_device is a
"device" on the "SPI" bus.

> Why don't we change this to something like "drm_device" or
> "tegra_drm_device"?

Other devices can be host1x devices. Some time ago work was being done
on a driver for the CSI/VI hardware (for camera or video input). The
idea was that it would also be instantiated as a host1x_device in some
other subsystem (V4L2 at the time).

The functionality here is generic and in no way DRM specific.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 5/5] drm/tegra: Add minimal power management

[Thierry Reding]

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

On Tue, Dec 23, 2014 at 03:32:43PM +0800, Mark Zhang wrote:
> +1

Does that translate into a "Reviewed-by" or "Tested-by"?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/5] gpu: host1x: Provide a proper struct bus_type

[Sean Paul]

On Fri, Dec 19, 2014 at 10:24 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> Previously the struct bus_type exported by the host1x infrastructure was
> only a very basic skeleton. Turn that implementation into a more full-
> fledged bus to support proper probe ordering and power management.
>
> Note that the bus infrastructure needs to be available before any of the
> drivers can be registered, so the bus needs to be registered before the
> host1x module. Otherwise drivers could be registered before the module
> is loaded and trigger a BUG_ON() in driver_register(). To ensure the bus
> infrastructure is always there, always build the code into the kernel
> when enabled and register it with a postcore initcall.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>


This is a nice improvement.

Reviewed-by: Sean Paul <seanpaul@chromium.org>


> ---
>  drivers/gpu/drm/tegra/drm.c |   4 +-
>  drivers/gpu/host1x/Makefile |   3 +-
>  drivers/gpu/host1x/bus.c    | 115 +++++++++++++++++++++++++++++++-------------
>  drivers/gpu/host1x/bus.h    |   3 --
>  drivers/gpu/host1x/dev.c    |   9 +---
>  include/linux/host1x.h      |  18 +++++--
>  6 files changed, 102 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index e549afeece1f..272c2dca3536 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -908,7 +908,9 @@ static const struct of_device_id host1x_drm_subdevs[] = {
>  };
>
>  static struct host1x_driver host1x_drm_driver = {
> -       .name = "drm",
> +       .driver = {
> +               .name = "drm",
> +       },
>         .probe = host1x_drm_probe,
>         .remove = host1x_drm_remove,
>         .subdevs = host1x_drm_subdevs,
> diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
> index c1189f004441..a3e667a1b6f5 100644
> --- a/drivers/gpu/host1x/Makefile
> +++ b/drivers/gpu/host1x/Makefile
> @@ -1,5 +1,4 @@
>  host1x-y = \
> -       bus.o \
>         syncpt.o \
>         dev.o \
>         intr.o \
> @@ -13,3 +12,5 @@ host1x-y = \
>         hw/host1x04.o
>
>  obj-$(CONFIG_TEGRA_HOST1X) += host1x.o
> +
> +obj-y += bus.o
> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index 0b52f0ea8871..28630a5e9397 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -72,13 +72,14 @@ static void host1x_subdev_del(struct host1x_subdev *subdev)
>  /**
>   * host1x_device_parse_dt() - scan device tree and add matching subdevices
>   */
> -static int host1x_device_parse_dt(struct host1x_device *device)
> +static int host1x_device_parse_dt(struct host1x_device *device,
> +                                 struct host1x_driver *driver)
>  {
>         struct device_node *np;
>         int err;
>
>         for_each_child_of_node(device->dev.parent->of_node, np) {
> -               if (of_match_node(device->driver->subdevs, np) &&
> +               if (of_match_node(driver->subdevs, np) &&
>                     of_device_is_available(np)) {
>                         err = host1x_subdev_add(device, np);
>                         if (err < 0)
> @@ -109,17 +110,12 @@ static void host1x_subdev_register(struct host1x_device *device,
>         mutex_unlock(&device->clients_lock);
>         mutex_unlock(&device->subdevs_lock);
>
> -       /*
> -        * When all subdevices have been registered, the composite device is
> -        * ready to be probed.
> -        */
>         if (list_empty(&device->subdevs)) {
> -               err = device->driver->probe(device);
> +               err = device_add(&device->dev);
>                 if (err < 0)
> -                       dev_err(&device->dev, "probe failed for %ps: %d\n",
> -                               device->driver, err);
> +                       dev_err(&device->dev, "failed to add: %d\n", err);
>                 else
> -                       device->bound = true;
> +                       device->registered = true;
>         }
>  }
>
> @@ -127,18 +123,16 @@ static void __host1x_subdev_unregister(struct host1x_device *device,
>                                        struct host1x_subdev *subdev)
>  {
>         struct host1x_client *client = subdev->client;
> -       int err;
>
>         /*
>          * If all subdevices have been activated, we're about to remove the
>          * first active subdevice, so unload the driver first.
>          */
> -       if (list_empty(&device->subdevs) && device->bound) {
> -               err = device->driver->remove(device);
> -               if (err < 0)
> -                       dev_err(&device->dev, "remove failed: %d\n", err);
> -
> -               device->bound = false;
> +       if (list_empty(&device->subdevs)) {
> +               if (device->registered) {
> +                       device->registered = false;
> +                       device_del(&device->dev);
> +               }
>         }
>
>         /*
> @@ -265,19 +259,65 @@ static int host1x_del_client(struct host1x *host1x,
>         return -ENODEV;
>  }
>
> +static int host1x_device_match(struct device *dev, struct device_driver *drv)
> +{
> +       return strcmp(dev_name(dev), drv->name) == 0;
> +}
> +
> +static int host1x_device_probe(struct device *dev)
> +{
> +       struct host1x_driver *driver = to_host1x_driver(dev->driver);
> +       struct host1x_device *device = to_host1x_device(dev);
> +
> +       if (driver->probe)
> +               return driver->probe(device);
> +
> +       return 0;
> +}
> +
> +static int host1x_device_remove(struct device *dev)
> +{
> +       struct host1x_driver *driver = to_host1x_driver(dev->driver);
> +       struct host1x_device *device = to_host1x_device(dev);
> +
> +       if (driver->remove)
> +               return driver->remove(device);
> +
> +       return 0;
> +}
> +
> +static void host1x_device_shutdown(struct device *dev)
> +{
> +       struct host1x_driver *driver = to_host1x_driver(dev->driver);
> +       struct host1x_device *device = to_host1x_device(dev);
> +
> +       if (driver->shutdown)
> +               driver->shutdown(device);
> +}
> +
> +static const struct dev_pm_ops host1x_device_pm_ops = {
> +       .suspend = pm_generic_suspend,
> +       .resume = pm_generic_resume,
> +       .freeze = pm_generic_freeze,
> +       .thaw = pm_generic_thaw,
> +       .poweroff = pm_generic_poweroff,
> +       .restore = pm_generic_restore,
> +};
> +
>  static struct bus_type host1x_bus_type = {
>         .name = "host1x",
> +       .match = host1x_device_match,
> +       .probe = host1x_device_probe,
> +       .remove = host1x_device_remove,
> +       .shutdown = host1x_device_shutdown,
> +       .pm = &host1x_device_pm_ops,
>  };
>
> -int host1x_bus_init(void)
> +static int host1x_bus_init(void)
>  {
>         return bus_register(&host1x_bus_type);
>  }
> -
> -void host1x_bus_exit(void)
> -{
> -       bus_unregister(&host1x_bus_type);
> -}
> +postcore_initcall(host1x_bus_init);
>
>  static void __host1x_device_del(struct host1x_device *device)
>  {
> @@ -347,6 +387,8 @@ static int host1x_device_add(struct host1x *host1x,
>         if (!device)
>                 return -ENOMEM;
>
> +       device_initialize(&device->dev);
> +
>         mutex_init(&device->subdevs_lock);
>         INIT_LIST_HEAD(&device->subdevs);
>         INIT_LIST_HEAD(&device->active);
> @@ -357,18 +399,14 @@ static int host1x_device_add(struct host1x *host1x,
>
>         device->dev.coherent_dma_mask = host1x->dev->coherent_dma_mask;
>         device->dev.dma_mask = &device->dev.coherent_dma_mask;
> +       dev_set_name(&device->dev, "%s", driver->driver.name);
>         device->dev.release = host1x_device_release;
> -       dev_set_name(&device->dev, "%s", driver->name);
>         device->dev.bus = &host1x_bus_type;
>         device->dev.parent = host1x->dev;
>
> -       err = device_register(&device->dev);
> -       if (err < 0)
> -               return err;
> -
> -       err = host1x_device_parse_dt(device);
> +       err = host1x_device_parse_dt(device, driver);
>         if (err < 0) {
> -               device_unregister(&device->dev);
> +               kfree(device);
>                 return err;
>         }
>
> @@ -399,7 +437,12 @@ static int host1x_device_add(struct host1x *host1x,
>  static void host1x_device_del(struct host1x *host1x,
>                               struct host1x_device *device)
>  {
> -       device_unregister(&device->dev);
> +       if (device->registered) {
> +               device->registered = false;
> +               device_del(&device->dev);
> +       }
> +
> +       put_device(&device->dev);
>  }
>
>  static void host1x_attach_driver(struct host1x *host1x,
> @@ -474,7 +517,8 @@ int host1x_unregister(struct host1x *host1x)
>         return 0;
>  }
>
> -int host1x_driver_register(struct host1x_driver *driver)
> +int host1x_driver_register_full(struct host1x_driver *driver,
> +                               struct module *owner)
>  {
>         struct host1x *host1x;
>
> @@ -491,9 +535,12 @@ int host1x_driver_register(struct host1x_driver *driver)
>
>         mutex_unlock(&devices_lock);
>
> -       return 0;
> +       driver->driver.bus = &host1x_bus_type;
> +       driver->driver.owner = owner;
> +
> +       return driver_register(&driver->driver);
>  }
> -EXPORT_SYMBOL(host1x_driver_register);
> +EXPORT_SYMBOL(host1x_driver_register_full);
>
>  void host1x_driver_unregister(struct host1x_driver *driver)
>  {
> diff --git a/drivers/gpu/host1x/bus.h b/drivers/gpu/host1x/bus.h
> index 4099e99212c8..c7d976e8ead7 100644
> --- a/drivers/gpu/host1x/bus.h
> +++ b/drivers/gpu/host1x/bus.h
> @@ -20,9 +20,6 @@
>
>  struct host1x;
>
> -int host1x_bus_init(void);
> -void host1x_bus_exit(void);
> -
>  int host1x_register(struct host1x *host1x);
>  int host1x_unregister(struct host1x *host1x);
>
> diff --git a/drivers/gpu/host1x/dev.c b/drivers/gpu/host1x/dev.c
> index 2529908d304b..18b36410347d 100644
> --- a/drivers/gpu/host1x/dev.c
> +++ b/drivers/gpu/host1x/dev.c
> @@ -216,13 +216,9 @@ static int __init tegra_host1x_init(void)
>  {
>         int err;
>
> -       err = host1x_bus_init();
> -       if (err < 0)
> -               return err;
> -
>         err = platform_driver_register(&tegra_host1x_driver);
>         if (err < 0)
> -               goto unregister_bus;
> +               return err;
>
>         err = platform_driver_register(&tegra_mipi_driver);
>         if (err < 0)
> @@ -232,8 +228,6 @@ static int __init tegra_host1x_init(void)
>
>  unregister_host1x:
>         platform_driver_unregister(&tegra_host1x_driver);
> -unregister_bus:
> -       host1x_bus_exit();
>         return err;
>  }
>  module_init(tegra_host1x_init);
> @@ -242,7 +236,6 @@ static void __exit tegra_host1x_exit(void)
>  {
>         platform_driver_unregister(&tegra_mipi_driver);
>         platform_driver_unregister(&tegra_host1x_driver);
> -       host1x_bus_exit();
>  }
>  module_exit(tegra_host1x_exit);
>
> diff --git a/include/linux/host1x.h b/include/linux/host1x.h
> index 7890b553d12e..464f33814a94 100644
> --- a/include/linux/host1x.h
> +++ b/include/linux/host1x.h
> @@ -250,17 +250,29 @@ void host1x_job_unpin(struct host1x_job *job);
>  struct host1x_device;
>
>  struct host1x_driver {
> +       struct device_driver driver;
> +
>         const struct of_device_id *subdevs;
>         struct list_head list;
> -       const char *name;
>
>         int (*probe)(struct host1x_device *device);
>         int (*remove)(struct host1x_device *device);
> +       void (*shutdown)(struct host1x_device *device);
>  };
>
> -int host1x_driver_register(struct host1x_driver *driver);
> +static inline struct host1x_driver *
> +to_host1x_driver(struct device_driver *driver)
> +{
> +       return container_of(driver, struct host1x_driver, driver);
> +}
> +
> +int host1x_driver_register_full(struct host1x_driver *driver,
> +                               struct module *owner);
>  void host1x_driver_unregister(struct host1x_driver *driver);
>
> +#define host1x_driver_register(driver) \
> +       host1x_driver_register_full(driver, THIS_MODULE)
> +
>  struct host1x_device {
>         struct host1x_driver *driver;
>         struct list_head list;
> @@ -273,7 +285,7 @@ struct host1x_device {
>         struct mutex clients_lock;
>         struct list_head clients;
>
> -       bool bound;
> +       bool registered;
>  };
>
>  static inline struct host1x_device *to_host1x_device(struct device *dev)
> --
> 2.1.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/tegra: Add minimal power management

[Sean Paul]

On Fri, Dec 19, 2014 at 10:24 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> For now only disable the KMS hotplug polling helper logic upon suspend
> and re-enable it on resume.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/tegra/drm.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 272c2dca3536..16c44b9abbd8 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -889,6 +889,30 @@ static int host1x_drm_remove(struct host1x_device *dev)
>         return 0;
>  }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int host1x_drm_suspend(struct device *dev)
> +{
> +       struct drm_device *drm = dev_get_drvdata(dev);
> +
> +       drm_kms_helper_poll_disable(drm);
> +
> +       return 0;
> +}
> +
> +static int host1x_drm_resume(struct device *dev)
> +{
> +       struct drm_device *drm = dev_get_drvdata(dev);
> +
> +       drm_kms_helper_poll_enable(drm);
> +
> +       return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops host1x_drm_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(host1x_drm_suspend, host1x_drm_resume)
> +};
> +
>  static const struct of_device_id host1x_drm_subdevs[] = {
>         { .compatible = "nvidia,tegra20-dc", },
>         { .compatible = "nvidia,tegra20-hdmi", },
> @@ -910,6 +934,7 @@ static const struct of_device_id host1x_drm_subdevs[] = {
>  static struct host1x_driver host1x_drm_driver = {
>         .driver = {
>                 .name = "drm",
> +               .pm = &host1x_drm_pm_ops,
>         },
>         .probe = host1x_drm_probe,
>         .remove = host1x_drm_remove,
> --
> 2.1.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] gpu: host1x: Provide a proper struct bus_type

[Mark Zhang]

Thanks for the explanation.

Reviewed-by: Mark Zhang <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Mark
On 01/05/2015 10:49 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Dec 23, 2014 at 03:30:20PM +0800, Mark Zhang wrote:
>> On 12/19/2014 11:24 PM, Thierry Reding wrote:
>>> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> Previously the struct bus_type exported by the host1x infrastructure was
>>> only a very basic skeleton. Turn that implementation into a more full-
>>> fledged bus to support proper probe ordering and power management.
>>>
>>> Note that the bus infrastructure needs to be available before any of the
>>> drivers can be registered, so the bus needs to be registered before the
>>> host1x module. Otherwise drivers could be registered before the module
>>> is loaded and trigger a BUG_ON() in driver_register(). To ensure the bus
>>> infrastructure is always there, always build the code into the kernel
>>> when enabled and register it with a postcore initcall.
>>>
>>
>> So this means there is no chance that host1x can be built as a kernel
>> module, right? I'm fine with that, just asking.
> 
> No, it means that not all of host1x can be built as a module. The host1x
> bus infrastructure will always be built-in when TEGRA_HOST1X is enabled.
> 
>>> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>> [...]
>>> diff --git a/drivers/gpu/host1x/Makefile b/drivers/gpu/host1x/Makefile
>>> index c1189f004441..a3e667a1b6f5 100644
>>> --- a/drivers/gpu/host1x/Makefile
>>> +++ b/drivers/gpu/host1x/Makefile
>>> @@ -1,5 +1,4 @@
>>>  host1x-y = \
>>> -	bus.o \
>>>  	syncpt.o \
>>>  	dev.o \
>>>  	intr.o \
>>> @@ -13,3 +12,5 @@ host1x-y = \
>>>  	hw/host1x04.o
>>>  
>>>  obj-$(CONFIG_TEGRA_HOST1X) += host1x.o
>>> +
>>> +obj-y += bus.o
>>
>> I didn't get it, why we need to do this?
> 
> If CONFIG_TEGRA_HOST1X=m, then obj-$(CONFIG_TEGRA_HOST1X) builds the
> bus.o into a module. But we want it to always be built-in. The build
> system will descend into the drivers/gpu/host1x directory only if the
> TEGRA_HOST1X symbol is selected (either =y or =m), therefore obj-y here
> will result in bus.o being built-in whether the rest of host1x is built
> as a module or built-in.
> 
>>> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
>>> index 0b52f0ea8871..28630a5e9397 100644
>>> --- a/drivers/gpu/host1x/bus.c
>>> +++ b/drivers/gpu/host1x/bus.c
>>> @@ -72,13 +72,14 @@ static void host1x_subdev_del(struct host1x_subdev *subdev)
>> [...]
>>>  
>>>  static inline struct host1x_device *to_host1x_device(struct device *dev)
>>>
>>
>> The change looks good to me. Just one thing I feel not comfortable:
>> "struct host1x_device" is not a real device, it represents the drm
>> device actually. The real tegra host1x device is represented by "struct
>> host1x". But the name "host1x_device" makes people confusing, I mean, it
>> will make people thinking it's the real "tegra host1x" device then bring
>> the reading difficulty.
> 
> The reason behind that name is that host1x provides a bus (real to some
> degree, but also virtual). host1x is the device that provides the bus
> whereas a host1x_device is a "device" on the "host1x" bus. That's just
> like an i2c_client is a "client" on the "I2C" bus. Or an spi_device is a
> "device" on the "SPI" bus.
> 
>> Why don't we change this to something like "drm_device" or
>> "tegra_drm_device"?
> 
> Other devices can be host1x devices. Some time ago work was being done
> on a driver for the CSI/VI hardware (for camera or video input). The
> idea was that it would also be instantiated as a host1x_device in some
> other subsystem (V4L2 at the time).
> 
> The functionality here is generic and in no way DRM specific.
> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1
> 

Re: [PATCH 5/5] drm/tegra: Add minimal power management

[Mark Zhang]

I mean:

Reviewed-by: Mark Zhang <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

To make display suspend/resume working, we need some other patches. I
will send out the patches soon, based on this patch set. Thanks Thierry.

Mark
On 01/05/2015 10:50 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Dec 23, 2014 at 03:32:43PM +0800, Mark Zhang wrote:
>> +1
> 
> Does that translate into a "Reviewed-by" or "Tested-by"?
> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1
>