Hi Doug, I incorporated the comments in v7. >Hi, > >On Mon, Jul 11, 2022 at 5:57 AM Vinod Polimera ><quic_vpolimer@xxxxxxxxxxx> wrote: >> >> @@ -359,6 +367,24 @@ void dp_catalog_ctrl_lane_mapping(struct >dp_catalog *dp_catalog) >> ln_mapping); >> } >> >> +void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog *dp_catalog, >> + bool enable) { >> + u32 val; >> + struct dp_catalog_private *catalog = container_of(dp_catalog, >> + struct dp_catalog_private, >> +dp_catalog); >> + >> + val = dp_read_link(catalog, REG_DP_MAINLINK_CTRL); >> + val &= ~DP_MAINLINK_CTRL_ENABLE; > >nit: the line above is useless. Remove. In the case that you're enabling you're >just adding the bit back in. In the case that you're disabling you're doing the >exact same operation below. > Incorporated the changes in v7 > >> @@ -610,6 +636,47 @@ 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); } >> >> +static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog) >> +{ >> + /* trigger sdp */ >> + dp_write_link(catalog, MMSS_DP_SDP_CFG3, UPDATE_SDP); >> + dp_write_link(catalog, MMSS_DP_SDP_CFG3, !UPDATE_SDP); > >!UPDATE_SDP is a really counter-intuitive way to say 0x0. > Changed to 0x0 in v7 > >> @@ -645,6 +712,20 @@ u32 dp_catalog_hpd_get_intr_status(struct >dp_catalog *dp_catalog) >> return isr & (mask | ~DP_DP_HPD_INT_MASK); } >> >> +int dp_catalog_ctrl_read_psr_interrupt_status(struct dp_catalog >> +*dp_catalog) > >Why is the return type "int" and not "u32". It's a hardware register. >It's u32 here. The caller assigns it to a u32. > Changed to u32 > >> +void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enter) { >> + struct dp_ctrl_private *ctrl = container_of(dp_ctrl, >> + struct dp_ctrl_private, dp_ctrl); >> + >> + if (!ctrl->panel->psr_cap.version) >> + return; >> + >> + /* >> + * When entering PSR, >> + * 1. Send PSR enter SDP and wait for the PSR_UPDATE_INT >> + * 2. Turn off video >> + * 3. Disable the mainlink >> + * >> + * When exiting PSR, >> + * 1. Enable the mainlink >> + * 2. Send the PSR exit SDP >> + */ >> + if (enter) { >> + reinit_completion(&ctrl->psr_op_comp); >> + dp_catalog_ctrl_set_psr(ctrl->catalog, true); >> + >> + if (!wait_for_completion_timeout(&ctrl->psr_op_comp, >> + PSR_OPERATION_COMPLETION_TIMEOUT_JIFFIES)) { >> + DRM_ERROR("PSR_ENTRY timedout\n"); >> + dp_catalog_ctrl_set_psr(ctrl->catalog, false); >> + return; >> + } >> + >> + dp_catalog_ctrl_state_ctrl(ctrl->catalog, 0); >> + >> + dp_catalog_ctrl_psr_mainlink_enable(ctrl->catalog, false); >> + } else { >> + dp_catalog_ctrl_psr_mainlink_enable(ctrl->catalog, >> + true); >> + >> + dp_catalog_ctrl_set_psr(ctrl->catalog, false); > >My PSR knowledge is not very strong, but I do remember a recent commit >from Brian Norris fly by for the Analogix controller. See commit >c4c6ef229593 ("drm/bridge: analogix_dp: Make PSR-exit block less"). > >In that commit it sounds as if we need to wait for _something_ (resync I >guess?) here and not just return instantly. > In our case, the HW abstracts the necessary settings for regular psr exit. However, we discovered some corner cases related to display off/suspend while sink is in psr, I am incorporating a step to enable video and wait for video ready in v7. > >> @@ -388,6 +388,8 @@ static int dp_display_process_hpd_high(struct >> dp_display_private *dp) >> >> edid = dp->panel->edid; >> >> + dp->dp_display.psr_supported = !!dp->panel->psr_cap.version; >> + > >nit: remove the "!!". You're already storing this in a "bool" which will handle >this for you. > Made this change in v7. > >> +static const struct drm_bridge_funcs edp_bridge_ops = { >> + .atomic_enable = edp_bridge_atomic_enable, >> + .atomic_disable = edp_bridge_atomic_disable, >> + .atomic_post_disable = edp_bridge_atomic_post_disable, >> + .mode_set = dp_bridge_mode_set, >> + .mode_valid = dp_bridge_mode_valid, >> + .atomic_reset = drm_atomic_helper_bridge_reset, >> + .atomic_duplicate_state = >drm_atomic_helper_bridge_duplicate_state, >> + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, >> + .atomic_check = edp_bridge_atomic_check, }; > >nit: the location of your new functions is a little weird. You've got: > >1. DP functions >2. eDP functions >3. eDP structure >4. DP structure > >I'd expect: > >1. DP functions >2. DP structure >3. eDP functions >4. eDP structure > Changed the order in v7 >-Doug