Re: [PATCH 5/5] drm/bridge: dw-mipi-dsi: Fix dsi registration during drm probing

From: Michael Nazzareno Trimarchi
Date: Sat Oct 16 2021 - 10:30:57 EST

Next message: Phillip Potter: "Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores"
Previous message: Sven Peter: "Re: [PATCH] iommu/dart: use kmemdup instead of kzalloc and memcpy"
In reply to: Michael Nazzareno Trimarchi: "Re: [PATCH 5/5] drm/bridge: dw-mipi-dsi: Fix dsi registration during drm probing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi

Forget this one I can not replicate. We have another problem

9862.010474] Hardware name: Rockchip PX30 EVB (DT)
[ 9862.015738] Call trace:
[ 9862.018471] dump_backtrace+0x0/0x19c
[ 9862.022586] show_stack+0x1c/0x70
[ 9862.026305] dump_stack_lvl+0x68/0x84
[ 9862.030408] dump_stack+0x1c/0x38
[ 9862.034125] ili9881c_unprepare+0x1c/0x4c
[ 9862.038619] drm_panel_unprepare+0x2c/0x4c
[ 9862.043212] panel_bridge_post_disable+0x18/0x24
[ 9862.048390] dw_mipi_dsi_bridge_post_disable+0x3c/0xf0
[ 9862.054153] drm_atomic_bridge_chain_post_disable+0x8c/0xd0
[ 9862.060399] disable_outputs+0x120/0x31c
[ 9862.064785] drm_atomic_helper_commit_tail_rpm+0x28/0xa0
[ 9862.070734] commit_tail+0xa4/0x184
[ 9862.074642] drm_atomic_helper_commit+0x164/0x37c
[ 9862.079902] drm_atomic_commit+0x50/0x60
[ 9862.084298] drm_atomic_helper_disable_all+0x1fc/0x210
[ 9862.090053] drm_atomic_helper_shutdown+0x80/0x130
[ 9862.095419] rockchip_drm_platform_shutdown+0x1c/0x30
[ 9862.101081] platform_shutdown+0x28/0x40
[ 9862.105477] device_shutdown+0x15c/0x330
[ 9862.109877] __do_sys_reboot+0x214/0x294
[ 9862.114273] __arm64_sys_reboot+0x28/0x3c
[ 9862.118765] invoke_syscall+0x48/0x114
[ 9862.122969] el0_svc_common.constprop.0+0x44/0xec
[ 9862.128241] do_el0_svc+0x28/0x90
[ 9862.131958] el0_svc+0x20/0x60
[ 9862.135381] el0t_64_sync_handler+0x1a8/0x1b0
[ 9862.140263] el0t_64_sync+0x1a0/0x1a4
[ 9862.145769] CPU: 0 PID: 1 Comm: systemd-shutdow Tainted: G W
5.15.0-rc5 #1
[ 9862.154957] Hardware name: Rockchip PX30 EVB (DT)
[ 9862.160221] Call trace:
[ 9862.162954] dump_backtrace+0x0/0x19c
[ 9862.167068] show_stack+0x1c/0x70
[ 9862.170787] dump_stack_lvl+0x68/0x84
[ 9862.174895] dump_stack+0x1c/0x38
[ 9862.178611] ili9881c_unprepare+0x1c/0x4c
[ 9862.183103] drm_panel_unprepare+0x2c/0x4c
[ 9862.187695] panel_bridge_post_disable+0x18/0x24
[ 9862.192872] drm_atomic_bridge_chain_post_disable+0x8c/0xd0
[ 9862.199117] disable_outputs+0x120/0x31c
[ 9862.203512] drm_atomic_helper_commit_tail_rpm+0x28/0xa0
[ 9862.209461] commit_tail+0xa4/0x184
[ 9862.213368] drm_atomic_helper_commit+0x164/0x37c
[ 9862.218629] drm_atomic_commit+0x50/0x60
[ 9862.223025] drm_atomic_helper_disable_all+0x1fc/0x210
[ 9862.228781] drm_atomic_helper_shutdown+0x80/0x130
[ 9862.234147] rockchip_drm_platform_shutdown+0x1c/0x30
[ 9862.239810] platform_shutdown+0x28/0x40
[ 9862.244205] device_shutdown+0x15c/0x330
[ 9862.248603] __do_sys_reboot+0x214/0x294
[ 9862.253000] __arm64_sys_reboot+0x28/0x3c
[ 9862.257492] invoke_syscall+0x48/0x114
[ 9862.261696] el0_svc_common.constprop.0+0x44/0xec
[ 9862.266970] do_el0_svc+0x28/0x90
[ 9862.270687] el0_svc+0x20/0x60
[ 9862.274111] el0t_64_sync_handler+0x1a8/0x1b0
[ 9862.278992] el0t_64_sync+0x1a0/0x1a4
[ 9862.283296] ------------[ cut here ]------------
[ 9862.288490] unbalanced disables for vcc3v3_lcd
[ 9862.293555] WARNING: CPU: 0 PID: 1 at drivers/regulator/core.c:2851
_regulator_disable+0xd4/0x190

The panel can be disable two times.

/*
* TODO Only way found to call panel-bridge post_disable &
* panel unprepare before the dsi "final" disable...
* This needs to be fixed in the drm_bridge framework and the API
* needs to be updated to manage our own call chains...
*/
if (dsi->panel_bridge->funcs->post_disable)
dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);

Is this comment relevant?

Michael

On Sat, Oct 16, 2021 at 3:32 PM Michael Nazzareno Trimarchi
<michael@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Hi Sam
>
> On Sat, Oct 16, 2021 at 2:25 PM Sam Ravnborg <sam@xxxxxxxxxxxx> wrote:
> >
> > Hi Michael,
> >
> > I fail to follow the logic in this patch.
> >
> >
> > On Sat, Oct 16, 2021 at 10:22:32AM +0000, Michael Trimarchi wrote:
> > > The dsi registration is implemented in rockchip platform driver.
> > > The attach can be called before the probe is terminated and therefore
> > > we need to be sure that corresponding entry during attach is valid
> > >
> > > Signed-off-by: Michael Trimarchi <michael@xxxxxxxxxxxxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 8 +++++++-
> > > drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c | 12 ++++++++----
> > > include/drm/bridge/dw_mipi_dsi.h | 2 +-
> > > 3 files changed, 16 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > > index e44e18a0112a..44b211be15fc 100644
> > > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > > @@ -362,8 +362,14 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host,
> > > dsi->device_found = true;
> > > }
> > >
> > > + /*
> > > + * NOTE: the dsi registration is implemented in
> > > + * platform driver, that to say dsi would be exist after
> > > + * probe is terminated. The call is done before the end of probe
> > > + * so we need to pass the dsi to the platform driver.
> > > + */
> > > if (pdata->host_ops && pdata->host_ops->attach) {
> > > - ret = pdata->host_ops->attach(pdata->priv_data, device);
> > > + ret = pdata->host_ops->attach(pdata->priv_data, device, dsi);
> > > if (ret < 0)
> > > return ret;
> > > }
> > > diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> > > index a2262bee5aa4..32ddc8642ec1 100644
> > > --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> > > +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c
> > > @@ -997,7 +997,8 @@ static const struct component_ops dw_mipi_dsi_rockchip_ops = {
> > > };
> > >
> > > static int dw_mipi_dsi_rockchip_host_attach(void *priv_data,
> > > - struct mipi_dsi_device *device)
> > > + struct mipi_dsi_device *device,
> > > + struct dw_mipi_dsi *dmd)
> > > {
> > > struct dw_mipi_dsi_rockchip *dsi = priv_data;
> > > struct device *second;
> > > @@ -1005,6 +1006,8 @@ static int dw_mipi_dsi_rockchip_host_attach(void *priv_data,
> > >
> > > mutex_lock(&dsi->usage_mutex);
> > >
> > > + dsi->dmd = dmd;
> > > +
> > > if (dsi->usage_mode != DW_DSI_USAGE_IDLE) {
> > > DRM_DEV_ERROR(dsi->dev, "dsi controller already in use\n");
> > > mutex_unlock(&dsi->usage_mutex);
> > > @@ -1280,6 +1283,7 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
> > > {
> > > struct device *dev = &pdev->dev;
> > > struct device_node *np = dev->of_node;
> > > + struct dw_mipi_dsi *dmd;
> > > struct dw_mipi_dsi_rockchip *dsi;
> > > struct phy_provider *phy_provider;
> > > struct resource *res;
> > > @@ -1391,9 +1395,9 @@ static int dw_mipi_dsi_rockchip_probe(struct platform_device *pdev)
> > > if (IS_ERR(phy_provider))
> > > return PTR_ERR(phy_provider);
> > >
> > > - dsi->dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
> > > - if (IS_ERR(dsi->dmd)) {
> > > - ret = PTR_ERR(dsi->dmd);
> > > + dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
> > > + if (IS_ERR(dmd)) {
> > > + ret = PTR_ERR(dmd);
> >
> > The memory pointed to by dmd is allocated in dw_mipi_dsi_probe(), but
> > the pointer is not saved here.
> > We rely on the attach operation to save the dmd pointer.
> >
> >
> > In other words - the attach operation must be called before we call
> > dw_mipi_dsi_rockchip_remove(), which uses the dmd member.
> >
> > This all looks wrong to me - are we papering over some other issue
>
> Ok, it's wrong. I was not expecting that call.Anyway this was my path
> on linux-next
>
> dw_mipi_dsi_rockchip_probe
> dw_mipi_dsi_probe -->start call
>
> dw_mipi_dsi_rockchip_host_attach <-- this was not able to use dmd
>
> dw_mipi_dsi_probe -> exit from the call
>
> Michael
>
> > here?
> >
> > Sam
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@xxxxxxxxxxxxxxxxxxxx
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@xxxxxxxxxxxxxxxxxxxx
> www.amarulasolutions.com

--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@xxxxxxxxxxxxxxxxxxxx
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@xxxxxxxxxxxxxxxxxxxx
www.amarulasolutions.com

Next message: Phillip Potter: "Re: [RFC PATCH] staging: r8188eu: Use completions instead of semaphores"
Previous message: Sven Peter: "Re: [PATCH] iommu/dart: use kmemdup instead of kzalloc and memcpy"
In reply to: Michael Nazzareno Trimarchi: "Re: [PATCH 5/5] drm/bridge: dw-mipi-dsi: Fix dsi registration during drm probing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]