On 28/09/2021 04:19, abhinavk@codeaurora.org wrote:
On 2021-09-27 18:06, Dmitry Baryshkov wrote:
On Tue, 28 Sept 2021 at 03:22, abhinavk@codeaurora.org wrote:
On 2021-09-25 12:43, Dmitry Baryshkov wrote:
On 21/09/2021 23:52, abhinavk@codeaurora.org wrote:
On 2021-09-21 10:47, Dmitry Baryshkov wrote:
Hi,
On Tue, 21 Sept 2021 at 20:01, abhinavk@codeaurora.org wrote: > > On 2021-09-21 09:22, Dmitry Baryshkov wrote: > > The DSI host might be left in some state by the bootloader. If
this
> > state generates an IRQ, it might hang the system by holding the > > interrupt line before the driver sets up the DSI host to the
known
> > state. > > > > Move the request/free_irq calls into
msm_dsi_host_power_on/_off calls,
> > so that we can be sure that the interrupt is delivered when
the host is
> > in the known state. > > > > Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector
support")
> > Signed-off-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org > > This is a valid change and we have seen interrupt storms in > downstream > happening > when like you said the bootloader leaves the DSI host in unknown > state. > Just one question below. > > > --- > > drivers/gpu/drm/msm/dsi/dsi_host.c | 21 ++++++++++++--------- > > 1 file changed, 12 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c > > b/drivers/gpu/drm/msm/dsi/dsi_host.c > > index e269df285136..cd842347a6b1 100644 > > --- a/drivers/gpu/drm/msm/dsi/dsi_host.c > > +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c > > @@ -1951,15 +1951,6 @@ int msm_dsi_host_modeset_init(struct > > mipi_dsi_host *host, > > return ret; > > } > > > > - ret = devm_request_irq(&pdev->dev, msm_host->irq, > > - dsi_host_irq, IRQF_TRIGGER_HIGH |
IRQF_ONESHOT,
> > - "dsi_isr", msm_host); > > - if (ret < 0) { > > - DRM_DEV_ERROR(&pdev->dev, "failed to request
IRQ%u: %d\n",
> > - msm_host->irq, ret); > > - return ret; > > - } > > - > > msm_host->dev = dev; > > ret = cfg_hnd->ops->tx_buf_alloc(msm_host, SZ_4K); > > if (ret) { > > @@ -2413,6 +2404,16 @@ int msm_dsi_host_power_on(struct
mipi_dsi_host
> > *host, > > if (msm_host->disp_en_gpio) > > gpiod_set_value(msm_host->disp_en_gpio, 1); > > > > + ret = devm_request_irq(&msm_host->pdev->dev, msm_host->irq, > > + dsi_host_irq, IRQF_TRIGGER_HIGH |
IRQF_ONESHOT,
> > + "dsi_isr", msm_host); > > + if (ret < 0) { > > + DRM_DEV_ERROR(&msm_host->pdev->dev, "failed to
request IRQ%u: %d\n",
> > + msm_host->irq, ret); > > + return ret; > > + } > > + > > + > > Do you want to move this to msm_dsi_host_enable()? > So without the controller being enabled it is still in unknown > state?
msm_dsi_host_power_on() reconfigures the host registers, so the
state
is known at the end of the power_on().
> Also do you want to do this after dsi0 and dsi1 are initialized to > account for > dual dsi cases?
I don't think this should matter. The host won't generate 'extra' interrupts in such case, will it?
We have seen cases where misconfiguration has caused interrupts to storm only on one DSI in some cases. So yes, I would prefer this is done after both are configured.
I've checked. The power_on is called from dsi_mgr_bridge_pre_enable() when both DSI hosts should be bound.
DSI being bound is enough? I thought the issue we are trying to address is that we need to have called msm_dsi_host_power_on() for both the hosts so that both are put in the known state before requesting the irq.
OR in other words move the irq_enable() to below location.
341 static void dsi_mgr_bridge_pre_enable(struct drm_bridge *bridge) 342 {
364 ret = msm_dsi_host_power_on(host, &phy_shared_timings[id], is_bonded_dsi, msm_dsi->phy); 365 if (ret) { 366 pr_err("%s: power on host %d failed, %d\n", __func__, id, ret); 367 goto host_on_fail; 368 } 369 370 if (is_bonded_dsi && msm_dsi1) { 371 ret = msm_dsi_host_power_on(msm_dsi1->host, 372 &phy_shared_timings[DSI_1], is_bonded_dsi, msm_dsi1->phy); 373 if (ret) { 374 pr_err("%s: power on host1 failed, %d\n", 375 __func__, ret); 376 goto host1_on_fail; 377 } 378 }
< move the irq enable here >
Ah, I see your point. What about moving to msm_dsi_host_enable() then?
Yes, I had suggested this a few replies ago. But only at the dsi_msgr we know if DSI1 is also done. So you can do it right after it in below location?
427 if (is_dual_dsi && msm_dsi1) { 428 ret = msm_dsi_host_enable(msm_dsi1->host); 429 if (ret) { 430 pr_err("%s: enable host1 failed, %d\n", __func__, ret); 431 goto host1_en_fail; 432 } 433 }
<enable_irq here? >
If there is DSI1, it was also powered on/programmed at the time of msm_dsi_host_enable, so enabling IRQs from it should be safe. Do you see any pitfalls from enabling the irq from that function?
> > msm_host->power_on = true; > > mutex_unlock(&msm_host->dev_mutex); > > > > @@ -2439,6 +2440,8 @@ int msm_dsi_host_power_off(struct
mipi_dsi_host
> > *host) > > goto unlock_ret; > > } > > > > + devm_free_irq(&msm_host->pdev->dev, msm_host->irq,
msm_host);
> > + > > dsi_ctrl_config(msm_host, false, NULL, NULL); > > > > if (msm_host->disp_en_gpio)