Currently the dp_display_irq_handler() is executed at msm_dp_modeset_init()
which ties irq registration to the DPU device's life cycle, while depending on
resources that are released as the DP device is torn down. Move register DP
driver irq handler at dp_display_probe() to have dp_display_irq_handler()
is tied with DP device.
Changes in v3:
-- move calling dp_display_irq_handler() to probe
Signed-off-by: Kuogee Hsieh <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
drivers/gpu/drm/msm/dp/dp_display.h | 1 -
2 files changed, 13 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 76f1395..c217430 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1193,30 +1193,23 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
return ret;
}
-int dp_display_request_irq(struct msm_dp *dp_display)
+static int dp_display_request_irq(struct dp_display_private *dp)
{
int rc = 0;
- struct dp_display_private *dp;
-
- if (!dp_display) {
- DRM_ERROR("invalid input\n");
- return -EINVAL;
- }
-
- dp = container_of(dp_display, struct dp_display_private, dp_display);
+ struct device *dev = &dp->pdev->dev;
- dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
if (!dp->irq) {
- DRM_ERROR("failed to get irq\n");
- return -EINVAL;
+ dp->irq = platform_get_irq(dp->pdev, 0);
+ if (!dp->irq) {
+ DRM_ERROR("failed to get irq\n");
+ return -EINVAL;
+ }
}
- rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
- dp_display_irq_handler,
+ rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
if (rc < 0) {
- DRM_ERROR("failed to request IRQ%u: %d\n",
- dp->irq, rc);
+ DRM_ERROR("failed to request IRQ%u: %d\n", dp->irq, rc);
return rc;
}
@@ -1287,6 +1280,10 @@ static int dp_display_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, &dp->dp_display);
+ rc = dp_display_request_irq(dp);
+ if (rc)
+ return rc;
+
rc = component_add(&pdev->dev, &dp_display_comp_ops);
if (rc) {
DRM_ERROR("component add failed, rc=%d\n", rc);
@@ -1549,12 +1546,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;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index 1e9415a..b3c08de 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -35,7 +35,6 @@ struct msm_dp {
int dp_display_set_plugged_cb(struct msm_dp *dp_display,
hdmi_codec_plugged_cb fn, struct device *codec_dev);
int dp_display_get_modes(struct msm_dp *dp_display);
-int dp_display_request_irq(struct msm_dp *dp_display);
bool dp_display_check_video_test(struct msm_dp *dp_display);
int dp_display_get_test_bpp(struct msm_dp *dp_display);
void dp_display_signal_audio_start(struct msm_dp *dp_display);
--
2.7.4
On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <[email protected]> wrote:
>
> Currently the dp_display_irq_handler() is executed at msm_dp_modeset_init()
> which ties irq registration to the DPU device's life cycle, while depending on
> resources that are released as the DP device is torn down. Move register DP
> driver irq handler at dp_display_probe() to have dp_display_irq_handler()
> is tied with DP device.
>
> Changes in v3:
> -- move calling dp_display_irq_handler() to probe
Was there a changelog for the previous reivions? What is the
difference between v1 and v2?
>
> Signed-off-by: Kuogee Hsieh <[email protected]>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
> drivers/gpu/drm/msm/dp/dp_display.h | 1 -
> 2 files changed, 13 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 76f1395..c217430 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1193,30 +1193,23 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
> return ret;
> }
>
> -int dp_display_request_irq(struct msm_dp *dp_display)
> +static int dp_display_request_irq(struct dp_display_private *dp)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> -
> - if (!dp_display) {
> - DRM_ERROR("invalid input\n");
> - return -EINVAL;
> - }
> -
> - dp = container_of(dp_display, struct dp_display_private, dp_display);
> + struct device *dev = &dp->pdev->dev;
>
> - dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
> if (!dp->irq) {
What is the point in this check?
> - DRM_ERROR("failed to get irq\n");
> - return -EINVAL;
> + dp->irq = platform_get_irq(dp->pdev, 0);
> + if (!dp->irq) {
> + DRM_ERROR("failed to get irq\n");
> + return -EINVAL;
> + }
> }
>
> - rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
> - dp_display_irq_handler,
> + rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
> IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
> if (rc < 0) {
> - DRM_ERROR("failed to request IRQ%u: %d\n",
> - dp->irq, rc);
> + DRM_ERROR("failed to request IRQ%u: %d\n", dp->irq, rc);
> return rc;
> }
>
> @@ -1287,6 +1280,10 @@ static int dp_display_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, &dp->dp_display);
>
> + rc = dp_display_request_irq(dp);
> + if (rc)
> + return rc;
This way the IRQ ends up being enabled in _probe. Are we ready to
handle it here? Is the DP device fully setup at this moment?
> +
> rc = component_add(&pdev->dev, &dp_display_comp_ops);
> if (rc) {
> DRM_ERROR("component add failed, rc=%d\n", rc);
> @@ -1549,12 +1546,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;
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index 1e9415a..b3c08de 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -35,7 +35,6 @@ struct msm_dp {
> int dp_display_set_plugged_cb(struct msm_dp *dp_display,
> hdmi_codec_plugged_cb fn, struct device *codec_dev);
> int dp_display_get_modes(struct msm_dp *dp_display);
> -int dp_display_request_irq(struct msm_dp *dp_display);
> bool dp_display_check_video_test(struct msm_dp *dp_display);
> int dp_display_get_test_bpp(struct msm_dp *dp_display);
> void dp_display_signal_audio_start(struct msm_dp *dp_display);
> --
> 2.7.4
>
--
With best wishes
Dmitry
On 9/15/2023 5:29 PM, Dmitry Baryshkov wrote:
> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <[email protected]> wrote:
>> Currently the dp_display_irq_handler() is executed at msm_dp_modeset_init()
>> which ties irq registration to the DPU device's life cycle, while depending on
>> resources that are released as the DP device is torn down. Move register DP
>> driver irq handler at dp_display_probe() to have dp_display_irq_handler()
>> is tied with DP device.
>>
>> Changes in v3:
>> -- move calling dp_display_irq_handler() to probe
> Was there a changelog for the previous reivions? What is the
> difference between v1 and v2?
Sorry, v2 is same as v3.
I submitted v2 first but found i forget to add change logs from review
comments of v1.
Therefore i submit v3 to add changes logs which missing at v2.
>
>> Signed-off-by: Kuogee Hsieh <[email protected]>
>> ---
>> drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
>> drivers/gpu/drm/msm/dp/dp_display.h | 1 -
>> 2 files changed, 13 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 76f1395..c217430 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1193,30 +1193,23 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
>> return ret;
>> }
>>
>> -int dp_display_request_irq(struct msm_dp *dp_display)
>> +static int dp_display_request_irq(struct dp_display_private *dp)
>> {
>> int rc = 0;
>> - struct dp_display_private *dp;
>> -
>> - if (!dp_display) {
>> - DRM_ERROR("invalid input\n");
>> - return -EINVAL;
>> - }
>> -
>> - dp = container_of(dp_display, struct dp_display_private, dp_display);
>> + struct device *dev = &dp->pdev->dev;
>>
>> - dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
>> if (!dp->irq) {
> What is the point in this check?
>
>> - DRM_ERROR("failed to get irq\n");
>> - return -EINVAL;
>> + dp->irq = platform_get_irq(dp->pdev, 0);
>> + if (!dp->irq) {
>> + DRM_ERROR("failed to get irq\n");
>> + return -EINVAL;
>> + }
>> }
>>
>> - rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
>> - dp_display_irq_handler,
>> + rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
>> IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
>> if (rc < 0) {
>> - DRM_ERROR("failed to request IRQ%u: %d\n",
>> - dp->irq, rc);
>> + DRM_ERROR("failed to request IRQ%u: %d\n", dp->irq, rc);
>> return rc;
>> }
>>
>> @@ -1287,6 +1280,10 @@ static int dp_display_probe(struct platform_device *pdev)
>>
>> platform_set_drvdata(pdev, &dp->dp_display);
>>
>> + rc = dp_display_request_irq(dp);
>> + if (rc)
>> + return rc;
> This way the IRQ ends up being enabled in _probe. Are we ready to
> handle it here? Is the DP device fully setup at this moment?
>
>> +
>> rc = component_add(&pdev->dev, &dp_display_comp_ops);
>> if (rc) {
>> DRM_ERROR("component add failed, rc=%d\n", rc);
>> @@ -1549,12 +1546,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;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>> index 1e9415a..b3c08de 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -35,7 +35,6 @@ struct msm_dp {
>> int dp_display_set_plugged_cb(struct msm_dp *dp_display,
>> hdmi_codec_plugged_cb fn, struct device *codec_dev);
>> int dp_display_get_modes(struct msm_dp *dp_display);
>> -int dp_display_request_irq(struct msm_dp *dp_display);
>> bool dp_display_check_video_test(struct msm_dp *dp_display);
>> int dp_display_get_test_bpp(struct msm_dp *dp_display);
>> void dp_display_signal_audio_start(struct msm_dp *dp_display);
>> --
>> 2.7.4
>>
>
On Mon, 18 Sept 2023 at 20:03, Kuogee Hsieh <[email protected]> wrote:
>
>
> On 9/15/2023 5:29 PM, Dmitry Baryshkov wrote:
> > On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <[email protected]> wrote:
> >> Currently the dp_display_irq_handler() is executed at msm_dp_modeset_init()
> >> which ties irq registration to the DPU device's life cycle, while depending on
> >> resources that are released as the DP device is torn down. Move register DP
> >> driver irq handler at dp_display_probe() to have dp_display_irq_handler()
> >> is tied with DP device.
> >>
> >> Changes in v3:
> >> -- move calling dp_display_irq_handler() to probe
> > Was there a changelog for the previous reivions? What is the
> > difference between v1 and v2?
>
> Sorry, v2 is same as v3.
>
> I submitted v2 first but found i forget to add change logs from review
> comments of v1.
>
> Therefore i submit v3 to add changes logs which missing at v2.
This doesn't consist of a change. It should have been a RESEND or just
responding with the changelog. V3 means new revision.
>
>
>
> >
> >> Signed-off-by: Kuogee Hsieh <[email protected]>
> >> ---
> >> drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
> >> drivers/gpu/drm/msm/dp/dp_display.h | 1 -
> >> 2 files changed, 13 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 76f1395..c217430 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1193,30 +1193,23 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
> >> return ret;
> >> }
> >>
> >> -int dp_display_request_irq(struct msm_dp *dp_display)
> >> +static int dp_display_request_irq(struct dp_display_private *dp)
> >> {
> >> int rc = 0;
> >> - struct dp_display_private *dp;
> >> -
> >> - if (!dp_display) {
> >> - DRM_ERROR("invalid input\n");
> >> - return -EINVAL;
> >> - }
> >> -
> >> - dp = container_of(dp_display, struct dp_display_private, dp_display);
> >> + struct device *dev = &dp->pdev->dev;
> >>
> >> - dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
> >> if (!dp->irq) {
> > What is the point in this check?
> >
> >> - DRM_ERROR("failed to get irq\n");
> >> - return -EINVAL;
> >> + dp->irq = platform_get_irq(dp->pdev, 0);
> >> + if (!dp->irq) {
> >> + DRM_ERROR("failed to get irq\n");
> >> + return -EINVAL;
> >> + }
> >> }
> >>
> >> - rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
> >> - dp_display_irq_handler,
> >> + rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
> >> IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
> >> if (rc < 0) {
> >> - DRM_ERROR("failed to request IRQ%u: %d\n",
> >> - dp->irq, rc);
> >> + DRM_ERROR("failed to request IRQ%u: %d\n", dp->irq, rc);
> >> return rc;
> >> }
> >>
> >> @@ -1287,6 +1280,10 @@ static int dp_display_probe(struct platform_device *pdev)
> >>
> >> platform_set_drvdata(pdev, &dp->dp_display);
> >>
> >> + rc = dp_display_request_irq(dp);
> >> + if (rc)
> >> + return rc;
> > This way the IRQ ends up being enabled in _probe. Are we ready to
> > handle it here? Is the DP device fully setup at this moment?
> >
> >> +
> >> rc = component_add(&pdev->dev, &dp_display_comp_ops);
> >> if (rc) {
> >> DRM_ERROR("component add failed, rc=%d\n", rc);
> >> @@ -1549,12 +1546,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;
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> >> index 1e9415a..b3c08de 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> >> @@ -35,7 +35,6 @@ struct msm_dp {
> >> int dp_display_set_plugged_cb(struct msm_dp *dp_display,
> >> hdmi_codec_plugged_cb fn, struct device *codec_dev);
> >> int dp_display_get_modes(struct msm_dp *dp_display);
> >> -int dp_display_request_irq(struct msm_dp *dp_display);
> >> bool dp_display_check_video_test(struct msm_dp *dp_display);
> >> int dp_display_get_test_bpp(struct msm_dp *dp_display);
> >> void dp_display_signal_audio_start(struct msm_dp *dp_display);
> >> --
> >> 2.7.4
> >>
> >
--
With best wishes
Dmitry
On 9/15/2023 5:29 PM, Dmitry Baryshkov wrote:
> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <[email protected]> wrote:
>> Currently the dp_display_irq_handler() is executed at msm_dp_modeset_init()
>> which ties irq registration to the DPU device's life cycle, while depending on
>> resources that are released as the DP device is torn down. Move register DP
>> driver irq handler at dp_display_probe() to have dp_display_irq_handler()
>> is tied with DP device.
>>
>> Changes in v3:
>> -- move calling dp_display_irq_handler() to probe
> Was there a changelog for the previous reivions? What is the
> difference between v1 and v2?
>
>> Signed-off-by: Kuogee Hsieh <[email protected]>
>> ---
>> drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
>> drivers/gpu/drm/msm/dp/dp_display.h | 1 -
>> 2 files changed, 13 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 76f1395..c217430 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -1193,30 +1193,23 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
>> return ret;
>> }
>>
>> -int dp_display_request_irq(struct msm_dp *dp_display)
>> +static int dp_display_request_irq(struct dp_display_private *dp)
>> {
>> int rc = 0;
>> - struct dp_display_private *dp;
>> -
>> - if (!dp_display) {
>> - DRM_ERROR("invalid input\n");
>> - return -EINVAL;
>> - }
>> -
>> - dp = container_of(dp_display, struct dp_display_private, dp_display);
>> + struct device *dev = &dp->pdev->dev;
>>
>> - dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
>> if (!dp->irq) {
> What is the point in this check?
>
>> - DRM_ERROR("failed to get irq\n");
>> - return -EINVAL;
>> + dp->irq = platform_get_irq(dp->pdev, 0);
>> + if (!dp->irq) {
>> + DRM_ERROR("failed to get irq\n");
>> + return -EINVAL;
>> + }
>> }
>>
>> - rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
>> - dp_display_irq_handler,
>> + rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
>> IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
>> if (rc < 0) {
>> - DRM_ERROR("failed to request IRQ%u: %d\n",
>> - dp->irq, rc);
>> + DRM_ERROR("failed to request IRQ%u: %d\n", dp->irq, rc);
>> return rc;
>> }
>>
>> @@ -1287,6 +1280,10 @@ static int dp_display_probe(struct platform_device *pdev)
>>
>> platform_set_drvdata(pdev, &dp->dp_display);
>>
>> + rc = dp_display_request_irq(dp);
>> + if (rc)
>> + return rc;
> This way the IRQ ends up being enabled in _probe. Are we ready to
> handle it here? Is the DP device fully setup at this moment?
The irq is enabled here.
but DP driver hpd hardware block has not yet be enabled. this means no
irq will be delivered.
.hpd_enable() will call pm_runtime_resume_and_get() and
dp_catalog_ctrl_hpd_enable().
after .hpd_enable() irq will be delivered and handled properly.
>> +
>> rc = component_add(&pdev->dev, &dp_display_comp_ops);
>> if (rc) {
>> DRM_ERROR("component add failed, rc=%d\n", rc);
>> @@ -1549,12 +1546,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;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>> index 1e9415a..b3c08de 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -35,7 +35,6 @@ struct msm_dp {
>> int dp_display_set_plugged_cb(struct msm_dp *dp_display,
>> hdmi_codec_plugged_cb fn, struct device *codec_dev);
>> int dp_display_get_modes(struct msm_dp *dp_display);
>> -int dp_display_request_irq(struct msm_dp *dp_display);
>> bool dp_display_check_video_test(struct msm_dp *dp_display);
>> int dp_display_get_test_bpp(struct msm_dp *dp_display);
>> void dp_display_signal_audio_start(struct msm_dp *dp_display);
>> --
>> 2.7.4
>>
>
On Sat, 23 Sept 2023 at 02:03, Kuogee Hsieh <[email protected]> wrote:
>
>
> On 9/15/2023 5:29 PM, Dmitry Baryshkov wrote:
> > On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <[email protected]> wrote:
> >> Currently the dp_display_irq_handler() is executed at msm_dp_modeset_init()
> >> which ties irq registration to the DPU device's life cycle, while depending on
> >> resources that are released as the DP device is torn down. Move register DP
> >> driver irq handler at dp_display_probe() to have dp_display_irq_handler()
> >> is tied with DP device.
> >>
> >> Changes in v3:
> >> -- move calling dp_display_irq_handler() to probe
> > Was there a changelog for the previous reivions? What is the
> > difference between v1 and v2?
> >
> >> Signed-off-by: Kuogee Hsieh <[email protected]>
> >> ---
> >> drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
> >> drivers/gpu/drm/msm/dp/dp_display.h | 1 -
> >> 2 files changed, 13 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 76f1395..c217430 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -1193,30 +1193,23 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
> >> return ret;
> >> }
> >>
> >> -int dp_display_request_irq(struct msm_dp *dp_display)
> >> +static int dp_display_request_irq(struct dp_display_private *dp)
> >> {
> >> int rc = 0;
> >> - struct dp_display_private *dp;
> >> -
> >> - if (!dp_display) {
> >> - DRM_ERROR("invalid input\n");
> >> - return -EINVAL;
> >> - }
> >> -
> >> - dp = container_of(dp_display, struct dp_display_private, dp_display);
> >> + struct device *dev = &dp->pdev->dev;
> >>
> >> - dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
> >> if (!dp->irq) {
> > What is the point in this check?
> >
> >> - DRM_ERROR("failed to get irq\n");
> >> - return -EINVAL;
> >> + dp->irq = platform_get_irq(dp->pdev, 0);
> >> + if (!dp->irq) {
> >> + DRM_ERROR("failed to get irq\n");
> >> + return -EINVAL;
> >> + }
> >> }
> >>
> >> - rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
> >> - dp_display_irq_handler,
> >> + rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
> >> IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
> >> if (rc < 0) {
> >> - DRM_ERROR("failed to request IRQ%u: %d\n",
> >> - dp->irq, rc);
> >> + DRM_ERROR("failed to request IRQ%u: %d\n", dp->irq, rc);
> >> return rc;
> >> }
> >>
> >> @@ -1287,6 +1280,10 @@ static int dp_display_probe(struct platform_device *pdev)
> >>
> >> platform_set_drvdata(pdev, &dp->dp_display);
> >>
> >> + rc = dp_display_request_irq(dp);
> >> + if (rc)
> >> + return rc;
> > This way the IRQ ends up being enabled in _probe. Are we ready to
> > handle it here? Is the DP device fully setup at this moment?
>
> The irq is enabled here.
>
> but DP driver hpd hardware block has not yet be enabled. this means no
> irq will be delivered.
There are other IRQ kinds, not only just HPD ones.
>
> .hpd_enable() will call pm_runtime_resume_and_get() and
> dp_catalog_ctrl_hpd_enable().
>
> after .hpd_enable() irq will be delivered and handled properly.
>
>
>
> >> +
> >> rc = component_add(&pdev->dev, &dp_display_comp_ops);
> >> if (rc) {
> >> DRM_ERROR("component add failed, rc=%d\n", rc);
> >> @@ -1549,12 +1546,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;
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> >> index 1e9415a..b3c08de 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> >> @@ -35,7 +35,6 @@ struct msm_dp {
> >> int dp_display_set_plugged_cb(struct msm_dp *dp_display,
> >> hdmi_codec_plugged_cb fn, struct device *codec_dev);
> >> int dp_display_get_modes(struct msm_dp *dp_display);
> >> -int dp_display_request_irq(struct msm_dp *dp_display);
> >> bool dp_display_check_video_test(struct msm_dp *dp_display);
> >> int dp_display_get_test_bpp(struct msm_dp *dp_display);
> >> void dp_display_signal_audio_start(struct msm_dp *dp_display);
> >> --
> >> 2.7.4
> >>
> >
--
With best wishes
Dmitry
On 9/23/2023 11:45 AM, Dmitry Baryshkov wrote:
> On Sat, 23 Sept 2023 at 02:03, Kuogee Hsieh <[email protected]> wrote:
>>
>> On 9/15/2023 5:29 PM, Dmitry Baryshkov wrote:
>>> On Sat, 16 Sept 2023 at 00:38, Kuogee Hsieh <[email protected]> wrote:
>>>> Currently the dp_display_irq_handler() is executed at msm_dp_modeset_init()
>>>> which ties irq registration to the DPU device's life cycle, while depending on
>>>> resources that are released as the DP device is torn down. Move register DP
>>>> driver irq handler at dp_display_probe() to have dp_display_irq_handler()
>>>> is tied with DP device.
>>>>
>>>> Changes in v3:
>>>> -- move calling dp_display_irq_handler() to probe
>>> Was there a changelog for the previous reivions? What is the
>>> difference between v1 and v2?
>>>
>>>> Signed-off-by: Kuogee Hsieh <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/msm/dp/dp_display.c | 35 +++++++++++++----------------------
>>>> drivers/gpu/drm/msm/dp/dp_display.h | 1 -
>>>> 2 files changed, 13 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index 76f1395..c217430 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -1193,30 +1193,23 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
>>>> return ret;
>>>> }
>>>>
>>>> -int dp_display_request_irq(struct msm_dp *dp_display)
>>>> +static int dp_display_request_irq(struct dp_display_private *dp)
>>>> {
>>>> int rc = 0;
>>>> - struct dp_display_private *dp;
>>>> -
>>>> - if (!dp_display) {
>>>> - DRM_ERROR("invalid input\n");
>>>> - return -EINVAL;
>>>> - }
>>>> -
>>>> - dp = container_of(dp_display, struct dp_display_private, dp_display);
>>>> + struct device *dev = &dp->pdev->dev;
>>>>
>>>> - dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
>>>> if (!dp->irq) {
>>> What is the point in this check?
>>>
>>>> - DRM_ERROR("failed to get irq\n");
>>>> - return -EINVAL;
>>>> + dp->irq = platform_get_irq(dp->pdev, 0);
>>>> + if (!dp->irq) {
>>>> + DRM_ERROR("failed to get irq\n");
>>>> + return -EINVAL;
>>>> + }
>>>> }
>>>>
>>>> - rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
>>>> - dp_display_irq_handler,
>>>> + rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
>>>> IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
>>>> if (rc < 0) {
>>>> - DRM_ERROR("failed to request IRQ%u: %d\n",
>>>> - dp->irq, rc);
>>>> + DRM_ERROR("failed to request IRQ%u: %d\n", dp->irq, rc);
>>>> return rc;
>>>> }
>>>>
>>>> @@ -1287,6 +1280,10 @@ static int dp_display_probe(struct platform_device *pdev)
>>>>
>>>> platform_set_drvdata(pdev, &dp->dp_display);
>>>>
>>>> + rc = dp_display_request_irq(dp);
>>>> + if (rc)
>>>> + return rc;
>>> This way the IRQ ends up being enabled in _probe. Are we ready to
>>> handle it here? Is the DP device fully setup at this moment?
>> The irq is enabled here.
>>
>> but DP driver hpd hardware block has not yet be enabled. this means no
>> irq will be delivered.
> There are other IRQ kinds, not only just HPD ones.
pm_runtime_resume_and_get() will enable host controller (including hpd and aux block).
so that as long as pm_runtime_resume_and_get() called, then all DP related interrupts will be handled accordingly.
>
>> .hpd_enable() will call pm_runtime_resume_and_get() and
>> dp_catalog_ctrl_hpd_enable().
>>
>> after .hpd_enable() irq will be delivered and handled properly.
>>
>>
>>
>>>> +
>>>> rc = component_add(&pdev->dev, &dp_display_comp_ops);
>>>> if (rc) {
>>>> DRM_ERROR("component add failed, rc=%d\n", rc);
>>>> @@ -1549,12 +1546,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;
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
>>>> index 1e9415a..b3c08de 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>>>> @@ -35,7 +35,6 @@ struct msm_dp {
>>>> int dp_display_set_plugged_cb(struct msm_dp *dp_display,
>>>> hdmi_codec_plugged_cb fn, struct device *codec_dev);
>>>> int dp_display_get_modes(struct msm_dp *dp_display);
>>>> -int dp_display_request_irq(struct msm_dp *dp_display);
>>>> bool dp_display_check_video_test(struct msm_dp *dp_display);
>>>> int dp_display_get_test_bpp(struct msm_dp *dp_display);
>>>> void dp_display_signal_audio_start(struct msm_dp *dp_display);
>>>> --
>>>> 2.7.4
>>>>
>
>