The internal_hpd flag is set to true by dp_bridge_hpd_enable() and set to
false by dp_bridge_hpd_disable() to handle GPIO pinmuxed into DP controller
case. HDP related interrupts can not be enabled until internal_hpd is set
to true. At current implementation dp_display_config_hpd() will initialize
DP host controller first followed by enabling HDP related interrupts if
internal_hpd was true at that time. Enable HDP related interrupts depends on
internal_hpd status may leave system with DP driver host is in running state
but without HDP related interrupts being enabled. This will prevent external
display from being detected. Eliminated this dependency by moving HDP related
interrupts enable/disable be done at dp_bridge_hpd_enable/disable() directly
regardless of internal_hpd status.
Changes in V3:
-- dp_catalog_ctrl_hpd_enable() and dp_catalog_ctrl_hpd_disable()
-- rewording ocmmit text
Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
Signed-off-by: Kuogee Hsieh <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_catalog.c | 15 +++++++-
drivers/gpu/drm/msm/dp/dp_catalog.h | 3 +-
drivers/gpu/drm/msm/dp/dp_display.c | 70 +++++++++++--------------------------
3 files changed, 37 insertions(+), 51 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 7a8cf1c..5142aeb 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -620,7 +620,7 @@ void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
config & DP_DP_HPD_INT_MASK);
}
-void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
+void dp_catalog_ctrl_hpd_enable(struct dp_catalog *dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
struct dp_catalog_private, dp_catalog);
@@ -635,6 +635,19 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN);
}
+void dp_catalog_ctrl_hpd_disable(struct dp_catalog *dp_catalog)
+{
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+
+ u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
+
+ reftimer &= ~DP_DP_HPD_REFTIMER_ENABLE;
+ dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
+
+ dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, 0);
+}
+
static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog)
{
/* trigger sdp */
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 82376a2..38786e8 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -104,7 +104,8 @@ bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool enable);
void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
u32 intr_mask, bool en);
-void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog);
+void dp_catalog_ctrl_hpd_enable(struct dp_catalog *dp_catalog);
+void dp_catalog_ctrl_hpd_disable(struct dp_catalog *dp_catalog);
void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog);
void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter);
u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog);
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 3e13acdf..69bbc5f 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -615,12 +615,6 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
dp->hpd_state = ST_MAINLINK_READY;
}
- /* enable HDP irq_hpd/replug interrupt */
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog,
- DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK,
- true);
-
drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n",
dp->dp_display.connector_type, state);
mutex_unlock(&dp->event_mutex);
@@ -658,12 +652,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
dp->dp_display.connector_type, state);
- /* disable irq_hpd/replug interrupts */
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog,
- DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK,
- false);
-
/* unplugged, no more irq_hpd handle */
dp_del_event(dp, EV_IRQ_HPD_INT);
@@ -687,10 +675,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
return 0;
}
- /* disable HPD plug interrupts */
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
-
/*
* We don't need separate work for disconnect as
* connect/attention interrupts are disabled
@@ -706,10 +690,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
/* signal the disconnect event early to ensure proper teardown */
dp_display_handle_plugged_change(&dp->dp_display, false);
- /* enable HDP plug interrupt to prepare for next plugin */
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
-
drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n",
dp->dp_display.connector_type, state);
@@ -1082,26 +1062,6 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
mutex_unlock(&dp_display->event_mutex);
}
-static void dp_display_config_hpd(struct dp_display_private *dp)
-{
-
- dp_display_host_init(dp);
- dp_catalog_ctrl_hpd_config(dp->catalog);
-
- /* Enable plug and unplug interrupts only if requested */
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog,
- DP_DP_HPD_PLUG_INT_MASK |
- DP_DP_HPD_UNPLUG_INT_MASK,
- true);
-
- /* Enable interrupt first time
- * we are leaving dp clocks on during disconnect
- * and never disable interrupt
- */
- enable_irq(dp->irq);
-}
-
void dp_display_set_psr(struct msm_dp *dp_display, bool enter)
{
struct dp_display_private *dp;
@@ -1176,7 +1136,7 @@ static int hpd_event_thread(void *data)
switch (todo->event_id) {
case EV_HPD_INIT_SETUP:
- dp_display_config_hpd(dp_priv);
+ dp_display_host_init(dp_priv);
break;
case EV_HPD_PLUG_INT:
dp_hpd_plug_handle(dp_priv, todo->data);
@@ -1394,13 +1354,8 @@ static int dp_pm_resume(struct device *dev)
/* turn on dp ctrl/phy */
dp_display_host_init(dp);
- dp_catalog_ctrl_hpd_config(dp->catalog);
-
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog,
- DP_DP_HPD_PLUG_INT_MASK |
- DP_DP_HPD_UNPLUG_INT_MASK,
- true);
+ if (dp_display->is_edp)
+ dp_catalog_ctrl_hpd_enable(dp->catalog);
if (dp_catalog_link_is_connected(dp->catalog)) {
/*
@@ -1568,7 +1523,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
if (aux_bus && dp->is_edp) {
dp_display_host_init(dp_priv);
- dp_catalog_ctrl_hpd_config(dp_priv->catalog);
+ dp_catalog_ctrl_hpd_enable(dp_priv->catalog);
dp_display_host_phy_init(dp_priv);
enable_irq(dp_priv->irq);
@@ -1801,16 +1756,33 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
{
struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
struct msm_dp *dp_display = dp_bridge->dp_display;
+ struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display);
+
+ mutex_lock(&dp->event_mutex);
+ dp_catalog_ctrl_hpd_enable(dp->catalog);
+
+ /* enable HDP interrupts */
+ dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, true);
+
+ enable_irq(dp->irq);
dp_display->internal_hpd = true;
+ mutex_unlock(&dp->event_mutex);
}
void dp_bridge_hpd_disable(struct drm_bridge *bridge)
{
struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
struct msm_dp *dp_display = dp_bridge->dp_display;
+ struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display);
+
+ mutex_lock(&dp->event_mutex);
+ /* disable HDP interrupts */
+ dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false);
+ dp_catalog_ctrl_hpd_disable(dp->catalog);
dp_display->internal_hpd = false;
+ mutex_unlock(&dp->event_mutex);
}
void dp_bridge_hpd_notify(struct drm_bridge *bridge,
--
2.7.4
On 23/05/2023 01:05, Bjorn Andersson wrote:
> On Mon, May 22, 2023 at 02:31:32PM -0700, Kuogee Hsieh wrote:
>> The internal_hpd flag is set to true by dp_bridge_hpd_enable() and set to
>> false by dp_bridge_hpd_disable() to handle GPIO pinmuxed into DP controller
>> case. HDP related interrupts can not be enabled until internal_hpd is set
>> to true. At current implementation dp_display_config_hpd() will initialize
>> DP host controller first followed by enabling HDP related interrupts if
>> internal_hpd was true at that time. Enable HDP related interrupts depends on
>> internal_hpd status may leave system with DP driver host is in running state
>> but without HDP related interrupts being enabled. This will prevent external
>> display from being detected. Eliminated this dependency by moving HDP related
>> interrupts enable/disable be done at dp_bridge_hpd_enable/disable() directly
>> regardless of internal_hpd status.
>>
>
> Thanks Kuogee, this looks quite good to me. But...
>
>> Changes in V3:
>> -- dp_catalog_ctrl_hpd_enable() and dp_catalog_ctrl_hpd_disable()
>> -- rewording ocmmit text
>>
>> Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
>> Signed-off-by: Kuogee Hsieh <[email protected]>
>> ---
>> drivers/gpu/drm/msm/dp/dp_catalog.c | 15 +++++++-
>> drivers/gpu/drm/msm/dp/dp_catalog.h | 3 +-
>> drivers/gpu/drm/msm/dp/dp_display.c | 70 +++++++++++--------------------------
>> 3 files changed, 37 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 7a8cf1c..5142aeb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -620,7 +620,7 @@ void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
>> config & DP_DP_HPD_INT_MASK);
>> }
>>
>> -void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>> +void dp_catalog_ctrl_hpd_enable(struct dp_catalog *dp_catalog)
>> {
>> struct dp_catalog_private *catalog = container_of(dp_catalog,
>> struct dp_catalog_private, dp_catalog);
>> @@ -635,6 +635,19 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
>> dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN);
>> }
>>
>> +void dp_catalog_ctrl_hpd_disable(struct dp_catalog *dp_catalog)
>> +{
>> + struct dp_catalog_private *catalog = container_of(dp_catalog,
>> + struct dp_catalog_private, dp_catalog);
>> +
>> + u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>> +
>> + reftimer &= ~DP_DP_HPD_REFTIMER_ENABLE;
>> + dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
>> +
>> + dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, 0);
>> +}
>> +
>> static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog)
>> {
>> /* trigger sdp */
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> index 82376a2..38786e8 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> @@ -104,7 +104,8 @@ bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
>> void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool enable);
>> void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
>> u32 intr_mask, bool en);
>> -void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog);
>> +void dp_catalog_ctrl_hpd_enable(struct dp_catalog *dp_catalog);
>> +void dp_catalog_ctrl_hpd_disable(struct dp_catalog *dp_catalog);
>> void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog);
>> void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter);
>> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 3e13acdf..69bbc5f 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -615,12 +615,6 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
>> dp->hpd_state = ST_MAINLINK_READY;
>> }
>>
>> - /* enable HDP irq_hpd/replug interrupt */
>> - if (dp->dp_display.internal_hpd)
>> - dp_catalog_hpd_config_intr(dp->catalog,
>> - DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK,
>> - true);
>> -
>> drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n",
>> dp->dp_display.connector_type, state);
>> mutex_unlock(&dp->event_mutex);
>> @@ -658,12 +652,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>> drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
>> dp->dp_display.connector_type, state);
>>
>> - /* disable irq_hpd/replug interrupts */
>> - if (dp->dp_display.internal_hpd)
>> - dp_catalog_hpd_config_intr(dp->catalog,
>> - DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK,
>> - false);
>> -
>> /* unplugged, no more irq_hpd handle */
>> dp_del_event(dp, EV_IRQ_HPD_INT);
>>
>> @@ -687,10 +675,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>> return 0;
>> }
>>
>> - /* disable HPD plug interrupts */
>> - if (dp->dp_display.internal_hpd)
>> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
>> -
>> /*
>> * We don't need separate work for disconnect as
>> * connect/attention interrupts are disabled
>> @@ -706,10 +690,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
>> /* signal the disconnect event early to ensure proper teardown */
>> dp_display_handle_plugged_change(&dp->dp_display, false);
>>
>> - /* enable HDP plug interrupt to prepare for next plugin */
>> - if (dp->dp_display.internal_hpd)
>> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
>> -
>> drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n",
>> dp->dp_display.connector_type, state);
>>
>> @@ -1082,26 +1062,6 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
>> mutex_unlock(&dp_display->event_mutex);
>> }
>>
>> -static void dp_display_config_hpd(struct dp_display_private *dp)
>> -{
>> -
>> - dp_display_host_init(dp);
>> - dp_catalog_ctrl_hpd_config(dp->catalog);
>> -
>> - /* Enable plug and unplug interrupts only if requested */
>> - if (dp->dp_display.internal_hpd)
>> - dp_catalog_hpd_config_intr(dp->catalog,
>> - DP_DP_HPD_PLUG_INT_MASK |
>> - DP_DP_HPD_UNPLUG_INT_MASK,
>> - true);
>> -
>> - /* Enable interrupt first time
>> - * we are leaving dp clocks on during disconnect
>> - * and never disable interrupt
>> - */
>> - enable_irq(dp->irq);
>
> ...we need dp->irq enabled for handling the other interrupts, otherwise
> e.g. AUX transfers will time out.
>
> I added enable_irq(dp_priv->irq) to the EV_HPD_INIT_SETUP case below,
> just for testing, and with that the patch seems to be working fine.
>
>
> Is there any reason why we need to delay its enablement to after we
> unmask the HPD interrupts?
As I wrote, I'd probably prefer to see enable_irq() and disable_irq()
calls gone.
>
> Regards,
> Bjorn
>
>> -}
>> -
>> void dp_display_set_psr(struct msm_dp *dp_display, bool enter)
>> {
>> struct dp_display_private *dp;
>> @@ -1176,7 +1136,7 @@ static int hpd_event_thread(void *data)
>>
>> switch (todo->event_id) {
>> case EV_HPD_INIT_SETUP:
>> - dp_display_config_hpd(dp_priv);
>> + dp_display_host_init(dp_priv);
>> break;
>> case EV_HPD_PLUG_INT:
>> dp_hpd_plug_handle(dp_priv, todo->data);
>> @@ -1394,13 +1354,8 @@ static int dp_pm_resume(struct device *dev)
>> /* turn on dp ctrl/phy */
>> dp_display_host_init(dp);
>>
>> - dp_catalog_ctrl_hpd_config(dp->catalog);
>> -
>> - if (dp->dp_display.internal_hpd)
>> - dp_catalog_hpd_config_intr(dp->catalog,
>> - DP_DP_HPD_PLUG_INT_MASK |
>> - DP_DP_HPD_UNPLUG_INT_MASK,
>> - true);
>> + if (dp_display->is_edp)
>> + dp_catalog_ctrl_hpd_enable(dp->catalog);
>>
>> if (dp_catalog_link_is_connected(dp->catalog)) {
>> /*
>> @@ -1568,7 +1523,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>>
>> if (aux_bus && dp->is_edp) {
>> dp_display_host_init(dp_priv);
>> - dp_catalog_ctrl_hpd_config(dp_priv->catalog);
>> + dp_catalog_ctrl_hpd_enable(dp_priv->catalog);
>> dp_display_host_phy_init(dp_priv);
>> enable_irq(dp_priv->irq);
>>
>> @@ -1801,16 +1756,33 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
>> {
>> struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
>> struct msm_dp *dp_display = dp_bridge->dp_display;
>> + struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display);
>> +
>> + mutex_lock(&dp->event_mutex);
>> + dp_catalog_ctrl_hpd_enable(dp->catalog);
>> +
>> + /* enable HDP interrupts */
>> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, true);
>> +
>> + enable_irq(dp->irq);
>>
>> dp_display->internal_hpd = true;
>> + mutex_unlock(&dp->event_mutex);
>> }
>>
>> void dp_bridge_hpd_disable(struct drm_bridge *bridge)
>> {
>> struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
>> struct msm_dp *dp_display = dp_bridge->dp_display;
>> + struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display);
>> +
>> + mutex_lock(&dp->event_mutex);
>> + /* disable HDP interrupts */
>> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false);
>> + dp_catalog_ctrl_hpd_disable(dp->catalog);
>>
>> dp_display->internal_hpd = false;
>> + mutex_unlock(&dp->event_mutex);
>> }
>>
>> void dp_bridge_hpd_notify(struct drm_bridge *bridge,
>> --
>> 2.7.4
>>
--
With best wishes
Dmitry
On Mon, May 22, 2023 at 02:31:32PM -0700, Kuogee Hsieh wrote:
> The internal_hpd flag is set to true by dp_bridge_hpd_enable() and set to
> false by dp_bridge_hpd_disable() to handle GPIO pinmuxed into DP controller
> case. HDP related interrupts can not be enabled until internal_hpd is set
> to true. At current implementation dp_display_config_hpd() will initialize
> DP host controller first followed by enabling HDP related interrupts if
> internal_hpd was true at that time. Enable HDP related interrupts depends on
> internal_hpd status may leave system with DP driver host is in running state
> but without HDP related interrupts being enabled. This will prevent external
> display from being detected. Eliminated this dependency by moving HDP related
> interrupts enable/disable be done at dp_bridge_hpd_enable/disable() directly
> regardless of internal_hpd status.
>
Thanks Kuogee, this looks quite good to me. But...
> Changes in V3:
> -- dp_catalog_ctrl_hpd_enable() and dp_catalog_ctrl_hpd_disable()
> -- rewording ocmmit text
>
> Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
> Signed-off-by: Kuogee Hsieh <[email protected]>
> ---
> drivers/gpu/drm/msm/dp/dp_catalog.c | 15 +++++++-
> drivers/gpu/drm/msm/dp/dp_catalog.h | 3 +-
> drivers/gpu/drm/msm/dp/dp_display.c | 70 +++++++++++--------------------------
> 3 files changed, 37 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 7a8cf1c..5142aeb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -620,7 +620,7 @@ void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
> config & DP_DP_HPD_INT_MASK);
> }
>
> -void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
> +void dp_catalog_ctrl_hpd_enable(struct dp_catalog *dp_catalog)
> {
> struct dp_catalog_private *catalog = container_of(dp_catalog,
> struct dp_catalog_private, dp_catalog);
> @@ -635,6 +635,19 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog)
> dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN);
> }
>
> +void dp_catalog_ctrl_hpd_disable(struct dp_catalog *dp_catalog)
> +{
> + struct dp_catalog_private *catalog = container_of(dp_catalog,
> + struct dp_catalog_private, dp_catalog);
> +
> + u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
> +
> + reftimer &= ~DP_DP_HPD_REFTIMER_ENABLE;
> + dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> +
> + dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, 0);
> +}
> +
> static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog)
> {
> /* trigger sdp */
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 82376a2..38786e8 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -104,7 +104,8 @@ bool dp_catalog_ctrl_mainlink_ready(struct dp_catalog *dp_catalog);
> void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool enable);
> void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
> u32 intr_mask, bool en);
> -void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog);
> +void dp_catalog_ctrl_hpd_enable(struct dp_catalog *dp_catalog);
> +void dp_catalog_ctrl_hpd_disable(struct dp_catalog *dp_catalog);
> void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog);
> void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter);
> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 3e13acdf..69bbc5f 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -615,12 +615,6 @@ static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
> dp->hpd_state = ST_MAINLINK_READY;
> }
>
> - /* enable HDP irq_hpd/replug interrupt */
> - if (dp->dp_display.internal_hpd)
> - dp_catalog_hpd_config_intr(dp->catalog,
> - DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK,
> - true);
> -
> drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n",
> dp->dp_display.connector_type, state);
> mutex_unlock(&dp->event_mutex);
> @@ -658,12 +652,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
> dp->dp_display.connector_type, state);
>
> - /* disable irq_hpd/replug interrupts */
> - if (dp->dp_display.internal_hpd)
> - dp_catalog_hpd_config_intr(dp->catalog,
> - DP_DP_IRQ_HPD_INT_MASK | DP_DP_HPD_REPLUG_INT_MASK,
> - false);
> -
> /* unplugged, no more irq_hpd handle */
> dp_del_event(dp, EV_IRQ_HPD_INT);
>
> @@ -687,10 +675,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> return 0;
> }
>
> - /* disable HPD plug interrupts */
> - if (dp->dp_display.internal_hpd)
> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, false);
> -
> /*
> * We don't need separate work for disconnect as
> * connect/attention interrupts are disabled
> @@ -706,10 +690,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
> /* signal the disconnect event early to ensure proper teardown */
> dp_display_handle_plugged_change(&dp->dp_display, false);
>
> - /* enable HDP plug interrupt to prepare for next plugin */
> - if (dp->dp_display.internal_hpd)
> - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true);
> -
> drm_dbg_dp(dp->drm_dev, "After, type=%d hpd_state=%d\n",
> dp->dp_display.connector_type, state);
>
> @@ -1082,26 +1062,6 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
> mutex_unlock(&dp_display->event_mutex);
> }
>
> -static void dp_display_config_hpd(struct dp_display_private *dp)
> -{
> -
> - dp_display_host_init(dp);
> - dp_catalog_ctrl_hpd_config(dp->catalog);
> -
> - /* Enable plug and unplug interrupts only if requested */
> - if (dp->dp_display.internal_hpd)
> - dp_catalog_hpd_config_intr(dp->catalog,
> - DP_DP_HPD_PLUG_INT_MASK |
> - DP_DP_HPD_UNPLUG_INT_MASK,
> - true);
> -
> - /* Enable interrupt first time
> - * we are leaving dp clocks on during disconnect
> - * and never disable interrupt
> - */
> - enable_irq(dp->irq);
...we need dp->irq enabled for handling the other interrupts, otherwise
e.g. AUX transfers will time out.
I added enable_irq(dp_priv->irq) to the EV_HPD_INIT_SETUP case below,
just for testing, and with that the patch seems to be working fine.
Is there any reason why we need to delay its enablement to after we
unmask the HPD interrupts?
Regards,
Bjorn
> -}
> -
> void dp_display_set_psr(struct msm_dp *dp_display, bool enter)
> {
> struct dp_display_private *dp;
> @@ -1176,7 +1136,7 @@ static int hpd_event_thread(void *data)
>
> switch (todo->event_id) {
> case EV_HPD_INIT_SETUP:
> - dp_display_config_hpd(dp_priv);
> + dp_display_host_init(dp_priv);
> break;
> case EV_HPD_PLUG_INT:
> dp_hpd_plug_handle(dp_priv, todo->data);
> @@ -1394,13 +1354,8 @@ static int dp_pm_resume(struct device *dev)
> /* turn on dp ctrl/phy */
> dp_display_host_init(dp);
>
> - dp_catalog_ctrl_hpd_config(dp->catalog);
> -
> - if (dp->dp_display.internal_hpd)
> - dp_catalog_hpd_config_intr(dp->catalog,
> - DP_DP_HPD_PLUG_INT_MASK |
> - DP_DP_HPD_UNPLUG_INT_MASK,
> - true);
> + if (dp_display->is_edp)
> + dp_catalog_ctrl_hpd_enable(dp->catalog);
>
> if (dp_catalog_link_is_connected(dp->catalog)) {
> /*
> @@ -1568,7 +1523,7 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
>
> if (aux_bus && dp->is_edp) {
> dp_display_host_init(dp_priv);
> - dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> + dp_catalog_ctrl_hpd_enable(dp_priv->catalog);
> dp_display_host_phy_init(dp_priv);
> enable_irq(dp_priv->irq);
>
> @@ -1801,16 +1756,33 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
> {
> struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
> struct msm_dp *dp_display = dp_bridge->dp_display;
> + struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> + mutex_lock(&dp->event_mutex);
> + dp_catalog_ctrl_hpd_enable(dp->catalog);
> +
> + /* enable HDP interrupts */
> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, true);
> +
> + enable_irq(dp->irq);
>
> dp_display->internal_hpd = true;
> + mutex_unlock(&dp->event_mutex);
> }
>
> void dp_bridge_hpd_disable(struct drm_bridge *bridge)
> {
> struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
> struct msm_dp *dp_display = dp_bridge->dp_display;
> + struct dp_display_private *dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> + mutex_lock(&dp->event_mutex);
> + /* disable HDP interrupts */
> + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_INT_MASK, false);
> + dp_catalog_ctrl_hpd_disable(dp->catalog);
>
> dp_display->internal_hpd = false;
> + mutex_unlock(&dp->event_mutex);
> }
>
> void dp_bridge_hpd_notify(struct drm_bridge *bridge,
> --
> 2.7.4
>
>>> -static void dp_display_config_hpd(struct dp_display_private *dp)
>>> -{
>>> -
>>> - dp_display_host_init(dp);
>>> - dp_catalog_ctrl_hpd_config(dp->catalog);
>>> -
>>> - /* Enable plug and unplug interrupts only if requested */
>>> - if (dp->dp_display.internal_hpd)
>>> - dp_catalog_hpd_config_intr(dp->catalog,
>>> - DP_DP_HPD_PLUG_INT_MASK |
>>> - DP_DP_HPD_UNPLUG_INT_MASK,
>>> - true);
>>> -
>>> - /* Enable interrupt first time
>>> - * we are leaving dp clocks on during disconnect
>>> - * and never disable interrupt
>>> - */
>>> - enable_irq(dp->irq);
>>
>> ...we need dp->irq enabled for handling the other interrupts, otherwise
>> e.g. AUX transfers will time out.
>>
>> I added enable_irq(dp_priv->irq) to the EV_HPD_INIT_SETUP case below,
>> just for testing, and with that the patch seems to be working fine.
>>
>>
>> Is there any reason why we need to delay its enablement to after we
>> unmask the HPD interrupts?
my though is aux transaction should happen after plugin interrupt detected.
can you please let me know what did you do to trigger aux timeout?
It does not happen on me at my test.
>
> As I wrote, I'd probably prefer to see enable_irq() and disable_irq()
> calls gone.
ok, i will move enable_irq() out of here.
>>
>> Regards,
>> Bjorn
>>
>
On Mon, May 22, 2023 at 03:35:13PM -0700, Kuogee Hsieh wrote:
>
>
> > > > ? -static void dp_display_config_hpd(struct dp_display_private *dp)
> > > > -{
> > > > -
> > > > -??? dp_display_host_init(dp);
> > > > -??? dp_catalog_ctrl_hpd_config(dp->catalog);
> > > > -
> > > > -??? /* Enable plug and unplug interrupts only if requested */
> > > > -??? if (dp->dp_display.internal_hpd)
> > > > -??????? dp_catalog_hpd_config_intr(dp->catalog,
> > > > -??????????????? DP_DP_HPD_PLUG_INT_MASK |
> > > > -??????????????? DP_DP_HPD_UNPLUG_INT_MASK,
> > > > -??????????????? true);
> > > > -
> > > > -??? /* Enable interrupt first time
> > > > -???? * we are leaving dp clocks on during disconnect
> > > > -???? * and never disable interrupt
> > > > -???? */
> > > > -??? enable_irq(dp->irq);
> > >
> > > ...we need dp->irq enabled for handling the other interrupts, otherwise
> > > e.g. AUX transfers will time out.
> > >
> > > I added enable_irq(dp_priv->irq) to the EV_HPD_INIT_SETUP case below,
> > > just for testing, and with that the patch seems to be working fine.
> > >
> > >
> > > Is there any reason why we need to delay its enablement to after we
> > > unmask the HPD interrupts?
>
> my though is aux transaction should happen after plugin interrupt detected.
>
I have a next_bridge which implements DRM_BRIDGE_OP_HPD, in which case
dp_bridge_hpd_enable() will never be called (the next_bridge's
hpd_enable will be called instead).
> can you please let me know what did you do to trigger aux timeout?
>
In this setup I just connect the cable, wait a few seconds and the
transfers fails - as there's no interrupts signalling them being
completed.
> It does not happen on me at my test.
>
Given that you have the HPD signal on a GPIO, you should be able to
rewrite your DTS in line with:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/qcom/sa8295p-adp.dts#n28
And pinmux the HPD signal as GPIO instead.
I believe this would allow you to test both code paths - without the
actual TCPM known to Linux.
> >
> > As I wrote, I'd probably prefer to see enable_irq() and disable_irq()
> > calls gone.
>
> ok, i will move enable_irq() out of here.
>
Thanks,
Bjorn
>
> > >
> > > Regards,
> > > Bjorn
> > >
> >