Re: [PATCH v2 10/16] drm/exynos: implement a drm bridge

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hi Michael,

On 11.09.2020 15:54, Michael Tretter wrote:
> Make the exynos_dsi driver a full drm bridge that can be found and used
> from other drivers.
>
> Other drivers can only attach to the bridge, if a mipi dsi device
> already attached to the bridge. This allows to defer the probe of the
> display pipe until the downstream bridges are available, too.
>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>

This one (and the whole series applied) still fails on Exynos boards:

[drm] Exynos DRM: using 11c00000.fimd device for DMA mapping operations
exynos-drm exynos-drm: bound 11c00000.fimd (ops fimd_component_ops)
OF: graph: no port node found in /soc/dsi@11c80000
8<--- cut here ---
Unable to handle kernel NULL pointer dereference at virtual address 00000084
pgd = (ptrval)
[00000084] *pgd=00000000
Internal error: Oops: 5 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted 
5.9.0-rc4-next-20200911-00010-g417dc70d70ec #1608
Hardware name: Samsung Exynos (Flattened Device Tree)
PC is at drm_bridge_attach+0x18/0x164
LR is at exynos_dsi_bind+0x88/0xa8
pc : [<c0628c08>]    lr : [<c064d560>]    psr: 20000013
sp : ef0dfca8  ip : 00000002  fp : c13190e0
r10: 00000000  r9 : ee46d580  r8 : c13190e0
r7 : ee438800  r6 : 00000018  r5 : ef253810  r4 : ef39e840
r3 : 00000000  r2 : 00000018  r1 : ef39e888  r0 : ef39e840
Flags: nzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 4000404a  DAC: 00000051
Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
Stack: (0xef0dfca8 to 0xef0e0000)
...
[<c0628c08>] (drm_bridge_attach) from [<c064d560>] 
(exynos_dsi_bind+0x88/0xa8)
[<c064d560>] (exynos_dsi_bind) from [<c066a800>] 
(component_bind_all+0xfc/0x290)
[<c066a800>] (component_bind_all) from [<c0649dc0>] 
(exynos_drm_bind+0xe4/0x19c)
[<c0649dc0>] (exynos_drm_bind) from [<c066ad74>] 
(try_to_bring_up_master+0x1e4/0x2c4)
[<c066ad74>] (try_to_bring_up_master) from [<c066b2b4>] 
(component_master_add_with_match+0xd4/0x108)
[<c066b2b4>] (component_master_add_with_match) from [<c0649ae8>] 
(exynos_drm_platform_probe+0xe4/0x110)
[<c0649ae8>] (exynos_drm_platform_probe) from [<c0674e6c>] 
(platform_drv_probe+0x6c/0xa4)
[<c0674e6c>] (platform_drv_probe) from [<c067242c>] 
(really_probe+0x200/0x4fc)
[<c067242c>] (really_probe) from [<c06728f0>] 
(driver_probe_device+0x78/0x1fc)
[<c06728f0>] (driver_probe_device) from [<c0672cd8>] 
(device_driver_attach+0x58/0x60)
[<c0672cd8>] (device_driver_attach) from [<c0672dbc>] 
(__driver_attach+0xdc/0x174)
[<c0672dbc>] (__driver_attach) from [<c06701b4>] 
(bus_for_each_dev+0x68/0xb4)
[<c06701b4>] (bus_for_each_dev) from [<c06714e8>] 
(bus_add_driver+0x158/0x214)
[<c06714e8>] (bus_add_driver) from [<c0673c1c>] (driver_register+0x78/0x110)
[<c0673c1c>] (driver_register) from [<c0649ca8>] 
(exynos_drm_init+0xe4/0x118)
[<c0649ca8>] (exynos_drm_init) from [<c0102484>] 
(do_one_initcall+0x8c/0x42c)
[<c0102484>] (do_one_initcall) from [<c11011c0>] 
(kernel_init_freeable+0x190/0x1dc)
[<c11011c0>] (kernel_init_freeable) from [<c0af7880>] 
(kernel_init+0x8/0x118)
[<c0af7880>] (kernel_init) from [<c0100114>] (ret_from_fork+0x14/0x20)
Exception stack(0xef0dffb0 to 0xef0dfff8)
...
---[ end trace ee27f313f9ed9da1 ]---

# arm-linux-gnueabi-addr2line -e vmlinux c0628c08
drivers/gpu/drm/drm_bridge.c:184 (discriminator 1)

I will try to debug it a bit more today.

> ---
> v2:
> - move attach of out_bridge to bridge_attach
> - add bridge_detach
> - don't cleanup encoder if create_connector failed
> ---
>   drivers/gpu/drm/exynos/exynos_drm_dsi.c | 103 +++++++++++++++++-------
>   1 file changed, 75 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 2d75f9877dc0..5e7c1a99a3ee 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -266,6 +266,7 @@ struct exynos_dsi_driver_data {
>   
>   struct exynos_dsi {
>   	struct drm_encoder encoder;
> +	struct drm_bridge bridge;
>   	struct mipi_dsi_host dsi_host;
>   	struct drm_connector connector;
>   	struct drm_panel *panel;
> @@ -1602,6 +1603,60 @@ static const struct drm_encoder_helper_funcs exynos_dsi_encoder_helper_funcs = {
>   	.disable = exynos_dsi_disable,
>   };
>   
> +static int exynos_dsi_bridge_attach(struct drm_bridge *bridge,
> +				    enum drm_bridge_attach_flags flags)
> +{
> +	struct exynos_dsi *dsi = bridge->driver_private;
> +	struct drm_encoder *encoder = bridge->encoder;
> +	int ret;
> +
> +	if (!dsi->out_bridge && !dsi->panel)
> +		return -EPROBE_DEFER;
> +
> +	if (dsi->out_bridge) {
> +		ret = drm_bridge_attach(encoder, dsi->out_bridge,
> +					bridge, flags);
> +		if (ret)
> +			return ret;
> +		list_splice_init(&encoder->bridge_chain, &dsi->bridge_chain);
> +	} else {
> +		ret = exynos_dsi_create_connector(encoder);
> +		if (ret)
> +			return ret;
> +
> +		if (dsi->panel) {
> +			dsi->connector.status = connector_status_connected;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void exynos_dsi_bridge_detach(struct drm_bridge *bridge)
> +{
> +	struct exynos_dsi *dsi = bridge->driver_private;
> +	struct drm_encoder *encoder = bridge->encoder;
> +	struct drm_device *drm = encoder->dev;
> +
> +	if (dsi->panel) {
> +		mutex_lock(&drm->mode_config.mutex);
> +		exynos_dsi_disable(&dsi->encoder);
> +		dsi->panel = NULL;
> +		dsi->connector.status = connector_status_disconnected;
> +		mutex_unlock(&drm->mode_config.mutex);
> +	} else {
> +		if (dsi->out_bridge->funcs->detach)
> +			dsi->out_bridge->funcs->detach(dsi->out_bridge);
> +		dsi->out_bridge = NULL;
> +		INIT_LIST_HEAD(&dsi->bridge_chain);
> +	}
> +}
> +
> +static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {
> +	.attach = exynos_dsi_bridge_attach,
> +	.detach = exynos_dsi_bridge_detach,
> +};
> +
>   MODULE_DEVICE_TABLE(of, exynos_dsi_of_match);
>   
>   static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
> @@ -1609,25 +1664,12 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
>   {
>   	struct exynos_dsi *dsi = host_to_dsi(host);
>   	const struct exynos_dsi_host_ops *ops = dsi->driver_data->host_ops;
> -	struct drm_encoder *encoder = &dsi->encoder;
>   	struct drm_bridge *out_bridge;
>   
> -	out_bridge  = of_drm_find_bridge(device->dev.of_node);
> +	out_bridge = of_drm_find_bridge(device->dev.of_node);
>   	if (out_bridge) {
> -		drm_bridge_attach(encoder, out_bridge, NULL, 0);
>   		dsi->out_bridge = out_bridge;
> -		list_splice_init(&encoder->bridge_chain, &dsi->bridge_chain);
>   	} else {
> -		int ret = exynos_dsi_create_connector(encoder);
> -
> -		if (ret) {
> -			DRM_DEV_ERROR(dsi->dev,
> -				      "failed to create connector ret = %d\n",
> -				      ret);
> -			drm_encoder_cleanup(encoder);
> -			return ret;
> -		}
> -
>   		dsi->panel = of_drm_find_panel(device->dev.of_node);
>   		if (IS_ERR(dsi->panel))
>   			dsi->panel = NULL;
> @@ -1662,20 +1704,6 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
>   {
>   	struct exynos_dsi *dsi = host_to_dsi(host);
>   	const struct exynos_dsi_host_ops *ops = dsi->driver_data->host_ops;
> -	struct drm_device *drm = dsi->encoder.dev;
> -
> -	if (dsi->panel) {
> -		mutex_lock(&drm->mode_config.mutex);
> -		exynos_dsi_disable(&dsi->encoder);
> -		dsi->panel = NULL;
> -		dsi->connector.status = connector_status_disconnected;
> -		mutex_unlock(&drm->mode_config.mutex);
> -	} else {
> -		if (dsi->out_bridge->funcs->detach)
> -			dsi->out_bridge->funcs->detach(dsi->out_bridge);
> -		dsi->out_bridge = NULL;
> -		INIT_LIST_HEAD(&dsi->bridge_chain);
> -	}
>   
>   	if (ops && ops->detach)
>   		ops->detach(dsi->dsi_host.dev, device);
> @@ -1786,7 +1814,15 @@ static int exynos_dsi_bind(struct device *dev, struct device *master,
>   		of_node_put(in_bridge_node);
>   	}
>   
> +	ret = drm_bridge_attach(encoder, &dsi->bridge, in_bridge, 0);
> +	if (ret)
> +		goto err;
> +
>   	return 0;
> +
> +err:
> +	drm_encoder_cleanup(encoder);
> +	return ret;
>   }
>   
>   static void exynos_dsi_unbind(struct device *dev, struct device *master,
> @@ -1796,6 +1832,8 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master,
>   	struct drm_encoder *encoder = &dsi->encoder;
>   
>   	exynos_dsi_disable(encoder);
> +
> +	drm_encoder_cleanup(encoder);
>   }
>   
>   static const struct component_ops exynos_dsi_component_ops = {
> @@ -1806,6 +1844,7 @@ static const struct component_ops exynos_dsi_component_ops = {
>   static struct exynos_dsi *__exynos_dsi_probe(struct platform_device *pdev)
>   {
>   	struct device *dev = &pdev->dev;
> +	struct drm_bridge *bridge;
>   	struct resource *res;
>   	struct exynos_dsi *dsi;
>   	int ret, i;
> @@ -1894,11 +1933,19 @@ static struct exynos_dsi *__exynos_dsi_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> +	bridge = &dsi->bridge;
> +	bridge->driver_private = dsi;
> +	bridge->funcs = &exynos_dsi_bridge_funcs;
> +	bridge->of_node = dev->of_node;
> +	drm_bridge_add(bridge);
> +
>   	return dsi;
>   }
>   
>   static void __exynos_dsi_remove(struct exynos_dsi *dsi)
>   {
> +	drm_bridge_remove(&dsi->bridge);
> +
>   	mipi_dsi_host_unregister(&dsi->dsi_host);
>   }
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland