[v6,11/11] drm/mediatek: dp: Don't register HPD interrupt handler for eDP case
Commit Message
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(-)
Comments
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
@@ -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;