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

abhinavk@xxxxxxxxxxxxxx · Tue, 21 Sep 2021 13:52:17 -0700

On 2021-09-21 10:47, Dmitry Baryshkov wrote:
Hi,

On Tue, 21 Sept 2021 at 20:01, <abhinavk@xxxxxxxxxxxxxx> 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@xxxxxxxxxx>

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)