| Message ID | 20180709134834.11035-4-heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09.07.2018 15:48, Heiko Stuebner wrote: > 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. Regards Andrzej > + > static void dw_mipi_message_config(struct dw_mipi_dsi *dsi, > const struct mipi_dsi_msg *msg) > { > @@ -867,6 +890,8 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, > dsi->dev = dev; > dsi->plat_data = plat_data; > > + mutex_init(&dsi->panel_mutex); > + > if (!plat_data->phy_ops->init || !plat_data->phy_ops->get_lane_mbps) { > DRM_ERROR("Phy not properly configured\n"); > return ERR_PTR(-ENODEV); > diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h > index 6d7f8eb5d9f2..131ff2569ed4 100644 > --- a/include/drm/bridge/dw_mipi_dsi.h > +++ b/include/drm/bridge/dw_mipi_dsi.h > @@ -31,6 +31,7 @@ struct dw_mipi_dsi_plat_data { > void *priv_data; > }; > > +bool dw_mipi_dsi_device_attached(struct dw_mipi_dsi *dsi); > struct dw_mipi_dsi *dw_mipi_dsi_probe(struct platform_device *pdev, > const struct dw_mipi_dsi_plat_data > *plat_data);
Hi Andrzej, Am Montag, 13. August 2018, 10:28:39 CEST schrieb Andrzej Hajda: > On 09.07.2018 15:48, Heiko Stuebner wrote: > > 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. Thanks Heiko [0] https://patchwork.kernel.org/patch/10470821/
On 13.08.2018 10:44, Heiko Stuebner wrote: > Hi Andrzej, > > Am Montag, 13. August 2018, 10:28:39 CEST schrieb Andrzej Hajda: >> On 09.07.2018 15:48, Heiko Stuebner wrote: >>> 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/ > > > >
Hi Andrzej, Am Montag, 13. August 2018, 12:23:21 CEST schrieb Andrzej Hajda: > On 13.08.2018 10:44, Heiko Stuebner wrote: > > Am Montag, 13. August 2018, 10:28:39 CEST schrieb Andrzej Hajda: > >> On 09.07.2018 15:48, Heiko Stuebner wrote: > >>> +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 I think that is the general consensus in all kernel parts, like when you request an irq, the driver should be able to handle it firing immediately, so similarly a dsi-sink should in a position to be directly usable after it attached to the dsi host. > [*]. > 3. Similar way it should be done in remove path. > > This way in bind callback all resources should be there. You're a genius ... That seems to work like a charm :-D . v4 following shortly :-) > [*]: 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. Actually I'd say it follows that paradigm way better now, as the component framework only tries to bring up the component hierarchy after all resources are in place. And now I even don't get any deferrals at all, where it was around 2 deferrals waiting for the panel before. So in terms of being deterministic this feels like a plus :-) Heiko
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); + static void dw_mipi_message_config(struct dw_mipi_dsi *dsi, const struct mipi_dsi_msg *msg) { @@ -867,6 +890,8 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, dsi->dev = dev; dsi->plat_data = plat_data; + mutex_init(&dsi->panel_mutex); + if (!plat_data->phy_ops->init || !plat_data->phy_ops->get_lane_mbps) { DRM_ERROR("Phy not properly configured\n"); return ERR_PTR(-ENODEV); diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h index 6d7f8eb5d9f2..131ff2569ed4 100644 --- a/include/drm/bridge/dw_mipi_dsi.h +++ b/include/drm/bridge/dw_mipi_dsi.h @@ -31,6 +31,7 @@ struct dw_mipi_dsi_plat_data { void *priv_data; }; +bool dw_mipi_dsi_device_attached(struct dw_mipi_dsi *dsi); struct dw_mipi_dsi *dw_mipi_dsi_probe(struct platform_device *pdev, const struct dw_mipi_dsi_plat_data *plat_data);
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(+)