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@xxxxxxxxx>
>>> ---
>>> 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/
>
>
>
>
