lkml.org 
[lkml]   [2021]   [May]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] drm/msm/dp: power off DP phy base on mainlink status at suspend
On 2021-05-26 15:30, Stephen Boyd wrote:
> Quoting Kuogee Hsieh (2021-05-26 11:08:23)
>> DP mainlink can be either enabled or disabled at the time of suspend
>> happen. Therefore DP phy teared down at suspend should base on
>> mainlink
>> status at that instance.
>
> Please add some more details here. The system crashes if you plug in
> the
> HDMI cable during system wide suspend. That seems to be because the DP
> phy isn't powered down during suspend if the HDMI cable is disconnected
> so we try to process the hpd plug event on the path to suspend instead
> of wait to bring up the phy and then the display?
>
> I'm trying to find the case when we would be entering suspend and only
> have called phy_init() without calling phy_exit(). What path is that? I
> guess it is dp_ctrl_off_link_stream() called when the sink count goes
> to
> 0? So plug in HDMI cable to apple dongle, unplug HDMI cable to apple
> dongle and phy_power_off() followed by phy_exit() followed by
> phy_init()
> and then enter suspend so we want to call phy_exit(). Then we only call
> phy_power_off() if we've called dp_ctrl_on()? I think I followed it
> all.
>
ok, will do
>>
>> Signed-off-by: Kuogee Hsieh <khsieh@codeaurora.org>
>> ---
>> drivers/gpu/drm/msm/dp/dp_ctrl.c | 5 ++++-
>> drivers/gpu/drm/msm/dp/dp_ctrl.h | 2 +-
>> drivers/gpu/drm/msm/dp/dp_display.c | 9 ++++++++-
>> 3 files changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index dbd8943..5115c05 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -1398,7 +1398,7 @@ int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl,
>> bool flip, bool reset)
>> * Perform required steps to uninitialize DP controller
>> * and its resources.
>> */
>> -void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl)
>> +void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl, bool mainlink_on)
>> {
>> struct dp_ctrl_private *ctrl;
>> struct dp_io *dp_io;
>> @@ -1414,6 +1414,9 @@ void dp_ctrl_host_deinit(struct dp_ctrl
>> *dp_ctrl)
>> phy = dp_io->phy;
>>
>> dp_catalog_ctrl_enable_irq(ctrl->catalog, false);
>> + if (mainlink_on)
>> + phy_power_off(phy);
>> +
>> phy_exit(phy);
>>
>> DRM_DEBUG_DP("Host deinitialized successfully\n");
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> index 25e4f75..a23ee2b 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> @@ -20,7 +20,7 @@ struct dp_ctrl {
>> };
>>
>> int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool
>> reset);
>> -void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl);
>> +void dp_ctrl_host_deinit(struct dp_ctrl *dp_ctrl, bool mainlink_on);
>> int dp_ctrl_on_link(struct dp_ctrl *dp_ctrl);
>> int dp_ctrl_on_stream(struct dp_ctrl *dp_ctrl);
>> int dp_ctrl_off_link_stream(struct dp_ctrl *dp_ctrl);
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index cdec0a3..88eeeb5 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -104,6 +104,8 @@ struct dp_display_private {
>>
>> bool encoder_mode_set;
>>
>> + bool mainlink_on;
>> +
>
> Is there a reason why this can't be stashed away in dp_ctrl.c in the
> 'struct dp_ctrl'? It seems to follow closely with dp_ctrl_*() APIs.
yes, I will do that.

>
>> /* wait for audio signaling */
>> struct completion audio_comp;
>>
>> @@ -353,11 +355,14 @@ static int dp_display_process_hpd_high(struct
>> dp_display_private *dp)
>> dp_link_psm_config(dp->link, &dp->panel->link_info, false);
>>
>> dp_link_reset_phy_params_vx_px(dp->link);
>> +
>> + dp->mainlink_on = false;
>
> Isn't this too late to be setting it to false? i.e. it should be false
> by default, and then set to false when a dp_ctrl_off() call is made?
>
>> rc = dp_ctrl_on_link(dp->ctrl);
>> if (rc) {
>> DRM_ERROR("failed to complete DP link training\n");
>> goto end;
>> }
>> + dp->mainlink_on = true;
>>
>> dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
>>
>> @@ -392,7 +397,7 @@ static void dp_display_host_deinit(struct
>> dp_display_private *dp)
>> return;
>> }
>>
>> - dp_ctrl_host_deinit(dp->ctrl);
>> + dp_ctrl_host_deinit(dp->ctrl, dp->mainlink_on);
>> dp_aux_deinit(dp->aux);
>> dp_power_deinit(dp->power);
>>
>> @@ -941,6 +946,8 @@ static int dp_display_disable(struct
>> dp_display_private *dp, u32 data)
>> dp->core_initialized = false;
>> }
>>
>> + dp->mainlink_on = false;
>> +
>> dp_display->power_on = false;
>>
>> return 0;
>
> It would certainly help to keep it contained to one file instead of
> two.
agree,

\
 
 \ /
  Last update: 2021-05-27 01:57    [W:0.212 / U:0.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site