[PATCH v3 3/8] drm/bridge/synopsys: dsi: add ability to check dsi-device attachment

[Andrzej Hajda]

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