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
