On 13.08.2018 10:44, Heiko Stuebner wrote:

Hi Andrzej,

Am Montag, 13. August 2018, 10:28:39 CEST schrieb Andrzej Hajda:

quoted

On 09.07.2018 15:48, Heiko Stuebner wrote:

quoted

When the panel-driver is build as a module it currently fails hard as the
panel cannot be probed directly:

dw_mipi_dsi_bind()
  __dw_mipi_dsi_probe()
    creates dsi bus
    creates panel device
    triggers panel module load
    panel not probed (module not loaded or panel probe slow)
  drm_bridge_attach
    fails with -EINVAL due to empty panel_bridge

Additionally the panel probing can run concurrently with dsi bringup
making it possible that the panel can already be found but dsi-attach
hasn't finished running.

The newly added function provides the ability for glue drivers to
check if a dsi device was actually attached and also protects
the attach part to prevent concurrency issues from panel-assignment
and drm_bridge_create.

Using that check glue drivers are able to for example defer probe/bind
in the case that the panel is not completely set up yet.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 25 +++++++++++++++++++
 include/drm/bridge/dw_mipi_dsi.h              |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index bb4aeca5c0f9..88fed22ff3f6 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c

@@ -12,6 +12,7 @@
 #include <linux/component.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>

@@ -219,6 +220,7 @@ struct dw_mipi_dsi {
 	struct drm_bridge bridge;
 	struct mipi_dsi_host dsi_host;
 	struct drm_bridge *panel_bridge;
+	struct mutex panel_mutex;
 	struct device *dev;
 	void __iomem *base;
 

@@ -296,10 +298,14 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
 			return PTR_ERR(bridge);
 	}
 
+	mutex_lock(&dsi->panel_mutex);
+
 	dsi->panel_bridge = bridge;
 
 	drm_bridge_add(&dsi->bridge);
 
+	mutex_unlock(&dsi->panel_mutex);
+
 	return 0;
 }
 

@@ -308,13 +314,30 @@ static int dw_mipi_dsi_host_detach(struct mipi_dsi_host *host,
 {
 	struct dw_mipi_dsi *dsi = host_to_dsi(host);
 
+	mutex_lock(&dsi->panel_mutex);
+
+	dsi->panel_bridge = NULL;
 	drm_of_panel_bridge_remove(host->dev->of_node, 1, 0);
 
 	drm_bridge_remove(&dsi->bridge);
 
+	mutex_unlock(&dsi->panel_mutex);
+
 	return 0;
 }
 
+bool dw_mipi_dsi_device_attached(struct dw_mipi_dsi *dsi)
+{
+	bool output;
+
+	mutex_lock(&dsi->panel_mutex);
+	output = !!dsi->panel_bridge;
+	mutex_unlock(&dsi->panel_mutex);
+
+	return output;
+}
+EXPORT_SYMBOL_GPL(dw_mipi_dsi_device_attached);

The function does not make sense. After releasing panel_mutex device can
be detached/(re-)attached multiple times. Ie it reports useless
information. Of course in most cases it will work as expected, but for
sure it is not bulletproof.

Ok. Can you suggest how we should check for the described case?
I.e. initially I tried to simply defer in dw_mipi_dsi_bridge_attach [0]
where you suggested to do that in probe.

I moved the check to bind - see patch 5 - to fix the issue regarding
panel only probing after the dsi bus gets created, so this function
is a means to check if the panel has finished attaching, or to defer
binding - see dw_mipi_dsi_rockchip_bind in patch 5.

So I'm somewhat out of ideas right now, how to do this right.

I am just after vacation, so please be kind if I write sth stupid :)

I would stick to the rule "do not expose functionality until you gather
required resources".
With this in mind I would try this way:
1. In bridge probe create mipi bus, but do not expose drm_bridge and do
not call component_add - because we still do not have the sink
(downstream panel or bridge).
2. In mipi_dsi_host_attach callback gather sink resource and then expose
drm_bridge and the component (by calling component_add) - it will work
with assumption the sink is registered/added before attaching to dsi bus
[*].
3. Similar way it should be done in remove path.

This way in bind callback all resources should be there.

[*]: This could be seen as sth against the rule "first resources, then
exposition", but since panel/bridge framework does not provide
notification about appearance of the objects, it works as a workaround
for missing notification system.

Regards
Andrzej

Thanks
Heiko

[0] https://patchwork.kernel.org/patch/10470821/

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