RE: [PATCH v6 04/10] drm/msm/dp: Add basic PSR support for eDP
Hi Doug,
I incorporated the comments in v7.
>Hi,
>
>On Mon, Jul 11, 2022 at 5:57 AM Vinod Polimera
> 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(>psr_op_comp);
>> + dp_catalog_ctrl_set_psr(ctrl->catalog, true);
>> +
>> + if (!wait_for_completion_timeout(>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
Re: [PATCH v6 04/10] drm/msm/dp: Add basic PSR support for eDP
Hi,
On Mon, Jul 11, 2022 at 5:57 AM Vinod Polimera
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.
> @@ -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.
> @@ -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.
> +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(>psr_op_comp);
> + dp_catalog_ctrl_set_psr(ctrl->catalog, true);
> +
> + if (!wait_for_completion_timeout(>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.
> @@ -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.
> +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
-Doug
Re: [PATCH v6 04/10] drm/msm/dp: Add basic PSR support for eDP
On 11/07/2022 15:56, Vinod Polimera wrote:
Add support for basic panel self refresh (PSR) feature for eDP.
Add a new interface to set PSR state in the sink from DPU.
Program the eDP controller to issue PSR enter and exit SDP to
the sink.
Signed-off-by: Sankeerth Billakanti
Signed-off-by: Vinod Polimera
Reviewed-by: Dmitry Baryshkov
---
drivers/gpu/drm/msm/dp/dp_catalog.c | 81 ++
drivers/gpu/drm/msm/dp/dp_catalog.h | 4 ++
drivers/gpu/drm/msm/dp/dp_ctrl.c| 73 +++
drivers/gpu/drm/msm/dp/dp_ctrl.h| 3 +
drivers/gpu/drm/msm/dp/dp_display.c | 14
drivers/gpu/drm/msm/dp/dp_display.h | 2 +
drivers/gpu/drm/msm/dp/dp_drm.c | 135 +++-
drivers/gpu/drm/msm/dp/dp_link.c| 36 ++
drivers/gpu/drm/msm/dp/dp_panel.c | 22 ++
drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++
drivers/gpu/drm/msm/dp/dp_reg.h | 27
11 files changed, 402 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 7257515..b9021ed 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -47,6 +47,14 @@
#define DP_INTERRUPT_STATUS2_MASK \
(DP_INTERRUPT_STATUS2 << DP_INTERRUPT_STATUS_MASK_SHIFT)
+#define DP_INTERRUPT_STATUS4 \
+ (PSR_UPDATE_INT | PSR_CAPTURE_INT | PSR_EXIT_INT | \
+ PSR_UPDATE_ERROR_INT | PSR_WAKE_ERROR_INT)
+
+#define DP_INTERRUPT_MASK4 \
+ (PSR_UPDATE_MASK | PSR_CAPTURE_MASK | PSR_EXIT_MASK | \
+ PSR_UPDATE_ERROR_MASK | PSR_WAKE_ERROR_MASK)
+
struct dp_catalog_private {
struct device *dev;
struct drm_device *drm_dev;
@@ -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;
+
+ if (enable)
+ val |= DP_MAINLINK_CTRL_ENABLE;
+ else
+ val &= ~DP_MAINLINK_CTRL_ENABLE;
+
+ dp_write_link(catalog, REG_DP_MAINLINK_CTRL, val);
+}
+
void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog,
bool enable)
{
@@ -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);
+}
+
+void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog)
+{
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+ u32 config;
+
+ /* enable PSR1 function */
+ config = dp_read_link(catalog, REG_PSR_CONFIG);
+ config |= PSR1_SUPPORTED;
+ dp_write_link(catalog, REG_PSR_CONFIG, config);
+
+ dp_write_ahb(catalog, REG_DP_INTR_MASK4, DP_INTERRUPT_MASK4);
+ dp_catalog_enable_sdp(catalog);
+}
+
+void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter)
+{
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+ u32 cmd;
+
+ cmd = dp_read_link(catalog, REG_PSR_CMD);
+
+ cmd &= ~(PSR_ENTER | PSR_EXIT);
+
+ if (enter)
+ cmd |= PSR_ENTER;
+ else
+ cmd |= PSR_EXIT;
+
+ dp_catalog_enable_sdp(catalog);
+ dp_write_link(catalog, REG_PSR_CMD, cmd);
+}
+
u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
@@ -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)
+{
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+ u32 intr, intr_ack;
+
+ intr = dp_read_ahb(catalog, REG_DP_INTR_STATUS4);
+ intr_ack = (intr & DP_INTERRUPT_STATUS4)
+ << DP_INTERRUPT_STATUS_ACK_SHIFT;
+ dp_write_ahb(catalog, REG_DP_INTR_STATUS4, intr_ack);
+
+ return intr;
+}
+
int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
diff --git
