Re: [Freedreno] [PATCH v4] drm/msm/dp: enable HDP plugin/unplugged interrupts at hpd_enable/disable

2023-05-23 Thread Kuogee Hsieh 



On 5/23/2023 5:34 AM, Dmitry Baryshkov wrote:

On 23/05/2023 15:35, Bjorn Andersson wrote:

On Mon, May 22, 2023 at 04:02:45PM -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.

Changes in V3:
-- dp_catalog_ctrl_hpd_enable() and dp_catalog_ctrl_hpd_disable()
-- rewording ocmmit text

Changes in V4:
-- replace dp_display_config_hpd() with dp_display_host_start()
-- move enable_irq() at dp_display_host_start();


I think what Dmitry was asking for was that you remove the disable_irq()
from dp_display_request_irq(), but perhaps I missed some argumentation
for why that can't/shouldn't be done?



Yes, I was asking to get it removed. Kuogee, is there any reason for 
keeping the enable_irq() / disable_irq() calls?


I do not know the story regard this.

I can only guess after devm_request_irq() irq is enabled which may cause 
some unexpected side effects.


I will remove both disable_irq() and enable_irq() and run more tests.

Re: [Freedreno] [PATCH v4] drm/msm/dp: enable HDP plugin/unplugged interrupts at hpd_enable/disable

2023-05-23 Thread Dmitry Baryshkov 

On 23/05/2023 15:35, Bjorn Andersson wrote:

On Mon, May 22, 2023 at 04:02:45PM -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.

Changes in V3:
-- dp_catalog_ctrl_hpd_enable() and dp_catalog_ctrl_hpd_disable()
-- rewording ocmmit text

Changes in V4:
-- replace dp_display_config_hpd() with dp_display_host_start()
-- move enable_irq() at dp_display_host_start();


I think what Dmitry was asking for was that you remove the disable_irq()
from dp_display_request_irq(), but perhaps I missed some argumentation
for why that can't/shouldn't be done?



Yes, I was asking to get it removed. Kuogee, is there any reason for 
keeping the enable_irq() / disable_irq() calls?


--
With best wishes
Dmitry

Re: [Freedreno] [PATCH v4] drm/msm/dp: enable HDP plugin/unplugged interrupts at hpd_enable/disable

2023-05-23 Thread Bjorn Andersson 
On Mon, May 22, 2023 at 04:02:45PM -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.
> 
> Changes in V3:
> -- dp_catalog_ctrl_hpd_enable() and dp_catalog_ctrl_hpd_disable()
> -- rewording ocmmit text
> 
> Changes in V4:
> -- replace dp_display_config_hpd() with dp_display_host_start()
> -- move enable_irq() at dp_display_host_start();

I think what Dmitry was asking for was that you remove the disable_irq()
from dp_display_request_irq(), but perhaps I missed some argumentation
for why that can't/shouldn't be done?

> 

Either way, this looks good and it works for me.

Reviewed-by: Bjorn Andersson 
Tested-by: Bjorn Andersson 

Thanks,
Bjorn

> Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
> Signed-off-by: Kuogee Hsieh 
> ---
>  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 | 58 
> +
>  3 files changed, 36 insertions(+), 40 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..370632b 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(>event_mutex);
> @@ -658,12 +652,6 @@ static int

[Freedreno] [PATCH v4] drm/msm/dp: enable HDP plugin/unplugged interrupts at hpd_enable/disable

2023-05-22 Thread Kuogee Hsieh 
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

Changes in V4:
-- replace dp_display_config_hpd() with dp_display_host_start()
-- move enable_irq() at dp_display_host_start();

Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
Signed-off-by: Kuogee Hsieh 
---
 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 | 58 +
 3 files changed, 36 insertions(+), 40 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..370632b 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(>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 */