[PATCH v5 1/6] drm/vc4: Avoid using vrefresh==0 mode in DSI htotal math.

[PATCH v5 1/6] drm/vc4: Avoid using vrefresh==0 mode in DSI htotal math.

[Eric Anholt]

The incoming mode might have a missing vrefresh field if it came from
drmModeSetCrtc(), which the kernel is supposed to calculate using
drm_mode_vrefresh().  We could either use that or the adjusted_mode's
original vrefresh value.

However, we can maintain a more exact vrefresh value (not just the
integer approximation), by scaling by the ratio of our clocks.

v2: Use math suggested by Andrzej Hajda instead.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 629d372633e6..57213f4e3c72 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -866,7 +866,8 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
 	adjusted_mode->clock = pixel_clock_hz / 1000 + 1;
 
 	/* Given the new pixel clock, adjust HFP to keep vrefresh the same. */
-	adjusted_mode->htotal = pixel_clock_hz / (mode->vrefresh * mode->vtotal);
+	adjusted_mode->htotal = (pixel_clock_hz / 1000 * mode->htotal /
+				 mode->clock);
 	adjusted_mode->hsync_end += adjusted_mode->htotal - mode->htotal;
 	adjusted_mode->hsync_start += adjusted_mode->htotal - mode->htotal;
 
-- 
2.11.0

[PATCH v5 1/6] drm/vc4: Avoid using vrefresh==0 mode in DSI htotal math.

[Eric Anholt]

The incoming mode might have a missing vrefresh field if it came from
drmModeSetCrtc(), which the kernel is supposed to calculate using
drm_mode_vrefresh().  We could either use that or the adjusted_mode's
original vrefresh value.

However, we can maintain a more exact vrefresh value (not just the
integer approximation), by scaling by the ratio of our clocks.

v2: Use math suggested by Andrzej Hajda instead.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 629d372633e6..57213f4e3c72 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -866,7 +866,8 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
 	adjusted_mode->clock = pixel_clock_hz / 1000 + 1;
 
 	/* Given the new pixel clock, adjust HFP to keep vrefresh the same. */
-	adjusted_mode->htotal = pixel_clock_hz / (mode->vrefresh * mode->vtotal);
+	adjusted_mode->htotal = (pixel_clock_hz / 1000 * mode->htotal /
+				 mode->clock);
 	adjusted_mode->hsync_end += adjusted_mode->htotal - mode->htotal;
 	adjusted_mode->hsync_start += adjusted_mode->htotal - mode->htotal;
 
-- 
2.11.0

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

[PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Eric Anholt]

This will let drivers reduce the error cleanup they need, in
particular the "is_panel_bridge" flag.

v2: Slight cleanup of remove function by Andrzej

Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h       |  3 +++
 2 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 685c1a480201..292ee92a9c97 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -195,3 +195,33 @@ void drm_panel_bridge_remove(struct drm_bridge *bridge)
 	devm_kfree(panel_bridge->panel->dev, bridge);
 }
 EXPORT_SYMBOL(drm_panel_bridge_remove);
+
+static void devm_drm_panel_bridge_release(struct device *dev, void *res)
+{
+	struct drm_bridge **bridge = res;
+
+	drm_panel_bridge_remove(*bridge);
+}
+
+struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
+					     struct drm_panel *panel,
+					     u32 connector_type)
+{
+	struct drm_bridge **ptr, *bridge;
+
+	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	bridge = drm_panel_bridge_add(panel, connector_type);
+	if (!IS_ERR(bridge)) {
+		*ptr = bridge;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return bridge;
+}
+EXPORT_SYMBOL(devm_drm_panel_bridge_add);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 1dc94d5392e2..6522d4cbc9d9 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -268,6 +268,9 @@ void drm_bridge_enable(struct drm_bridge *bridge);
 struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
 					u32 connector_type);
 void drm_panel_bridge_remove(struct drm_bridge *bridge);
+struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
+					     struct drm_panel *panel,
+					     u32 connector_type);
 #endif
 
 #endif
-- 
2.11.0

[PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Eric Anholt]

This will let drivers reduce the error cleanup they need, in
particular the "is_panel_bridge" flag.

v2: Slight cleanup of remove function by Andrzej

Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
---
 drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h       |  3 +++
 2 files changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 685c1a480201..292ee92a9c97 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -195,3 +195,33 @@ void drm_panel_bridge_remove(struct drm_bridge *bridge)
 	devm_kfree(panel_bridge->panel->dev, bridge);
 }
 EXPORT_SYMBOL(drm_panel_bridge_remove);
+
+static void devm_drm_panel_bridge_release(struct device *dev, void *res)
+{
+	struct drm_bridge **bridge = res;
+
+	drm_panel_bridge_remove(*bridge);
+}
+
+struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
+					     struct drm_panel *panel,
+					     u32 connector_type)
+{
+	struct drm_bridge **ptr, *bridge;
+
+	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	bridge = drm_panel_bridge_add(panel, connector_type);
+	if (!IS_ERR(bridge)) {
+		*ptr = bridge;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return bridge;
+}
+EXPORT_SYMBOL(devm_drm_panel_bridge_add);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 1dc94d5392e2..6522d4cbc9d9 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -268,6 +268,9 @@ void drm_bridge_enable(struct drm_bridge *bridge);
 struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
 					u32 connector_type);
 void drm_panel_bridge_remove(struct drm_bridge *bridge);
+struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
+					     struct drm_panel *panel,
+					     u32 connector_type);
 #endif
 
 #endif
-- 
2.11.0

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

[PATCH v5 3/6] drm/vc4: Delay DSI host registration until the panel has probed.

[Eric Anholt]

The vc4 driver was unusual in that it was delaying the panel lookup
until the attach step, while most DSI hosts will -EPROBE_DEFER until
they get a panel.

v2: Drop a debug message that slipped in.

Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> (v1)
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 57213f4e3c72..769f247c52a9 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -33,6 +33,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_mipi_dsi.h>
+#include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
@@ -504,7 +505,6 @@ struct vc4_dsi {
 	struct mipi_dsi_host dsi_host;
 	struct drm_encoder *encoder;
 	struct drm_bridge *bridge;
-	bool is_panel_bridge;
 
 	void __iomem *regs;
 
@@ -1289,7 +1289,6 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
 			       struct mipi_dsi_device *device)
 {
 	struct vc4_dsi *dsi = host_to_dsi(host);
-	int ret = 0;
 
 	dsi->lanes = device->lanes;
 	dsi->channel = device->channel;
@@ -1324,34 +1323,12 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
 		return 0;
 	}
 
-	dsi->bridge = of_drm_find_bridge(device->dev.of_node);
-	if (!dsi->bridge) {
-		struct drm_panel *panel =
-			of_drm_find_panel(device->dev.of_node);
-
-		dsi->bridge = drm_panel_bridge_add(panel,
-						   DRM_MODE_CONNECTOR_DSI);
-		if (IS_ERR(dsi->bridge)) {
-			ret = PTR_ERR(dsi->bridge);
-			dsi->bridge = NULL;
-			return ret;
-		}
-		dsi->is_panel_bridge = true;
-	}
-
 	return drm_bridge_attach(dsi->encoder, dsi->bridge, NULL);
 }
 
 static int vc4_dsi_host_detach(struct mipi_dsi_host *host,
 			       struct mipi_dsi_device *device)
 {
-	struct vc4_dsi *dsi = host_to_dsi(host);
-
-	if (dsi->is_panel_bridge) {
-		drm_panel_bridge_remove(dsi->bridge);
-		dsi->bridge = NULL;
-	}
-
 	return 0;
 }
 
@@ -1495,6 +1472,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 	struct vc4_dev *vc4 = to_vc4_dev(drm);
 	struct vc4_dsi *dsi;
 	struct vc4_dsi_encoder *vc4_dsi_encoder;
+	struct drm_panel *panel;
 	const struct of_device_id *match;
 	dma_cap_mask_t dma_mask;
 	int ret;
@@ -1598,6 +1576,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 	}
 
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
+					  &panel, &dsi->bridge);
+	if (ret)
+		return ret;
+
+	if (panel) {
+		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
+							DRM_MODE_CONNECTOR_DSI);
+		if (IS_ERR(dsi->bridge))
+			return PTR_ERR(dsi->bridge);
+	}
+
 	/* The esc clock rate is supposed to always be 100Mhz. */
 	ret = clk_set_rate(dsi->escape_clock, 100 * 1000000);
 	if (ret) {
-- 
2.11.0

[PATCH v5 3/6] drm/vc4: Delay DSI host registration until the panel has probed.

[Eric Anholt]

The vc4 driver was unusual in that it was delaying the panel lookup
until the attach step, while most DSI hosts will -EPROBE_DEFER until
they get a panel.

v2: Drop a debug message that slipped in.

Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> (v1)
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 57213f4e3c72..769f247c52a9 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -33,6 +33,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_mipi_dsi.h>
+#include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
@@ -504,7 +505,6 @@ struct vc4_dsi {
 	struct mipi_dsi_host dsi_host;
 	struct drm_encoder *encoder;
 	struct drm_bridge *bridge;
-	bool is_panel_bridge;
 
 	void __iomem *regs;
 
@@ -1289,7 +1289,6 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
 			       struct mipi_dsi_device *device)
 {
 	struct vc4_dsi *dsi = host_to_dsi(host);
-	int ret = 0;
 
 	dsi->lanes = device->lanes;
 	dsi->channel = device->channel;
@@ -1324,34 +1323,12 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
 		return 0;
 	}
 
-	dsi->bridge = of_drm_find_bridge(device->dev.of_node);
-	if (!dsi->bridge) {
-		struct drm_panel *panel =
-			of_drm_find_panel(device->dev.of_node);
-
-		dsi->bridge = drm_panel_bridge_add(panel,
-						   DRM_MODE_CONNECTOR_DSI);
-		if (IS_ERR(dsi->bridge)) {
-			ret = PTR_ERR(dsi->bridge);
-			dsi->bridge = NULL;
-			return ret;
-		}
-		dsi->is_panel_bridge = true;
-	}
-
 	return drm_bridge_attach(dsi->encoder, dsi->bridge, NULL);
 }
 
 static int vc4_dsi_host_detach(struct mipi_dsi_host *host,
 			       struct mipi_dsi_device *device)
 {
-	struct vc4_dsi *dsi = host_to_dsi(host);
-
-	if (dsi->is_panel_bridge) {
-		drm_panel_bridge_remove(dsi->bridge);
-		dsi->bridge = NULL;
-	}
-
 	return 0;
 }
 
@@ -1495,6 +1472,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 	struct vc4_dev *vc4 = to_vc4_dev(drm);
 	struct vc4_dsi *dsi;
 	struct vc4_dsi_encoder *vc4_dsi_encoder;
+	struct drm_panel *panel;
 	const struct of_device_id *match;
 	dma_cap_mask_t dma_mask;
 	int ret;
@@ -1598,6 +1576,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 		return ret;
 	}
 
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
+					  &panel, &dsi->bridge);
+	if (ret)
+		return ret;
+
+	if (panel) {
+		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
+							DRM_MODE_CONNECTOR_DSI);
+		if (IS_ERR(dsi->bridge))
+			return PTR_ERR(dsi->bridge);
+	}
+
 	/* The esc clock rate is supposed to always be 100Mhz. */
 	ret = clk_set_rate(dsi->escape_clock, 100 * 1000000);
 	if (ret) {
-- 
2.11.0

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

[PATCH v5 4/6] drm: Allow DSI devices to be registered before the host registers.

[Eric Anholt]

When a mipi_dsi_host is registered, the DT is walked to find any child
nodes with compatible strings.  Those get registered as DSI devices,
and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.

There is one special case currently, the adv7533 bridge, where the
bridge probes on I2C, and during the bridge attach step it looks up
the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
mipi_dsi_driver).

For the Raspberry Pi panel, though, we also need to attach on I2C (our
control bus), but don't have a bridge driver.  The lack of a bridge's
attach() step like adv7533 uses means that we aren't able to delay the
mipi_dsi_device creation until the mipi_dsi_host is present.

To fix this, we extend mipi_dsi_device_register_full() to allow being
called with a NULL host, which puts the device on a queue waiting for
a host to appear.  When a new host is registered, we fill in the host
value and finish the device creation process.

v2: Cleanly error out if one attempts to mipi_dsi_attach() without a
    host.  Fix handling of DSI with multiple devices, and OF node
    refcounting.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 63 ++++++++++++++++++++++++++++++++++--------
 include/drm/drm_mipi_dsi.h     |  3 ++
 2 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 1160a579e0dc..77d439254da6 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -45,6 +45,13 @@
  * subset of the MIPI DCS command set.
  */
 
+static DEFINE_MUTEX(host_lock);
+static LIST_HEAD(host_list);
+/* List of struct mipi_dsi_device which were registered while no host
+ * was available.
+ */
+static LIST_HEAD(unattached_device_list);
+
 static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
@@ -138,10 +145,12 @@ static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host)
 
 	dsi->host = host;
 	dsi->dev.bus = &mipi_dsi_bus_type;
-	dsi->dev.parent = host->dev;
 	dsi->dev.type = &mipi_dsi_device_type;
 
-	device_initialize(&dsi->dev);
+	if (dsi->host) {
+		dsi->dev.parent = host->dev;
+		device_initialize(&dsi->dev);
+	}
 
 	return dsi;
 }
@@ -206,7 +215,7 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
 			      const struct mipi_dsi_device_info *info)
 {
 	struct mipi_dsi_device *dsi;
-	struct device *dev = host->dev;
+	struct device *dev = host ? host->dev : NULL;
 	int ret;
 
 	if (!info) {
@@ -230,11 +239,17 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
 	dsi->channel = info->channel;
 	strlcpy(dsi->name, info->type, sizeof(dsi->name));
 
-	ret = mipi_dsi_device_add(dsi);
-	if (ret) {
-		dev_err(dev, "failed to add DSI device %d\n", ret);
-		kfree(dsi);
-		return ERR_PTR(ret);
+	if (!dsi->host) {
+		mutex_lock(&host_lock);
+		list_add(&dsi->list, &unattached_device_list);
+		mutex_unlock(&host_lock);
+	} else {
+		ret = mipi_dsi_device_add(dsi);
+		if (ret) {
+			dev_err(dev, "failed to add DSI device %d\n", ret);
+			kfree(dsi);
+			return ERR_PTR(ret);
+		}
 	}
 
 	return dsi;
@@ -251,9 +266,6 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
 }
 EXPORT_SYMBOL(mipi_dsi_device_unregister);
 
-static DEFINE_MUTEX(host_lock);
-static LIST_HEAD(host_list);
-
 /**
  * of_find_mipi_dsi_host_by_node() - find the MIPI DSI host matching a
  *				     device tree node
@@ -285,6 +297,7 @@ EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node);
 int mipi_dsi_host_register(struct mipi_dsi_host *host)
 {
 	struct device_node *node;
+	struct mipi_dsi_device *dsi, *temp;
 
 	for_each_available_child_of_node(host->dev->of_node, node) {
 		/* skip nodes without reg property */
@@ -295,6 +308,27 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
 
 	mutex_lock(&host_lock);
 	list_add_tail(&host->list, &host_list);
+
+	/* If any DSI devices were registered under our OF node, then
+	 * connect our host to it and probe them now.
+	 */
+	list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
+		struct device_node *host_node = of_get_parent(dsi->dev.of_node);
+
+		if (!of_node_cmp(host_node->name, "ports"))
+			host_node = of_get_next_parent(host_node);
+
+		if (host_node == host->dev->of_node) {
+			dsi->host = host;
+			dsi->dev.parent = host->dev;
+			device_initialize(&dsi->dev);
+
+			mipi_dsi_device_add(dsi);
+			list_del_init(&dsi->list);
+		}
+
+		of_node_put(host_node);
+	}
 	mutex_unlock(&host_lock);
 
 	return 0;
@@ -326,7 +360,12 @@ EXPORT_SYMBOL(mipi_dsi_host_unregister);
  */
 int mipi_dsi_attach(struct mipi_dsi_device *dsi)
 {
-	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
+	const struct mipi_dsi_host_ops *ops;
+
+	if (!dsi->host)
+		return -EINVAL;
+
+	ops = dsi->host->ops;
 
 	if (!ops || !ops->attach)
 		return -ENOSYS;
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 4fef19064b0f..dfe640fec9ba 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -168,6 +168,7 @@ struct mipi_dsi_device_info {
  * @format: pixel format for video mode
  * @lanes: number of active data lanes
  * @mode_flags: DSI operation mode related flags
+ * @list: Entry on the unattached_device_list
  */
 struct mipi_dsi_device {
 	struct mipi_dsi_host *host;
@@ -178,6 +179,8 @@ struct mipi_dsi_device {
 	unsigned int lanes;
 	enum mipi_dsi_pixel_format format;
 	unsigned long mode_flags;
+
+	struct list_head list;
 };
 
 #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
-- 
2.11.0

[PATCH v5 4/6] drm: Allow DSI devices to be registered before the host registers.

[Eric Anholt]

When a mipi_dsi_host is registered, the DT is walked to find any child
nodes with compatible strings.  Those get registered as DSI devices,
and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.

There is one special case currently, the adv7533 bridge, where the
bridge probes on I2C, and during the bridge attach step it looks up
the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
mipi_dsi_driver).

For the Raspberry Pi panel, though, we also need to attach on I2C (our
control bus), but don't have a bridge driver.  The lack of a bridge's
attach() step like adv7533 uses means that we aren't able to delay the
mipi_dsi_device creation until the mipi_dsi_host is present.

To fix this, we extend mipi_dsi_device_register_full() to allow being
called with a NULL host, which puts the device on a queue waiting for
a host to appear.  When a new host is registered, we fill in the host
value and finish the device creation process.

v2: Cleanly error out if one attempts to mipi_dsi_attach() without a
    host.  Fix handling of DSI with multiple devices, and OF node
    refcounting.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 63 ++++++++++++++++++++++++++++++++++--------
 include/drm/drm_mipi_dsi.h     |  3 ++
 2 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 1160a579e0dc..77d439254da6 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -45,6 +45,13 @@
  * subset of the MIPI DCS command set.
  */
 
+static DEFINE_MUTEX(host_lock);
+static LIST_HEAD(host_list);
+/* List of struct mipi_dsi_device which were registered while no host
+ * was available.
+ */
+static LIST_HEAD(unattached_device_list);
+
 static int mipi_dsi_device_match(struct device *dev, struct device_driver *drv)
 {
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(dev);
@@ -138,10 +145,12 @@ static struct mipi_dsi_device *mipi_dsi_device_alloc(struct mipi_dsi_host *host)
 
 	dsi->host = host;
 	dsi->dev.bus = &mipi_dsi_bus_type;
-	dsi->dev.parent = host->dev;
 	dsi->dev.type = &mipi_dsi_device_type;
 
-	device_initialize(&dsi->dev);
+	if (dsi->host) {
+		dsi->dev.parent = host->dev;
+		device_initialize(&dsi->dev);
+	}
 
 	return dsi;
 }
@@ -206,7 +215,7 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
 			      const struct mipi_dsi_device_info *info)
 {
 	struct mipi_dsi_device *dsi;
-	struct device *dev = host->dev;
+	struct device *dev = host ? host->dev : NULL;
 	int ret;
 
 	if (!info) {
@@ -230,11 +239,17 @@ mipi_dsi_device_register_full(struct mipi_dsi_host *host,
 	dsi->channel = info->channel;
 	strlcpy(dsi->name, info->type, sizeof(dsi->name));
 
-	ret = mipi_dsi_device_add(dsi);
-	if (ret) {
-		dev_err(dev, "failed to add DSI device %d\n", ret);
-		kfree(dsi);
-		return ERR_PTR(ret);
+	if (!dsi->host) {
+		mutex_lock(&host_lock);
+		list_add(&dsi->list, &unattached_device_list);
+		mutex_unlock(&host_lock);
+	} else {
+		ret = mipi_dsi_device_add(dsi);
+		if (ret) {
+			dev_err(dev, "failed to add DSI device %d\n", ret);
+			kfree(dsi);
+			return ERR_PTR(ret);
+		}
 	}
 
 	return dsi;
@@ -251,9 +266,6 @@ void mipi_dsi_device_unregister(struct mipi_dsi_device *dsi)
 }
 EXPORT_SYMBOL(mipi_dsi_device_unregister);
 
-static DEFINE_MUTEX(host_lock);
-static LIST_HEAD(host_list);
-
 /**
  * of_find_mipi_dsi_host_by_node() - find the MIPI DSI host matching a
  *				     device tree node
@@ -285,6 +297,7 @@ EXPORT_SYMBOL(of_find_mipi_dsi_host_by_node);
 int mipi_dsi_host_register(struct mipi_dsi_host *host)
 {
 	struct device_node *node;
+	struct mipi_dsi_device *dsi, *temp;
 
 	for_each_available_child_of_node(host->dev->of_node, node) {
 		/* skip nodes without reg property */
@@ -295,6 +308,27 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
 
 	mutex_lock(&host_lock);
 	list_add_tail(&host->list, &host_list);
+
+	/* If any DSI devices were registered under our OF node, then
+	 * connect our host to it and probe them now.
+	 */
+	list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
+		struct device_node *host_node = of_get_parent(dsi->dev.of_node);
+
+		if (!of_node_cmp(host_node->name, "ports"))
+			host_node = of_get_next_parent(host_node);
+
+		if (host_node == host->dev->of_node) {
+			dsi->host = host;
+			dsi->dev.parent = host->dev;
+			device_initialize(&dsi->dev);
+
+			mipi_dsi_device_add(dsi);
+			list_del_init(&dsi->list);
+		}
+
+		of_node_put(host_node);
+	}
 	mutex_unlock(&host_lock);
 
 	return 0;
@@ -326,7 +360,12 @@ EXPORT_SYMBOL(mipi_dsi_host_unregister);
  */
 int mipi_dsi_attach(struct mipi_dsi_device *dsi)
 {
-	const struct mipi_dsi_host_ops *ops = dsi->host->ops;
+	const struct mipi_dsi_host_ops *ops;
+
+	if (!dsi->host)
+		return -EINVAL;
+
+	ops = dsi->host->ops;
 
 	if (!ops || !ops->attach)
 		return -ENOSYS;
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 4fef19064b0f..dfe640fec9ba 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -168,6 +168,7 @@ struct mipi_dsi_device_info {
  * @format: pixel format for video mode
  * @lanes: number of active data lanes
  * @mode_flags: DSI operation mode related flags
+ * @list: Entry on the unattached_device_list
  */
 struct mipi_dsi_device {
 	struct mipi_dsi_host *host;
@@ -178,6 +179,8 @@ struct mipi_dsi_device {
 	unsigned int lanes;
 	enum mipi_dsi_pixel_format format;
 	unsigned long mode_flags;
+
+	struct list_head list;
 };
 
 #define MIPI_DSI_MODULE_PREFIX "mipi-dsi:"
-- 
2.11.0

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

[PATCH v5 5/6] dt-bindings: Document the Raspberry Pi Touchscreen nodes.

[Eric Anholt]

This doesn't yet cover input, but the driver does get the display
working when the firmware is disabled from talking to our I2C lines.

Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../panel/raspberrypi,7inch-touchscreen.txt        | 49 ++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt

diff --git a/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt
new file mode 100644
index 000000000000..e9e19c059260
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt
@@ -0,0 +1,49 @@
+This binding covers the official 7" (800x480) Raspberry Pi touchscreen
+panel.
+
+This DSI panel contains:
+
+- TC358762 DSI->DPI bridge
+- Atmel microcontroller on I2C for power sequencing the DSI bridge and
+  controlling backlight
+- Touchscreen controller on I2C for touch input
+
+and this binding covers the DSI display parts but not its touch input.
+
+Required properties:
+- compatible:	Must be "raspberrypi,7inch-touchscreen-panel"
+- reg:		Must be "45"
+- port:		See panel-common.txt
+
+Example:
+
+dsi1: dsi@7e700000 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	<...>
+
+	port {
+		dsi_out_port: endpoint {
+			remote-endpoint = <&panel_dsi_port>;
+		};
+	};
+};
+
+i2c_dsi: i2c {
+	compatible = "i2c-gpio";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	gpios = <&gpio 28 0
+		 &gpio 29 0>;
+
+	lcd@45 {
+		compatible = "raspberrypi,7inch-touchscreen-panel";
+		reg = <0x45>;
+
+		port {
+			panel_dsi_port: endpoint {
+				remote-endpoint = <&dsi_out_port>;
+			};
+		};
+	};
+};
-- 
2.11.0

[PATCH v5 5/6] dt-bindings: Document the Raspberry Pi Touchscreen nodes.

[Eric Anholt]

This doesn't yet cover input, but the driver does get the display
working when the firmware is disabled from talking to our I2C lines.

Signed-off-by: Eric Anholt <eric@anholt.net>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../panel/raspberrypi,7inch-touchscreen.txt        | 49 ++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt

diff --git a/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt
new file mode 100644
index 000000000000..e9e19c059260
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/raspberrypi,7inch-touchscreen.txt
@@ -0,0 +1,49 @@
+This binding covers the official 7" (800x480) Raspberry Pi touchscreen
+panel.
+
+This DSI panel contains:
+
+- TC358762 DSI->DPI bridge
+- Atmel microcontroller on I2C for power sequencing the DSI bridge and
+  controlling backlight
+- Touchscreen controller on I2C for touch input
+
+and this binding covers the DSI display parts but not its touch input.
+
+Required properties:
+- compatible:	Must be "raspberrypi,7inch-touchscreen-panel"
+- reg:		Must be "45"
+- port:		See panel-common.txt
+
+Example:
+
+dsi1: dsi@7e700000 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	<...>
+
+	port {
+		dsi_out_port: endpoint {
+			remote-endpoint = <&panel_dsi_port>;
+		};
+	};
+};
+
+i2c_dsi: i2c {
+	compatible = "i2c-gpio";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	gpios = <&gpio 28 0
+		 &gpio 29 0>;
+
+	lcd@45 {
+		compatible = "raspberrypi,7inch-touchscreen-panel";
+		reg = <0x45>;
+
+		port {
+			panel_dsi_port: endpoint {
+				remote-endpoint = <&dsi_out_port>;
+			};
+		};
+	};
+};
-- 
2.11.0

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

[PATCH v5 6/6] drm/panel: Add support for the Raspberry Pi 7" Touchscreen.

[Eric Anholt]

This driver communicates with the Atmel microcontroller for sequencing
the poweron of the TC358762 DSI-DPI bridge and controlling the
backlight PWM.

v2: Set the same default orientation as the closed source firmware
    used, which is the best for viewing angle.
v3: Rewrite as an i2c client driver after bridge driver rejection.
v4: Finish probe without the DSI host, using the new delayed
    registration, and attach to the host during mipi_dsi_driver probe.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/panel/Kconfig                      |   8 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 505 +++++++++++++++++++++
 3 files changed, 514 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index d84a031fae24..d6f1969b8a3b 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -73,6 +73,14 @@ config DRM_PANEL_PANASONIC_VVX10F034N00
 	  WUXGA (1920x1200) Novatek NT1397-based DSI panel as found in some
 	  Xperia Z2 tablets
 
+config DRM_PANEL_RASPBERRYPI_TOUCHSCREEN
+	tristate "Raspberry Pi 7-inch touchscreen panel"
+	depends on DRM_MIPI_DSI
+	help
+	  Say Y here if you want to enable support for the Raspberry
+	  Pi 7" Touchscreen.  To compile this driver as a module,
+	  choose M here.
+
 config DRM_PANEL_SAMSUNG_S6E3HA2
 	tristate "Samsung S6E3HA2 DSI video mode panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 9f6610d08b00..bd17fac21c7b 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
 obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
 obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
+obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
new file mode 100644
index 000000000000..b23331051d79
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
@@ -0,0 +1,505 @@
+/*
+ * Copyright © 2016-2017 Broadcom
+ *
+ * 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.
+ *
+ * Portions of this file (derived from panel-simple.c) are:
+ *
+ * Copyright (C) 2013, NVIDIA Corporation.  All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sub license,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+/**
+ * Raspberry Pi 7" touchscreen panel driver.
+ *
+ * The 7" touchscreen consists of a DPI LCD panel, a Toshiba
+ * TC358762XBG DSI-DPI bridge, and an I2C-connected Atmel ATTINY88-MUR
+ * controlling power management, the LCD PWM, and initial register
+ * setup of the Tohsiba.
+ *
+ * This driver controls the TC358762 and ATTINY88, presenting a DSI
+ * device with a drm_panel.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/fb.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_graph.h>
+#include <linux/pm.h>
+
+#include <drm/drm_panel.h>
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#define RPI_DSI_DRIVER_NAME "rpi-ts-dsi"
+
+/* I2C registers of the Atmel microcontroller. */
+enum REG_ADDR {
+	REG_ID = 0x80,
+	REG_PORTA, /* BIT(2) for horizontal flip, BIT(3) for vertical flip */
+	REG_PORTB,
+	REG_PORTC,
+	REG_PORTD,
+	REG_POWERON,
+	REG_PWM,
+	REG_DDRA,
+	REG_DDRB,
+	REG_DDRC,
+	REG_DDRD,
+	REG_TEST,
+	REG_WR_ADDRL,
+	REG_WR_ADDRH,
+	REG_READH,
+	REG_READL,
+	REG_WRITEH,
+	REG_WRITEL,
+	REG_ID2,
+};
+
+/* We only turn the PWM on or off, without varying values. */
+#define RPI_TOUCHSCREEN_MAX_BRIGHTNESS 1
+
+/* DSI D-PHY Layer Registers */
+#define D0W_DPHYCONTTX		0x0004
+#define CLW_DPHYCONTRX		0x0020
+#define D0W_DPHYCONTRX		0x0024
+#define D1W_DPHYCONTRX		0x0028
+#define COM_DPHYCONTRX		0x0038
+#define CLW_CNTRL		0x0040
+#define D0W_CNTRL		0x0044
+#define D1W_CNTRL		0x0048
+#define DFTMODE_CNTRL		0x0054
+
+/* DSI PPI Layer Registers */
+#define PPI_STARTPPI		0x0104
+#define PPI_BUSYPPI		0x0108
+#define PPI_LINEINITCNT		0x0110
+#define PPI_LPTXTIMECNT		0x0114
+#define PPI_CLS_ATMR		0x0140
+#define PPI_D0S_ATMR		0x0144
+#define PPI_D1S_ATMR		0x0148
+#define PPI_D0S_CLRSIPOCOUNT	0x0164
+#define PPI_D1S_CLRSIPOCOUNT	0x0168
+#define CLS_PRE			0x0180
+#define D0S_PRE			0x0184
+#define D1S_PRE			0x0188
+#define CLS_PREP		0x01A0
+#define D0S_PREP		0x01A4
+#define D1S_PREP		0x01A8
+#define CLS_ZERO		0x01C0
+#define D0S_ZERO		0x01C4
+#define D1S_ZERO		0x01C8
+#define PPI_CLRFLG		0x01E0
+#define PPI_CLRSIPO		0x01E4
+#define HSTIMEOUT		0x01F0
+#define HSTIMEOUTENABLE		0x01F4
+
+/* DSI Protocol Layer Registers */
+#define DSI_STARTDSI		0x0204
+#define DSI_BUSYDSI		0x0208
+#define DSI_LANEENABLE		0x0210
+# define DSI_LANEENABLE_CLOCK		BIT(0)
+# define DSI_LANEENABLE_D0		BIT(1)
+# define DSI_LANEENABLE_D1		BIT(2)
+
+#define DSI_LANESTATUS0		0x0214
+#define DSI_LANESTATUS1		0x0218
+#define DSI_INTSTATUS		0x0220
+#define DSI_INTMASK		0x0224
+#define DSI_INTCLR		0x0228
+#define DSI_LPTXTO		0x0230
+#define DSI_MODE		0x0260
+#define DSI_PAYLOAD0		0x0268
+#define DSI_PAYLOAD1		0x026C
+#define DSI_SHORTPKTDAT		0x0270
+#define DSI_SHORTPKTREQ		0x0274
+#define DSI_BTASTA		0x0278
+#define DSI_BTACLR		0x027C
+
+/* DSI General Registers */
+#define DSIERRCNT		0x0300
+#define DSISIGMOD		0x0304
+
+/* DSI Application Layer Registers */
+#define APLCTRL			0x0400
+#define APLSTAT			0x0404
+#define APLERR			0x0408
+#define PWRMOD			0x040C
+#define RDPKTLN			0x0410
+#define PXLFMT			0x0414
+#define MEMWRCMD		0x0418
+
+/* LCDC/DPI Host Registers */
+#define LCDCTRL			0x0420
+#define HSR			0x0424
+#define HDISPR			0x0428
+#define VSR			0x042C
+#define VDISPR			0x0430
+#define VFUEN			0x0434
+
+/* DBI-B Host Registers */
+#define DBIBCTRL		0x0440
+
+/* SPI Master Registers */
+#define SPICMR			0x0450
+#define SPITCR			0x0454
+
+/* System Controller Registers */
+#define SYSSTAT			0x0460
+#define SYSCTRL			0x0464
+#define SYSPLL1			0x0468
+#define SYSPLL2			0x046C
+#define SYSPLL3			0x0470
+#define SYSPMCTRL		0x047C
+
+/* GPIO Registers */
+#define GPIOC			0x0480
+#define GPIOO			0x0484
+#define GPIOI			0x0488
+
+/* I2C Registers */
+#define I2CCLKCTRL		0x0490
+
+/* Chip/Rev Registers */
+#define IDREG			0x04A0
+
+/* Debug Registers */
+#define WCMDQUEUE		0x0500
+#define RCMDQUEUE		0x0504
+
+struct rpi_touchscreen {
+	struct drm_panel base;
+	struct mipi_dsi_device *dsi;
+	struct i2c_client *i2c;
+};
+
+static const struct drm_display_mode rpi_touchscreen_modes[] = {
+	{
+		/* Modeline comes from the Raspberry Pi firmware, with HFP=1
+		 * plugged in and clock re-computed from that.
+		 */
+		.clock = 25979400 / 1000,
+		.hdisplay = 800,
+		.hsync_start = 800 + 1,
+		.hsync_end = 800 + 1 + 2,
+		.htotal = 800 + 1 + 2 + 46,
+		.vdisplay = 480,
+		.vsync_start = 480 + 7,
+		.vsync_end = 480 + 7 + 2,
+		.vtotal = 480 + 7 + 2 + 21,
+		.vrefresh = 60,
+	},
+};
+
+static struct rpi_touchscreen *panel_to_ts(struct drm_panel *panel)
+{
+	return container_of(panel, struct rpi_touchscreen, base);
+}
+
+static u8 rpi_touchscreen_i2c_read(struct rpi_touchscreen *ts, u8 reg)
+{
+	return i2c_smbus_read_byte_data(ts->i2c, reg);
+}
+
+static void rpi_touchscreen_i2c_write(struct rpi_touchscreen *ts,
+				      u8 reg, u8 val)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(ts->i2c, reg, val);
+	if (ret)
+		dev_err(&ts->dsi->dev, "I2C write failed: %d\n", ret);
+}
+
+static int rpi_touchscreen_write(struct rpi_touchscreen *ts, u16 reg, u32 val)
+{
+#if 0
+	/* The firmware uses LP DSI transactions like this to bring up
+	 * the hardware, which should be faster than using I2C to then
+	 * pass to the Toshiba.  However, I was unable to get it to
+	 * work.
+	 */
+	u8 msg[] = {
+		reg,
+		reg >> 8,
+		val,
+		val >> 8,
+		val >> 16,
+		val >> 24,
+	};
+
+	mipi_dsi_dcs_write_buffer(ts->dsi, msg, sizeof(msg));
+#else
+	rpi_touchscreen_i2c_write(ts, REG_WR_ADDRH, reg >> 8);
+	rpi_touchscreen_i2c_write(ts, REG_WR_ADDRL, reg);
+	rpi_touchscreen_i2c_write(ts, REG_WRITEH, val >> 8);
+	rpi_touchscreen_i2c_write(ts, REG_WRITEL, val);
+#endif
+
+	return 0;
+}
+
+static int rpi_touchscreen_disable(struct drm_panel *panel)
+{
+	struct rpi_touchscreen *ts = panel_to_ts(panel);
+
+	rpi_touchscreen_i2c_write(ts, REG_PWM, 0);
+
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+	udelay(1);
+
+	return 0;
+}
+
+static int rpi_touchscreen_noop(struct drm_panel *panel)
+{
+	return 0;
+}
+
+static int rpi_touchscreen_enable(struct drm_panel *panel)
+{
+	struct rpi_touchscreen *ts = panel_to_ts(panel);
+	int i;
+
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 1);
+	/* Wait for nPWRDWN to go low to indicate poweron is done. */
+	for (i = 0; i < 100; i++) {
+		if (rpi_touchscreen_i2c_read(ts, REG_PORTB) & 1)
+			break;
+	}
+
+	rpi_touchscreen_write(ts, DSI_LANEENABLE,
+			      DSI_LANEENABLE_CLOCK |
+			      DSI_LANEENABLE_D0);
+	rpi_touchscreen_write(ts, PPI_D0S_CLRSIPOCOUNT, 0x05);
+	rpi_touchscreen_write(ts, PPI_D1S_CLRSIPOCOUNT, 0x05);
+	rpi_touchscreen_write(ts, PPI_D0S_ATMR, 0x00);
+	rpi_touchscreen_write(ts, PPI_D1S_ATMR, 0x00);
+	rpi_touchscreen_write(ts, PPI_LPTXTIMECNT, 0x03);
+
+	rpi_touchscreen_write(ts, SPICMR, 0x00);
+	rpi_touchscreen_write(ts, LCDCTRL, 0x00100150);
+	rpi_touchscreen_write(ts, SYSCTRL, 0x040f);
+	msleep(100);
+
+	rpi_touchscreen_write(ts, PPI_STARTPPI, 0x01);
+	rpi_touchscreen_write(ts, DSI_STARTDSI, 0x01);
+	msleep(100);
+
+	/* Turn on the backlight. */
+	rpi_touchscreen_i2c_write(ts, REG_PWM, 255);
+
+	/* Default to the same orientation as the closed source
+	 * firmware used for the panel.  Runtime rotation
+	 * configuration will be supported using VC4's plane
+	 * orientation bits.
+	 */
+	rpi_touchscreen_i2c_write(ts, REG_PORTA, BIT(2));
+
+	return 0;
+}
+
+static int rpi_touchscreen_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct drm_device *drm = panel->drm;
+	unsigned int i, num = 0;
+	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+
+	for (i = 0; i < ARRAY_SIZE(rpi_touchscreen_modes); i++) {
+		const struct drm_display_mode *m = &rpi_touchscreen_modes[i];
+		struct drm_display_mode *mode;
+
+		mode = drm_mode_duplicate(drm, m);
+		if (!mode) {
+			dev_err(drm->dev, "failed to add mode %ux%u@%u\n",
+				m->hdisplay, m->vdisplay, m->vrefresh);
+			continue;
+		}
+
+		mode->type |= DRM_MODE_TYPE_DRIVER;
+
+		if (i == 0)
+			mode->type |= DRM_MODE_TYPE_PREFERRED;
+
+		drm_mode_set_name(mode);
+
+		drm_mode_probed_add(connector, mode);
+		num++;
+	}
+
+	connector->display_info.bpc = 8;
+	connector->display_info.width_mm = 154;
+	connector->display_info.height_mm = 86;
+	drm_display_info_set_bus_formats(&connector->display_info,
+					 &bus_format, 1);
+
+	return num;
+}
+
+static const struct drm_panel_funcs rpi_touchscreen_funcs = {
+	.disable = rpi_touchscreen_disable,
+	.unprepare = rpi_touchscreen_noop,
+	.prepare = rpi_touchscreen_noop,
+	.enable = rpi_touchscreen_enable,
+	.get_modes = rpi_touchscreen_get_modes,
+};
+
+static int rpi_touchscreen_probe(struct i2c_client *i2c,
+				 const struct i2c_device_id *id)
+{
+	struct device *dev = &i2c->dev;
+	struct rpi_touchscreen *ts;
+	struct device_node *endpoint;
+	int ret, ver;
+	struct mipi_dsi_device_info info = {
+		.type = RPI_DSI_DRIVER_NAME,
+		.channel = 0,
+		.node = NULL,
+	};
+
+	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, ts);
+
+	ts->i2c = i2c;
+
+	ver = rpi_touchscreen_i2c_read(ts, REG_ID);
+	if (ver < 0) {
+		dev_err(dev, "Atmel I2C read failed: %d\n", ver);
+		return -ENODEV;
+	}
+
+	switch (ver) {
+	case 0xde: /* ver 1 */
+	case 0xc3: /* ver 2 */
+		break;
+	default:
+		dev_err(dev, "Unknown Atmel firmware revision: 0x%02x\n", ver);
+		return -ENODEV;
+	}
+
+	/* Turn off at boot, so we can cleanly sequence powering on. */
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+
+	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
+	info.node = of_graph_get_remote_port(endpoint);
+	of_node_put(endpoint);
+
+	ts->dsi = mipi_dsi_device_register_full(NULL, &info);
+	if (IS_ERR(ts->dsi)) {
+		dev_err(dev, "DSI device registration failed: %ld\n",
+			PTR_ERR(ts->dsi));
+		return PTR_ERR(ts->dsi);
+	}
+
+	ts->dsi->mode_flags = (MIPI_DSI_MODE_VIDEO |
+			       MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+			       MIPI_DSI_MODE_LPM);
+	ts->dsi->format = MIPI_DSI_FMT_RGB888;
+	ts->dsi->lanes = 1;
+
+	ts->base.dev = dev;
+	ts->base.funcs = &rpi_touchscreen_funcs;
+
+	/* This appears last, as it's what will unblock the DSI host
+	 * driver's probe function and start the attach process.
+	 */
+	ret = drm_panel_add(&ts->base);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rpi_touchscreen_remove(struct i2c_client *i2c)
+{
+	struct rpi_touchscreen *ts = i2c_get_clientdata(i2c);
+
+	mipi_dsi_detach(ts->dsi);
+
+	drm_panel_remove(&ts->base);
+
+	mipi_dsi_device_unregister(ts->dsi);
+	kfree(ts->dsi);
+
+	return 0;
+}
+
+static int rpi_touchscreen_dsi_probe(struct mipi_dsi_device *dsi)
+{
+	int ret = mipi_dsi_attach(dsi);
+
+	if (ret)
+		dev_err(&dsi->dev, "failed to attach dsi to host: %d\n", ret);
+
+	return ret;
+}
+
+static struct mipi_dsi_driver rpi_touchscreen_dsi_driver = {
+	.driver.name = RPI_DSI_DRIVER_NAME,
+	.probe = rpi_touchscreen_dsi_probe,
+};
+
+static const struct of_device_id rpi_touchscreen_of_ids[] = {
+	{ .compatible = "raspberrypi,7inch-touchscreen-panel" },
+	{ } /* sentinel */
+};
+MODULE_DEVICE_TABLE(of, rpi_touchscreen_of_ids);
+
+static struct i2c_driver rpi_touchscreen_driver = {
+	.driver = {
+		.name = "rpi_touchscreen",
+		.of_match_table = rpi_touchscreen_of_ids,
+	},
+	.probe = rpi_touchscreen_probe,
+	.remove = rpi_touchscreen_remove,
+};
+
+static int __init rpi_touchscreen_init(void)
+{
+	mipi_dsi_driver_register(&rpi_touchscreen_dsi_driver);
+	return i2c_add_driver(&rpi_touchscreen_driver);
+}
+module_init(rpi_touchscreen_init);
+
+static void __exit rpi_touchscreen_exit(void)
+{
+	i2c_del_driver(&rpi_touchscreen_driver);
+	mipi_dsi_driver_unregister(&rpi_touchscreen_dsi_driver);
+}
+module_exit(rpi_touchscreen_exit);
+
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi 7-inch touchscreen driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

[PATCH v5 6/6] drm/panel: Add support for the Raspberry Pi 7" Touchscreen.

[Eric Anholt]

This driver communicates with the Atmel microcontroller for sequencing
the poweron of the TC358762 DSI-DPI bridge and controlling the
backlight PWM.

v2: Set the same default orientation as the closed source firmware
    used, which is the best for viewing angle.
v3: Rewrite as an i2c client driver after bridge driver rejection.
v4: Finish probe without the DSI host, using the new delayed
    registration, and attach to the host during mipi_dsi_driver probe.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/panel/Kconfig                      |   8 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 .../gpu/drm/panel/panel-raspberrypi-touchscreen.c  | 505 +++++++++++++++++++++
 3 files changed, 514 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index d84a031fae24..d6f1969b8a3b 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -73,6 +73,14 @@ config DRM_PANEL_PANASONIC_VVX10F034N00
 	  WUXGA (1920x1200) Novatek NT1397-based DSI panel as found in some
 	  Xperia Z2 tablets
 
+config DRM_PANEL_RASPBERRYPI_TOUCHSCREEN
+	tristate "Raspberry Pi 7-inch touchscreen panel"
+	depends on DRM_MIPI_DSI
+	help
+	  Say Y here if you want to enable support for the Raspberry
+	  Pi 7" Touchscreen.  To compile this driver as a module,
+	  choose M here.
+
 config DRM_PANEL_SAMSUNG_S6E3HA2
 	tristate "Samsung S6E3HA2 DSI video mode panel"
 	depends on OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 9f6610d08b00..bd17fac21c7b 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o
 obj-$(CONFIG_DRM_PANEL_JDI_LT070ME05000) += panel-jdi-lt070me05000.o
 obj-$(CONFIG_DRM_PANEL_LG_LG4573) += panel-lg-lg4573.o
 obj-$(CONFIG_DRM_PANEL_PANASONIC_VVX10F034N00) += panel-panasonic-vvx10f034n00.o
+obj-$(CONFIG_DRM_PANEL_RASPBERRYPI_TOUCHSCREEN) += panel-raspberrypi-touchscreen.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_LD9040) += panel-samsung-ld9040.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E3HA2) += panel-samsung-s6e3ha2.o
 obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += panel-samsung-s6e8aa0.o
diff --git a/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
new file mode 100644
index 000000000000..b23331051d79
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-raspberrypi-touchscreen.c
@@ -0,0 +1,505 @@
+/*
+ * Copyright © 2016-2017 Broadcom
+ *
+ * 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.
+ *
+ * Portions of this file (derived from panel-simple.c) are:
+ *
+ * Copyright (C) 2013, NVIDIA Corporation.  All rights reserved.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sub license,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial portions
+ * of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+/**
+ * Raspberry Pi 7" touchscreen panel driver.
+ *
+ * The 7" touchscreen consists of a DPI LCD panel, a Toshiba
+ * TC358762XBG DSI-DPI bridge, and an I2C-connected Atmel ATTINY88-MUR
+ * controlling power management, the LCD PWM, and initial register
+ * setup of the Tohsiba.
+ *
+ * This driver controls the TC358762 and ATTINY88, presenting a DSI
+ * device with a drm_panel.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/fb.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_graph.h>
+#include <linux/pm.h>
+
+#include <drm/drm_panel.h>
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#define RPI_DSI_DRIVER_NAME "rpi-ts-dsi"
+
+/* I2C registers of the Atmel microcontroller. */
+enum REG_ADDR {
+	REG_ID = 0x80,
+	REG_PORTA, /* BIT(2) for horizontal flip, BIT(3) for vertical flip */
+	REG_PORTB,
+	REG_PORTC,
+	REG_PORTD,
+	REG_POWERON,
+	REG_PWM,
+	REG_DDRA,
+	REG_DDRB,
+	REG_DDRC,
+	REG_DDRD,
+	REG_TEST,
+	REG_WR_ADDRL,
+	REG_WR_ADDRH,
+	REG_READH,
+	REG_READL,
+	REG_WRITEH,
+	REG_WRITEL,
+	REG_ID2,
+};
+
+/* We only turn the PWM on or off, without varying values. */
+#define RPI_TOUCHSCREEN_MAX_BRIGHTNESS 1
+
+/* DSI D-PHY Layer Registers */
+#define D0W_DPHYCONTTX		0x0004
+#define CLW_DPHYCONTRX		0x0020
+#define D0W_DPHYCONTRX		0x0024
+#define D1W_DPHYCONTRX		0x0028
+#define COM_DPHYCONTRX		0x0038
+#define CLW_CNTRL		0x0040
+#define D0W_CNTRL		0x0044
+#define D1W_CNTRL		0x0048
+#define DFTMODE_CNTRL		0x0054
+
+/* DSI PPI Layer Registers */
+#define PPI_STARTPPI		0x0104
+#define PPI_BUSYPPI		0x0108
+#define PPI_LINEINITCNT		0x0110
+#define PPI_LPTXTIMECNT		0x0114
+#define PPI_CLS_ATMR		0x0140
+#define PPI_D0S_ATMR		0x0144
+#define PPI_D1S_ATMR		0x0148
+#define PPI_D0S_CLRSIPOCOUNT	0x0164
+#define PPI_D1S_CLRSIPOCOUNT	0x0168
+#define CLS_PRE			0x0180
+#define D0S_PRE			0x0184
+#define D1S_PRE			0x0188
+#define CLS_PREP		0x01A0
+#define D0S_PREP		0x01A4
+#define D1S_PREP		0x01A8
+#define CLS_ZERO		0x01C0
+#define D0S_ZERO		0x01C4
+#define D1S_ZERO		0x01C8
+#define PPI_CLRFLG		0x01E0
+#define PPI_CLRSIPO		0x01E4
+#define HSTIMEOUT		0x01F0
+#define HSTIMEOUTENABLE		0x01F4
+
+/* DSI Protocol Layer Registers */
+#define DSI_STARTDSI		0x0204
+#define DSI_BUSYDSI		0x0208
+#define DSI_LANEENABLE		0x0210
+# define DSI_LANEENABLE_CLOCK		BIT(0)
+# define DSI_LANEENABLE_D0		BIT(1)
+# define DSI_LANEENABLE_D1		BIT(2)
+
+#define DSI_LANESTATUS0		0x0214
+#define DSI_LANESTATUS1		0x0218
+#define DSI_INTSTATUS		0x0220
+#define DSI_INTMASK		0x0224
+#define DSI_INTCLR		0x0228
+#define DSI_LPTXTO		0x0230
+#define DSI_MODE		0x0260
+#define DSI_PAYLOAD0		0x0268
+#define DSI_PAYLOAD1		0x026C
+#define DSI_SHORTPKTDAT		0x0270
+#define DSI_SHORTPKTREQ		0x0274
+#define DSI_BTASTA		0x0278
+#define DSI_BTACLR		0x027C
+
+/* DSI General Registers */
+#define DSIERRCNT		0x0300
+#define DSISIGMOD		0x0304
+
+/* DSI Application Layer Registers */
+#define APLCTRL			0x0400
+#define APLSTAT			0x0404
+#define APLERR			0x0408
+#define PWRMOD			0x040C
+#define RDPKTLN			0x0410
+#define PXLFMT			0x0414
+#define MEMWRCMD		0x0418
+
+/* LCDC/DPI Host Registers */
+#define LCDCTRL			0x0420
+#define HSR			0x0424
+#define HDISPR			0x0428
+#define VSR			0x042C
+#define VDISPR			0x0430
+#define VFUEN			0x0434
+
+/* DBI-B Host Registers */
+#define DBIBCTRL		0x0440
+
+/* SPI Master Registers */
+#define SPICMR			0x0450
+#define SPITCR			0x0454
+
+/* System Controller Registers */
+#define SYSSTAT			0x0460
+#define SYSCTRL			0x0464
+#define SYSPLL1			0x0468
+#define SYSPLL2			0x046C
+#define SYSPLL3			0x0470
+#define SYSPMCTRL		0x047C
+
+/* GPIO Registers */
+#define GPIOC			0x0480
+#define GPIOO			0x0484
+#define GPIOI			0x0488
+
+/* I2C Registers */
+#define I2CCLKCTRL		0x0490
+
+/* Chip/Rev Registers */
+#define IDREG			0x04A0
+
+/* Debug Registers */
+#define WCMDQUEUE		0x0500
+#define RCMDQUEUE		0x0504
+
+struct rpi_touchscreen {
+	struct drm_panel base;
+	struct mipi_dsi_device *dsi;
+	struct i2c_client *i2c;
+};
+
+static const struct drm_display_mode rpi_touchscreen_modes[] = {
+	{
+		/* Modeline comes from the Raspberry Pi firmware, with HFP=1
+		 * plugged in and clock re-computed from that.
+		 */
+		.clock = 25979400 / 1000,
+		.hdisplay = 800,
+		.hsync_start = 800 + 1,
+		.hsync_end = 800 + 1 + 2,
+		.htotal = 800 + 1 + 2 + 46,
+		.vdisplay = 480,
+		.vsync_start = 480 + 7,
+		.vsync_end = 480 + 7 + 2,
+		.vtotal = 480 + 7 + 2 + 21,
+		.vrefresh = 60,
+	},
+};
+
+static struct rpi_touchscreen *panel_to_ts(struct drm_panel *panel)
+{
+	return container_of(panel, struct rpi_touchscreen, base);
+}
+
+static u8 rpi_touchscreen_i2c_read(struct rpi_touchscreen *ts, u8 reg)
+{
+	return i2c_smbus_read_byte_data(ts->i2c, reg);
+}
+
+static void rpi_touchscreen_i2c_write(struct rpi_touchscreen *ts,
+				      u8 reg, u8 val)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(ts->i2c, reg, val);
+	if (ret)
+		dev_err(&ts->dsi->dev, "I2C write failed: %d\n", ret);
+}
+
+static int rpi_touchscreen_write(struct rpi_touchscreen *ts, u16 reg, u32 val)
+{
+#if 0
+	/* The firmware uses LP DSI transactions like this to bring up
+	 * the hardware, which should be faster than using I2C to then
+	 * pass to the Toshiba.  However, I was unable to get it to
+	 * work.
+	 */
+	u8 msg[] = {
+		reg,
+		reg >> 8,
+		val,
+		val >> 8,
+		val >> 16,
+		val >> 24,
+	};
+
+	mipi_dsi_dcs_write_buffer(ts->dsi, msg, sizeof(msg));
+#else
+	rpi_touchscreen_i2c_write(ts, REG_WR_ADDRH, reg >> 8);
+	rpi_touchscreen_i2c_write(ts, REG_WR_ADDRL, reg);
+	rpi_touchscreen_i2c_write(ts, REG_WRITEH, val >> 8);
+	rpi_touchscreen_i2c_write(ts, REG_WRITEL, val);
+#endif
+
+	return 0;
+}
+
+static int rpi_touchscreen_disable(struct drm_panel *panel)
+{
+	struct rpi_touchscreen *ts = panel_to_ts(panel);
+
+	rpi_touchscreen_i2c_write(ts, REG_PWM, 0);
+
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+	udelay(1);
+
+	return 0;
+}
+
+static int rpi_touchscreen_noop(struct drm_panel *panel)
+{
+	return 0;
+}
+
+static int rpi_touchscreen_enable(struct drm_panel *panel)
+{
+	struct rpi_touchscreen *ts = panel_to_ts(panel);
+	int i;
+
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 1);
+	/* Wait for nPWRDWN to go low to indicate poweron is done. */
+	for (i = 0; i < 100; i++) {
+		if (rpi_touchscreen_i2c_read(ts, REG_PORTB) & 1)
+			break;
+	}
+
+	rpi_touchscreen_write(ts, DSI_LANEENABLE,
+			      DSI_LANEENABLE_CLOCK |
+			      DSI_LANEENABLE_D0);
+	rpi_touchscreen_write(ts, PPI_D0S_CLRSIPOCOUNT, 0x05);
+	rpi_touchscreen_write(ts, PPI_D1S_CLRSIPOCOUNT, 0x05);
+	rpi_touchscreen_write(ts, PPI_D0S_ATMR, 0x00);
+	rpi_touchscreen_write(ts, PPI_D1S_ATMR, 0x00);
+	rpi_touchscreen_write(ts, PPI_LPTXTIMECNT, 0x03);
+
+	rpi_touchscreen_write(ts, SPICMR, 0x00);
+	rpi_touchscreen_write(ts, LCDCTRL, 0x00100150);
+	rpi_touchscreen_write(ts, SYSCTRL, 0x040f);
+	msleep(100);
+
+	rpi_touchscreen_write(ts, PPI_STARTPPI, 0x01);
+	rpi_touchscreen_write(ts, DSI_STARTDSI, 0x01);
+	msleep(100);
+
+	/* Turn on the backlight. */
+	rpi_touchscreen_i2c_write(ts, REG_PWM, 255);
+
+	/* Default to the same orientation as the closed source
+	 * firmware used for the panel.  Runtime rotation
+	 * configuration will be supported using VC4's plane
+	 * orientation bits.
+	 */
+	rpi_touchscreen_i2c_write(ts, REG_PORTA, BIT(2));
+
+	return 0;
+}
+
+static int rpi_touchscreen_get_modes(struct drm_panel *panel)
+{
+	struct drm_connector *connector = panel->connector;
+	struct drm_device *drm = panel->drm;
+	unsigned int i, num = 0;
+	static const u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+
+	for (i = 0; i < ARRAY_SIZE(rpi_touchscreen_modes); i++) {
+		const struct drm_display_mode *m = &rpi_touchscreen_modes[i];
+		struct drm_display_mode *mode;
+
+		mode = drm_mode_duplicate(drm, m);
+		if (!mode) {
+			dev_err(drm->dev, "failed to add mode %ux%u@%u\n",
+				m->hdisplay, m->vdisplay, m->vrefresh);
+			continue;
+		}
+
+		mode->type |= DRM_MODE_TYPE_DRIVER;
+
+		if (i == 0)
+			mode->type |= DRM_MODE_TYPE_PREFERRED;
+
+		drm_mode_set_name(mode);
+
+		drm_mode_probed_add(connector, mode);
+		num++;
+	}
+
+	connector->display_info.bpc = 8;
+	connector->display_info.width_mm = 154;
+	connector->display_info.height_mm = 86;
+	drm_display_info_set_bus_formats(&connector->display_info,
+					 &bus_format, 1);
+
+	return num;
+}
+
+static const struct drm_panel_funcs rpi_touchscreen_funcs = {
+	.disable = rpi_touchscreen_disable,
+	.unprepare = rpi_touchscreen_noop,
+	.prepare = rpi_touchscreen_noop,
+	.enable = rpi_touchscreen_enable,
+	.get_modes = rpi_touchscreen_get_modes,
+};
+
+static int rpi_touchscreen_probe(struct i2c_client *i2c,
+				 const struct i2c_device_id *id)
+{
+	struct device *dev = &i2c->dev;
+	struct rpi_touchscreen *ts;
+	struct device_node *endpoint;
+	int ret, ver;
+	struct mipi_dsi_device_info info = {
+		.type = RPI_DSI_DRIVER_NAME,
+		.channel = 0,
+		.node = NULL,
+	};
+
+	ts = devm_kzalloc(dev, sizeof(*ts), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, ts);
+
+	ts->i2c = i2c;
+
+	ver = rpi_touchscreen_i2c_read(ts, REG_ID);
+	if (ver < 0) {
+		dev_err(dev, "Atmel I2C read failed: %d\n", ver);
+		return -ENODEV;
+	}
+
+	switch (ver) {
+	case 0xde: /* ver 1 */
+	case 0xc3: /* ver 2 */
+		break;
+	default:
+		dev_err(dev, "Unknown Atmel firmware revision: 0x%02x\n", ver);
+		return -ENODEV;
+	}
+
+	/* Turn off at boot, so we can cleanly sequence powering on. */
+	rpi_touchscreen_i2c_write(ts, REG_POWERON, 0);
+
+	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
+	info.node = of_graph_get_remote_port(endpoint);
+	of_node_put(endpoint);
+
+	ts->dsi = mipi_dsi_device_register_full(NULL, &info);
+	if (IS_ERR(ts->dsi)) {
+		dev_err(dev, "DSI device registration failed: %ld\n",
+			PTR_ERR(ts->dsi));
+		return PTR_ERR(ts->dsi);
+	}
+
+	ts->dsi->mode_flags = (MIPI_DSI_MODE_VIDEO |
+			       MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+			       MIPI_DSI_MODE_LPM);
+	ts->dsi->format = MIPI_DSI_FMT_RGB888;
+	ts->dsi->lanes = 1;
+
+	ts->base.dev = dev;
+	ts->base.funcs = &rpi_touchscreen_funcs;
+
+	/* This appears last, as it's what will unblock the DSI host
+	 * driver's probe function and start the attach process.
+	 */
+	ret = drm_panel_add(&ts->base);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rpi_touchscreen_remove(struct i2c_client *i2c)
+{
+	struct rpi_touchscreen *ts = i2c_get_clientdata(i2c);
+
+	mipi_dsi_detach(ts->dsi);
+
+	drm_panel_remove(&ts->base);
+
+	mipi_dsi_device_unregister(ts->dsi);
+	kfree(ts->dsi);
+
+	return 0;
+}
+
+static int rpi_touchscreen_dsi_probe(struct mipi_dsi_device *dsi)
+{
+	int ret = mipi_dsi_attach(dsi);
+
+	if (ret)
+		dev_err(&dsi->dev, "failed to attach dsi to host: %d\n", ret);
+
+	return ret;
+}
+
+static struct mipi_dsi_driver rpi_touchscreen_dsi_driver = {
+	.driver.name = RPI_DSI_DRIVER_NAME,
+	.probe = rpi_touchscreen_dsi_probe,
+};
+
+static const struct of_device_id rpi_touchscreen_of_ids[] = {
+	{ .compatible = "raspberrypi,7inch-touchscreen-panel" },
+	{ } /* sentinel */
+};
+MODULE_DEVICE_TABLE(of, rpi_touchscreen_of_ids);
+
+static struct i2c_driver rpi_touchscreen_driver = {
+	.driver = {
+		.name = "rpi_touchscreen",
+		.of_match_table = rpi_touchscreen_of_ids,
+	},
+	.probe = rpi_touchscreen_probe,
+	.remove = rpi_touchscreen_remove,
+};
+
+static int __init rpi_touchscreen_init(void)
+{
+	mipi_dsi_driver_register(&rpi_touchscreen_dsi_driver);
+	return i2c_add_driver(&rpi_touchscreen_driver);
+}
+module_init(rpi_touchscreen_init);
+
+static void __exit rpi_touchscreen_exit(void)
+{
+	i2c_del_driver(&rpi_touchscreen_driver);
+	mipi_dsi_driver_unregister(&rpi_touchscreen_dsi_driver);
+}
+module_exit(rpi_touchscreen_exit);
+
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi 7-inch touchscreen driver");
+MODULE_LICENSE("GPL v2");
-- 
2.11.0

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

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Philippe CORNU]

On 07/18/2017 11:05 PM, Eric Anholt wrote:
> This will let drivers reduce the error cleanup they need, in
> particular the "is_panel_bridge" flag.
> 
> v2: Slight cleanup of remove function by Andrzej
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Reviewed-by: Philippe Cornu <philippe.cornu@st.com>
Tested-by: Philippe Cornu <philippe.cornu@st.com>

> ---
>   drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
>   include/drm/drm_bridge.h       |  3 +++
>   2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 685c1a480201..292ee92a9c97 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -195,3 +195,33 @@ void drm_panel_bridge_remove(struct drm_bridge *bridge)
>   	devm_kfree(panel_bridge->panel->dev, bridge);
>   }
>   EXPORT_SYMBOL(drm_panel_bridge_remove);
> +
> +static void devm_drm_panel_bridge_release(struct device *dev, void *res)
> +{
> +	struct drm_bridge **bridge = res;
> +
> +	drm_panel_bridge_remove(*bridge);
> +}
> +
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> +					     struct drm_panel *panel,
> +					     u32 connector_type)
> +{
> +	struct drm_bridge **ptr, *bridge;
> +
> +	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	bridge = drm_panel_bridge_add(panel, connector_type);
> +	if (!IS_ERR(bridge)) {
> +		*ptr = bridge;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return bridge;
> +}
> +EXPORT_SYMBOL(devm_drm_panel_bridge_add);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 1dc94d5392e2..6522d4cbc9d9 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -268,6 +268,9 @@ void drm_bridge_enable(struct drm_bridge *bridge);
>   struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>   					u32 connector_type);
>   void drm_panel_bridge_remove(struct drm_bridge *bridge);
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> +					     struct drm_panel *panel,
> +					     u32 connector_type);
>   #endif
>   
>   #endif
> 

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Philippe CORNU]

On 07/18/2017 11:05 PM, Eric Anholt wrote:
> This will let drivers reduce the error cleanup they need, in
> particular the "is_panel_bridge" flag.
> 
> v2: Slight cleanup of remove function by Andrzej
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

Reviewed-by: Philippe Cornu <philippe.cornu@st.com>
Tested-by: Philippe Cornu <philippe.cornu@st.com>

> ---
>   drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
>   include/drm/drm_bridge.h       |  3 +++
>   2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 685c1a480201..292ee92a9c97 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -195,3 +195,33 @@ void drm_panel_bridge_remove(struct drm_bridge *bridge)
>   	devm_kfree(panel_bridge->panel->dev, bridge);
>   }
>   EXPORT_SYMBOL(drm_panel_bridge_remove);
> +
> +static void devm_drm_panel_bridge_release(struct device *dev, void *res)
> +{
> +	struct drm_bridge **bridge = res;
> +
> +	drm_panel_bridge_remove(*bridge);
> +}
> +
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> +					     struct drm_panel *panel,
> +					     u32 connector_type)
> +{
> +	struct drm_bridge **ptr, *bridge;
> +
> +	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	bridge = drm_panel_bridge_add(panel, connector_type);
> +	if (!IS_ERR(bridge)) {
> +		*ptr = bridge;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return bridge;
> +}
> +EXPORT_SYMBOL(devm_drm_panel_bridge_add);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 1dc94d5392e2..6522d4cbc9d9 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -268,6 +268,9 @@ void drm_bridge_enable(struct drm_bridge *bridge);
>   struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>   					u32 connector_type);
>   void drm_panel_bridge_remove(struct drm_bridge *bridge);
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> +					     struct drm_panel *panel,
> +					     u32 connector_type);
>   #endif
>   
>   #endif
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 4/6] drm: Allow DSI devices to be registered before the host registers.

[Eric Anholt]

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

Eric Anholt <eric@anholt.net> writes:

> When a mipi_dsi_host is registered, the DT is walked to find any child
> nodes with compatible strings.  Those get registered as DSI devices,
> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>
> There is one special case currently, the adv7533 bridge, where the
> bridge probes on I2C, and during the bridge attach step it looks up
> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
> mipi_dsi_driver).
>
> For the Raspberry Pi panel, though, we also need to attach on I2C (our
> control bus), but don't have a bridge driver.  The lack of a bridge's
> attach() step like adv7533 uses means that we aren't able to delay the
> mipi_dsi_device creation until the mipi_dsi_host is present.
>
> To fix this, we extend mipi_dsi_device_register_full() to allow being
> called with a NULL host, which puts the device on a queue waiting for
> a host to appear.  When a new host is registered, we fill in the host
> value and finish the device creation process.
>
> v2: Cleanly error out if one attempts to mipi_dsi_attach() without a
>     host.  Fix handling of DSI with multiple devices, and OF node
>     refcounting.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 63 ++++++++++++++++++++++++++++++++++--------
>  include/drm/drm_mipi_dsi.h     |  3 ++
>  2 files changed, 54 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 1160a579e0dc..77d439254da6 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c

> @@ -295,6 +308,27 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>  
>  	mutex_lock(&host_lock);
>  	list_add_tail(&host->list, &host_list);
> +
> +	/* If any DSI devices were registered under our OF node, then
> +	 * connect our host to it and probe them now.
> +	 */
> +	list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
> +		struct device_node *host_node = of_get_parent(dsi->dev.of_node);
> +
> +		if (!of_node_cmp(host_node->name, "ports"))
> +			host_node = of_get_next_parent(host_node);
> +
> +		if (host_node == host->dev->of_node) {
> +			dsi->host = host;
> +			dsi->dev.parent = host->dev;
> +			device_initialize(&dsi->dev);
> +
> +			mipi_dsi_device_add(dsi);
> +			list_del_init(&dsi->list);
> +		}
> +
> +		of_node_put(host_node);
> +	}

Kbuild test caught that of_get_next_parent() isn't defined for
!CONFIG_OF, so I've put this loop under IS_ENABLED(CONFIG_OF) for v3.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v5 4/6] drm: Allow DSI devices to be registered before the host registers.

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2621 bytes --]

Eric Anholt <eric@anholt.net> writes:

> When a mipi_dsi_host is registered, the DT is walked to find any child
> nodes with compatible strings.  Those get registered as DSI devices,
> and most DSI panel drivers are mipi_dsi_drivers that attach to those nodes.
>
> There is one special case currently, the adv7533 bridge, where the
> bridge probes on I2C, and during the bridge attach step it looks up
> the mipi_dsi_host and registers the mipi_dsi_device (for its own stub
> mipi_dsi_driver).
>
> For the Raspberry Pi panel, though, we also need to attach on I2C (our
> control bus), but don't have a bridge driver.  The lack of a bridge's
> attach() step like adv7533 uses means that we aren't able to delay the
> mipi_dsi_device creation until the mipi_dsi_host is present.
>
> To fix this, we extend mipi_dsi_device_register_full() to allow being
> called with a NULL host, which puts the device on a queue waiting for
> a host to appear.  When a new host is registered, we fill in the host
> value and finish the device creation process.
>
> v2: Cleanly error out if one attempts to mipi_dsi_attach() without a
>     host.  Fix handling of DSI with multiple devices, and OF node
>     refcounting.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c | 63 ++++++++++++++++++++++++++++++++++--------
>  include/drm/drm_mipi_dsi.h     |  3 ++
>  2 files changed, 54 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 1160a579e0dc..77d439254da6 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c

> @@ -295,6 +308,27 @@ int mipi_dsi_host_register(struct mipi_dsi_host *host)
>  
>  	mutex_lock(&host_lock);
>  	list_add_tail(&host->list, &host_list);
> +
> +	/* If any DSI devices were registered under our OF node, then
> +	 * connect our host to it and probe them now.
> +	 */
> +	list_for_each_entry_safe(dsi, temp, &unattached_device_list, list) {
> +		struct device_node *host_node = of_get_parent(dsi->dev.of_node);
> +
> +		if (!of_node_cmp(host_node->name, "ports"))
> +			host_node = of_get_next_parent(host_node);
> +
> +		if (host_node == host->dev->of_node) {
> +			dsi->host = host;
> +			dsi->dev.parent = host->dev;
> +			device_initialize(&dsi->dev);
> +
> +			mipi_dsi_device_add(dsi);
> +			list_del_init(&dsi->list);
> +		}
> +
> +		of_node_put(host_node);
> +	}

Kbuild test caught that of_get_next_parent() isn't defined for
!CONFIG_OF, so I've put this loop under IS_ENABLED(CONFIG_OF) for v3.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Eric Anholt]

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

Philippe CORNU <philippe.cornu@st.com> writes:

> On 07/18/2017 11:05 PM, Eric Anholt wrote:
>> This will let drivers reduce the error cleanup they need, in
>> particular the "is_panel_bridge" flag.
>> 
>> v2: Slight cleanup of remove function by Andrzej
>> 
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>
> Reviewed-by: Philippe Cornu <philippe.cornu@st.com>
> Tested-by: Philippe Cornu <philippe.cornu@st.com>

I've pushed this patch from the series, but hopefully I can get some
review on the rest.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 573 bytes --]

Philippe CORNU <philippe.cornu@st.com> writes:

> On 07/18/2017 11:05 PM, Eric Anholt wrote:
>> This will let drivers reduce the error cleanup they need, in
>> particular the "is_panel_bridge" flag.
>> 
>> v2: Slight cleanup of remove function by Andrzej
>> 
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
>
> Reviewed-by: Philippe Cornu <philippe.cornu@st.com>
> Tested-by: Philippe Cornu <philippe.cornu@st.com>

I've pushed this patch from the series, but hopefully I can get some
review on the rest.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v5 1/6] drm/vc4: Avoid using vrefresh==0 mode in DSI htotal math.

[Boris Brezillon]

On Tue, 18 Jul 2017 14:05:05 -0700
Eric Anholt <eric@anholt.net> wrote:

> The incoming mode might have a missing vrefresh field if it came from
> drmModeSetCrtc(), which the kernel is supposed to calculate using
> drm_mode_vrefresh().  We could either use that or the adjusted_mode's
> original vrefresh value.
> 
> However, we can maintain a more exact vrefresh value (not just the
> integer approximation), by scaling by the ratio of our clocks.
> 
> v2: Use math suggested by Andrzej Hajda instead.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/vc4/vc4_dsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 629d372633e6..57213f4e3c72 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -866,7 +866,8 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
>  	adjusted_mode->clock = pixel_clock_hz / 1000 + 1;
>  
>  	/* Given the new pixel clock, adjust HFP to keep vrefresh the same. */
> -	adjusted_mode->htotal = pixel_clock_hz / (mode->vrefresh * mode->vtotal);
> +	adjusted_mode->htotal = (pixel_clock_hz / 1000 * mode->htotal /
> +				 mode->clock);

Hm, I'm not sure I understand this. Shouldn't we have something like:

	adjusted_mode->htotal = (adjusted_mode->clock * mode->htotal) /
				mode->clock;

Is there a reason for doing '+ 1' when you calculate the adjusted
pixel clock rate but not here?

>  	adjusted_mode->hsync_end += adjusted_mode->htotal - mode->htotal;
>  	adjusted_mode->hsync_start += adjusted_mode->htotal - mode->htotal;
>  

Re: [PATCH v5 3/6] drm/vc4: Delay DSI host registration until the panel has probed.

[Boris Brezillon]

On Tue, 18 Jul 2017 14:05:07 -0700
Eric Anholt <eric@anholt.net> wrote:

> The vc4 driver was unusual in that it was delaying the panel lookup
> until the attach step, while most DSI hosts will -EPROBE_DEFER until
> they get a panel.
> 
> v2: Drop a debug message that slipped in.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> (v1)

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
>  drivers/gpu/drm/vc4/vc4_dsi.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> index 57213f4e3c72..769f247c52a9 100644
> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> @@ -33,6 +33,7 @@
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>  #include <linux/clk.h>
>  #include <linux/clk-provider.h>
> @@ -504,7 +505,6 @@ struct vc4_dsi {
>  	struct mipi_dsi_host dsi_host;
>  	struct drm_encoder *encoder;
>  	struct drm_bridge *bridge;
> -	bool is_panel_bridge;
>  
>  	void __iomem *regs;
>  
> @@ -1289,7 +1289,6 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
>  			       struct mipi_dsi_device *device)
>  {
>  	struct vc4_dsi *dsi = host_to_dsi(host);
> -	int ret = 0;
>  
>  	dsi->lanes = device->lanes;
>  	dsi->channel = device->channel;
> @@ -1324,34 +1323,12 @@ static int vc4_dsi_host_attach(struct mipi_dsi_host *host,
>  		return 0;
>  	}
>  
> -	dsi->bridge = of_drm_find_bridge(device->dev.of_node);
> -	if (!dsi->bridge) {
> -		struct drm_panel *panel =
> -			of_drm_find_panel(device->dev.of_node);
> -
> -		dsi->bridge = drm_panel_bridge_add(panel,
> -						   DRM_MODE_CONNECTOR_DSI);
> -		if (IS_ERR(dsi->bridge)) {
> -			ret = PTR_ERR(dsi->bridge);
> -			dsi->bridge = NULL;
> -			return ret;
> -		}
> -		dsi->is_panel_bridge = true;
> -	}
> -
>  	return drm_bridge_attach(dsi->encoder, dsi->bridge, NULL);
>  }
>  
>  static int vc4_dsi_host_detach(struct mipi_dsi_host *host,
>  			       struct mipi_dsi_device *device)
>  {
> -	struct vc4_dsi *dsi = host_to_dsi(host);
> -
> -	if (dsi->is_panel_bridge) {
> -		drm_panel_bridge_remove(dsi->bridge);
> -		dsi->bridge = NULL;
> -	}
> -
>  	return 0;
>  }
>  
> @@ -1495,6 +1472,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>  	struct vc4_dev *vc4 = to_vc4_dev(drm);
>  	struct vc4_dsi *dsi;
>  	struct vc4_dsi_encoder *vc4_dsi_encoder;
> +	struct drm_panel *panel;
>  	const struct of_device_id *match;
>  	dma_cap_mask_t dma_mask;
>  	int ret;
> @@ -1598,6 +1576,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
>  		return ret;
>  	}
>  
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> +					  &panel, &dsi->bridge);
> +	if (ret)
> +		return ret;
> +
> +	if (panel) {
> +		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
> +							DRM_MODE_CONNECTOR_DSI);
> +		if (IS_ERR(dsi->bridge))
> +			return PTR_ERR(dsi->bridge);
> +	}
> +
>  	/* The esc clock rate is supposed to always be 100Mhz. */
>  	ret = clk_set_rate(dsi->escape_clock, 100 * 1000000);
>  	if (ret) {

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Laurent Pinchart]

Hi Eric,

(CC'ing Daniel)

Thank you for the patch.

On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> This will let drivers reduce the error cleanup they need, in
> particular the "is_panel_bridge" flag.
> 
> v2: Slight cleanup of remove function by Andrzej

I just want to point out that, in the context of Daniel's work on hot-unplug, 
90% of the devm_* allocations are wrong and will get in the way. All DRM core 
objects that are accessible one way or another from userspace will need to be 
properly reference-counted and freed only when the last reference disappears, 
which could be well after the corresponding device is removed. I believe this 
could be one such objects :-/

> Signed-off-by: Eric Anholt <eric@anholt.net>
> Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
>  drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
>  include/drm/drm_bridge.h       |  3 +++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 685c1a480201..292ee92a9c97 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -195,3 +195,33 @@ void drm_panel_bridge_remove(struct drm_bridge *bridge)
> devm_kfree(panel_bridge->panel->dev, bridge);
>  }
>  EXPORT_SYMBOL(drm_panel_bridge_remove);
> +
> +static void devm_drm_panel_bridge_release(struct device *dev, void *res)
> +{
> +	struct drm_bridge **bridge = res;
> +
> +	drm_panel_bridge_remove(*bridge);
> +}
> +
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> +					     struct drm_panel *panel,
> +					     u32 connector_type)
> +{
> +	struct drm_bridge **ptr, *bridge;
> +
> +	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
> +			   GFP_KERNEL);
> +	if (!ptr)
> +		return ERR_PTR(-ENOMEM);
> +
> +	bridge = drm_panel_bridge_add(panel, connector_type);
> +	if (!IS_ERR(bridge)) {
> +		*ptr = bridge;
> +		devres_add(dev, ptr);
> +	} else {
> +		devres_free(ptr);
> +	}
> +
> +	return bridge;
> +}
> +EXPORT_SYMBOL(devm_drm_panel_bridge_add);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 1dc94d5392e2..6522d4cbc9d9 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -268,6 +268,9 @@ void drm_bridge_enable(struct drm_bridge *bridge);
>  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
>  					u32 connector_type);
>  void drm_panel_bridge_remove(struct drm_bridge *bridge);
> +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> +					     struct drm_panel *panel,
> +					     u32 connector_type);
>  #endif
> 
>  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Laurent Pinchart]

Now CC'ing Daniel with his correct address :-/ I'll blame auto-completion in 
my e-mail client. Sorry for the noise.

On Friday 04 Aug 2017 16:46:24 Laurent Pinchart wrote:
> Hi Eric,
> 
> (CC'ing Daniel)
> 
> Thank you for the patch.
> 
> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> > This will let drivers reduce the error cleanup they need, in
> > particular the "is_panel_bridge" flag.
> > 
> > v2: Slight cleanup of remove function by Andrzej
> 
> I just want to point out that, in the context of Daniel's work on
> hot-unplug, 90% of the devm_* allocations are wrong and will get in the
> way. All DRM core objects that are accessible one way or another from
> userspace will need to be properly reference-counted and freed only when
> the last reference disappears, which could be well after the corresponding
> device is removed. I believe this could be one such objects :-/
> 
> > Signed-off-by: Eric Anholt <eric@anholt.net>
> > Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>
> > ---
> > 
> >  drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
> >  include/drm/drm_bridge.h       |  3 +++
> >  2 files changed, 33 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/bridge/panel.c
> > b/drivers/gpu/drm/bridge/panel.c index 685c1a480201..292ee92a9c97 100644
> > --- a/drivers/gpu/drm/bridge/panel.c
> > +++ b/drivers/gpu/drm/bridge/panel.c
> > @@ -195,3 +195,33 @@ void drm_panel_bridge_remove(struct drm_bridge
> > *bridge) devm_kfree(panel_bridge->panel->dev, bridge);
> > 
> >  }
> >  EXPORT_SYMBOL(drm_panel_bridge_remove);
> > 
> > +
> > +static void devm_drm_panel_bridge_release(struct device *dev, void *res)
> > +{
> > +	struct drm_bridge **bridge = res;
> > +
> > +	drm_panel_bridge_remove(*bridge);
> > +}
> > +
> > +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> > +					     struct drm_panel *panel,
> > +					     u32 connector_type)
> > +{
> > +	struct drm_bridge **ptr, *bridge;
> > +
> > +	ptr = devres_alloc(devm_drm_panel_bridge_release, sizeof(*ptr),
> > +			   GFP_KERNEL);
> > +	if (!ptr)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	bridge = drm_panel_bridge_add(panel, connector_type);
> > +	if (!IS_ERR(bridge)) {
> > +		*ptr = bridge;
> > +		devres_add(dev, ptr);
> > +	} else {
> > +		devres_free(ptr);
> > +	}
> > +
> > +	return bridge;
> > +}
> > +EXPORT_SYMBOL(devm_drm_panel_bridge_add);
> > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> > index 1dc94d5392e2..6522d4cbc9d9 100644
> > --- a/include/drm/drm_bridge.h
> > +++ b/include/drm/drm_bridge.h
> > @@ -268,6 +268,9 @@ void drm_bridge_enable(struct drm_bridge *bridge);
> > 
> >  struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel,
> >  
> >  					u32 connector_type);
> >  
> >  void drm_panel_bridge_remove(struct drm_bridge *bridge);
> > 
> > +struct drm_bridge *devm_drm_panel_bridge_add(struct device *dev,
> > +					     struct drm_panel *panel,
> > +					     u32 connector_type);
> > 
> >  #endif
> >  
> >  #endif

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Eric Anholt]

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

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hi Eric,
>
> (CC'ing Daniel)
>
> Thank you for the patch.
>
> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
>> This will let drivers reduce the error cleanup they need, in
>> particular the "is_panel_bridge" flag.
>> 
>> v2: Slight cleanup of remove function by Andrzej
>
> I just want to point out that, in the context of Daniel's work on hot-unplug, 
> 90% of the devm_* allocations are wrong and will get in the way. All DRM core 
> objects that are accessible one way or another from userspace will need to be 
> properly reference-counted and freed only when the last reference disappears, 
> which could be well after the corresponding device is removed. I believe this 
> could be one such objects :-/

Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
devices, like our SOC platform devices (current panel-bridge consumers),
this still seems like an excellent simplification of memory management.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v5 1/6] drm/vc4: Avoid using vrefresh==0 mode in DSI htotal math.

[Eric Anholt]

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

Boris Brezillon <boris.brezillon@free-electrons.com> writes:

> On Tue, 18 Jul 2017 14:05:05 -0700
> Eric Anholt <eric@anholt.net> wrote:
>
>> The incoming mode might have a missing vrefresh field if it came from
>> drmModeSetCrtc(), which the kernel is supposed to calculate using
>> drm_mode_vrefresh().  We could either use that or the adjusted_mode's
>> original vrefresh value.
>> 
>> However, we can maintain a more exact vrefresh value (not just the
>> integer approximation), by scaling by the ratio of our clocks.
>> 
>> v2: Use math suggested by Andrzej Hajda instead.
>> 
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>>  drivers/gpu/drm/vc4/vc4_dsi.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
>> index 629d372633e6..57213f4e3c72 100644
>> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
>> @@ -866,7 +866,8 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
>>  	adjusted_mode->clock = pixel_clock_hz / 1000 + 1;
>>  
>>  	/* Given the new pixel clock, adjust HFP to keep vrefresh the same. */
>> -	adjusted_mode->htotal = pixel_clock_hz / (mode->vrefresh * mode->vtotal);
>> +	adjusted_mode->htotal = (pixel_clock_hz / 1000 * mode->htotal /
>> +				 mode->clock);
>
> Hm, I'm not sure I understand this. Shouldn't we have something like:
>
> 	adjusted_mode->htotal = (adjusted_mode->clock * mode->htotal) /
> 				mode->clock;
>
> Is there a reason for doing '+ 1' when you calculate the adjusted
> pixel clock rate but not here?

We're actually expecting to get within epsilon of pixel_clock_hz, but we
have to bump our clk_set_rate() to a higher value because the clock
driver will give you a bad divider if you ask for anything less than the
rate it can provide.

How about I don't increment the adjusted_mode->clock (since it'll be
userspace visible I think), and instead move that and the "Round up"
comment to the clk_set_rate()?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v5 1/6] drm/vc4: Avoid using vrefresh==0 mode in DSI htotal math.

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 2051 bytes --]

Boris Brezillon <boris.brezillon@free-electrons.com> writes:

> On Tue, 18 Jul 2017 14:05:05 -0700
> Eric Anholt <eric@anholt.net> wrote:
>
>> The incoming mode might have a missing vrefresh field if it came from
>> drmModeSetCrtc(), which the kernel is supposed to calculate using
>> drm_mode_vrefresh().  We could either use that or the adjusted_mode's
>> original vrefresh value.
>> 
>> However, we can maintain a more exact vrefresh value (not just the
>> integer approximation), by scaling by the ratio of our clocks.
>> 
>> v2: Use math suggested by Andrzej Hajda instead.
>> 
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>>  drivers/gpu/drm/vc4/vc4_dsi.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
>> index 629d372633e6..57213f4e3c72 100644
>> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
>> @@ -866,7 +866,8 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
>>  	adjusted_mode->clock = pixel_clock_hz / 1000 + 1;
>>  
>>  	/* Given the new pixel clock, adjust HFP to keep vrefresh the same. */
>> -	adjusted_mode->htotal = pixel_clock_hz / (mode->vrefresh * mode->vtotal);
>> +	adjusted_mode->htotal = (pixel_clock_hz / 1000 * mode->htotal /
>> +				 mode->clock);
>
> Hm, I'm not sure I understand this. Shouldn't we have something like:
>
> 	adjusted_mode->htotal = (adjusted_mode->clock * mode->htotal) /
> 				mode->clock;
>
> Is there a reason for doing '+ 1' when you calculate the adjusted
> pixel clock rate but not here?

We're actually expecting to get within epsilon of pixel_clock_hz, but we
have to bump our clk_set_rate() to a higher value because the clock
driver will give you a bad divider if you ask for anything less than the
rate it can provide.

How about I don't increment the adjusted_mode->clock (since it'll be
userspace visible I think), and instead move that and the "Round up"
comment to the clk_set_rate()?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Laurent Pinchart]

Hi Eric,

On Friday 04 Aug 2017 13:43:25 Eric Anholt wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> >> This will let drivers reduce the error cleanup they need, in
> >> particular the "is_panel_bridge" flag.
> >> 
> >> v2: Slight cleanup of remove function by Andrzej
> > 
> > I just want to point out that, in the context of Daniel's work on
> > hot-unplug, 90% of the devm_* allocations are wrong and will get in the
> > way. All DRM core objects that are accessible one way or another from
> > userspace will need to be properly reference-counted and freed only when
> > the last reference disappears, which could be well after the
> > corresponding device is removed. I believe this could be one such objects
> > :-/
> 
> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
> devices, like our SOC platform devices (current panel-bridge consumers),
> this still seems like an excellent simplification of memory management.

It encourages driver writers to write code that they will have to fix later. 
That's certainly a simplification, but certainly not a good thing in my 
opinion :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Ilia Mirkin]

On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>
>> Hi Eric,
>>
>> (CC'ing Daniel)
>>
>> Thank you for the patch.
>>
>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
>>> This will let drivers reduce the error cleanup they need, in
>>> particular the "is_panel_bridge" flag.
>>>
>>> v2: Slight cleanup of remove function by Andrzej
>>
>> I just want to point out that, in the context of Daniel's work on hot-unplug,
>> 90% of the devm_* allocations are wrong and will get in the way. All DRM core
>> objects that are accessible one way or another from userspace will need to be
>> properly reference-counted and freed only when the last reference disappears,
>> which could be well after the corresponding device is removed. I believe this
>> could be one such objects :-/
>
> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
> devices, like our SOC platform devices (current panel-bridge consumers),
> this still seems like an excellent simplification of memory management.

At that point you may as well make your module non-unloadable, and
return failure when trying to remove a device from management by the
driver (whatever the opposite of "probe" is, I forget). Hotplugging
doesn't only happen when physically removing, it can happen for all
kinds of reasons... and userspace may still hold references in some of
those cases.

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Ilia Mirkin]

On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>
>> Hi Eric,
>>
>> (CC'ing Daniel)
>>
>> Thank you for the patch.
>>
>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
>>> This will let drivers reduce the error cleanup they need, in
>>> particular the "is_panel_bridge" flag.
>>>
>>> v2: Slight cleanup of remove function by Andrzej
>>
>> I just want to point out that, in the context of Daniel's work on hot-unplug,
>> 90% of the devm_* allocations are wrong and will get in the way. All DRM core
>> objects that are accessible one way or another from userspace will need to be
>> properly reference-counted and freed only when the last reference disappears,
>> which could be well after the corresponding device is removed. I believe this
>> could be one such objects :-/
>
> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
> devices, like our SOC platform devices (current panel-bridge consumers),
> this still seems like an excellent simplification of memory management.

At that point you may as well make your module non-unloadable, and
return failure when trying to remove a device from management by the
driver (whatever the opposite of "probe" is, I forget). Hotplugging
doesn't only happen when physically removing, it can happen for all
kinds of reasons... and userspace may still hold references in some of
those cases.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Noralf Trønnes]

(I had to switch to Daniel's Intel address to get this sent)

Den 05.08.2017 00.19, skrev Ilia Mirkin:
> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>>
>>> Hi Eric,
>>>
>>> (CC'ing Daniel)
>>>
>>> Thank you for the patch.
>>>
>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
>>>> This will let drivers reduce the error cleanup they need, in
>>>> particular the "is_panel_bridge" flag.
>>>>
>>>> v2: Slight cleanup of remove function by Andrzej
>>> I just want to point out that, in the context of Daniel's work on hot-unplug,
>>> 90% of the devm_* allocations are wrong and will get in the way. All DRM core
>>> objects that are accessible one way or another from userspace will need to be
>>> properly reference-counted and freed only when the last reference disappears,
>>> which could be well after the corresponding device is removed. I believe this
>>> could be one such objects :-/
>> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
>> devices, like our SOC platform devices (current panel-bridge consumers),
>> this still seems like an excellent simplification of memory management.
> At that point you may as well make your module non-unloadable, and
> return failure when trying to remove a device from management by the
> driver (whatever the opposite of "probe" is, I forget). Hotplugging
> doesn't only happen when physically removing, it can happen for all
> kinds of reasons... and userspace may still hold references in some of
> those cases.

If drm_open() gets a ref on dev->dev and puts it in drm_release(),
won't that delay devm_* cleanup until userspace is done?

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Noralf Trønnes]

(I had to switch to Daniel's Intel address to get this sent)

Den 05.08.2017 00.19, skrev Ilia Mirkin:
> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>>
>>> Hi Eric,
>>>
>>> (CC'ing Daniel)
>>>
>>> Thank you for the patch.
>>>
>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
>>>> This will let drivers reduce the error cleanup they need, in
>>>> particular the "is_panel_bridge" flag.
>>>>
>>>> v2: Slight cleanup of remove function by Andrzej
>>> I just want to point out that, in the context of Daniel's work on hot-unplug,
>>> 90% of the devm_* allocations are wrong and will get in the way. All DRM core
>>> objects that are accessible one way or another from userspace will need to be
>>> properly reference-counted and freed only when the last reference disappears,
>>> which could be well after the corresponding device is removed. I believe this
>>> could be one such objects :-/
>> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
>> devices, like our SOC platform devices (current panel-bridge consumers),
>> this still seems like an excellent simplification of memory management.
> At that point you may as well make your module non-unloadable, and
> return failure when trying to remove a device from management by the
> driver (whatever the opposite of "probe" is, I forget). Hotplugging
> doesn't only happen when physically removing, it can happen for all
> kinds of reasons... and userspace may still hold references in some of
> those cases.

If drm_open() gets a ref on dev->dev and puts it in drm_release(),
won't that delay devm_* cleanup until userspace is done?

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

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Noralf Trønnes]

Den 05.08.2017 12.59, skrev Noralf Trønnes:
> (I had to switch to Daniel's Intel address to get this sent)
>
> Den 05.08.2017 00.19, skrev Ilia Mirkin:
>> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>>>
>>>> Hi Eric,
>>>>
>>>> (CC'ing Daniel)
>>>>
>>>> Thank you for the patch.
>>>>
>>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
>>>>> This will let drivers reduce the error cleanup they need, in
>>>>> particular the "is_panel_bridge" flag.
>>>>>
>>>>> v2: Slight cleanup of remove function by Andrzej
>>>> I just want to point out that, in the context of Daniel's work on 
>>>> hot-unplug,
>>>> 90% of the devm_* allocations are wrong and will get in the way. 
>>>> All DRM core
>>>> objects that are accessible one way or another from userspace will 
>>>> need to be
>>>> properly reference-counted and freed only when the last reference 
>>>> disappears,
>>>> which could be well after the corresponding device is removed. I 
>>>> believe this
>>>> could be one such objects :-/
>>> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
>>> devices, like our SOC platform devices (current panel-bridge 
>>> consumers),
>>> this still seems like an excellent simplification of memory management.
>> At that point you may as well make your module non-unloadable, and
>> return failure when trying to remove a device from management by the
>> driver (whatever the opposite of "probe" is, I forget). Hotplugging
>> doesn't only happen when physically removing, it can happen for all
>> kinds of reasons... and userspace may still hold references in some of
>> those cases.
>
> If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> won't that delay devm_* cleanup until userspace is done?
>

It seems plausible looking at the code:

void device_initialize(struct device *dev)
{
[...]
     kobject_init(&dev->kobj, &device_ktype);
[...]
}

static struct kobj_type device_ktype = {
     .release    = device_release,
};

/**
  * device_release - free device structure.
  * @kobj: device's kobject.
  *
  * This is called once the reference count for the object
  * reaches 0. We forward the call to the device's release
  * method, which should handle actually freeing the structure.
  */
static void device_release(struct kobject *kobj)
{
[...]
     devres_release_all(dev);
[...]
}

Last put call chain:
put_device() -> kobject_put() -> kref_put() -> kobject_release() ->
kobject_cleanup() -> device_release() -> devres_release_all()

But I haven't actually tried it, so I might be mistaken.

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Daniel Vetter]

On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> (I had to switch to Daniel's Intel address to get this sent)
> 
> Den 05.08.2017 00.19, skrev Ilia Mirkin:
> > On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> > > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > > 
> > > > Hi Eric,
> > > > 
> > > > (CC'ing Daniel)
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> > > > > This will let drivers reduce the error cleanup they need, in
> > > > > particular the "is_panel_bridge" flag.
> > > > > 
> > > > > v2: Slight cleanup of remove function by Andrzej
> > > > I just want to point out that, in the context of Daniel's work on hot-unplug,
> > > > 90% of the devm_* allocations are wrong and will get in the way. All DRM core
> > > > objects that are accessible one way or another from userspace will need to be
> > > > properly reference-counted and freed only when the last reference disappears,
> > > > which could be well after the corresponding device is removed. I believe this
> > > > could be one such objects :-/
> > > Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
> > > devices, like our SOC platform devices (current panel-bridge consumers),
> > > this still seems like an excellent simplification of memory management.
> > At that point you may as well make your module non-unloadable, and
> > return failure when trying to remove a device from management by the
> > driver (whatever the opposite of "probe" is, I forget). Hotplugging
> > doesn't only happen when physically removing, it can happen for all
> > kinds of reasons... and userspace may still hold references in some of
> > those cases.
> 
> If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> won't that delay devm_* cleanup until userspace is done?

No. drm_device is the thing that is refcounted for userspace references
like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
don't).

devm_ otoh is tied to the lifetime of the underlying device, and that one
can get outlived by drm_device. Or at least afaiui, devm_ stuff is nuked
on unplug, and not when the final sw reference of the struct device
disappears.

Not sure tough, it's complicated.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Daniel Vetter]

On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> (I had to switch to Daniel's Intel address to get this sent)
> 
> Den 05.08.2017 00.19, skrev Ilia Mirkin:
> > On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> > > Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > > 
> > > > Hi Eric,
> > > > 
> > > > (CC'ing Daniel)
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> > > > > This will let drivers reduce the error cleanup they need, in
> > > > > particular the "is_panel_bridge" flag.
> > > > > 
> > > > > v2: Slight cleanup of remove function by Andrzej
> > > > I just want to point out that, in the context of Daniel's work on hot-unplug,
> > > > 90% of the devm_* allocations are wrong and will get in the way. All DRM core
> > > > objects that are accessible one way or another from userspace will need to be
> > > > properly reference-counted and freed only when the last reference disappears,
> > > > which could be well after the corresponding device is removed. I believe this
> > > > could be one such objects :-/
> > > Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
> > > devices, like our SOC platform devices (current panel-bridge consumers),
> > > this still seems like an excellent simplification of memory management.
> > At that point you may as well make your module non-unloadable, and
> > return failure when trying to remove a device from management by the
> > driver (whatever the opposite of "probe" is, I forget). Hotplugging
> > doesn't only happen when physically removing, it can happen for all
> > kinds of reasons... and userspace may still hold references in some of
> > those cases.
> 
> If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> won't that delay devm_* cleanup until userspace is done?

No. drm_device is the thing that is refcounted for userspace references
like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
don't).

devm_ otoh is tied to the lifetime of the underlying device, and that one
can get outlived by drm_device. Or at least afaiui, devm_ stuff is nuked
on unplug, and not when the final sw reference of the struct device
disappears.

Not sure tough, it's complicated.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Laurent Pinchart]

Hi Daniel,

On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:
> On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> > Den 05.08.2017 00.19, skrev Ilia Mirkin:
> >> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> >>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> >>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> >>>>> This will let drivers reduce the error cleanup they need, in
> >>>>> particular the "is_panel_bridge" flag.
> >>>>> 
> >>>>> v2: Slight cleanup of remove function by Andrzej
> >>>> 
> >>>> I just want to point out that, in the context of Daniel's work on
> >>>> hot-unplug, 90% of the devm_* allocations are wrong and will get in
> >>>> the way. All DRM core objects that are accessible one way or
> >>>> another from userspace will need to be properly reference-counted
> >>>> and freed only when the last reference disappears, which could be
> >>>> well after the corresponding device is removed. I believe this
> >>>> could be one such objects :-/
> >>> 
> >>> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
> >>> devices, like our SOC platform devices (current panel-bridge
> >>> consumers), this still seems like an excellent simplification of
> >>> memory management.
> >> 
> >> At that point you may as well make your module non-unloadable, and
> >> return failure when trying to remove a device from management by the
> >> driver (whatever the opposite of "probe" is, I forget). Hotplugging
> >> doesn't only happen when physically removing, it can happen for all
> >> kinds of reasons... and userspace may still hold references in some of
> >> those cases.
> > 
> > If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> > won't that delay devm_* cleanup until userspace is done?
> 
> No. drm_device is the thing that is refcounted for userspace references
> like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
> don't).
> 
> devm_ otoh is tied to the lifetime of the underlying device, and that one
> can get outlived by drm_device. Or at least afaiui, devm_ stuff is nuked
> on unplug, and not when the final sw reference of the struct device
> disappears.
> 
> Not sure tough, it's complicated.

It's complicated when you have to understand the behaviour by reading the 
code, but the behaviour isn't that complex. devm resources are released both

1. right after the driver's .remove() operation returns
2. when the device is deleted (last reference released)

It's the first one that makes devm_* allocation unsuitable for any structure 
that is accessible from userspace (such as in file operation handlers).

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Laurent Pinchart]

Hi Daniel,

On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:
> On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> > Den 05.08.2017 00.19, skrev Ilia Mirkin:
> >> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> >>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> >>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> >>>>> This will let drivers reduce the error cleanup they need, in
> >>>>> particular the "is_panel_bridge" flag.
> >>>>> 
> >>>>> v2: Slight cleanup of remove function by Andrzej
> >>>> 
> >>>> I just want to point out that, in the context of Daniel's work on
> >>>> hot-unplug, 90% of the devm_* allocations are wrong and will get in
> >>>> the way. All DRM core objects that are accessible one way or
> >>>> another from userspace will need to be properly reference-counted
> >>>> and freed only when the last reference disappears, which could be
> >>>> well after the corresponding device is removed. I believe this
> >>>> could be one such objects :-/
> >>> 
> >>> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
> >>> devices, like our SOC platform devices (current panel-bridge
> >>> consumers), this still seems like an excellent simplification of
> >>> memory management.
> >> 
> >> At that point you may as well make your module non-unloadable, and
> >> return failure when trying to remove a device from management by the
> >> driver (whatever the opposite of "probe" is, I forget). Hotplugging
> >> doesn't only happen when physically removing, it can happen for all
> >> kinds of reasons... and userspace may still hold references in some of
> >> those cases.
> > 
> > If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> > won't that delay devm_* cleanup until userspace is done?
> 
> No. drm_device is the thing that is refcounted for userspace references
> like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
> don't).
> 
> devm_ otoh is tied to the lifetime of the underlying device, and that one
> can get outlived by drm_device. Or at least afaiui, devm_ stuff is nuked
> on unplug, and not when the final sw reference of the struct device
> disappears.
> 
> Not sure tough, it's complicated.

It's complicated when you have to understand the behaviour by reading the 
code, but the behaviour isn't that complex. devm resources are released both

1. right after the driver's .remove() operation returns
2. when the device is deleted (last reference released)

It's the first one that makes devm_* allocation unsuitable for any structure 
that is accessible from userspace (such as in file operation handlers).

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Noralf Trønnes]

Den 07.08.2017 12.22, skrev Laurent Pinchart:
> Hi Daniel,
>
> On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:
>> On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
>>> Den 05.08.2017 00.19, skrev Ilia Mirkin:
>>>> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
>>>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
>>>>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
>>>>>>> This will let drivers reduce the error cleanup they need, in
>>>>>>> particular the "is_panel_bridge" flag.
>>>>>>>
>>>>>>> v2: Slight cleanup of remove function by Andrzej
>>>>>> I just want to point out that, in the context of Daniel's work on
>>>>>> hot-unplug, 90% of the devm_* allocations are wrong and will get in
>>>>>> the way. All DRM core objects that are accessible one way or
>>>>>> another from userspace will need to be properly reference-counted
>>>>>> and freed only when the last reference disappears, which could be
>>>>>> well after the corresponding device is removed. I believe this
>>>>>> could be one such objects :-/
>>>>> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
>>>>> devices, like our SOC platform devices (current panel-bridge
>>>>> consumers), this still seems like an excellent simplification of
>>>>> memory management.
>>>> At that point you may as well make your module non-unloadable, and
>>>> return failure when trying to remove a device from management by the
>>>> driver (whatever the opposite of "probe" is, I forget). Hotplugging
>>>> doesn't only happen when physically removing, it can happen for all
>>>> kinds of reasons... and userspace may still hold references in some of
>>>> those cases.
>>> If drm_open() gets a ref on dev->dev and puts it in drm_release(),
>>> won't that delay devm_* cleanup until userspace is done?
>> No. drm_device is the thing that is refcounted for userspace references
>> like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
>> don't).
>>
>> devm_ otoh is tied to the lifetime of the underlying device, and that one
>> can get outlived by drm_device. Or at least afaiui, devm_ stuff is nuked
>> on unplug, and not when the final sw reference of the struct device
>> disappears.
>>
>> Not sure tough, it's complicated.
> It's complicated when you have to understand the behaviour by reading the
> code, but the behaviour isn't that complex. devm resources are released both
>
> 1. right after the driver's .remove() operation returns
> 2. when the device is deleted (last reference released)
>
> It's the first one that makes devm_* allocation unsuitable for any structure
> that is accessible from userspace (such as in file operation handlers).
>

I see, when the device is removed, the driver is released. No help
holding a ref on struct device.
I did a test and this is the stack trace in devm_tinydrm_release():

[  129.433179] [<c0016148>] (unwind_backtrace) from [<c0013c90>] 
(show_stack+0x20/0x24)
[  129.433213] [<c0013c90>] (show_stack) from [<c0319e78>] 
(dump_stack+0x20/0x28)
[  129.433242] [<c0319e78>] (dump_stack) from [<c0021d18>] 
(__warn+0xe4/0x10c)
[  129.433264] [<c0021d18>] (__warn) from [<c0021e0c>] 
(warn_slowpath_null+0x30/0x38)
[  129.433324] [<c0021e0c>] (warn_slowpath_null) from [<bf2ca3cc>] 
(devm_tinydrm_release+0x24/0x48 [tinydrm])
[  129.433389] [<bf2ca3cc>] (devm_tinydrm_release [tinydrm]) from 
[<c03bf7f0>] (devm_action_release+0x1c/0x20)
[  129.433414] [<c03bf7f0>] (devm_action_release) from [<c03bfc70>] 
(release_nodes+0x188/0x21c)
[  129.433437] [<c03bfc70>] (release_nodes) from [<c03c076c>] 
(devres_release_all+0x44/0x64)
[  129.433461] [<c03c076c>] (devres_release_all) from [<c03bcdb8>] 
(__device_release_driver+0x9c/0x118)
[  129.433480] [<c03bcdb8>] (__device_release_driver) from [<c03bce60>] 
(device_release_driver+0x2c/0x38)
[  129.433499] [<c03bce60>] (device_release_driver) from [<c03bbe04>] 
(bus_remove_device+0xe4/0x114)
[  129.433532] [<c03bbe04>] (bus_remove_device) from [<c03b8980>] 
(device_del+0x118/0x22c)
[  129.433554] [<c03b8980>] (device_del) from [<c03b8ab0>] 
(device_unregister+0x1c/0x30)
[  129.433592] [<c03b8ab0>] (device_unregister) from [<c04062c0>] 
(spi_unregister_device+0x60/0x64)
[  129.433617] [<c04062c0>] (spi_unregister_device) from [<c0409438>] 
(of_spi_notify+0x70/0x164)
[  129.433642] [<c0409438>] (of_spi_notify) from [<c0040148>] 
(notifier_call_chain+0x54/0x94)
[  129.433664] [<c0040148>] (notifier_call_chain) from [<c00405dc>] 
(__blocking_notifier_call_chain+0x58/0x70)
[  129.433683] [<c00405dc>] (__blocking_notifier_call_chain) from 
[<c004061c>] (blocking_notifier_call_chain+0x28/0x30)
[  129.433721] [<c004061c>] (blocking_notifier_call_chain) from 
[<c04b7058>] (__of_changeset_entry_notify+0x90/0xe0)
[  129.433748] [<c04b7058>] (__of_changeset_entry_notify) from 
[<c04b7aec>] (__of_changeset_revert+0x70/0xcc)
[  129.433774] [<c04b7aec>] (__of_changeset_revert) from [<c04bb46c>] 
(of_overlay_destroy+0x10c/0x1bc)
[  129.433795] [<c04bb46c>] (of_overlay_destroy) from [<c04b6954>] 
(cfs_overlay_release+0x2c/0x50)
[  129.433832] [<c04b6954>] (cfs_overlay_release) from [<c01be83c>] 
(config_item_release+0x68/0x8c)
[  129.433859] [<c01be83c>] (config_item_release) from [<c01be7d0>] 
(config_item_put+0x44/0x48)
[  129.433879] [<c01be7d0>] (config_item_put) from [<c01bcf50>] 
(configfs_rmdir+0x190/0x220)
[  129.433902] [<c01bcf50>] (configfs_rmdir) from [<c0152204>] 
(vfs_rmdir+0x80/0x118)
[  129.433922] [<c0152204>] (vfs_rmdir) from [<c01547f8>] 
(do_rmdir+0x144/0x1a0)
[  129.433941] [<c01547f8>] (do_rmdir) from [<c01551f0>] 
(SyS_rmdir+0x20/0x24)
[  129.433966] [<c01551f0>] (SyS_rmdir) from [<c000fe40>] 
(ret_fast_syscall+0x0/0x1c)

This means that tinydrm won't work with usb devices.
I need to look into moving cleanup to drm_driver->cleanup.

Noralf.

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Daniel Vetter]

On Mon, Aug 07, 2017 at 01:22:23PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:
> > On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> > > Den 05.08.2017 00.19, skrev Ilia Mirkin:
> > >> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> > >>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > >>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> > >>>>> This will let drivers reduce the error cleanup they need, in
> > >>>>> particular the "is_panel_bridge" flag.
> > >>>>> 
> > >>>>> v2: Slight cleanup of remove function by Andrzej
> > >>>> 
> > >>>> I just want to point out that, in the context of Daniel's work on
> > >>>> hot-unplug, 90% of the devm_* allocations are wrong and will get in
> > >>>> the way. All DRM core objects that are accessible one way or
> > >>>> another from userspace will need to be properly reference-counted
> > >>>> and freed only when the last reference disappears, which could be
> > >>>> well after the corresponding device is removed. I believe this
> > >>>> could be one such objects :-/
> > >>> 
> > >>> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
> > >>> devices, like our SOC platform devices (current panel-bridge
> > >>> consumers), this still seems like an excellent simplification of
> > >>> memory management.
> > >> 
> > >> At that point you may as well make your module non-unloadable, and
> > >> return failure when trying to remove a device from management by the
> > >> driver (whatever the opposite of "probe" is, I forget). Hotplugging
> > >> doesn't only happen when physically removing, it can happen for all
> > >> kinds of reasons... and userspace may still hold references in some of
> > >> those cases.
> > > 
> > > If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> > > won't that delay devm_* cleanup until userspace is done?
> > 
> > No. drm_device is the thing that is refcounted for userspace references
> > like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
> > don't).
> > 
> > devm_ otoh is tied to the lifetime of the underlying device, and that one
> > can get outlived by drm_device. Or at least afaiui, devm_ stuff is nuked
> > on unplug, and not when the final sw reference of the struct device
> > disappears.
> > 
> > Not sure tough, it's complicated.
> 
> It's complicated when you have to understand the behaviour by reading the 
> code, but the behaviour isn't that complex. devm resources are released both
> 
> 1. right after the driver's .remove() operation returns
> 2. when the device is deleted (last reference released)

Right, I had vague memories when reading the code ... But iirc there's
also some way to delay cleanup until it's deleted, maybe we could exploit
that (and wrap it up into a drm_devm_add wrapper)?

Plan B, but that is a lot more ugly, would be to create a fake struct
device that we never bind (hence remove can't be called) and use to track
the lifetime of drm_device stuff. Would avoid re-inventing all the devm_
functions, but would also result in a dummy device in sysfs.

Why is this stuff tied to struct device and not struct kobject anyway. Or
at least something we could embed in random cool place (like drm_device).

Greg, any ideas how we could simplify management of stuff that's created
at driver load, but its lifetime tied to something which isn't directly a
struct device?

> It's the first one that makes devm_* allocation unsuitable for any structure 
> that is accessible from userspace (such as in file operation handlers).

Yeah, if we could release at 2. it would be perfect I think ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Daniel Vetter]

On Mon, Aug 07, 2017 at 01:22:23PM +0300, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:
> > On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> > > Den 05.08.2017 00.19, skrev Ilia Mirkin:
> > >> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> > >>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > >>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> > >>>>> This will let drivers reduce the error cleanup they need, in
> > >>>>> particular the "is_panel_bridge" flag.
> > >>>>> 
> > >>>>> v2: Slight cleanup of remove function by Andrzej
> > >>>> 
> > >>>> I just want to point out that, in the context of Daniel's work on
> > >>>> hot-unplug, 90% of the devm_* allocations are wrong and will get in
> > >>>> the way. All DRM core objects that are accessible one way or
> > >>>> another from userspace will need to be properly reference-counted
> > >>>> and freed only when the last reference disappears, which could be
> > >>>> well after the corresponding device is removed. I believe this
> > >>>> could be one such objects :-/
> > >>> 
> > >>> Sure, if you're hotplugging, your life is pain.  For non-hotpluggable
> > >>> devices, like our SOC platform devices (current panel-bridge
> > >>> consumers), this still seems like an excellent simplification of
> > >>> memory management.
> > >> 
> > >> At that point you may as well make your module non-unloadable, and
> > >> return failure when trying to remove a device from management by the
> > >> driver (whatever the opposite of "probe" is, I forget). Hotplugging
> > >> doesn't only happen when physically removing, it can happen for all
> > >> kinds of reasons... and userspace may still hold references in some of
> > >> those cases.
> > > 
> > > If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> > > won't that delay devm_* cleanup until userspace is done?
> > 
> > No. drm_device is the thing that is refcounted for userspace references
> > like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
> > don't).
> > 
> > devm_ otoh is tied to the lifetime of the underlying device, and that one
> > can get outlived by drm_device. Or at least afaiui, devm_ stuff is nuked
> > on unplug, and not when the final sw reference of the struct device
> > disappears.
> > 
> > Not sure tough, it's complicated.
> 
> It's complicated when you have to understand the behaviour by reading the 
> code, but the behaviour isn't that complex. devm resources are released both
> 
> 1. right after the driver's .remove() operation returns
> 2. when the device is deleted (last reference released)

Right, I had vague memories when reading the code ... But iirc there's
also some way to delay cleanup until it's deleted, maybe we could exploit
that (and wrap it up into a drm_devm_add wrapper)?

Plan B, but that is a lot more ugly, would be to create a fake struct
device that we never bind (hence remove can't be called) and use to track
the lifetime of drm_device stuff. Would avoid re-inventing all the devm_
functions, but would also result in a dummy device in sysfs.

Why is this stuff tied to struct device and not struct kobject anyway. Or
at least something we could embed in random cool place (like drm_device).

Greg, any ideas how we could simplify management of stuff that's created
at driver load, but its lifetime tied to something which isn't directly a
struct device?

> It's the first one that makes devm_* allocation unsuitable for any structure 
> that is accessible from userspace (such as in file operation handlers).

Yeah, if we could release at 2. it would be perfect I think ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Laurent Pinchart]

Hi Daniel,

On Monday 07 Aug 2017 16:59:39 Daniel Vetter wrote:
> On Mon, Aug 07, 2017 at 01:22:23PM +0300, Laurent Pinchart wrote:
> > On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:
> >> On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> >>> Den 05.08.2017 00.19, skrev Ilia Mirkin:
> >>>> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> >>>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> >>>>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> >>>>>>> This will let drivers reduce the error cleanup they need, in
> >>>>>>> particular the "is_panel_bridge" flag.
> >>>>>>> 
> >>>>>>> v2: Slight cleanup of remove function by Andrzej
> >>>>>> 
> >>>>>> I just want to point out that, in the context of Daniel's work on
> >>>>>> hot-unplug, 90% of the devm_* allocations are wrong and will get in
> >>>>>> the way. All DRM core objects that are accessible one way or
> >>>>>> another from userspace will need to be properly reference-counted
> >>>>>> and freed only when the last reference disappears, which could be
> >>>>>> well after the corresponding device is removed. I believe this
> >>>>>> could be one such objects :-/
> >>>>> 
> >>>>> Sure, if you're hotplugging, your life is pain.  For
> >>>>> non-hotpluggable devices, like our SOC platform devices (current
> >>>>> panel-bridge consumers), this still seems like an excellent
> >>>>> simplification of memory management.
> >>>> 
> >>>> At that point you may as well make your module non-unloadable, and
> >>>> return failure when trying to remove a device from management by the
> >>>> driver (whatever the opposite of "probe" is, I forget). Hotplugging
> >>>> doesn't only happen when physically removing, it can happen for all
> >>>> kinds of reasons... and userspace may still hold references in some
> >>>> of those cases.
> >>> 
> >>> If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> >>> won't that delay devm_* cleanup until userspace is done?
> >> 
> >> No. drm_device is the thing that is refcounted for userspace references
> >> like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
> >> don't).
> >> 
> >> devm_ otoh is tied to the lifetime of the underlying device, and that
> >> one can get outlived by drm_device. Or at least afaiui, devm_ stuff is
> >> nuked on unplug, and not when the final sw reference of the struct device
> >> disappears.
> >> 
> >> Not sure tough, it's complicated.
> > 
> > It's complicated when you have to understand the behaviour by reading the
> > code, but the behaviour isn't that complex. devm resources are released
> > both
> > 
> > 1. right after the driver's .remove() operation returns
> > 2. when the device is deleted (last reference released)
> 
> Right, I had vague memories when reading the code ... But iirc there's
> also some way to delay cleanup until it's deleted, maybe we could exploit
> that (and wrap it up into a drm_devm_add wrapper)?
> 
> Plan B, but that is a lot more ugly, would be to create a fake struct
> device that we never bind (hence remove can't be called) and use to track
> the lifetime of drm_device stuff. Would avoid re-inventing all the devm_
> functions, but would also result in a dummy device in sysfs.

If we wanted to go that way, we should decouple devm_* from struct device (or 
even struct kobject) and tie it to a new resource tracker structure. The 
current devm_* API would remain the same, embedding that resource tracker 
object in struct device, but we could also use the machinery with the resource 
tracker object directly.

However, I'm not convince we should do so. We don't have any automatic memory 
allocation system for DRM objects (such as framebuffers or planes). Instead, 
drivers must implement a destroy operation for the object, and the core calls 
it when the last reference to the object goes away. This works fine, and I 
don't see what would prevent use from implementing the same mechanism for 
bridges or other similar structures.

> Why is this stuff tied to struct device and not struct kobject anyway. Or
> at least something we could embed in random cool place (like drm_device).
> 
> Greg, any ideas how we could simplify management of stuff that's created
> at driver load, but its lifetime tied to something which isn't directly a
> struct device?
> 
> > It's the first one that makes devm_* allocation unsuitable for any
> > structure that is accessible from userspace (such as in file operation
> > handlers).
> 
> Yeah, if we could release at 2. it would be perfect I think ...

It wouldn't, as unbind/rebind cycles would allocate new objects each time 
without freeing the previous ones until the device is deleted.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 2/6] drm/bridge: Add a devm_ allocator for panel bridge.

[Laurent Pinchart]

Hi Daniel,

On Monday 07 Aug 2017 16:59:39 Daniel Vetter wrote:
> On Mon, Aug 07, 2017 at 01:22:23PM +0300, Laurent Pinchart wrote:
> > On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:
> >> On Sat, Aug 05, 2017 at 12:59:07PM +0200, Noralf Trønnes wrote:
> >>> Den 05.08.2017 00.19, skrev Ilia Mirkin:
> >>>> On Fri, Aug 4, 2017 at 4:43 PM, Eric Anholt <eric@anholt.net> wrote:
> >>>>> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> >>>>>> On Tuesday 18 Jul 2017 14:05:06 Eric Anholt wrote:
> >>>>>>> This will let drivers reduce the error cleanup they need, in
> >>>>>>> particular the "is_panel_bridge" flag.
> >>>>>>> 
> >>>>>>> v2: Slight cleanup of remove function by Andrzej
> >>>>>> 
> >>>>>> I just want to point out that, in the context of Daniel's work on
> >>>>>> hot-unplug, 90% of the devm_* allocations are wrong and will get in
> >>>>>> the way. All DRM core objects that are accessible one way or
> >>>>>> another from userspace will need to be properly reference-counted
> >>>>>> and freed only when the last reference disappears, which could be
> >>>>>> well after the corresponding device is removed. I believe this
> >>>>>> could be one such objects :-/
> >>>>> 
> >>>>> Sure, if you're hotplugging, your life is pain.  For
> >>>>> non-hotpluggable devices, like our SOC platform devices (current
> >>>>> panel-bridge consumers), this still seems like an excellent
> >>>>> simplification of memory management.
> >>>> 
> >>>> At that point you may as well make your module non-unloadable, and
> >>>> return failure when trying to remove a device from management by the
> >>>> driver (whatever the opposite of "probe" is, I forget). Hotplugging
> >>>> doesn't only happen when physically removing, it can happen for all
> >>>> kinds of reasons... and userspace may still hold references in some
> >>>> of those cases.
> >>> 
> >>> If drm_open() gets a ref on dev->dev and puts it in drm_release(),
> >>> won't that delay devm_* cleanup until userspace is done?
> >> 
> >> No. drm_device is the thing that is refcounted for userspace references
> >> like open FD (we're not perfect about it, e.g. sysfs and dma-buf/fence
> >> don't).
> >> 
> >> devm_ otoh is tied to the lifetime of the underlying device, and that
> >> one can get outlived by drm_device. Or at least afaiui, devm_ stuff is
> >> nuked on unplug, and not when the final sw reference of the struct device
> >> disappears.
> >> 
> >> Not sure tough, it's complicated.
> > 
> > It's complicated when you have to understand the behaviour by reading the
> > code, but the behaviour isn't that complex. devm resources are released
> > both
> > 
> > 1. right after the driver's .remove() operation returns
> > 2. when the device is deleted (last reference released)
> 
> Right, I had vague memories when reading the code ... But iirc there's
> also some way to delay cleanup until it's deleted, maybe we could exploit
> that (and wrap it up into a drm_devm_add wrapper)?
> 
> Plan B, but that is a lot more ugly, would be to create a fake struct
> device that we never bind (hence remove can't be called) and use to track
> the lifetime of drm_device stuff. Would avoid re-inventing all the devm_
> functions, but would also result in a dummy device in sysfs.

If we wanted to go that way, we should decouple devm_* from struct device (or 
even struct kobject) and tie it to a new resource tracker structure. The 
current devm_* API would remain the same, embedding that resource tracker 
object in struct device, but we could also use the machinery with the resource 
tracker object directly.

However, I'm not convince we should do so. We don't have any automatic memory 
allocation system for DRM objects (such as framebuffers or planes). Instead, 
drivers must implement a destroy operation for the object, and the core calls 
it when the last reference to the object goes away. This works fine, and I 
don't see what would prevent use from implementing the same mechanism for 
bridges or other similar structures.

> Why is this stuff tied to struct device and not struct kobject anyway. Or
> at least something we could embed in random cool place (like drm_device).
> 
> Greg, any ideas how we could simplify management of stuff that's created
> at driver load, but its lifetime tied to something which isn't directly a
> struct device?
> 
> > It's the first one that makes devm_* allocation unsuitable for any
> > structure that is accessible from userspace (such as in file operation
> > handlers).
> 
> Yeah, if we could release at 2. it would be perfect I think ...

It wouldn't, as unbind/rebind cycles would allocate new objects each time 
without freeing the previous ones until the device is deleted.

-- 
Regards,

Laurent Pinchart

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

Re: [PATCH v5 1/6] drm/vc4: Avoid using vrefresh==0 mode in DSI htotal math.

[Boris Brezillon]

Le Fri, 04 Aug 2017 14:15:56 -0700,
Eric Anholt <eric@anholt.net> a écrit :

> Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> 
> > On Tue, 18 Jul 2017 14:05:05 -0700
> > Eric Anholt <eric@anholt.net> wrote:
> >  
> >> The incoming mode might have a missing vrefresh field if it came from
> >> drmModeSetCrtc(), which the kernel is supposed to calculate using
> >> drm_mode_vrefresh().  We could either use that or the adjusted_mode's
> >> original vrefresh value.
> >> 
> >> However, we can maintain a more exact vrefresh value (not just the
> >> integer approximation), by scaling by the ratio of our clocks.
> >> 
> >> v2: Use math suggested by Andrzej Hajda instead.
> >> 
> >> Signed-off-by: Eric Anholt <eric@anholt.net>
> >> ---
> >>  drivers/gpu/drm/vc4/vc4_dsi.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> >> index 629d372633e6..57213f4e3c72 100644
> >> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> >> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> >> @@ -866,7 +866,8 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
> >>  	adjusted_mode->clock = pixel_clock_hz / 1000 + 1;
> >>  
> >>  	/* Given the new pixel clock, adjust HFP to keep vrefresh the same. */
> >> -	adjusted_mode->htotal = pixel_clock_hz / (mode->vrefresh * mode->vtotal);
> >> +	adjusted_mode->htotal = (pixel_clock_hz / 1000 * mode->htotal /
> >> +				 mode->clock);  
> >
> > Hm, I'm not sure I understand this. Shouldn't we have something like:
> >
> > 	adjusted_mode->htotal = (adjusted_mode->clock * mode->htotal) /
> > 				mode->clock;
> >
> > Is there a reason for doing '+ 1' when you calculate the adjusted
> > pixel clock rate but not here?  
> 
> We're actually expecting to get within epsilon of pixel_clock_hz, but we
> have to bump our clk_set_rate() to a higher value because the clock
> driver will give you a bad divider if you ask for anything less than the
> rate it can provide.
> 
> How about I don't increment the adjusted_mode->clock (since it'll be
> userspace visible I think), and instead move that and the "Round up"
> comment to the clk_set_rate()?

Sounds good.

Re: [PATCH v5 1/6] drm/vc4: Avoid using vrefresh==0 mode in DSI htotal math.

[Boris Brezillon]

Le Fri, 04 Aug 2017 14:15:56 -0700,
Eric Anholt <eric@anholt.net> a écrit :

> Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> 
> > On Tue, 18 Jul 2017 14:05:05 -0700
> > Eric Anholt <eric@anholt.net> wrote:
> >  
> >> The incoming mode might have a missing vrefresh field if it came from
> >> drmModeSetCrtc(), which the kernel is supposed to calculate using
> >> drm_mode_vrefresh().  We could either use that or the adjusted_mode's
> >> original vrefresh value.
> >> 
> >> However, we can maintain a more exact vrefresh value (not just the
> >> integer approximation), by scaling by the ratio of our clocks.
> >> 
> >> v2: Use math suggested by Andrzej Hajda instead.
> >> 
> >> Signed-off-by: Eric Anholt <eric@anholt.net>
> >> ---
> >>  drivers/gpu/drm/vc4/vc4_dsi.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> >> index 629d372633e6..57213f4e3c72 100644
> >> --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> >> +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> >> @@ -866,7 +866,8 @@ static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder,
> >>  	adjusted_mode->clock = pixel_clock_hz / 1000 + 1;
> >>  
> >>  	/* Given the new pixel clock, adjust HFP to keep vrefresh the same. */
> >> -	adjusted_mode->htotal = pixel_clock_hz / (mode->vrefresh * mode->vtotal);
> >> +	adjusted_mode->htotal = (pixel_clock_hz / 1000 * mode->htotal /
> >> +				 mode->clock);  
> >
> > Hm, I'm not sure I understand this. Shouldn't we have something like:
> >
> > 	adjusted_mode->htotal = (adjusted_mode->clock * mode->htotal) /
> > 				mode->clock;
> >
> > Is there a reason for doing '+ 1' when you calculate the adjusted
> > pixel clock rate but not here?  
> 
> We're actually expecting to get within epsilon of pixel_clock_hz, but we
> have to bump our clk_set_rate() to a higher value because the clock
> driver will give you a bad divider if you ask for anything less than the
> rate it can provide.
> 
> How about I don't increment the adjusted_mode->clock (since it'll be
> userspace visible I think), and instead move that and the "Round up"
> comment to the clk_set_rate()?

Sounds good.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel