| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c4d6e019-1582-c94f-d56d-271c420d72ea@linaro.org>
Date: Tue, 23 May 2023 01:03:26 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Bjorn Andersson <andersson@...nel.org>,
Kuogee Hsieh <quic_khsieh@...cinc.com>
Cc: robdclark@...il.com, sean@...rly.run, swboyd@...omium.org,
dianders@...omium.org, vkoul@...nel.org, daniel@...ll.ch,
airlied@...il.com, agross@...nel.org,
marijn.suijten@...ainline.org, quic_abhinavk@...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 v3] drm/msm/dp: enable HDP plugin/unplugged interrupts at
hpd_enable/disable
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 <quic_khsieh@...cinc.com>
>> ---
>> 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
Powered by blists - more mailing lists