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

[abhinavk@codeaurora.org]

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.

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