| Message ID | 1688773943-3887-1-git-send-email-quic_khsieh@quicinc.com |
|---|---|
| Headers | show |
| Series | incorporate pm runtime framework and eDP clean up | expand |
On 08/07/2023 02:52, Kuogee Hsieh wrote: > EV_HPD_INIT_SETUP flag is used to trigger the initialization of external > DP host controller. Since external DP host controller initialization had > been incorporated into pm_runtime_resume(), this flag become obsolete. > Lets get rid of it. > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_display.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 2c5706a..44580c2 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -55,7 +55,6 @@ enum { > enum { > EV_NO_EVENT, > /* hpd events */ > - EV_HPD_INIT_SETUP, > EV_HPD_PLUG_INT, > EV_IRQ_HPD_INT, > EV_HPD_UNPLUG_INT, > @@ -1119,9 +1118,6 @@ static int hpd_event_thread(void *data) > spin_unlock_irqrestore(&dp_priv->event_lock, flag); > > switch (todo->event_id) { > - case EV_HPD_INIT_SETUP: > - dp_display_host_init(dp_priv); > - break; > case EV_HPD_PLUG_INT: > dp_hpd_plug_handle(dp_priv, todo->data); > break; > @@ -1483,15 +1479,7 @@ void __exit msm_dp_unregister(void) > > void msm_dp_irq_postinstall(struct msm_dp *dp_display) > { > - struct dp_display_private *dp; > - > - if (!dp_display) > - return; > - > - dp = container_of(dp_display, struct dp_display_private, dp_display); > > - if (!dp_display->is_edp) > - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0); > } Why do you keep an empty function? > > bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
On 08/07/2023 02:52, Kuogee Hsieh wrote: > In preparation of moving edp of_dp_aux_populate_bus() to > dp_display_probe(), move dp_display_request_irq(), > dp->parser->parse() and dp_power_client_init() to dp_display_probe() > too. > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_display.c | 48 +++++++++++++++++-------------------- > drivers/gpu/drm/msm/dp/dp_display.h | 1 - > 2 files changed, 22 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 44580c2..185f1eb 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -290,12 +290,6 @@ static int dp_display_bind(struct device *dev, struct device *master, > goto end; > } > > - rc = dp_power_client_init(dp->power); > - if (rc) { > - DRM_ERROR("Power client create failed\n"); > - goto end; > - } > - > rc = dp_register_audio_driver(dev, dp->audio); > if (rc) { > DRM_ERROR("Audio registration Dp failed\n"); > @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp) > goto error; > } > > + rc = dp->parser->parse(dp->parser); > + if (rc) { > + DRM_ERROR("device tree parsing failed\n"); > + goto error; > + } > + > dp->catalog = dp_catalog_get(dev, &dp->parser->io); > if (IS_ERR(dp->catalog)) { > rc = PTR_ERR(dp->catalog); > @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp) > goto error; > } > > + rc = dp_power_client_init(dp->power); > + if (rc) { > + DRM_ERROR("Power client create failed\n"); > + goto error; > + } > + > dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp); > if (IS_ERR(dp->aux)) { > rc = PTR_ERR(dp->aux); > @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0); > + if (!dp->irq) { > + DRM_ERROR("failed to get irq\n"); > + return -EINVAL; > + } > } Use platform_get_irq() from probe() function. > > - 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", > @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, &dp->dp_display); > > + dp_display_request_irq(dp); > + Error checking? Are we completely ready to handle interrupts at this point? > rc = component_add(&pdev->dev, &dp_display_comp_ops); > if (rc) { > DRM_ERROR("component add failed, rc=%d\n", rc); > @@ -1574,12 +1576,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);
On 08/07/2023 02:52, Kuogee Hsieh wrote: > Move of_dp_aux_populate_bus() to dp_display_probe() for eDP > from dp_display_bind() so that probe deferral cases can be > handled effectively > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_aux.c | 25 ++++++++++++ > drivers/gpu/drm/msm/dp/dp_display.c | 79 +++++++++++++++++++------------------ > 2 files changed, 65 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c > index c592064..c1baffb 100644 > --- a/drivers/gpu/drm/msm/dp/dp_aux.c > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > @@ -505,6 +505,21 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux) > drm_dp_aux_unregister(dp_aux); > } > > +static int dp_wait_hpd_asserted(struct drm_dp_aux *dp_aux, > + unsigned long wait_us) > +{ > + int ret; > + struct dp_aux_private *aux; > + > + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > + > + pm_runtime_get_sync(aux->dev); > + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); > + pm_runtime_put_sync(aux->dev); > + > + return ret; > +} > + > struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog, > bool is_edp) > { > @@ -528,6 +543,16 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog, > aux->catalog = catalog; > aux->retry_cnt = 0; > > + /* > + * Use the drm_dp_aux_init() to use the aux adapter > + * before registering aux with the DRM device. > + */ > + aux->dp_aux.name = "dpu_dp_aux"; > + aux->dp_aux.dev = dev; > + aux->dp_aux.transfer = dp_aux_transfer; > + aux->dp_aux.wait_hpd_asserted = dp_wait_hpd_asserted; > + drm_dp_aux_init(&aux->dp_aux); > + > return &aux->dp_aux; > } > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 185f1eb..7ed4bea 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -302,10 +302,6 @@ static int dp_display_bind(struct device *dev, struct device *master, > goto end; > } > > - pm_runtime_enable(dev); > - pm_runtime_set_autosuspend_delay(dev, 1000); > - pm_runtime_use_autosuspend(dev); > - > return 0; > end: > return rc; > @@ -322,8 +318,6 @@ static void dp_display_unbind(struct device *dev, struct device *master, > > kthread_stop(dp->ev_tsk); > > - of_dp_aux_depopulate_bus(dp->aux); > - > dp_power_client_deinit(dp->power); > dp_unregister_audio_driver(dev, dp->audio); > dp_aux_unregister(dp->aux); > @@ -1245,6 +1239,29 @@ static const struct msm_dp_desc *dp_display_get_desc(struct platform_device *pde > return NULL; > } > > +static void of_dp_aux_depopulate_bus_void(void *data) > +{ > + of_dp_aux_depopulate_bus(data); > +} > + > +static int dp_display_auxbus_emulation(struct dp_display_private *dp) Why is it called emulation? > +{ > + struct device *dev = &dp->pdev->dev; > + struct device_node *aux_bus; > + int ret = 0; > + > + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); > + > + if (aux_bus) { > + ret = devm_of_dp_aux_populate_bus(dp->aux, NULL); And here you missed the whole point of why we have been asking for. Please add a sensible `done_probing' callback, which will call component_add(). This way the DP component will only be registered when the panel has been probed. Keeping us from the component binding retries and corresponding side effects. > + > + devm_add_action_or_reset(dev, of_dp_aux_depopulate_bus_void, > + dp->aux); Useless, it's already handled by the devm_ part of the devm_of_dp_aux_populate_bus(). > + } > + > + return ret; > +} > + > static int dp_display_probe(struct platform_device *pdev) > { > int rc = 0; > @@ -1290,8 +1307,18 @@ static int dp_display_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, &dp->dp_display); > > + pm_runtime_enable(&pdev->dev); > + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); > + pm_runtime_use_autosuspend(&pdev->dev); Can we have this in probe right from the patch #2? > + > dp_display_request_irq(dp); > > + if (dp->dp_display.is_edp) { > + rc = dp_display_auxbus_emulation(dp); > + if (rc) > + DRM_ERROR("eDP aux-bus emulation failed, rc=%d\n", rc); > + } > + > rc = component_add(&pdev->dev, &dp_display_comp_ops); > if (rc) { > DRM_ERROR("component add failed, rc=%d\n", rc); > @@ -1306,11 +1333,14 @@ static int dp_display_remove(struct platform_device *pdev) > struct dp_display_private *dp = dev_get_dp_display_private(&pdev->dev); > > component_del(&pdev->dev, &dp_display_comp_ops); > - dp_display_deinit_sub_modules(dp); > - > platform_set_drvdata(pdev, NULL); > + > + pm_runtime_dont_use_autosuspend(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > pm_runtime_put_sync_suspend(&pdev->dev); > > + dp_display_deinit_sub_modules(dp); > + > return 0; > } > > @@ -1514,31 +1544,10 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor) > > static int dp_display_get_next_bridge(struct msm_dp *dp) > { > - int rc; > + int rc = 0; > struct dp_display_private *dp_priv; > - struct device_node *aux_bus; > - struct device *dev; > > dp_priv = container_of(dp, struct dp_display_private, dp_display); > - dev = &dp_priv->pdev->dev; > - aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); > - > - if (aux_bus && dp->is_edp) { > - /* > - * The code below assumes that the panel will finish probing > - * by the time devm_of_dp_aux_populate_ep_devices() returns. > - * This isn't a great assumption since it will fail if the > - * panel driver is probed asynchronously but is the best we > - * can do without a bigger driver reorganization. > - */ > - rc = of_dp_aux_populate_bus(dp_priv->aux, NULL); > - of_node_put(aux_bus); > - if (rc) > - goto error; > - } else if (dp->is_edp) { > - DRM_ERROR("eDP aux_bus not found\n"); > - return -ENODEV; > - } > > /* > * External bridges are mandatory for eDP interfaces: one has to > @@ -1551,17 +1560,9 @@ static int dp_display_get_next_bridge(struct msm_dp *dp) > if (!dp->is_edp && rc == -ENODEV) > return 0; > > - if (!rc) { > + if (!rc) > dp->next_bridge = dp_priv->parser->next_bridge; > - return 0; > - } > > -error: > - if (dp->is_edp) { > - of_dp_aux_depopulate_bus(dp_priv->aux); > - dp_display_host_phy_exit(dp_priv); > - dp_display_host_deinit(dp_priv); > - } > return rc; > } >
On 08/07/2023 02:52, Kuogee Hsieh wrote: > EV_HPD_INIT_SETUP flag is used to trigger the initialization of external > DP host controller. Since external DP host controller initialization had > been incorporated into pm_runtime_resume(), this flag become obsolete. > Lets get rid of it. And another question. Between patches #2 and #3 we have both INIT_SETUP event and the PM doing dp_display_host_init(). Will it work correctly? > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_display.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 2c5706a..44580c2 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -55,7 +55,6 @@ enum { > enum { > EV_NO_EVENT, > /* hpd events */ > - EV_HPD_INIT_SETUP, > EV_HPD_PLUG_INT, > EV_IRQ_HPD_INT, > EV_HPD_UNPLUG_INT, > @@ -1119,9 +1118,6 @@ static int hpd_event_thread(void *data) > spin_unlock_irqrestore(&dp_priv->event_lock, flag); > > switch (todo->event_id) { > - case EV_HPD_INIT_SETUP: > - dp_display_host_init(dp_priv); > - break; > case EV_HPD_PLUG_INT: > dp_hpd_plug_handle(dp_priv, todo->data); > break; > @@ -1483,15 +1479,7 @@ void __exit msm_dp_unregister(void) > > void msm_dp_irq_postinstall(struct msm_dp *dp_display) > { > - struct dp_display_private *dp; > - > - if (!dp_display) > - return; > - > - dp = container_of(dp_display, struct dp_display_private, dp_display); > > - if (!dp_display->is_edp) > - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0); > } > > bool msm_dp_wide_bus_available(const struct msm_dp *dp_display)
On 08/07/2023 02:52, Kuogee Hsieh wrote: > Incorporate pm runtime framework into DP driver and clean up eDP > by moving of_dp_aux_populate_bus() to probe() Please use sensible prefix for cover letters too. It helps people understand, which driver/area is touched by the patchset. > > Kuogee Hsieh (5): > drm/msm/dp: remove pm_runtime_xxx() from dp_power.c > drm/msm/dp: incorporate pm_runtime framework into DP driver > drm/msm/dp: delete EV_HPD_INIT_SETUP > drm/msm/dp: move relevant dp initialization code from bind() to > probe() > drm/msm/dp: move of_dp_aux_populate_bus() to probe for eDP > > drivers/gpu/drm/msm/dp/dp_aux.c | 28 +++++ > drivers/gpu/drm/msm/dp/dp_display.c | 204 +++++++++++++++++++++--------------- > drivers/gpu/drm/msm/dp/dp_display.h | 1 - > drivers/gpu/drm/msm/dp/dp_power.c | 9 -- > 4 files changed, 145 insertions(+), 97 deletions(-) >
On Fri, Jul 07, 2023 at 04:52:22PM -0700, Kuogee Hsieh wrote: > In preparation of moving edp of_dp_aux_populate_bus() to > dp_display_probe(), move dp_display_request_irq(), > dp->parser->parse() and dp_power_client_init() to dp_display_probe() > too. > > Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > --- > drivers/gpu/drm/msm/dp/dp_display.c | 48 +++++++++++++++++-------------------- > drivers/gpu/drm/msm/dp/dp_display.h | 1 - > 2 files changed, 22 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 44580c2..185f1eb 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -290,12 +290,6 @@ static int dp_display_bind(struct device *dev, struct device *master, > goto end; > } > > - rc = dp_power_client_init(dp->power); > - if (rc) { > - DRM_ERROR("Power client create failed\n"); > - goto end; > - } > - > rc = dp_register_audio_driver(dev, dp->audio); > if (rc) { > DRM_ERROR("Audio registration Dp failed\n"); > @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp) > goto error; > } > > + rc = dp->parser->parse(dp->parser); Today dp_init_sub_modules() just allocates memory for all the modules and ties them together. While I don't fancy this way of structuring device drivers in Linux, I think it's reasonable to retain that design for now, and perform the parsing and power initialization in dp_display_probe(). > + if (rc) { > + DRM_ERROR("device tree parsing failed\n"); > + goto error; > + } > + > dp->catalog = dp_catalog_get(dev, &dp->parser->io); > if (IS_ERR(dp->catalog)) { > rc = PTR_ERR(dp->catalog); > @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp) > goto error; > } > > + rc = dp_power_client_init(dp->power); > + if (rc) { > + DRM_ERROR("Power client create failed\n"); > + goto error; > + } > + > dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp); > if (IS_ERR(dp->aux)) { > rc = PTR_ERR(dp->aux); > @@ -1196,26 +1202,20 @@ 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; > - } Love this, but it's unrelated to the rest of the patch. > - > - 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 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, This is fixing a bug where currently the dp_display_irq_handler() registration is tied to the DPU device's life cycle, while depending on resources that are released as the DP device is torn down. It would be nice if this was not hidden in a patch that claims to just move calls around. Regards, Bjorn > IRQF_TRIGGER_HIGH, "dp_display_isr", dp); > if (rc < 0) { > DRM_ERROR("failed to request IRQ%u: %d\n", > @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, &dp->dp_display); > > + dp_display_request_irq(dp); > + > rc = component_add(&pdev->dev, &dp_display_comp_ops); > if (rc) { > DRM_ERROR("component add failed, rc=%d\n", rc); > @@ -1574,12 +1576,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 7/7/2023 5:34 PM, Dmitry Baryshkov wrote: > On 08/07/2023 02:52, Kuogee Hsieh wrote: >> EV_HPD_INIT_SETUP flag is used to trigger the initialization of external >> DP host controller. Since external DP host controller initialization had >> been incorporated into pm_runtime_resume(), this flag become obsolete. >> Lets get rid of it. > > And another question. Between patches #2 and #3 we have both > INIT_SETUP event and the PM doing dp_display_host_init(). Will it work > correctly? yes, i had added if (!dp->core_initialized) into dp_display_host_init(). should I merge this patch into patch #2? > >> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> --- >> drivers/gpu/drm/msm/dp/dp_display.c | 12 ------------ >> 1 file changed, 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >> b/drivers/gpu/drm/msm/dp/dp_display.c >> index 2c5706a..44580c2 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -55,7 +55,6 @@ enum { >> enum { >> EV_NO_EVENT, >> /* hpd events */ >> - EV_HPD_INIT_SETUP, >> EV_HPD_PLUG_INT, >> EV_IRQ_HPD_INT, >> EV_HPD_UNPLUG_INT, >> @@ -1119,9 +1118,6 @@ static int hpd_event_thread(void *data) >> spin_unlock_irqrestore(&dp_priv->event_lock, flag); >> switch (todo->event_id) { >> - case EV_HPD_INIT_SETUP: >> - dp_display_host_init(dp_priv); >> - break; >> case EV_HPD_PLUG_INT: >> dp_hpd_plug_handle(dp_priv, todo->data); >> break; >> @@ -1483,15 +1479,7 @@ void __exit msm_dp_unregister(void) >> void msm_dp_irq_postinstall(struct msm_dp *dp_display) >> { >> - struct dp_display_private *dp; >> - >> - if (!dp_display) >> - return; >> - >> - dp = container_of(dp_display, struct dp_display_private, >> dp_display); >> - if (!dp_display->is_edp) >> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0); >> } >> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) >
On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote: > On 08/07/2023 02:52, Kuogee Hsieh wrote: >> In preparation of moving edp of_dp_aux_populate_bus() to >> dp_display_probe(), move dp_display_request_irq(), >> dp->parser->parse() and dp_power_client_init() to dp_display_probe() >> too. >> >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >> --- >> drivers/gpu/drm/msm/dp/dp_display.c | 48 >> +++++++++++++++++-------------------- >> drivers/gpu/drm/msm/dp/dp_display.h | 1 - >> 2 files changed, 22 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >> b/drivers/gpu/drm/msm/dp/dp_display.c >> index 44580c2..185f1eb 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -290,12 +290,6 @@ static int dp_display_bind(struct device *dev, >> struct device *master, >> goto end; >> } >> - rc = dp_power_client_init(dp->power); >> - if (rc) { >> - DRM_ERROR("Power client create failed\n"); >> - goto end; >> - } >> - >> rc = dp_register_audio_driver(dev, dp->audio); >> if (rc) { >> DRM_ERROR("Audio registration Dp failed\n"); >> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct >> dp_display_private *dp) >> goto error; >> } >> + rc = dp->parser->parse(dp->parser); >> + if (rc) { >> + DRM_ERROR("device tree parsing failed\n"); >> + goto error; >> + } >> + >> dp->catalog = dp_catalog_get(dev, &dp->parser->io); >> if (IS_ERR(dp->catalog)) { >> rc = PTR_ERR(dp->catalog); >> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct >> dp_display_private *dp) >> goto error; >> } >> + rc = dp_power_client_init(dp->power); >> + if (rc) { >> + DRM_ERROR("Power client create failed\n"); >> + goto error; >> + } >> + >> dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp); >> if (IS_ERR(dp->aux)) { >> rc = PTR_ERR(dp->aux); >> @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0); >> + if (!dp->irq) { >> + DRM_ERROR("failed to get irq\n"); >> + return -EINVAL; >> + } >> } > > Use platform_get_irq() from probe() function. > >> - 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", >> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct >> platform_device *pdev) >> platform_set_drvdata(pdev, &dp->dp_display); >> + dp_display_request_irq(dp); >> + > > Error checking? > Are we completely ready to handle interrupts at this point? not until dp_display_host_init() is called which will be called from pm_runtime_resume() later. > >> rc = component_add(&pdev->dev, &dp_display_comp_ops); >> if (rc) { >> DRM_ERROR("component add failed, rc=%d\n", rc); >> @@ -1574,12 +1576,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); >
On 10/07/2023 19:57, Kuogee Hsieh wrote: > > On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote: >> On 08/07/2023 02:52, Kuogee Hsieh wrote: >>> In preparation of moving edp of_dp_aux_populate_bus() to >>> dp_display_probe(), move dp_display_request_irq(), >>> dp->parser->parse() and dp_power_client_init() to dp_display_probe() >>> too. >>> >>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/dp/dp_display.c | 48 >>> +++++++++++++++++-------------------- >>> drivers/gpu/drm/msm/dp/dp_display.h | 1 - >>> 2 files changed, 22 insertions(+), 27 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>> b/drivers/gpu/drm/msm/dp/dp_display.c >>> index 44580c2..185f1eb 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>> @@ -290,12 +290,6 @@ static int dp_display_bind(struct device *dev, >>> struct device *master, >>> goto end; >>> } >>> - rc = dp_power_client_init(dp->power); >>> - if (rc) { >>> - DRM_ERROR("Power client create failed\n"); >>> - goto end; >>> - } >>> - >>> rc = dp_register_audio_driver(dev, dp->audio); >>> if (rc) { >>> DRM_ERROR("Audio registration Dp failed\n"); >>> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct >>> dp_display_private *dp) >>> goto error; >>> } >>> + rc = dp->parser->parse(dp->parser); >>> + if (rc) { >>> + DRM_ERROR("device tree parsing failed\n"); >>> + goto error; >>> + } >>> + >>> dp->catalog = dp_catalog_get(dev, &dp->parser->io); >>> if (IS_ERR(dp->catalog)) { >>> rc = PTR_ERR(dp->catalog); >>> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct >>> dp_display_private *dp) >>> goto error; >>> } >>> + rc = dp_power_client_init(dp->power); >>> + if (rc) { >>> + DRM_ERROR("Power client create failed\n"); >>> + goto error; >>> + } >>> + >>> dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp); >>> if (IS_ERR(dp->aux)) { >>> rc = PTR_ERR(dp->aux); >>> @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0); >>> + if (!dp->irq) { >>> + DRM_ERROR("failed to get irq\n"); >>> + return -EINVAL; >>> + } >>> } >> >> Use platform_get_irq() from probe() function. >> >>> - 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", >>> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct >>> platform_device *pdev) >>> platform_set_drvdata(pdev, &dp->dp_display); >>> + dp_display_request_irq(dp); >>> + >> >> Error checking? >> Are we completely ready to handle interrupts at this point? > not until dp_display_host_init() is called which will be called from > pm_runtime_resume() later. But once you request_irq(), you should be ready for the IRQs to be delivered right away. >> >>> rc = component_add(&pdev->dev, &dp_display_comp_ops); >>> if (rc) { >>> DRM_ERROR("component add failed, rc=%d\n", rc); >>> @@ -1574,12 +1576,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); >>
On 10/07/2023 19:52, Kuogee Hsieh wrote: > > On 7/7/2023 5:34 PM, Dmitry Baryshkov wrote: >> On 08/07/2023 02:52, Kuogee Hsieh wrote: >>> EV_HPD_INIT_SETUP flag is used to trigger the initialization of external >>> DP host controller. Since external DP host controller initialization had >>> been incorporated into pm_runtime_resume(), this flag become obsolete. >>> Lets get rid of it. >> >> And another question. Between patches #2 and #3 we have both >> INIT_SETUP event and the PM doing dp_display_host_init(). Will it work >> correctly? > > yes, i had added if (!dp->core_initialized) into dp_display_host_init(). > > should I merge this patch into patch #2? You can remove a call to dp_display_host_init() in patch #2 and then drop EV_HOST_INIT / msm_dp_irq_postinstall() here. > >> >>> >>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>> --- >>> drivers/gpu/drm/msm/dp/dp_display.c | 12 ------------ >>> 1 file changed, 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>> b/drivers/gpu/drm/msm/dp/dp_display.c >>> index 2c5706a..44580c2 100644 >>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>> @@ -55,7 +55,6 @@ enum { >>> enum { >>> EV_NO_EVENT, >>> /* hpd events */ >>> - EV_HPD_INIT_SETUP, >>> EV_HPD_PLUG_INT, >>> EV_IRQ_HPD_INT, >>> EV_HPD_UNPLUG_INT, >>> @@ -1119,9 +1118,6 @@ static int hpd_event_thread(void *data) >>> spin_unlock_irqrestore(&dp_priv->event_lock, flag); >>> switch (todo->event_id) { >>> - case EV_HPD_INIT_SETUP: >>> - dp_display_host_init(dp_priv); >>> - break; >>> case EV_HPD_PLUG_INT: >>> dp_hpd_plug_handle(dp_priv, todo->data); >>> break; >>> @@ -1483,15 +1479,7 @@ void __exit msm_dp_unregister(void) >>> void msm_dp_irq_postinstall(struct msm_dp *dp_display) >>> { >>> - struct dp_display_private *dp; >>> - >>> - if (!dp_display) >>> - return; >>> - >>> - dp = container_of(dp_display, struct dp_display_private, >>> dp_display); >>> - if (!dp_display->is_edp) >>> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0); >>> } >>> bool msm_dp_wide_bus_available(const struct msm_dp *dp_display) >>
[Restored CC list] On Mon, 10 Jul 2023 at 20:08, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: > > > On 7/7/2023 5:32 PM, Dmitry Baryshkov wrote: > > On 08/07/2023 02:52, Kuogee Hsieh wrote: > >> Move of_dp_aux_populate_bus() to dp_display_probe() for eDP > >> from dp_display_bind() so that probe deferral cases can be > >> handled effectively > >> > >> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> > >> --- > >> drivers/gpu/drm/msm/dp/dp_aux.c | 25 ++++++++++++ > >> drivers/gpu/drm/msm/dp/dp_display.c | 79 > >> +++++++++++++++++++------------------ > >> 2 files changed, 65 insertions(+), 39 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c > >> b/drivers/gpu/drm/msm/dp/dp_aux.c > >> index c592064..c1baffb 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c > >> @@ -505,6 +505,21 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux) > >> drm_dp_aux_unregister(dp_aux); > >> } > >> +static int dp_wait_hpd_asserted(struct drm_dp_aux *dp_aux, > >> + unsigned long wait_us) > >> +{ > >> + int ret; > >> + struct dp_aux_private *aux; > >> + > >> + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); > >> + > >> + pm_runtime_get_sync(aux->dev); > >> + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); > >> + pm_runtime_put_sync(aux->dev); > >> + > >> + return ret; > >> +} > >> + > >> struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog > >> *catalog, > >> bool is_edp) > >> { > >> @@ -528,6 +543,16 @@ struct drm_dp_aux *dp_aux_get(struct device > >> *dev, struct dp_catalog *catalog, > >> aux->catalog = catalog; > >> aux->retry_cnt = 0; > >> + /* > >> + * Use the drm_dp_aux_init() to use the aux adapter > >> + * before registering aux with the DRM device. > >> + */ > >> + aux->dp_aux.name = "dpu_dp_aux"; > >> + aux->dp_aux.dev = dev; > >> + aux->dp_aux.transfer = dp_aux_transfer; > >> + aux->dp_aux.wait_hpd_asserted = dp_wait_hpd_asserted; > >> + drm_dp_aux_init(&aux->dp_aux); > >> + > >> return &aux->dp_aux; > >> } > >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > >> b/drivers/gpu/drm/msm/dp/dp_display.c > >> index 185f1eb..7ed4bea 100644 > >> --- a/drivers/gpu/drm/msm/dp/dp_display.c > >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c > >> @@ -302,10 +302,6 @@ static int dp_display_bind(struct device *dev, > >> struct device *master, > >> goto end; > >> } > >> - pm_runtime_enable(dev); > >> - pm_runtime_set_autosuspend_delay(dev, 1000); > >> - pm_runtime_use_autosuspend(dev); > >> - > >> return 0; > >> end: > >> return rc; > >> @@ -322,8 +318,6 @@ static void dp_display_unbind(struct device *dev, > >> struct device *master, > >> kthread_stop(dp->ev_tsk); > >> - of_dp_aux_depopulate_bus(dp->aux); > >> - > >> dp_power_client_deinit(dp->power); > >> dp_unregister_audio_driver(dev, dp->audio); > >> dp_aux_unregister(dp->aux); > >> @@ -1245,6 +1239,29 @@ static const struct msm_dp_desc > >> *dp_display_get_desc(struct platform_device *pde > >> return NULL; > >> } > >> +static void of_dp_aux_depopulate_bus_void(void *data) > >> +{ > >> + of_dp_aux_depopulate_bus(data); > >> +} > >> + > >> +static int dp_display_auxbus_emulation(struct dp_display_private *dp) > > > > Why is it called emulation? > > > >> +{ > >> + struct device *dev = &dp->pdev->dev; > >> + struct device_node *aux_bus; > >> + int ret = 0; > >> + > >> + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); > >> + > >> + if (aux_bus) { > >> + ret = devm_of_dp_aux_populate_bus(dp->aux, NULL); > > > > And here you missed the whole point of why we have been asking for. > > Please add a sensible `done_probing' callback, which will call > > component_add(). This way the DP component will only be registered > > when the panel has been probed. Keeping us from the component binding > > retries and corresponding side effects. > > > >> + > >> + devm_add_action_or_reset(dev, of_dp_aux_depopulate_bus_void, > >> + dp->aux); > > > > Useless, it's already handled by the devm_ part of the > > devm_of_dp_aux_populate_bus(). > > > >> + } > >> + > >> + return ret; > >> +} > >> + > >> static int dp_display_probe(struct platform_device *pdev) > >> { > >> int rc = 0; > >> @@ -1290,8 +1307,18 @@ static int dp_display_probe(struct > >> platform_device *pdev) > >> platform_set_drvdata(pdev, &dp->dp_display); > >> + pm_runtime_enable(&pdev->dev); > >> + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); > >> + pm_runtime_use_autosuspend(&pdev->dev); > > > > Can we have this in probe right from the patch #2? > > no, at patch#2, devm_of_dp_aux_populate_bus() is done ta bind timing. > > The device used by pm_runtime_get_sync() of generic_edp_panel_probe() > which is derived from devm_of_dp_aux_populate_bus() is different the > &pdev->dev here. Excuse me, I don't get your answer. In patch #2 you have added pm_runtime_enable() / etc to dp_display_bind(). In this patch you are moving these calls to dp_display_probe(). I think that the latter is a better place for enabling runtime PM and as such I've asked you to squash this chunk into patch #2. Why isn't that going to work? If I'm not mistaken here, the panel's call to pm_runtime_get_sync() will wake up the panel and all the parent devices, including the DP. That's what I meant in my comment regarding PM calls in the patch #1. pm_runtime_get_sync() / resume() / etc. do not only increase the runtime PM count. They do other things to parent devices, linked devices, etc. > > > > >> + > >> dp_display_request_irq(dp); > >> + if (dp->dp_display.is_edp) { > >> + rc = dp_display_auxbus_emulation(dp); > >> + if (rc) > >> + DRM_ERROR("eDP aux-bus emulation failed, rc=%d\n", rc); > >> + } > >> + > >> rc = component_add(&pdev->dev, &dp_display_comp_ops); > >> if (rc) { > >> DRM_ERROR("component add failed, rc=%d\n", rc); > >> @@ -1306,11 +1333,14 @@ static int dp_display_remove(struct > >> platform_device *pdev) > >> struct dp_display_private *dp = > >> dev_get_dp_display_private(&pdev->dev); > >> component_del(&pdev->dev, &dp_display_comp_ops); > >> - dp_display_deinit_sub_modules(dp); > >> - > >> platform_set_drvdata(pdev, NULL); > >> + > >> + pm_runtime_dont_use_autosuspend(&pdev->dev); > >> + pm_runtime_disable(&pdev->dev); > >> pm_runtime_put_sync_suspend(&pdev->dev); > >> + dp_display_deinit_sub_modules(dp); > >> + > >> return 0; > >> } > >> @@ -1514,31 +1544,10 @@ void msm_dp_debugfs_init(struct msm_dp > >> *dp_display, struct drm_minor *minor) > >> static int dp_display_get_next_bridge(struct msm_dp *dp) > >> { > >> - int rc; > >> + int rc = 0; > >> struct dp_display_private *dp_priv; > >> - struct device_node *aux_bus; > >> - struct device *dev; > >> dp_priv = container_of(dp, struct dp_display_private, > >> dp_display); > >> - dev = &dp_priv->pdev->dev; > >> - aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); > >> - > >> - if (aux_bus && dp->is_edp) { > >> - /* > >> - * The code below assumes that the panel will finish probing > >> - * by the time devm_of_dp_aux_populate_ep_devices() returns. > >> - * This isn't a great assumption since it will fail if the > >> - * panel driver is probed asynchronously but is the best we > >> - * can do without a bigger driver reorganization. > >> - */ > >> - rc = of_dp_aux_populate_bus(dp_priv->aux, NULL); > >> - of_node_put(aux_bus); > >> - if (rc) > >> - goto error; > >> - } else if (dp->is_edp) { > >> - DRM_ERROR("eDP aux_bus not found\n"); > >> - return -ENODEV; > >> - } > >> /* > >> * External bridges are mandatory for eDP interfaces: one has to > >> @@ -1551,17 +1560,9 @@ static int dp_display_get_next_bridge(struct > >> msm_dp *dp) > >> if (!dp->is_edp && rc == -ENODEV) > >> return 0; > >> - if (!rc) { > >> + if (!rc) > >> dp->next_bridge = dp_priv->parser->next_bridge; > >> - return 0; > >> - } > >> -error: > >> - if (dp->is_edp) { > >> - of_dp_aux_depopulate_bus(dp_priv->aux); > >> - dp_display_host_phy_exit(dp_priv); > >> - dp_display_host_deinit(dp_priv); > >> - } > >> return rc; > >> } > >
On 7/10/2023 11:13 AM, Dmitry Baryshkov wrote: > On 10/07/2023 19:57, Kuogee Hsieh wrote: >> >> On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote: >>> On 08/07/2023 02:52, Kuogee Hsieh wrote: >>>> In preparation of moving edp of_dp_aux_populate_bus() to >>>> dp_display_probe(), move dp_display_request_irq(), >>>> dp->parser->parse() and dp_power_client_init() to dp_display_probe() >>>> too. >>>> >>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/dp/dp_display.c | 48 >>>> +++++++++++++++++-------------------- >>>> drivers/gpu/drm/msm/dp/dp_display.h | 1 - >>>> 2 files changed, 22 insertions(+), 27 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>> index 44580c2..185f1eb 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>> @@ -290,12 +290,6 @@ static int dp_display_bind(struct device *dev, >>>> struct device *master, >>>> goto end; >>>> } >>>> - rc = dp_power_client_init(dp->power); >>>> - if (rc) { >>>> - DRM_ERROR("Power client create failed\n"); >>>> - goto end; >>>> - } >>>> - >>>> rc = dp_register_audio_driver(dev, dp->audio); >>>> if (rc) { >>>> DRM_ERROR("Audio registration Dp failed\n"); >>>> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct >>>> dp_display_private *dp) >>>> goto error; >>>> } >>>> + rc = dp->parser->parse(dp->parser); >>>> + if (rc) { >>>> + DRM_ERROR("device tree parsing failed\n"); >>>> + goto error; >>>> + } >>>> + >>>> dp->catalog = dp_catalog_get(dev, &dp->parser->io); >>>> if (IS_ERR(dp->catalog)) { >>>> rc = PTR_ERR(dp->catalog); >>>> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct >>>> dp_display_private *dp) >>>> goto error; >>>> } >>>> + rc = dp_power_client_init(dp->power); >>>> + if (rc) { >>>> + DRM_ERROR("Power client create failed\n"); >>>> + goto error; >>>> + } >>>> + >>>> dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp); >>>> if (IS_ERR(dp->aux)) { >>>> rc = PTR_ERR(dp->aux); >>>> @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0); >>>> + if (!dp->irq) { >>>> + DRM_ERROR("failed to get irq\n"); >>>> + return -EINVAL; >>>> + } >>>> } >>> >>> Use platform_get_irq() from probe() function. >>> >>>> - 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", >>>> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct >>>> platform_device *pdev) >>>> platform_set_drvdata(pdev, &dp->dp_display); >>>> + dp_display_request_irq(dp); >>>> + >>> >>> Error checking? >>> Are we completely ready to handle interrupts at this point? >> not until dp_display_host_init() is called which will be called from >> pm_runtime_resume() later. > > But once you request_irq(), you should be ready for the IRQs to be > delivered right away. At this point, the DP controller interrupts mask bit is not enabled yet. Therefore interrupts will not happen until dp_bridge_hpd_enable() is called to initialize dp host controller and then enabled mask bits. > >>> >>>> rc = component_add(&pdev->dev, &dp_display_comp_ops); >>>> if (rc) { >>>> DRM_ERROR("component add failed, rc=%d\n", rc); >>>> @@ -1574,12 +1576,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); >>> >
On 17/07/2023 20:16, Kuogee Hsieh wrote: > > On 7/10/2023 11:13 AM, Dmitry Baryshkov wrote: >> On 10/07/2023 19:57, Kuogee Hsieh wrote: >>> >>> On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote: >>>> On 08/07/2023 02:52, Kuogee Hsieh wrote: >>>>> In preparation of moving edp of_dp_aux_populate_bus() to >>>>> dp_display_probe(), move dp_display_request_irq(), >>>>> dp->parser->parse() and dp_power_client_init() to dp_display_probe() >>>>> too. >>>>> >>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>>>> --- >>>>> drivers/gpu/drm/msm/dp/dp_display.c | 48 >>>>> +++++++++++++++++-------------------- >>>>> drivers/gpu/drm/msm/dp/dp_display.h | 1 - >>>>> 2 files changed, 22 insertions(+), 27 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>>> index 44580c2..185f1eb 100644 >>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>>> @@ -290,12 +290,6 @@ static int dp_display_bind(struct device *dev, >>>>> struct device *master, >>>>> goto end; >>>>> } >>>>> - rc = dp_power_client_init(dp->power); >>>>> - if (rc) { >>>>> - DRM_ERROR("Power client create failed\n"); >>>>> - goto end; >>>>> - } >>>>> - >>>>> rc = dp_register_audio_driver(dev, dp->audio); >>>>> if (rc) { >>>>> DRM_ERROR("Audio registration Dp failed\n"); >>>>> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct >>>>> dp_display_private *dp) >>>>> goto error; >>>>> } >>>>> + rc = dp->parser->parse(dp->parser); >>>>> + if (rc) { >>>>> + DRM_ERROR("device tree parsing failed\n"); >>>>> + goto error; >>>>> + } >>>>> + >>>>> dp->catalog = dp_catalog_get(dev, &dp->parser->io); >>>>> if (IS_ERR(dp->catalog)) { >>>>> rc = PTR_ERR(dp->catalog); >>>>> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct >>>>> dp_display_private *dp) >>>>> goto error; >>>>> } >>>>> + rc = dp_power_client_init(dp->power); >>>>> + if (rc) { >>>>> + DRM_ERROR("Power client create failed\n"); >>>>> + goto error; >>>>> + } >>>>> + >>>>> dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp); >>>>> if (IS_ERR(dp->aux)) { >>>>> rc = PTR_ERR(dp->aux); >>>>> @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0); >>>>> + if (!dp->irq) { >>>>> + DRM_ERROR("failed to get irq\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> } >>>> >>>> Use platform_get_irq() from probe() function. >>>> >>>>> - 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", >>>>> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct >>>>> platform_device *pdev) >>>>> platform_set_drvdata(pdev, &dp->dp_display); >>>>> + dp_display_request_irq(dp); >>>>> + >>>> >>>> Error checking? >>>> Are we completely ready to handle interrupts at this point? >>> not until dp_display_host_init() is called which will be called from >>> pm_runtime_resume() later. >> >> But once you request_irq(), you should be ready for the IRQs to be >> delivered right away. > > At this point, the DP controller interrupts mask bit is not enabled yet. > > Therefore interrupts will not happen until dp_bridge_hpd_enable() is > called to initialize dp host controller and then enabled mask bits. Are AUX and CTRL interrupts also disabled? What about any stray/pending interrupts? Just take it as a rule of thumb. Once request_irq() has been called without the IRQ_NOAUTOEN flag, the driver should be prepared to handle the incoming interrupt requests. >>>>> rc = component_add(&pdev->dev, &dp_display_comp_ops); >>>>> if (rc) { >>>>> DRM_ERROR("component add failed, rc=%d\n", rc); >>>>> @@ -1574,12 +1576,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); >>>> >>
On 7/17/2023 10:22 AM, Dmitry Baryshkov wrote: > On 17/07/2023 20:16, Kuogee Hsieh wrote: >> >> On 7/10/2023 11:13 AM, Dmitry Baryshkov wrote: >>> On 10/07/2023 19:57, Kuogee Hsieh wrote: >>>> >>>> On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote: >>>>> On 08/07/2023 02:52, Kuogee Hsieh wrote: >>>>>> In preparation of moving edp of_dp_aux_populate_bus() to >>>>>> dp_display_probe(), move dp_display_request_irq(), >>>>>> dp->parser->parse() and dp_power_client_init() to dp_display_probe() >>>>>> too. >>>>>> >>>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>>>>> --- >>>>>> drivers/gpu/drm/msm/dp/dp_display.c | 48 >>>>>> +++++++++++++++++-------------------- >>>>>> drivers/gpu/drm/msm/dp/dp_display.h | 1 - >>>>>> 2 files changed, 22 insertions(+), 27 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> index 44580c2..185f1eb 100644 >>>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>>>> @@ -290,12 +290,6 @@ static int dp_display_bind(struct device >>>>>> *dev, struct device *master, >>>>>> goto end; >>>>>> } >>>>>> - rc = dp_power_client_init(dp->power); >>>>>> - if (rc) { >>>>>> - DRM_ERROR("Power client create failed\n"); >>>>>> - goto end; >>>>>> - } >>>>>> - >>>>>> rc = dp_register_audio_driver(dev, dp->audio); >>>>>> if (rc) { >>>>>> DRM_ERROR("Audio registration Dp failed\n"); >>>>>> @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct >>>>>> dp_display_private *dp) >>>>>> goto error; >>>>>> } >>>>>> + rc = dp->parser->parse(dp->parser); >>>>>> + if (rc) { >>>>>> + DRM_ERROR("device tree parsing failed\n"); >>>>>> + goto error; >>>>>> + } >>>>>> + >>>>>> dp->catalog = dp_catalog_get(dev, &dp->parser->io); >>>>>> if (IS_ERR(dp->catalog)) { >>>>>> rc = PTR_ERR(dp->catalog); >>>>>> @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct >>>>>> dp_display_private *dp) >>>>>> goto error; >>>>>> } >>>>>> + rc = dp_power_client_init(dp->power); >>>>>> + if (rc) { >>>>>> + DRM_ERROR("Power client create failed\n"); >>>>>> + goto error; >>>>>> + } >>>>>> + >>>>>> dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp); >>>>>> if (IS_ERR(dp->aux)) { >>>>>> rc = PTR_ERR(dp->aux); >>>>>> @@ -1196,26 +1202,20 @@ 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 = irq_of_parse_and_map(dp->pdev->dev.of_node, 0); >>>>>> + if (!dp->irq) { >>>>>> + DRM_ERROR("failed to get irq\n"); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> } >>>>> >>>>> Use platform_get_irq() from probe() function. >>>>> >>>>>> - 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", >>>>>> @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct >>>>>> platform_device *pdev) >>>>>> platform_set_drvdata(pdev, &dp->dp_display); >>>>>> + dp_display_request_irq(dp); >>>>>> + >>>>> >>>>> Error checking? >>>>> Are we completely ready to handle interrupts at this point? >>>> not until dp_display_host_init() is called which will be called >>>> from pm_runtime_resume() later. >>> >>> But once you request_irq(), you should be ready for the IRQs to be >>> delivered right away. >> >> At this point, the DP controller interrupts mask bit is not enabled yet. >> >> Therefore interrupts will not happen until dp_bridge_hpd_enable() is >> called to initialize dp host controller and then enabled mask bits. > > Are AUX and CTRL interrupts also disabled? What about any > stray/pending interrupts? Just take it as a rule of thumb. Once > request_irq() has been called without the IRQ_NOAUTOEN flag, the > driver should be prepared to handle the incoming interrupt requests. yes, both AUX and CTRL are disabled. edp population do need irq to handle aux transfer during probe. it should work by checking core_initialized flag at irq handle to filter out stray/pending interrupts. > >>>>>> rc = component_add(&pdev->dev, &dp_display_comp_ops); >>>>>> if (rc) { >>>>>> DRM_ERROR("component add failed, rc=%d\n", rc); >>>>>> @@ -1574,12 +1576,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); >>>>> >>> >
On 7/10/2023 11:24 AM, Dmitry Baryshkov wrote: > [Restored CC list] > > On Mon, 10 Jul 2023 at 20:08, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote: >> >> On 7/7/2023 5:32 PM, Dmitry Baryshkov wrote: >>> On 08/07/2023 02:52, Kuogee Hsieh wrote: >>>> Move of_dp_aux_populate_bus() to dp_display_probe() for eDP >>>> from dp_display_bind() so that probe deferral cases can be >>>> handled effectively >>>> >>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>>> --- >>>> drivers/gpu/drm/msm/dp/dp_aux.c | 25 ++++++++++++ >>>> drivers/gpu/drm/msm/dp/dp_display.c | 79 >>>> +++++++++++++++++++------------------ >>>> 2 files changed, 65 insertions(+), 39 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c >>>> b/drivers/gpu/drm/msm/dp/dp_aux.c >>>> index c592064..c1baffb 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c >>>> @@ -505,6 +505,21 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux) >>>> drm_dp_aux_unregister(dp_aux); >>>> } >>>> +static int dp_wait_hpd_asserted(struct drm_dp_aux *dp_aux, >>>> + unsigned long wait_us) >>>> +{ >>>> + int ret; >>>> + struct dp_aux_private *aux; >>>> + >>>> + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); >>>> + >>>> + pm_runtime_get_sync(aux->dev); >>>> + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); >>>> + pm_runtime_put_sync(aux->dev); >>>> + >>>> + return ret; >>>> +} >>>> + >>>> struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog >>>> *catalog, >>>> bool is_edp) >>>> { >>>> @@ -528,6 +543,16 @@ struct drm_dp_aux *dp_aux_get(struct device >>>> *dev, struct dp_catalog *catalog, >>>> aux->catalog = catalog; >>>> aux->retry_cnt = 0; >>>> + /* >>>> + * Use the drm_dp_aux_init() to use the aux adapter >>>> + * before registering aux with the DRM device. >>>> + */ >>>> + aux->dp_aux.name = "dpu_dp_aux"; >>>> + aux->dp_aux.dev = dev; >>>> + aux->dp_aux.transfer = dp_aux_transfer; >>>> + aux->dp_aux.wait_hpd_asserted = dp_wait_hpd_asserted; >>>> + drm_dp_aux_init(&aux->dp_aux); >>>> + >>>> return &aux->dp_aux; >>>> } >>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>> index 185f1eb..7ed4bea 100644 >>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>> @@ -302,10 +302,6 @@ static int dp_display_bind(struct device *dev, >>>> struct device *master, >>>> goto end; >>>> } >>>> - pm_runtime_enable(dev); >>>> - pm_runtime_set_autosuspend_delay(dev, 1000); >>>> - pm_runtime_use_autosuspend(dev); >>>> - >>>> return 0; >>>> end: >>>> return rc; >>>> @@ -322,8 +318,6 @@ static void dp_display_unbind(struct device *dev, >>>> struct device *master, >>>> kthread_stop(dp->ev_tsk); >>>> - of_dp_aux_depopulate_bus(dp->aux); >>>> - >>>> dp_power_client_deinit(dp->power); >>>> dp_unregister_audio_driver(dev, dp->audio); >>>> dp_aux_unregister(dp->aux); >>>> @@ -1245,6 +1239,29 @@ static const struct msm_dp_desc >>>> *dp_display_get_desc(struct platform_device *pde >>>> return NULL; >>>> } >>>> +static void of_dp_aux_depopulate_bus_void(void *data) >>>> +{ >>>> + of_dp_aux_depopulate_bus(data); >>>> +} >>>> + >>>> +static int dp_display_auxbus_emulation(struct dp_display_private *dp) >>> Why is it called emulation? >>> >>>> +{ >>>> + struct device *dev = &dp->pdev->dev; >>>> + struct device_node *aux_bus; >>>> + int ret = 0; >>>> + >>>> + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >>>> + >>>> + if (aux_bus) { >>>> + ret = devm_of_dp_aux_populate_bus(dp->aux, NULL); >>> And here you missed the whole point of why we have been asking for. >>> Please add a sensible `done_probing' callback, which will call >>> component_add(). This way the DP component will only be registered >>> when the panel has been probed. Keeping us from the component binding >>> retries and corresponding side effects. >>> >>>> + >>>> + devm_add_action_or_reset(dev, of_dp_aux_depopulate_bus_void, >>>> + dp->aux); >>> Useless, it's already handled by the devm_ part of the >>> devm_of_dp_aux_populate_bus(). >>> >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> static int dp_display_probe(struct platform_device *pdev) >>>> { >>>> int rc = 0; >>>> @@ -1290,8 +1307,18 @@ static int dp_display_probe(struct >>>> platform_device *pdev) >>>> platform_set_drvdata(pdev, &dp->dp_display); >>>> + pm_runtime_enable(&pdev->dev); >>>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); >>>> + pm_runtime_use_autosuspend(&pdev->dev); >>> Can we have this in probe right from the patch #2? >> no, at patch#2, devm_of_dp_aux_populate_bus() is done ta bind timing. >> >> The device used by pm_runtime_get_sync() of generic_edp_panel_probe() >> which is derived from devm_of_dp_aux_populate_bus() is different the >> &pdev->dev here. > Excuse me, I don't get your answer. In patch #2 you have added > pm_runtime_enable() / etc to dp_display_bind(). > In this patch you are moving these calls to dp_display_probe(). I > think that the latter is a better place for enabling runtime PM and as > such I've asked you to squash this chunk into patch #2. > Why isn't that going to work? > > If I'm not mistaken here, the panel's call to pm_runtime_get_sync() > will wake up the panel and all the parent devices, including the DP. > That's what I meant in my comment regarding PM calls in the patch #1. > pm_runtime_get_sync() / resume() / etc. do not only increase the > runtime PM count. They do other things to parent devices, linked > devices, etc. sorry for late response, yes, pm_runtime_enable() at probe() is better and i did that original. but it is not work. I found that, 1) at dp_display_bind(), dev is mdss 2) at probe() dev is dp 3) pm_runtime_enable(dp's dev) and generic_edp_panel_probe() --> pm_runtime_get_sync(mdss's dev) >>>> + >>>> dp_display_request_irq(dp); >>>> + if (dp->dp_display.is_edp) { >>>> + rc = dp_display_auxbus_emulation(dp); >>>> + if (rc) >>>> + DRM_ERROR("eDP aux-bus emulation failed, rc=%d\n", rc); >>>> + } >>>> + >>>> rc = component_add(&pdev->dev, &dp_display_comp_ops); >>>> if (rc) { >>>> DRM_ERROR("component add failed, rc=%d\n", rc); >>>> @@ -1306,11 +1333,14 @@ static int dp_display_remove(struct >>>> platform_device *pdev) >>>> struct dp_display_private *dp = >>>> dev_get_dp_display_private(&pdev->dev); >>>> component_del(&pdev->dev, &dp_display_comp_ops); >>>> - dp_display_deinit_sub_modules(dp); >>>> - >>>> platform_set_drvdata(pdev, NULL); >>>> + >>>> + pm_runtime_dont_use_autosuspend(&pdev->dev); >>>> + pm_runtime_disable(&pdev->dev); >>>> pm_runtime_put_sync_suspend(&pdev->dev); >>>> + dp_display_deinit_sub_modules(dp); >>>> + >>>> return 0; >>>> } >>>> @@ -1514,31 +1544,10 @@ void msm_dp_debugfs_init(struct msm_dp >>>> *dp_display, struct drm_minor *minor) >>>> static int dp_display_get_next_bridge(struct msm_dp *dp) >>>> { >>>> - int rc; >>>> + int rc = 0; >>>> struct dp_display_private *dp_priv; >>>> - struct device_node *aux_bus; >>>> - struct device *dev; >>>> dp_priv = container_of(dp, struct dp_display_private, >>>> dp_display); >>>> - dev = &dp_priv->pdev->dev; >>>> - aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >>>> - >>>> - if (aux_bus && dp->is_edp) { >>>> - /* >>>> - * The code below assumes that the panel will finish probing >>>> - * by the time devm_of_dp_aux_populate_ep_devices() returns. >>>> - * This isn't a great assumption since it will fail if the >>>> - * panel driver is probed asynchronously but is the best we >>>> - * can do without a bigger driver reorganization. >>>> - */ >>>> - rc = of_dp_aux_populate_bus(dp_priv->aux, NULL); >>>> - of_node_put(aux_bus); >>>> - if (rc) >>>> - goto error; >>>> - } else if (dp->is_edp) { >>>> - DRM_ERROR("eDP aux_bus not found\n"); >>>> - return -ENODEV; >>>> - } >>>> /* >>>> * External bridges are mandatory for eDP interfaces: one has to >>>> @@ -1551,17 +1560,9 @@ static int dp_display_get_next_bridge(struct >>>> msm_dp *dp) >>>> if (!dp->is_edp && rc == -ENODEV) >>>> return 0; >>>> - if (!rc) { >>>> + if (!rc) >>>> dp->next_bridge = dp_priv->parser->next_bridge; >>>> - return 0; >>>> - } >>>> -error: >>>> - if (dp->is_edp) { >>>> - of_dp_aux_depopulate_bus(dp_priv->aux); >>>> - dp_display_host_phy_exit(dp_priv); >>>> - dp_display_host_deinit(dp_priv); >>>> - } >>>> return rc; >>>> } > >
On 20/07/2023 23:27, Kuogee Hsieh wrote: > > On 7/10/2023 11:24 AM, Dmitry Baryshkov wrote: >> [Restored CC list] >> >> On Mon, 10 Jul 2023 at 20:08, Kuogee Hsieh <quic_khsieh@quicinc.com> >> wrote: >>> >>> On 7/7/2023 5:32 PM, Dmitry Baryshkov wrote: >>>> On 08/07/2023 02:52, Kuogee Hsieh wrote: >>>>> Move of_dp_aux_populate_bus() to dp_display_probe() for eDP >>>>> from dp_display_bind() so that probe deferral cases can be >>>>> handled effectively >>>>> >>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com> >>>>> --- >>>>> drivers/gpu/drm/msm/dp/dp_aux.c | 25 ++++++++++++ >>>>> drivers/gpu/drm/msm/dp/dp_display.c | 79 >>>>> +++++++++++++++++++------------------ >>>>> 2 files changed, 65 insertions(+), 39 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c >>>>> b/drivers/gpu/drm/msm/dp/dp_aux.c >>>>> index c592064..c1baffb 100644 >>>>> --- a/drivers/gpu/drm/msm/dp/dp_aux.c >>>>> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c >>>>> @@ -505,6 +505,21 @@ void dp_aux_unregister(struct drm_dp_aux *dp_aux) >>>>> drm_dp_aux_unregister(dp_aux); >>>>> } >>>>> +static int dp_wait_hpd_asserted(struct drm_dp_aux *dp_aux, >>>>> + unsigned long wait_us) >>>>> +{ >>>>> + int ret; >>>>> + struct dp_aux_private *aux; >>>>> + >>>>> + aux = container_of(dp_aux, struct dp_aux_private, dp_aux); >>>>> + >>>>> + pm_runtime_get_sync(aux->dev); >>>>> + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog); >>>>> + pm_runtime_put_sync(aux->dev); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog >>>>> *catalog, >>>>> bool is_edp) >>>>> { >>>>> @@ -528,6 +543,16 @@ struct drm_dp_aux *dp_aux_get(struct device >>>>> *dev, struct dp_catalog *catalog, >>>>> aux->catalog = catalog; >>>>> aux->retry_cnt = 0; >>>>> + /* >>>>> + * Use the drm_dp_aux_init() to use the aux adapter >>>>> + * before registering aux with the DRM device. >>>>> + */ >>>>> + aux->dp_aux.name = "dpu_dp_aux"; >>>>> + aux->dp_aux.dev = dev; >>>>> + aux->dp_aux.transfer = dp_aux_transfer; >>>>> + aux->dp_aux.wait_hpd_asserted = dp_wait_hpd_asserted; >>>>> + drm_dp_aux_init(&aux->dp_aux); >>>>> + >>>>> return &aux->dp_aux; >>>>> } >>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >>>>> b/drivers/gpu/drm/msm/dp/dp_display.c >>>>> index 185f1eb..7ed4bea 100644 >>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c >>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >>>>> @@ -302,10 +302,6 @@ static int dp_display_bind(struct device *dev, >>>>> struct device *master, >>>>> goto end; >>>>> } >>>>> - pm_runtime_enable(dev); >>>>> - pm_runtime_set_autosuspend_delay(dev, 1000); >>>>> - pm_runtime_use_autosuspend(dev); >>>>> - >>>>> return 0; >>>>> end: >>>>> return rc; >>>>> @@ -322,8 +318,6 @@ static void dp_display_unbind(struct device *dev, >>>>> struct device *master, >>>>> kthread_stop(dp->ev_tsk); >>>>> - of_dp_aux_depopulate_bus(dp->aux); >>>>> - >>>>> dp_power_client_deinit(dp->power); >>>>> dp_unregister_audio_driver(dev, dp->audio); >>>>> dp_aux_unregister(dp->aux); >>>>> @@ -1245,6 +1239,29 @@ static const struct msm_dp_desc >>>>> *dp_display_get_desc(struct platform_device *pde >>>>> return NULL; >>>>> } >>>>> +static void of_dp_aux_depopulate_bus_void(void *data) >>>>> +{ >>>>> + of_dp_aux_depopulate_bus(data); >>>>> +} >>>>> + >>>>> +static int dp_display_auxbus_emulation(struct dp_display_private *dp) >>>> Why is it called emulation? >>>> >>>>> +{ >>>>> + struct device *dev = &dp->pdev->dev; >>>>> + struct device_node *aux_bus; >>>>> + int ret = 0; >>>>> + >>>>> + aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >>>>> + >>>>> + if (aux_bus) { >>>>> + ret = devm_of_dp_aux_populate_bus(dp->aux, NULL); >>>> And here you missed the whole point of why we have been asking for. >>>> Please add a sensible `done_probing' callback, which will call >>>> component_add(). This way the DP component will only be registered >>>> when the panel has been probed. Keeping us from the component binding >>>> retries and corresponding side effects. >>>> >>>>> + >>>>> + devm_add_action_or_reset(dev, of_dp_aux_depopulate_bus_void, >>>>> + dp->aux); >>>> Useless, it's already handled by the devm_ part of the >>>> devm_of_dp_aux_populate_bus(). >>>> >>>>> + } >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> static int dp_display_probe(struct platform_device *pdev) >>>>> { >>>>> int rc = 0; >>>>> @@ -1290,8 +1307,18 @@ static int dp_display_probe(struct >>>>> platform_device *pdev) >>>>> platform_set_drvdata(pdev, &dp->dp_display); >>>>> + pm_runtime_enable(&pdev->dev); >>>>> + pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); >>>>> + pm_runtime_use_autosuspend(&pdev->dev); >>>> Can we have this in probe right from the patch #2? >>> no, at patch#2, devm_of_dp_aux_populate_bus() is done ta bind timing. >>> >>> The device used by pm_runtime_get_sync() of generic_edp_panel_probe() >>> which is derived from devm_of_dp_aux_populate_bus() is different the >>> &pdev->dev here. >> Excuse me, I don't get your answer. In patch #2 you have added >> pm_runtime_enable() / etc to dp_display_bind(). >> In this patch you are moving these calls to dp_display_probe(). I >> think that the latter is a better place for enabling runtime PM and as >> such I've asked you to squash this chunk into patch #2. >> Why isn't that going to work? >> >> If I'm not mistaken here, the panel's call to pm_runtime_get_sync() >> will wake up the panel and all the parent devices, including the DP. >> That's what I meant in my comment regarding PM calls in the patch #1. >> pm_runtime_get_sync() / resume() / etc. do not only increase the >> runtime PM count. They do other things to parent devices, linked >> devices, etc. > > sorry for late response, > > yes, pm_runtime_enable() at probe() is better and i did that original. > but it is not work. > > I found that, > > 1) at dp_display_bind(), dev is mdss If the 'dev' is the issue, you can always use dp_display_private::pdev. > > 2) at probe() dev is dp > > 3) pm_runtime_enable(dp's dev) and generic_edp_panel_probe() --> > pm_runtime_get_sync(mdss's dev) I might be missing something. Please describe, what exactly doesn't work. > > > >>>>> + >>>>> dp_display_request_irq(dp); >>>>> + if (dp->dp_display.is_edp) { >>>>> + rc = dp_display_auxbus_emulation(dp); >>>>> + if (rc) >>>>> + DRM_ERROR("eDP aux-bus emulation failed, rc=%d\n", rc); >>>>> + } >>>>> + >>>>> rc = component_add(&pdev->dev, &dp_display_comp_ops); >>>>> if (rc) { >>>>> DRM_ERROR("component add failed, rc=%d\n", rc); >>>>> @@ -1306,11 +1333,14 @@ static int dp_display_remove(struct >>>>> platform_device *pdev) >>>>> struct dp_display_private *dp = >>>>> dev_get_dp_display_private(&pdev->dev); >>>>> component_del(&pdev->dev, &dp_display_comp_ops); >>>>> - dp_display_deinit_sub_modules(dp); >>>>> - >>>>> platform_set_drvdata(pdev, NULL); >>>>> + >>>>> + pm_runtime_dont_use_autosuspend(&pdev->dev); >>>>> + pm_runtime_disable(&pdev->dev); >>>>> pm_runtime_put_sync_suspend(&pdev->dev); >>>>> + dp_display_deinit_sub_modules(dp); >>>>> + >>>>> return 0; >>>>> } >>>>> @@ -1514,31 +1544,10 @@ void msm_dp_debugfs_init(struct msm_dp >>>>> *dp_display, struct drm_minor *minor) >>>>> static int dp_display_get_next_bridge(struct msm_dp *dp) >>>>> { >>>>> - int rc; >>>>> + int rc = 0; >>>>> struct dp_display_private *dp_priv; >>>>> - struct device_node *aux_bus; >>>>> - struct device *dev; >>>>> dp_priv = container_of(dp, struct dp_display_private, >>>>> dp_display); >>>>> - dev = &dp_priv->pdev->dev; >>>>> - aux_bus = of_get_child_by_name(dev->of_node, "aux-bus"); >>>>> - >>>>> - if (aux_bus && dp->is_edp) { >>>>> - /* >>>>> - * The code below assumes that the panel will finish probing >>>>> - * by the time devm_of_dp_aux_populate_ep_devices() returns. >>>>> - * This isn't a great assumption since it will fail if the >>>>> - * panel driver is probed asynchronously but is the best we >>>>> - * can do without a bigger driver reorganization. >>>>> - */ >>>>> - rc = of_dp_aux_populate_bus(dp_priv->aux, NULL); >>>>> - of_node_put(aux_bus); >>>>> - if (rc) >>>>> - goto error; >>>>> - } else if (dp->is_edp) { >>>>> - DRM_ERROR("eDP aux_bus not found\n"); >>>>> - return -ENODEV; >>>>> - } >>>>> /* >>>>> * External bridges are mandatory for eDP interfaces: one >>>>> has to >>>>> @@ -1551,17 +1560,9 @@ static int dp_display_get_next_bridge(struct >>>>> msm_dp *dp) >>>>> if (!dp->is_edp && rc == -ENODEV) >>>>> return 0; >>>>> - if (!rc) { >>>>> + if (!rc) >>>>> dp->next_bridge = dp_priv->parser->next_bridge; >>>>> - return 0; >>>>> - } >>>>> -error: >>>>> - if (dp->is_edp) { >>>>> - of_dp_aux_depopulate_bus(dp_priv->aux); >>>>> - dp_display_host_phy_exit(dp_priv); >>>>> - dp_display_host_deinit(dp_priv); >>>>> - } >>>>> return rc; >>>>> } >> >>
Incorporate pm runtime framework into DP driver and clean up eDP by moving of_dp_aux_populate_bus() to probe() Kuogee Hsieh (5): drm/msm/dp: remove pm_runtime_xxx() from dp_power.c drm/msm/dp: incorporate pm_runtime framework into DP driver drm/msm/dp: delete EV_HPD_INIT_SETUP drm/msm/dp: move relevant dp initialization code from bind() to probe() drm/msm/dp: move of_dp_aux_populate_bus() to probe for eDP drivers/gpu/drm/msm/dp/dp_aux.c | 28 +++++ drivers/gpu/drm/msm/dp/dp_display.c | 204 +++++++++++++++++++++--------------- drivers/gpu/drm/msm/dp/dp_display.h | 1 - drivers/gpu/drm/msm/dp/dp_power.c | 9 -- 4 files changed, 145 insertions(+), 97 deletions(-)