[Freedreno] [PATCH] drm/msm/dsi: do not install irq handler before power up the host

[Dmitry Baryshkov]

On 21/09/2021 23:52, abhinavk at codeaurora.org wrote:
> On 2021-09-21 10:47, Dmitry Baryshkov wrote:
>> Hi,
>>
>> On Tue, 21 Sept 2021 at 20:01, <abhinavk at 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 at 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.

> 
>>>
>>> >       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)


-- 
With best wishes
Dmitry