Message ID | 20230717141438.274419-12-angelogioacchino.delregno@collabora.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | MediaTek DisplayPort: support eDP and aux-bus | expand |
Reviewed-by: Alexandre Mergnat <amergnat@baylibre.com> On 17/07/2023 16:14, AngeloGioacchino Del Regno wrote: > The interrupt handler for HPD is useful only if a display is actually > supposed to be hotpluggable, as that manages the machinery to perform > cable (un)plug detection, debouncing and setup for re-training. > > Since eDP panels are not supposed to be hotpluggable we can avoid > using the HPD interrupts altogether and rely on HPD polling only > for the suspend/resume case, saving us some spinlocking action and > the overhead of interrupts firing at every suspend/resume cycle, > achieving a faster (even if just slightly) display resume.
Hi, Angelo: On Mon, 2023-07-17 at 16:14 +0200, AngeloGioacchino Del Regno wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > The interrupt handler for HPD is useful only if a display is > actually > supposed to be hotpluggable, as that manages the machinery to perform > cable (un)plug detection, debouncing and setup for re-training. > > Since eDP panels are not supposed to be hotpluggable we can avoid > using the HPD interrupts altogether and rely on HPD polling only > for the suspend/resume case, saving us some spinlocking action and > the overhead of interrupts firing at every suspend/resume cycle, > achieving a faster (even if just slightly) display resume. > > Signed-off-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > --- > drivers/gpu/drm/mediatek/mtk_dp.c | 81 ++++++++++++++++++----------- > -- > 1 file changed, 46 insertions(+), 35 deletions(-) > > diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c > b/drivers/gpu/drm/mediatek/mtk_dp.c > index e74295ba9707..c06fcc7318e7 100644 > --- a/drivers/gpu/drm/mediatek/mtk_dp.c > +++ b/drivers/gpu/drm/mediatek/mtk_dp.c > @@ -2182,9 +2182,11 @@ static int mtk_dp_bridge_attach(struct > drm_bridge *bridge, > > mtk_dp->drm_dev = bridge->dev; > > - irq_clear_status_flags(mtk_dp->irq, IRQ_NOAUTOEN); > - enable_irq(mtk_dp->irq); > - mtk_dp_hwirq_enable(mtk_dp, true); > + if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) { > + irq_clear_status_flags(mtk_dp->irq, IRQ_NOAUTOEN); > + enable_irq(mtk_dp->irq); > + mtk_dp_hwirq_enable(mtk_dp, true); > + } > > return 0; > > @@ -2199,8 +2201,10 @@ static void mtk_dp_bridge_detach(struct > drm_bridge *bridge) > { > struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); > > - mtk_dp_hwirq_enable(mtk_dp, false); > - disable_irq(mtk_dp->irq); > + if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) { > + mtk_dp_hwirq_enable(mtk_dp, false); > + disable_irq(mtk_dp->irq); > + } > mtk_dp->drm_dev = NULL; > mtk_dp_poweroff(mtk_dp); > drm_dp_aux_unregister(&mtk_dp->aux); > @@ -2579,40 +2583,44 @@ static int mtk_dp_probe(struct > platform_device *pdev) > mtk_dp->dev = dev; > mtk_dp->data = (struct mtk_dp_data > *)of_device_get_match_data(dev); > > - mtk_dp->irq = platform_get_irq(pdev, 0); > - if (mtk_dp->irq < 0) > - return dev_err_probe(dev, mtk_dp->irq, > - "failed to request dp irq > resource\n"); > - > - mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, > 1, 0); > - if (IS_ERR(mtk_dp->next_bridge) && > - PTR_ERR(mtk_dp->next_bridge) == -ENODEV) > - mtk_dp->next_bridge = NULL; > - else if (IS_ERR(mtk_dp->next_bridge)) > - return dev_err_probe(dev, PTR_ERR(mtk_dp->next_bridge), > - "Failed to get bridge\n"); > - Why remove next_bridge setting? Regards, CK > ret = mtk_dp_dt_parse(mtk_dp, pdev); > if (ret) > return dev_err_probe(dev, ret, "Failed to parse dt\n"); > > + /* > + * Request the interrupt and install service routine only if we > are > + * on full DisplayPort. > + * For eDP, polling the HPD instead is more convenient because > we > + * don't expect any (un)plug events during runtime, hence we > can > + * avoid some locking. > + */ > + if (mtk_dp->data->bridge_type != DRM_MODE_CONNECTOR_eDP) { > + mtk_dp->irq = platform_get_irq(pdev, 0); > + if (mtk_dp->irq < 0) > + return dev_err_probe(dev, mtk_dp->irq, > + "failed to request dp irq > resource\n"); > + > + spin_lock_init(&mtk_dp->irq_thread_lock); > + > + irq_set_status_flags(mtk_dp->irq, IRQ_NOAUTOEN); > + ret = devm_request_threaded_irq(dev, mtk_dp->irq, > mtk_dp_hpd_event, > + mtk_dp_hpd_event_thread > , > + IRQ_TYPE_LEVEL_HIGH, > dev_name(dev), > + mtk_dp); > + if (ret) > + return dev_err_probe(dev, ret, > + "failed to request > mediatek dptx irq\n"); > + > + mtk_dp->need_debounce = true; > + timer_setup(&mtk_dp->debounce_timer, > mtk_dp_debounce_timer, 0); > + } > + > mtk_dp->aux.name = "aux_mtk_dp"; > mtk_dp->aux.dev = dev; > mtk_dp->aux.transfer = mtk_dp_aux_transfer; > mtk_dp->aux.wait_hpd_asserted = mtk_dp_wait_hpd_asserted; > drm_dp_aux_init(&mtk_dp->aux); > > - spin_lock_init(&mtk_dp->irq_thread_lock); > - > - irq_set_status_flags(mtk_dp->irq, IRQ_NOAUTOEN); > - ret = devm_request_threaded_irq(dev, mtk_dp->irq, > mtk_dp_hpd_event, > - mtk_dp_hpd_event_thread, > - IRQ_TYPE_LEVEL_HIGH, > dev_name(dev), > - mtk_dp); > - if (ret) > - return dev_err_probe(dev, ret, > - "failed to request mediatek dptx > irq\n"); > - > platform_set_drvdata(pdev, mtk_dp); > > if (mtk_dp->data->audio_supported) { > @@ -2634,9 +2642,6 @@ static int mtk_dp_probe(struct platform_device > *pdev) > mtk_dp->bridge.of_node = dev->of_node; > mtk_dp->bridge.type = mtk_dp->data->bridge_type; > > - mtk_dp->need_debounce = true; > - timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0); > - > if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) { > /* > * Set the data lanes to idle in case the bootloader > didn't > @@ -2647,6 +2652,9 @@ static int mtk_dp_probe(struct platform_device > *pdev) > mtk_dp_initialize_aux_settings(mtk_dp); > mtk_dp_power_enable(mtk_dp); > > + /* Disable HW interrupts: we don't need any for eDP */ > + mtk_dp_hwirq_enable(mtk_dp, false); > + > /* > * Power on the AUX to allow reading the EDID from aux- > bus: > * please note that it is necessary to call power off > in the > @@ -2692,7 +2700,8 @@ static int mtk_dp_remove(struct platform_device > *pdev) > > pm_runtime_put(&pdev->dev); > pm_runtime_disable(&pdev->dev); > - del_timer_sync(&mtk_dp->debounce_timer); > + if (mtk_dp->data->bridge_type != DRM_MODE_CONNECTOR_eDP) > + del_timer_sync(&mtk_dp->debounce_timer); > platform_device_unregister(mtk_dp->phy_dev); > if (mtk_dp->audio_pdev) > platform_device_unregister(mtk_dp->audio_pdev); > @@ -2706,7 +2715,8 @@ static int mtk_dp_suspend(struct device *dev) > struct mtk_dp *mtk_dp = dev_get_drvdata(dev); > > mtk_dp_power_disable(mtk_dp); > - mtk_dp_hwirq_enable(mtk_dp, false); > + if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) > + mtk_dp_hwirq_enable(mtk_dp, false); > pm_runtime_put_sync(dev); > > return 0; > @@ -2718,7 +2728,8 @@ static int mtk_dp_resume(struct device *dev) > > pm_runtime_get_sync(dev); > mtk_dp_init_port(mtk_dp); > - mtk_dp_hwirq_enable(mtk_dp, true); > + if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) > + mtk_dp_hwirq_enable(mtk_dp, true); > mtk_dp_power_enable(mtk_dp); > > return 0; > -- > 2.40.1
Il 25/07/23 08:37, CK Hu (胡俊光) ha scritto: > Hi, Angelo: > > On Mon, 2023-07-17 at 16:14 +0200, AngeloGioacchino Del Regno wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> The interrupt handler for HPD is useful only if a display is >> actually >> supposed to be hotpluggable, as that manages the machinery to perform >> cable (un)plug detection, debouncing and setup for re-training. >> >> Since eDP panels are not supposed to be hotpluggable we can avoid >> using the HPD interrupts altogether and rely on HPD polling only >> for the suspend/resume case, saving us some spinlocking action and >> the overhead of interrupts firing at every suspend/resume cycle, >> achieving a faster (even if just slightly) display resume. >> >> Signed-off-by: AngeloGioacchino Del Regno < >> angelogioacchino.delregno@collabora.com> >> --- >> drivers/gpu/drm/mediatek/mtk_dp.c | 81 ++++++++++++++++++----------- >> -- >> 1 file changed, 46 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c >> b/drivers/gpu/drm/mediatek/mtk_dp.c >> index e74295ba9707..c06fcc7318e7 100644 >> --- a/drivers/gpu/drm/mediatek/mtk_dp.c >> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c >> @@ -2182,9 +2182,11 @@ static int mtk_dp_bridge_attach(struct >> drm_bridge *bridge, >> >> mtk_dp->drm_dev = bridge->dev; >> >> - irq_clear_status_flags(mtk_dp->irq, IRQ_NOAUTOEN); >> - enable_irq(mtk_dp->irq); >> - mtk_dp_hwirq_enable(mtk_dp, true); >> + if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) { >> + irq_clear_status_flags(mtk_dp->irq, IRQ_NOAUTOEN); >> + enable_irq(mtk_dp->irq); >> + mtk_dp_hwirq_enable(mtk_dp, true); >> + } >> >> return 0; >> >> @@ -2199,8 +2201,10 @@ static void mtk_dp_bridge_detach(struct >> drm_bridge *bridge) >> { >> struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); >> >> - mtk_dp_hwirq_enable(mtk_dp, false); >> - disable_irq(mtk_dp->irq); >> + if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) { >> + mtk_dp_hwirq_enable(mtk_dp, false); >> + disable_irq(mtk_dp->irq); >> + } >> mtk_dp->drm_dev = NULL; >> mtk_dp_poweroff(mtk_dp); >> drm_dp_aux_unregister(&mtk_dp->aux); >> @@ -2579,40 +2583,44 @@ static int mtk_dp_probe(struct >> platform_device *pdev) >> mtk_dp->dev = dev; >> mtk_dp->data = (struct mtk_dp_data >> *)of_device_get_match_data(dev); >> >> - mtk_dp->irq = platform_get_irq(pdev, 0); >> - if (mtk_dp->irq < 0) >> - return dev_err_probe(dev, mtk_dp->irq, >> - "failed to request dp irq >> resource\n"); >> - >> - mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, >> 1, 0); >> - if (IS_ERR(mtk_dp->next_bridge) && >> - PTR_ERR(mtk_dp->next_bridge) == -ENODEV) >> - mtk_dp->next_bridge = NULL; >> - else if (IS_ERR(mtk_dp->next_bridge)) >> - return dev_err_probe(dev, PTR_ERR(mtk_dp->next_bridge), >> - "Failed to get bridge\n"); >> - > > Why remove next_bridge setting? > Hello CK, Thanks for making me notice that! The removal of this code snippet belongs to patch [9/11] `drm/mediatek: dp: Add support for embedded DisplayPort aux-bus` because in that patch I am moving the call to devm_drm_of_get_bridge() to function mtk_dp_edp_link_panel(), which is called as a .done_probing() callback if the eDP panel is on aux-bus, or "manually" if not on aux-bus. I'll send a v7 with the removal of this snippet in the right patch. Thanks again, Angelo
diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c index e74295ba9707..c06fcc7318e7 100644 --- a/drivers/gpu/drm/mediatek/mtk_dp.c +++ b/drivers/gpu/drm/mediatek/mtk_dp.c @@ -2182,9 +2182,11 @@ static int mtk_dp_bridge_attach(struct drm_bridge *bridge, mtk_dp->drm_dev = bridge->dev; - irq_clear_status_flags(mtk_dp->irq, IRQ_NOAUTOEN); - enable_irq(mtk_dp->irq); - mtk_dp_hwirq_enable(mtk_dp, true); + if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) { + irq_clear_status_flags(mtk_dp->irq, IRQ_NOAUTOEN); + enable_irq(mtk_dp->irq); + mtk_dp_hwirq_enable(mtk_dp, true); + } return 0; @@ -2199,8 +2201,10 @@ static void mtk_dp_bridge_detach(struct drm_bridge *bridge) { struct mtk_dp *mtk_dp = mtk_dp_from_bridge(bridge); - mtk_dp_hwirq_enable(mtk_dp, false); - disable_irq(mtk_dp->irq); + if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) { + mtk_dp_hwirq_enable(mtk_dp, false); + disable_irq(mtk_dp->irq); + } mtk_dp->drm_dev = NULL; mtk_dp_poweroff(mtk_dp); drm_dp_aux_unregister(&mtk_dp->aux); @@ -2579,40 +2583,44 @@ static int mtk_dp_probe(struct platform_device *pdev) mtk_dp->dev = dev; mtk_dp->data = (struct mtk_dp_data *)of_device_get_match_data(dev); - mtk_dp->irq = platform_get_irq(pdev, 0); - if (mtk_dp->irq < 0) - return dev_err_probe(dev, mtk_dp->irq, - "failed to request dp irq resource\n"); - - mtk_dp->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node, 1, 0); - if (IS_ERR(mtk_dp->next_bridge) && - PTR_ERR(mtk_dp->next_bridge) == -ENODEV) - mtk_dp->next_bridge = NULL; - else if (IS_ERR(mtk_dp->next_bridge)) - return dev_err_probe(dev, PTR_ERR(mtk_dp->next_bridge), - "Failed to get bridge\n"); - ret = mtk_dp_dt_parse(mtk_dp, pdev); if (ret) return dev_err_probe(dev, ret, "Failed to parse dt\n"); + /* + * Request the interrupt and install service routine only if we are + * on full DisplayPort. + * For eDP, polling the HPD instead is more convenient because we + * don't expect any (un)plug events during runtime, hence we can + * avoid some locking. + */ + if (mtk_dp->data->bridge_type != DRM_MODE_CONNECTOR_eDP) { + mtk_dp->irq = platform_get_irq(pdev, 0); + if (mtk_dp->irq < 0) + return dev_err_probe(dev, mtk_dp->irq, + "failed to request dp irq resource\n"); + + spin_lock_init(&mtk_dp->irq_thread_lock); + + irq_set_status_flags(mtk_dp->irq, IRQ_NOAUTOEN); + ret = devm_request_threaded_irq(dev, mtk_dp->irq, mtk_dp_hpd_event, + mtk_dp_hpd_event_thread, + IRQ_TYPE_LEVEL_HIGH, dev_name(dev), + mtk_dp); + if (ret) + return dev_err_probe(dev, ret, + "failed to request mediatek dptx irq\n"); + + mtk_dp->need_debounce = true; + timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0); + } + mtk_dp->aux.name = "aux_mtk_dp"; mtk_dp->aux.dev = dev; mtk_dp->aux.transfer = mtk_dp_aux_transfer; mtk_dp->aux.wait_hpd_asserted = mtk_dp_wait_hpd_asserted; drm_dp_aux_init(&mtk_dp->aux); - spin_lock_init(&mtk_dp->irq_thread_lock); - - irq_set_status_flags(mtk_dp->irq, IRQ_NOAUTOEN); - ret = devm_request_threaded_irq(dev, mtk_dp->irq, mtk_dp_hpd_event, - mtk_dp_hpd_event_thread, - IRQ_TYPE_LEVEL_HIGH, dev_name(dev), - mtk_dp); - if (ret) - return dev_err_probe(dev, ret, - "failed to request mediatek dptx irq\n"); - platform_set_drvdata(pdev, mtk_dp); if (mtk_dp->data->audio_supported) { @@ -2634,9 +2642,6 @@ static int mtk_dp_probe(struct platform_device *pdev) mtk_dp->bridge.of_node = dev->of_node; mtk_dp->bridge.type = mtk_dp->data->bridge_type; - mtk_dp->need_debounce = true; - timer_setup(&mtk_dp->debounce_timer, mtk_dp_debounce_timer, 0); - if (mtk_dp->bridge.type == DRM_MODE_CONNECTOR_eDP) { /* * Set the data lanes to idle in case the bootloader didn't @@ -2647,6 +2652,9 @@ static int mtk_dp_probe(struct platform_device *pdev) mtk_dp_initialize_aux_settings(mtk_dp); mtk_dp_power_enable(mtk_dp); + /* Disable HW interrupts: we don't need any for eDP */ + mtk_dp_hwirq_enable(mtk_dp, false); + /* * Power on the AUX to allow reading the EDID from aux-bus: * please note that it is necessary to call power off in the @@ -2692,7 +2700,8 @@ static int mtk_dp_remove(struct platform_device *pdev) pm_runtime_put(&pdev->dev); pm_runtime_disable(&pdev->dev); - del_timer_sync(&mtk_dp->debounce_timer); + if (mtk_dp->data->bridge_type != DRM_MODE_CONNECTOR_eDP) + del_timer_sync(&mtk_dp->debounce_timer); platform_device_unregister(mtk_dp->phy_dev); if (mtk_dp->audio_pdev) platform_device_unregister(mtk_dp->audio_pdev); @@ -2706,7 +2715,8 @@ static int mtk_dp_suspend(struct device *dev) struct mtk_dp *mtk_dp = dev_get_drvdata(dev); mtk_dp_power_disable(mtk_dp); - mtk_dp_hwirq_enable(mtk_dp, false); + if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) + mtk_dp_hwirq_enable(mtk_dp, false); pm_runtime_put_sync(dev); return 0; @@ -2718,7 +2728,8 @@ static int mtk_dp_resume(struct device *dev) pm_runtime_get_sync(dev); mtk_dp_init_port(mtk_dp); - mtk_dp_hwirq_enable(mtk_dp, true); + if (mtk_dp->bridge.type != DRM_MODE_CONNECTOR_eDP) + mtk_dp_hwirq_enable(mtk_dp, true); mtk_dp_power_enable(mtk_dp); return 0;
The interrupt handler for HPD is useful only if a display is actually supposed to be hotpluggable, as that manages the machinery to perform cable (un)plug detection, debouncing and setup for re-training. Since eDP panels are not supposed to be hotpluggable we can avoid using the HPD interrupts altogether and rely on HPD polling only for the suspend/resume case, saving us some spinlocking action and the overhead of interrupts firing at every suspend/resume cycle, achieving a faster (even if just slightly) display resume. Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> --- drivers/gpu/drm/mediatek/mtk_dp.c | 81 ++++++++++++++++++------------- 1 file changed, 46 insertions(+), 35 deletions(-)