[RFC PATCH 0/3] drm/msm/dp: several fixes for the IRQ handling

[RFC PATCH 0/3] drm/msm/dp: several fixes for the IRQ handling

[Dmitry Baryshkov]

Johan Hovold has reported that returning a probe deferral from the
msm_dp_modeset_init() can cause issues because the IRQ is not freed
properly. This (compile-tested only) series tries to fix the issue by
moving devm_request_irq() to the probe callback.

Dmitry Baryshkov (3):
  drm/msm/dp: fold disable_irq into devm_request_irq
  drm/msm/dp: switch to using platform_get_irq()
  drm/msm/dp: move dp_request_irq() call to dp_display_probe()

 drivers/gpu/drm/msm/dp/dp_display.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

-- 
2.35.1

[RFC PATCH 1/3] drm/msm/dp: fold disable_irq into devm_request_irq

[Dmitry Baryshkov]

Calling disable_irq() right after devm_request_irq() is a bad practice.
it leaves a small window when the driver doesn't expect the IRQ, but the
hardware still can trigger it. Use the IRQF_NO_AUTOEN flag to prevent
the request_irq from enabling the IRQ line.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bfd0aeff3f0d..3173e6962a78 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1251,13 +1251,12 @@ int dp_display_request_irq(struct msm_dp *dp_display)
 
 	rc = devm_request_irq(&dp->pdev->dev, dp->irq,
 			dp_display_irq_handler,
-			IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
+			IRQF_TRIGGER_HIGH | IRQF_NO_AUTOEN, "dp_display_isr", dp);
 	if (rc < 0) {
 		DRM_ERROR("failed to request IRQ%u: %d\n",
 				dp->irq, rc);
 		return rc;
 	}
-	disable_irq(dp->irq);
 
 	return 0;
 }
-- 
2.35.1

Re: [RFC PATCH 1/3] drm/msm/dp: fold disable_irq into devm_request_irq

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-09-01 02:15:25)
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bfd0aeff3f0d..3173e6962a78 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1251,13 +1251,12 @@ int dp_display_request_irq(struct msm_dp *dp_display)
>
>         rc = devm_request_irq(&dp->pdev->dev, dp->irq,
>                         dp_display_irq_handler,
> -                       IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
> +                       IRQF_TRIGGER_HIGH | IRQF_NO_AUTOEN, "dp_display_isr", dp);
>         if (rc < 0) {
>                 DRM_ERROR("failed to request IRQ%u: %d\n",
>                                 dp->irq, rc);
>                 return rc;
>         }
> -       disable_irq(dp->irq);

It would be better to not disable the irq at all and mask the irq in the
hardware before requesting the irq. Can you add at least add a TODO for
that?

[RFC PATCH 2/3] drm/msm/dp: switch to using platform_get_irq()

[Dmitry Baryshkov]

There is little point in using irq_of_parse_and_map(). Switch to plain
and usual platform_get_irq() for parsing the DP IRQ line.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 3173e6962a78..40c7f91abec9 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1243,8 +1243,8 @@ int dp_display_request_irq(struct msm_dp *dp_display)
 
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 
-	dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
-	if (!dp->irq) {
+	dp->irq = platform_get_irq(dp->pdev, 0);
+	if (dp->irq < 0) {
 		DRM_ERROR("failed to get irq\n");
 		return -EINVAL;
 	}
-- 
2.35.1

Re: [RFC PATCH 2/3] drm/msm/dp: switch to using platform_get_irq()

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-09-01 02:15:26)
> There is little point in using irq_of_parse_and_map(). Switch to plain
> and usual platform_get_irq() for parsing the DP IRQ line.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 3173e6962a78..40c7f91abec9 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1243,8 +1243,8 @@ int dp_display_request_irq(struct msm_dp *dp_display)
>
>         dp = container_of(dp_display, struct dp_display_private, dp_display);
>
> -       dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
> -       if (!dp->irq) {
> +       dp->irq = platform_get_irq(dp->pdev, 0);
> +       if (dp->irq < 0) {
>                 DRM_ERROR("failed to get irq\n");

Can drop the error as well because platform_get_irq() prints an error on
failure.

[RFC PATCH 3/3] drm/msm/dp: move dp_request_irq() call to dp_display_probe()

[Dmitry Baryshkov]

As the MDSS registers the IRQ domain before populating child devices,
there is little point in deferring the IRQ request up to the
msm_dp_modeset_init(). Following the 'get resources as early as
possible' paradigm, move dp_request_irq() call to dp_display_probe().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/dp/dp_display.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 40c7f91abec9..0fb3cb773bec 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1309,6 +1309,12 @@ static int dp_display_probe(struct platform_device *pdev)
 	dp->dp_display.is_edp =
 		(dp->dp_display.connector_type == DRM_MODE_CONNECTOR_eDP);
 
+	rc = dp_display_request_irq(&dp->dp_display);
+	if (rc) {
+		DRM_ERROR("request_irq failed, ret=%d\n", rc);
+		return rc;
+	}
+
 	rc = dp_init_sub_modules(dp);
 	if (rc) {
 		DRM_ERROR("init sub module failed\n");
@@ -1600,12 +1606,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
 
 	dp_priv = container_of(dp_display, struct dp_display_private, dp_display);
 
-	ret = dp_display_request_irq(dp_display);
-	if (ret) {
-		DRM_ERROR("request_irq failed, ret=%d\n", ret);
-		return ret;
-	}
-
 	ret = dp_display_get_next_bridge(dp_display);
 	if (ret)
 		return ret;
-- 
2.35.1

Re: [RFC PATCH 3/3] drm/msm/dp: move dp_request_irq() call to dp_display_probe()

[Stephen Boyd]

Quoting Dmitry Baryshkov (2022-09-01 02:15:27)
> As the MDSS registers the IRQ domain before populating child devices,
> there is little point in deferring the IRQ request up to the
> msm_dp_modeset_init(). Following the 'get resources as early as
> possible' paradigm, move dp_request_irq() call to dp_display_probe().
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

Reviewed-by: Stephen Boyd <swboyd@chromium.org>

Re: [RFC PATCH 0/3] drm/msm/dp: several fixes for the IRQ handling

[Johan Hovold]

On Thu, Sep 01, 2022 at 12:15:24PM +0300, Dmitry Baryshkov wrote:
> Johan Hovold has reported that returning a probe deferral from the
> msm_dp_modeset_init() can cause issues because the IRQ is not freed
> properly. This (compile-tested only) series tries to fix the issue by
> moving devm_request_irq() to the probe callback.

Please try to reproduce the issue yourself before posting untested RFCs.
We're all short on time. 

Johan

Re: [RFC PATCH 0/3] drm/msm/dp: several fixes for the IRQ handling

[Dmitry Baryshkov]

On 01/09/2022 12:20, Johan Hovold wrote:
> On Thu, Sep 01, 2022 at 12:15:24PM +0300, Dmitry Baryshkov wrote:
>> Johan Hovold has reported that returning a probe deferral from the
>> msm_dp_modeset_init() can cause issues because the IRQ is not freed
>> properly. This (compile-tested only) series tries to fix the issue by
>> moving devm_request_irq() to the probe callback.
> 
> Please try to reproduce the issue yourself before posting untested RFCs.
> We're all short on time.

I do not have a working DP setup. Thus it's either this, or nothing.

-- 
With best wishes
Dmitry

Re: [RFC PATCH 0/3] drm/msm/dp: several fixes for the IRQ handling

[Johan Hovold]

On Thu, Sep 01, 2022 at 12:21:36PM +0300, Dmitry Baryshkov wrote:
> On 01/09/2022 12:20, Johan Hovold wrote:
> > On Thu, Sep 01, 2022 at 12:15:24PM +0300, Dmitry Baryshkov wrote:
> >> Johan Hovold has reported that returning a probe deferral from the
> >> msm_dp_modeset_init() can cause issues because the IRQ is not freed
> >> properly. This (compile-tested only) series tries to fix the issue by
> >> moving devm_request_irq() to the probe callback.
> > 
> > Please try to reproduce the issue yourself before posting untested RFCs.
> > We're all short on time.
> 
> I do not have a working DP setup. Thus it's either this, or nothing.

Ok, you could have mentioned that as the above sounds a bit lazy.

I don't have time to work on this right now as I mentioned elsewhere.
Avoiding probe deferral by fiddling with the config serves as a fragile
workaround until then.

Johan

Re: [RFC PATCH 0/3] drm/msm/dp: several fixes for the IRQ handling

[Manivannan Sadhasivam]

On Thu, Sep 01, 2022 at 12:15:24PM +0300, Dmitry Baryshkov wrote:
> Johan Hovold has reported that returning a probe deferral from the
> msm_dp_modeset_init() can cause issues because the IRQ is not freed
> properly. This (compile-tested only) series tries to fix the issue by
> moving devm_request_irq() to the probe callback.
> 

This series fixes the probe deferral issue on Lenovo Thinkpad X13s. But I
didn't look close enough to verify if all other resource deallocation are just
fine.

Thanks for the quick series, Dmitry!

Regards,
Mani

> Dmitry Baryshkov (3):
>   drm/msm/dp: fold disable_irq into devm_request_irq
>   drm/msm/dp: switch to using platform_get_irq()
>   drm/msm/dp: move dp_request_irq() call to dp_display_probe()
> 
>  drivers/gpu/drm/msm/dp/dp_display.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> -- 
> 2.35.1
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [RFC PATCH 0/3] drm/msm/dp: several fixes for the IRQ handling

[Johan Hovold]

On Thu, Sep 01, 2022 at 12:15:24PM +0300, Dmitry Baryshkov wrote:
> Johan Hovold has reported that returning a probe deferral from the
> msm_dp_modeset_init() can cause issues because the IRQ is not freed
> properly. This (compile-tested only) series tries to fix the issue by
> moving devm_request_irq() to the probe callback.

For reference, here's an analysis of the underlying problem and a series
of fixes that addresses this:

	https://lore.kernel.org/all/20220913085320.8577-1-johan+linaro@kernel.org/

Note that moving the irq request to probe also fixes the immediate
issue, but that can now be done as a follow-on cleanup (optimisation)
instead.

> Dmitry Baryshkov (3):
>   drm/msm/dp: fold disable_irq into devm_request_irq
>   drm/msm/dp: switch to using platform_get_irq()
>   drm/msm/dp: move dp_request_irq() call to dp_display_probe()
> 
>  drivers/gpu/drm/msm/dp/dp_display.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)

Johan