| 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: <a507d7f2-c1f8-f8bd-24fa-15c4a2fb5a8b@quicinc.com>
Date: Fri, 14 Jan 2022 09:04:46 -0800
From: Kuogee Hsieh <quic_khsieh@...cinc.com>
To: Stephen Boyd <swboyd@...omium.org>, <agross@...nel.org>,
<airlied@...ux.ie>, <bjorn.andersson@...aro.org>,
<daniel@...ll.ch>, <dmitry.baryshkov@...aro.org>,
<dri-devel@...ts.freedesktop.org>, <robdclark@...il.com>,
<sean@...rly.run>, <vkoul@...nel.org>
CC: <quic_abhinavk@...cinc.com>, <aravindh@...eaurora.org>,
<quic_sbillaka@...cinc.com>, <freedreno@...ts.freedesktop.org>,
<linux-arm-msm@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v13 1/4] drm/msm/dp: do not initialize phy until plugin
interrupt received
On 1/13/2022 6:42 PM, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2022-01-13 15:53:36)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 7cc4d21..b3c5404 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -83,6 +83,7 @@ struct dp_display_private {
>>
>> /* state variables */
>> bool core_initialized;
>> + bool phy_initialized;
>> bool hpd_irq_on;
>> bool audio_supported;
>>
>> @@ -372,21 +373,38 @@ static int dp_display_process_hpd_high(struct dp_display_private *dp)
>> return rc;
>> }
>>
>> -static void dp_display_host_init(struct dp_display_private *dp, int reset)
>> +static void dp_display_host_phy_init(struct dp_display_private *dp)
>> {
>> - bool flip = false;
>> + DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
>> + dp->core_initialized, dp->phy_initialized);
>>
>> + if (!dp->phy_initialized) {
>> + dp_ctrl_phy_init(dp->ctrl);
>> + dp->phy_initialized = true;
>> + }
>> +}
>> +
>> +static void dp_display_host_phy_exit(struct dp_display_private *dp)
>> +{
>> + DRM_DEBUG_DP("core_init=%d phy_init=%d\n",
>> + dp->core_initialized, dp->phy_initialized);
>> +
>> + if (dp->phy_initialized) {
>> + dp_ctrl_phy_exit(dp->ctrl);
>> + dp->phy_initialized = false;
>> + }
>> +}
>> +
>> +static void dp_display_host_init(struct dp_display_private *dp)
>> +{
>> DRM_DEBUG_DP("core_initialized=%d\n", dp->core_initialized);
>> if (dp->core_initialized) {
> When is this true? From what I see dp_display_host_init() is only called
> from two places: resume where core_initialized has been set to false
> during suspend or from dp_display_config_hpd() kicked by the kthread
> where core_initialized is also false.
>
> Also, I see that dp_display_host_deinit() is only called from suspend
> now, so 'core_initialized' is almost always true, except for on the
> resume path and before the kthread is started and in the case that the
> driver probes but can't start the kthread for some reason (is that
> real?).
Yes, that true.
The purpose of dp_display_host_init() is to configure both dp
controller and hpd controller to be ready to receive
HPD plugged-in interrupts and at plugged-in handler enable dp phy.
Therefore core_initialized is set to true at both booting up and resume
and false at suspend.
>> DRM_DEBUG_DP("DP core already initialized\n");
>> return;
>> }
>>
>> - if (dp->usbpd->orientation == ORIENTATION_CC2)
>> - flip = true;
>> -
>> - dp_power_init(dp->power, flip);
>> - dp_ctrl_host_init(dp->ctrl, flip, reset);
>> + dp_power_init(dp->power, false);
>> + dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>> dp_aux_init(dp->aux);
>> dp->core_initialized = true;
>> }
>> @@ -892,12 +901,19 @@ static int dp_display_disable(struct dp_display_private *dp, u32 data)
>>
>> dp_display->audio_enabled = false;
>>
>> - /* triggered by irq_hpd with sink_count = 0 */
>> if (dp->link->sink_count == 0) {
>> + /*
>> + * irq_hpd with sink_count = 0
>> + * hdmi unplugged out of dongle
>> + */
>> dp_ctrl_off_link_stream(dp->ctrl);
>> } else {
>> + /*
>> + * unplugged interrupt
>> + * dongle unplugged out of DUT
>> + */
>> dp_ctrl_off(dp->ctrl);
>> - dp->core_initialized = false;
>> + dp_display_host_phy_exit(dp);
>> }
>>
>> dp_display->power_on = false;
>> @@ -1027,7 +1043,7 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp)
>> static void dp_display_config_hpd(struct dp_display_private *dp)
>> {
>>
>> - dp_display_host_init(dp, true);
>> + dp_display_host_init(dp);
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>> /* Enable interrupt first time
>> @@ -1306,20 +1322,23 @@ static int dp_pm_resume(struct device *dev)
>> dp->hpd_state = ST_DISCONNECTED;
>>
>> /* turn on dp ctrl/phy */
>> - dp_display_host_init(dp, true);
>> + dp_display_host_init(dp);
>>
>> dp_catalog_ctrl_hpd_config(dp->catalog);
>>
>> - /*
>> - * set sink to normal operation mode -- D0
>> - * before dpcd read
>> - */
>> - dp_link_psm_config(dp->link, &dp->panel->link_info, false);
>>
>> if (dp_catalog_link_is_connected(dp->catalog)) {
>> + /*
>> + * set sink to normal operation mode -- D0
>> + * before dpcd read
>> + */
>> + dp_display_host_phy_init(dp);
>> + dp_link_psm_config(dp->link, &dp->panel->link_info, false);
>> sink_count = drm_dp_read_sink_count(dp->aux);
>> if (sink_count < 0)
>> sink_count = 0;
>> +
>> + dp_display_host_phy_exit(dp);
>> }
>>
>> dp->link->sink_count = sink_count;
>> @@ -1363,7 +1382,11 @@ static int dp_pm_suspend(struct device *dev)
>> if (dp_power_clk_status(dp->power, DP_CTRL_PM))
>> dp_ctrl_off_link_stream(dp->ctrl);
>>
>> + dp_display_host_phy_exit(dp);
>> +
> Why is there a newline here?
>
>> dp_display_host_deinit(dp);
>> + } else {
>> + dp_display_host_phy_exit(dp);
>> }
>>
>> dp->hpd_state = ST_SUSPENDED;
> There's a dp->core_initialized = false right here but it's not in the
> diff window. It's redundant now because the hunk above is basically
>
> if (dp->core_initialized == true) {
> ...
> dp_display_host_phy_exit(dp);
> dp_display_host_deinit(dp);
> } else {
> dp_display_host_phy_exit(dp);
> }
>
> dp->hpd_state = ST_SUSPENDED;
>
> and dp_display_host_deinit() sets core_initialized to false, thus
> core_initialized will be false here already. Can you remove the
> duplicate assignment?
yes,
>> @@ -1535,7 +1558,7 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder)
>> state = dp_display->hpd_state;
>>
>> if (state == ST_DISPLAY_OFF)
>> - dp_display_host_init(dp_display, true);
>> + dp_display_host_phy_init(dp_display);
>>
>> dp_display_enable(dp_display, 0);
>>
Powered by blists - more mailing lists