Message-ID: <a67f3250-b1f1-88b7-65a3-17b7e1274a48@linaro.org>
Date:   Mon, 22 May 2023 23:16:57 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Kuogee Hsieh <quic_khsieh@...cinc.com>, robdclark@...il.com,
        sean@...rly.run, swboyd@...omium.org, dianders@...omium.org,
        vkoul@...nel.org, daniel@...ll.ch, airlied@...il.com,
        agross@...nel.org, andersson@...nel.org,
        marijn.suijten@...ainline.org
Cc:     quic_abhinavk@...cinc.com, quic_aravindh@...cinc.com,
        quic_jesszhan@...cinc.com, quic_sbillaka@...cinc.com,
        freedreno@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm/msm/dp: enable HDP plugin/unplugged interrupts at
 hpd_enable/disable

On 22/05/2023 23:08, Kuogee Hsieh wrote:
> The internal_hpd flag was introduced to handle external DP HPD derived
> from GPIO pinmuxed into DP controller. HPD plug/unplug interrupts cannot
> be enabled until internal_hpd flag is set to true.
> At both bootup and resume time, the DP driver will enable external DP
> plugin interrupts and handle plugin interrupt accordingly.


> However
> dp_bridge_hpd_enable() bridge ops function was called to set internal_hpd
> to true later than where DP driver expected during both bootup and resume
> time.

Excuse me, can't parse this.

> This prevent DP driver from detecting external plugin interrupt due
> to HPD related interrupts are not enabled on time. Hence external display

prevents
not being enabled

> can not be lit up properly.
> 
> Move HPD related interrupts control of both enable/disable HDP hardware
> block and setup individual interrupt mask bits to dp_bridge_hpd_enable()
> and dp_bridge_hpd_disable().
> 
> Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
> Signed-off-by: Kuogee Hsieh <quic_khsieh@...cinc.com>

This looks pretty good to me, minor nit below.

> ---
>   drivers/gpu/drm/msm/dp/dp_catalog.c | 19 ++++++----
>   drivers/gpu/drm/msm/dp/dp_catalog.h |  2 +-
>   drivers/gpu/drm/msm/dp/dp_display.c | 70 +++++++++++--------------------------
>   3 files changed, 35 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 7a8cf1c..1da711e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -620,19 +620,26 @@ 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_config(struct dp_catalog *dp_catalog, bool enable)

Could you please split it into _enable and _disable functions? Having 
dp_catalog_ctrl_hpd_config(.., false) doesn't sound logical.

>   {
>   	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);
>   
> -	/* Configure REFTIMER and enable it */
> -	reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> -	dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> +	if (enable) {
> +		/* Configure REFTIMER and enable it */
> +		reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> +		dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> +
> +		/* Enable HPD */
> +		dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN);
> +	} else {
> +		reftimer &= ~DP_DP_HPD_REFTIMER_ENABLE;
> +		dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
>   
> -	/* Enable HPD */
> -	dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, DP_DP_HPD_CTRL_HPD_EN);
> +		dp_write_aux(catalog, REG_DP_DP_HPD_CTRL, 0);
> +	}
>   }
>   
>   static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog)
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 82376a2..818c036 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -104,7 +104,7 @@ 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_config(struct dp_catalog *dp_catalog, bool enable);
>   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..fee23e5 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_config(dp->catalog, true);
>   
>   	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_config(dp_priv->catalog, true);
>   		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_config(dp->catalog, true);
> +
> +	/* 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_config(dp->catalog, false);
>   
>   	dp_display->internal_hpd = false;
> +	mutex_unlock(&dp->event_mutex);
>   }
>   
>   void dp_bridge_hpd_notify(struct drm_bridge *bridge,

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists