[PATCH v6 04/10] drm/msm/dp: Add basic PSR support for eDP

[Sankeerth Billakanti]

Hi Doug,
I incorporated the comments in v7.

>Hi,
>
>On Mon, Jul 11, 2022 at 5:57 AM Vinod Polimera
><quic_vpolimer at quicinc.com> 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