drm/vc4: Avoid using vrefresh==0 mode in DSI htotal math.

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 <[email protected]>

Reviewed-by Andrzej Hajda <[email protected]>

---
  drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
  include/drm/drm_bridge.h       |  3 +++
  2 files changed, 33 insertions(+)

drivers/gpu/drm/bridge/panel.c +30 -0

index 685c1a480201..292ee92a9c97 100644
@@ -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);

include/drm/drm_bridge.h +0 -0

index 1dc94d5392e2..6522d4cbc9d9 100644

Quoted 7 lines · 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 <[email protected]>

Reviewed-by Andrzej Hajda <[email protected]>

Reviewed-by Philippe Cornu <[email protected]>

Tested-by Philippe Cornu <[email protected]>

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 <[email protected]>

Reviewed-by Andrzej Hajda <[email protected]>

---
 drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h       |  3 +++
 2 files changed, 33 insertions(+)

drivers/gpu/drm/bridge/panel.c +30 -0

index 685c1a480201..292ee92a9c97 100644
@@ -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);

include/drm/drm_bridge.h +3 -0

index 1dc94d5392e2..6522d4cbc9d9 100644
@@ -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

Hi Eric,

(CC'ing Daniel)

Thank you for the patch.

Quoted 4 lines · 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 :-/

Quoted 67 lines

Signed-off-by Eric Anholt <[email protected]>

Reviewed-by Andrzej Hajda <[email protected]>

---

 drivers/gpu/drm/bridge/panel.c | 30 ++++++++++++++++++++++++++++++
 include/drm/drm_bridge.h       |  3 +++
 2 files changed, 33 insertions(+)

(quoted diff) +0 -0

b/drivers/gpu/drm/bridge/panel.c index 685c1a480201..292ee92a9c97 100644

drivers/gpu/drm/bridge/panel.c +30 -0

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

include/drm/drm_bridge.h +3 -0

index 1dc94d5392e2..6522d4cbc9d9 100644
@@ -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

Hi Eric,

(CC'ing Daniel)

Thank you for the patch.

Quoted 3 lines · 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.

Quoted 1 line

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 :-/

Quoted 6 lines · Laurent Pinchart

Quoted 4 lines · 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

Quoted 7 lines

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.

Quoted 10 lines · Laurent Pinchart

Hi Eric,

(CC'ing Daniel)

Thank you for the patch.

Quoted 3 lines · 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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 19 lines

Quoted 10 lines · Laurent Pinchart

Hi Eric,

(CC'ing Daniel)

Thank you for the patch.

Quoted 3 lines · 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.

Quoted 7 lines

Quoted 1 line

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 :-/

Quoted 3 lines

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.

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

Den 05.08.2017 00.19, skrev Ilia Mirkin:

Quoted 23 lines · Eric Anholt

Quoted 19 lines

Quoted 10 lines · Laurent Pinchart

Hi Eric,

(CC'ing Daniel)

Thank you for the patch.

Quoted 3 lines · 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.

Quoted 7 lines

Quoted 1 line

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 :-/

Quoted 3 lines

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.

Quoted 6 lines

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?

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

Den 05.08.2017 00.19, skrev Ilia Mirkin:

Quoted 23 lines · Eric Anholt

Quoted 19 lines

Quoted 10 lines · Laurent Pinchart

Hi Eric,

(CC'ing Daniel)

Thank you for the patch.

Quoted 3 lines · 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.

Quoted 7 lines

Quoted 1 line

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 :-/

Quoted 3 lines

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.

Quoted 6 lines

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?

Quoted 29 lines · Noralf Trønnes

Den 05.08.2017 00.19, skrev Ilia Mirkin:

Quoted 21 lines · Eric Anholt

Quoted 15 lines

Quoted 6 lines · Laurent Pinchart

Quoted 4 lines · 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

Quoted 7 lines

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 :-/

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Hi Daniel,

Quoted 32 lines · On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:

Quoted 19 lines · Noralf Trønnes

Den 05.08.2017 00.19, skrev Ilia Mirkin:

Quoted 18 lines · Eric Anholt

Quoted 17 lines

Quoted 4 lines · Laurent Pinchart

Quoted 3 lines · 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.

Quoted 8 lines

Quoted 1 line

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.

Quoted 8 lines

Quoted 6 lines

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

Quoted 6 lines

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

Hi Daniel,

Quoted 42 lines · On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:

Quoted 29 lines · Noralf Trønnes

Den 05.08.2017 00.19, skrev Ilia Mirkin:

Quoted 21 lines · Eric Anholt

Quoted 15 lines

Quoted 6 lines · Laurent Pinchart

Quoted 4 lines · 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

Quoted 7 lines

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 :-/

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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

Quoted 43 lines · Laurent Pinchart

Quoted 42 lines · On Monday 07 Aug 2017 11:25:07 Daniel Vetter wrote:

Quoted 29 lines · Noralf Trønnes

Den 05.08.2017 00.19, skrev Ilia Mirkin:

Quoted 21 lines · Eric Anholt

Quoted 15 lines

Quoted 6 lines · Laurent Pinchart

Quoted 4 lines · 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

Quoted 7 lines

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 :-/

Quoted 4 lines

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.

Quoted 6 lines

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.

Quoted 2 lines

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.

Quoted 8 lines

Quoted 1 line

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?

Quoted 3 lines

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

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 <[email protected]>

Reviewed-by Andrzej Hajda <[email protected]> (v1)

---
 drivers/gpu/drm/vc4/vc4_dsi.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

drivers/gpu/drm/vc4/vc4_dsi.c +14 -24

index 57213f4e3c72..769f247c52a9 100644
@@ -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) {

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 <[email protected]>

---
 drivers/gpu/drm/drm_mipi_dsi.c | 63 ++++++++++++++++++++++++++++++++++--------
 include/drm/drm_mipi_dsi.h     |  3 ++
 2 files changed, 54 insertions(+), 12 deletions(-)

drivers/gpu/drm/drm_mipi_dsi.c +11 -0

index 1160a579e0dc..77d439254da6 100644
@@ -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) {

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 <[email protected]>

---
 drivers/gpu/drm/vc4/vc4_dsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/vc4/vc4_dsi.c +0 -0

index 629d372633e6..57213f4e3c72 100644

drivers/gpu/drm/vc4/vc4_dsi.c +0 -0

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

Quoted 20 lines · On Tue, 18 Jul 2017 14:05:05 -0700 Eric Anholt <[email protected]> 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 <[email protected]>

---
 drivers/gpu/drm/vc4/vc4_dsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/vc4/vc4_dsi.c +0 -0

index 629d372633e6..57213f4e3c72 100644

drivers/gpu/drm/vc4/vc4_dsi.c static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder, · line 870

Quoted 6 lines

drivers/gpu/drm/vc4/vc4_dsi.c +2 -1

@@ -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?

Quoted 32 lines · Boris Brezillon

Quoted 20 lines · On Tue, 18 Jul 2017 14:05:05 -0700 Eric Anholt <[email protected]> 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 <[email protected]>

---
 drivers/gpu/drm/vc4/vc4_dsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/vc4/vc4_dsi.c +0 -0

index 629d372633e6..57213f4e3c72 100644

drivers/gpu/drm/vc4/vc4_dsi.c static bool vc4_dsi_encoder_mode_fixup(struct drm_encoder *encoder, · line 870

Quoted 6 lines

drivers/gpu/drm/vc4/vc4_dsi.c +2 -1

@@ -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:

Quoted 5 lines

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