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

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

[Dmitry Baryshkov]

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>
---
 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;
+	}
+
+
 	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)
-- 
2.30.2

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

[abhinavk]

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?
Also do you want to do this after dsi0 and dsi1 are initialized to 
account for
dual dsi cases?

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

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

[Dmitry Baryshkov]

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?

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

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

[Dmitry Baryshkov]

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?

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

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

[abhinavk]

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)

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

[abhinavk]

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)

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

[Dmitry Baryshkov]

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.

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

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

[Dmitry Baryshkov]

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.

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

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

[abhinavk]

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

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

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

[abhinavk]

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

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

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

[Dmitry Baryshkov]

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?

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

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

[Dmitry Baryshkov]

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?

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

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

[abhinavk]

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

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

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

[abhinavk]

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

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

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

[Dmitry Baryshkov]

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)


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

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)


-- 
With best wishes
Dmitry

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

[abhinavk]

On 2021-09-27 18:29, Dmitry Baryshkov wrote:
> 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?

Just about symmetry. We will enable_irq() for DSI0 when DSI0 and DSI1 
are powered on
But for DSI1, we will enable it when its powered ON but not enabled.
Hence i thought its better this way.

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

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

[abhinavk]

On 2021-09-27 18:29, Dmitry Baryshkov wrote:
> 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?

Just about symmetry. We will enable_irq() for DSI0 when DSI0 and DSI1 
are powered on
But for DSI1, we will enable it when its powered ON but not enabled.
Hence i thought its better this way.

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

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

[Dmitry Baryshkov]

On 28/09/2021 04:33, abhinavk@codeaurora.org wrote:
> On 2021-09-27 18:29, Dmitry Baryshkov wrote:
>> 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?
> 
> Just about symmetry. We will enable_irq() for DSI0 when DSI0 and DSI1 
> are powered on
> But for DSI1, we will enable it when its powered ON but not enabled.
> Hence i thought its better this way.

Ah. Then it would be better to call it between power_on() and enable(). 
I'll send v3.

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

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

[Dmitry Baryshkov]

On 28/09/2021 04:33, abhinavk@codeaurora.org wrote:
> On 2021-09-27 18:29, Dmitry Baryshkov wrote:
>> 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?
> 
> Just about symmetry. We will enable_irq() for DSI0 when DSI0 and DSI1 
> are powered on
> But for DSI1, we will enable it when its powered ON but not enabled.
> Hence i thought its better this way.

Ah. Then it would be better to call it between power_on() and enable(). 
I'll send v3.

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

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

[Dmitry Baryshkov]

On 28/09/2021 04:40, Dmitry Baryshkov wrote:
> On 28/09/2021 04:33, abhinavk@codeaurora.org wrote:
>> On 2021-09-27 18:29, Dmitry Baryshkov wrote:
>>> 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?
>>
>> Just about symmetry. We will enable_irq() for DSI0 when DSI0 and DSI1 
>> are powered on
>> But for DSI1, we will enable it when its powered ON but not enabled.
>> Hence i thought its better this way.
> 
> Ah. Then it would be better to call it between power_on() and enable(). 
> I'll send v3.

V3 sent, moving the enable_irq() out of msm_dsi_host_power_on.

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

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

[Dmitry Baryshkov]

On 28/09/2021 04:40, Dmitry Baryshkov wrote:
> On 28/09/2021 04:33, abhinavk@codeaurora.org wrote:
>> On 2021-09-27 18:29, Dmitry Baryshkov wrote:
>>> 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?
>>
>> Just about symmetry. We will enable_irq() for DSI0 when DSI0 and DSI1 
>> are powered on
>> But for DSI1, we will enable it when its powered ON but not enabled.
>> Hence i thought its better this way.
> 
> Ah. Then it would be better to call it between power_on() and enable(). 
> I'll send v3.

V3 sent, moving the enable_irq() out of msm_dsi_host_power_on.

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

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

[Andrzej Hajda]

W dniu 21.09.2021 o 18:22, Dmitry Baryshkov pisze:
> 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.


The established practice is to request IRQ in probe, to avoid 
auto-enabling use IRQF_NO_AUTOEN flag.

Then you can call enable_irq in power-on.


Regards

Andrzej



>
> Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   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;
> +	}
> +
> +
>   	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)

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

[Dmitry Baryshkov]

On 21/09/2021 21:26, Andrzej Hajda wrote:
> W dniu 21.09.2021 o 18:22, Dmitry Baryshkov pisze:
>> 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.
> 
> 
> The established practice is to request IRQ in probe, to avoid
> auto-enabling use IRQF_NO_AUTOEN flag.
> 
> Then you can call enable_irq in power-on.

Nice idea, thank you! I somehow missed addition of IRQF_NO_AUTOEN. I've 
used it in v2 ([PATCH v2] drm/msm/dsi: do not enable irq handler before 
powering up the host).

> 
> 
> Regards
> 
> Andrzej
> 
> 
> 
>>
>> Fixes: a689554ba6ed ("drm/msm: Initial add DSI connector support")
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>    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;
>> +	}
>> +
>> +
>>    	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