*** BLURB HERE ***
Vinod Polimera (4):
drm/msm/dp: Add basic PSR support for eDP
drm/bridge: use atomic enable/disable for bridge callbacks
drm/msm/disp/dpu1: use atomic enable/disable callbacks for encoder
functions
drm/msm/disp/dpu1: add PSR support for eDP interface in dpu driver
drivers/gpu/drm/bridge/panel.c | 102 ++++++++++++++--
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 31 ++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 40 ++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +-
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 | 63 ++++++++++
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 | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 177 +++++++++++++++++++++++++++-
drivers/gpu/drm/msm/dp/dp_link.c | 22 ++++
drivers/gpu/drm/msm/dp/dp_panel.c | 21 ++++
drivers/gpu/drm/msm/dp/dp_panel.h | 6 +
drivers/gpu/drm/msm/dp/dp_reg.h | 19 +++
drivers/gpu/drm/msm/msm_drv.h | 6 +
16 files changed, 568 insertions(+), 24 deletions(-)
--
2.7.4
Use atomic enable/disable for bridge callbacks to access certain
states like self-refresh.
This change avoids panel prepare/unprepare based on self-refresh
state.
Signed-off-by: Sankeerth Billakanti <[email protected]>
Signed-off-by: Kalyan Thota <[email protected]>
Signed-off-by: Vinod Polimera <[email protected]>
Changes in V2:
- As per review suggestion by Dmitry.
---
drivers/gpu/drm/bridge/panel.c | 102 +++++++++++++++++++++++++++++++++++++----
1 file changed, 94 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index b32295a..5c7dc82 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -41,6 +41,40 @@ static int panel_bridge_connector_get_modes(struct drm_connector *connector)
return drm_panel_get_modes(panel_bridge->panel, connector);
}
+static struct drm_crtc *bridge_drm_get_old_connector_crtc(struct drm_encoder *encoder,
+ struct drm_atomic_state *state)
+{
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state;
+
+ connector = drm_atomic_get_old_connector_for_encoder(state, encoder);
+ if (!connector)
+ return NULL;
+
+ conn_state = drm_atomic_get_old_connector_state(state, connector);
+ if (!conn_state)
+ return NULL;
+
+ return conn_state->crtc;
+}
+
+static struct drm_crtc *bridge_drm_get_new_connector_crtc(struct drm_encoder *encoder,
+ struct drm_atomic_state *state)
+{
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state;
+
+ connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
+ if (!connector)
+ return NULL;
+
+ conn_state = drm_atomic_get_new_connector_state(state, connector);
+ if (!conn_state)
+ return NULL;
+
+ return conn_state->crtc;
+}
+
static const struct drm_connector_helper_funcs
panel_bridge_connector_helper_funcs = {
.get_modes = panel_bridge_connector_get_modes,
@@ -102,30 +136,82 @@ static void panel_bridge_detach(struct drm_bridge *bridge)
drm_connector_cleanup(connector);
}
-static void panel_bridge_pre_enable(struct drm_bridge *bridge)
+static void panel_bridge_pre_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+ struct drm_atomic_state *old_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state;
+
+ crtc = bridge_drm_get_new_connector_crtc(encoder, old_state);
+ if (!crtc)
+ return;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
+ if (old_crtc_state && old_crtc_state->self_refresh_active)
+ return;
drm_panel_prepare(panel_bridge->panel);
}
-static void panel_bridge_enable(struct drm_bridge *bridge)
+static void panel_bridge_enable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+ struct drm_atomic_state *old_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state;
+
+ crtc = bridge_drm_get_new_connector_crtc(encoder, old_state);
+ if (!crtc)
+ return;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
+ if (old_crtc_state && old_crtc_state->self_refresh_active)
+ return;
drm_panel_enable(panel_bridge->panel);
}
-static void panel_bridge_disable(struct drm_bridge *bridge)
+static void panel_bridge_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+ struct drm_atomic_state *old_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *new_crtc_state;
+
+ crtc = bridge_drm_get_old_connector_crtc(encoder, old_state);
+ if (!crtc)
+ return;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
+ if (new_crtc_state && new_crtc_state->self_refresh_active)
+ return;
drm_panel_disable(panel_bridge->panel);
}
-static void panel_bridge_post_disable(struct drm_bridge *bridge)
+static void panel_bridge_post_disable(struct drm_bridge *bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
+ struct drm_atomic_state *old_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *new_crtc_state;
+
+ crtc = bridge_drm_get_old_connector_crtc(encoder, old_state);
+ if (!crtc)
+ return;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
+ if (new_crtc_state && new_crtc_state->self_refresh_active)
+ return;
drm_panel_unprepare(panel_bridge->panel);
}
@@ -141,10 +227,10 @@ static int panel_bridge_get_modes(struct drm_bridge *bridge,
static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
.attach = panel_bridge_attach,
.detach = panel_bridge_detach,
- .pre_enable = panel_bridge_pre_enable,
- .enable = panel_bridge_enable,
- .disable = panel_bridge_disable,
- .post_disable = panel_bridge_post_disable,
+ .atomic_pre_enable = panel_bridge_pre_enable,
+ .atomic_enable = panel_bridge_enable,
+ .atomic_disable = panel_bridge_disable,
+ .atomic_post_disable = panel_bridge_post_disable,
.get_modes = panel_bridge_get_modes,
.atomic_reset = drm_atomic_helper_bridge_reset,
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
--
2.7.4
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 <[email protected]>
Changes in v2:
- Use dp bridge to set psr entry/exit instead of dpu_enocder
- Don't modify whitespaces
- set self refresh aware from atomic_check
- set self refresh aware only if psr is supported
- provide a stub for msm_dp_display_set_psr
---
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 | 63 +++++++++++++
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 | 1 +
drivers/gpu/drm/msm/dp/dp_drm.c | 177 ++++++++++++++++++++++++++++++++++--
drivers/gpu/drm/msm/dp/dp_link.c | 22 +++++
drivers/gpu/drm/msm/dp/dp_panel.c | 21 +++++
drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++
drivers/gpu/drm/msm/dp/dp_reg.h | 19 ++++
drivers/gpu/drm/msm/msm_drv.h | 6 ++
12 files changed, 411 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
index 8a6d3ea..3cd223d 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.c
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
@@ -45,6 +45,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 dp_io *io;
@@ -343,6 +351,20 @@ 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 mainlink_ctrl;
+ struct dp_catalog_private *catalog = container_of(dp_catalog,
+ struct dp_catalog_private, dp_catalog);
+
+ mainlink_ctrl = dp_read_link(catalog, REG_DP_MAINLINK_CTRL);
+ mainlink_ctrl &= ~DP_MAINLINK_CTRL_ENABLE;
+ mainlink_ctrl |= (enable & DP_MAINLINK_CTRL_ENABLE);
+
+ dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl);
+}
+
void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog,
bool enable)
{
@@ -581,6 +603,51 @@ 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, 0x1);
+ dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x0);
+}
+
+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 psr_config;
+
+ /* enable PSR1 function */
+ psr_config = dp_read_link(catalog, REG_PSR_CONFIG);
+ psr_config |= BIT(0);
+ dp_write_link(catalog, REG_PSR_CONFIG, psr_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 psr_cmd;
+
+ psr_cmd = dp_read_link(catalog, REG_PSR_CMD);
+
+ /*
+ * BIT(0) - send psr entry SDP
+ * BIT(1) - sned psr exit SDP
+ */
+ psr_cmd &= ~(BIT(0) | BIT(1));
+
+ if (enter)
+ psr_cmd |= BIT(0);
+ else
+ psr_cmd |= BIT(1);
+
+ dp_catalog_enable_sdp(catalog);
+ dp_write_link(catalog, REG_PSR_CMD, psr_cmd);
+}
+
u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
{
struct dp_catalog_private *catalog = container_of(dp_catalog,
@@ -608,6 +675,20 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
return isr;
}
+int dp_catalog_ctrl_get_psr_interrupt(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 a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
index 6965afa..9b1b199 100644
--- a/drivers/gpu/drm/msm/dp/dp_catalog.h
+++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
@@ -91,6 +91,7 @@ void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 state);
void dp_catalog_ctrl_config_ctrl(struct dp_catalog *dp_catalog, u32 config);
void dp_catalog_ctrl_lane_mapping(struct dp_catalog *dp_catalog);
void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog, bool enable);
+void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog *dp_catalog, bool enable);
void dp_catalog_ctrl_config_misc(struct dp_catalog *dp_catalog, u32 cc, u32 tb);
void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32 rate,
u32 stream_rate_khz, bool fixed_nvid);
@@ -101,12 +102,15 @@ void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool enable);
void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
u32 intr_mask, bool en);
void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog);
+void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog);
+void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter);
u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog);
u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog);
void dp_catalog_ctrl_phy_reset(struct dp_catalog *dp_catalog);
int dp_catalog_ctrl_update_vx_px(struct dp_catalog *dp_catalog, u8 v_level,
u8 p_level);
int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog);
+int dp_catalog_ctrl_get_psr_interrupt(struct dp_catalog *dp_catalog);
void dp_catalog_ctrl_update_transfer_unit(struct dp_catalog *dp_catalog,
u32 dp_tu, u32 valid_boundary,
u32 valid_boundary2);
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
index 88ca6c3..ba828ea 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
@@ -21,6 +21,7 @@
#define DP_KHZ_TO_HZ 1000
#define IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES (30 * HZ / 1000) /* 30 ms */
+#define PSR_OPERATION_COMPLETION_TIMEOUT_JIFFIES (300 * HZ / 1000) /* 300 ms */
#define WAIT_FOR_VIDEO_READY_TIMEOUT_JIFFIES (HZ / 2)
#define DP_CTRL_INTR_READY_FOR_VIDEO BIT(0)
@@ -78,6 +79,7 @@ struct dp_ctrl_private {
struct dp_catalog *catalog;
struct completion idle_comp;
+ struct completion psr_op_comp;
struct completion video_comp;
};
@@ -151,6 +153,9 @@ static void dp_ctrl_config_ctrl(struct dp_ctrl_private *ctrl)
config |= DP_CONFIGURATION_CTRL_STATIC_DYNAMIC_CN;
config |= DP_CONFIGURATION_CTRL_SYNC_ASYNC_CLK;
+ if (ctrl->panel->psr_cap.version)
+ config |= DP_CONFIGURATION_CTRL_SEND_VSC;
+
dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
}
@@ -1365,6 +1370,52 @@ static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl)
return ret;
}
+void dp_ctrl_config_psr(struct dp_ctrl *dp_ctrl)
+{
+ struct dp_ctrl_private *ctrl;
+ u8 psr_config;
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+ if (!ctrl->panel->psr_cap.version)
+ return;
+
+ dp_catalog_ctrl_config_psr(ctrl->catalog);
+
+ psr_config = DP_PSR_ENABLE;
+ drm_dp_dpcd_write(ctrl->aux, DP_PSR_EN_CFG, &psr_config, 1);
+}
+
+void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enter)
+{
+ struct dp_ctrl_private *ctrl;
+
+ ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+
+ if (!ctrl->panel->psr_cap.version)
+ return;
+
+ 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);
+ }
+}
+
int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
{
struct dp_ctrl_private *ctrl;
@@ -1964,6 +2015,17 @@ void dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
+ if (ctrl->panel->psr_cap.version) {
+ isr = dp_catalog_ctrl_get_psr_interrupt(ctrl->catalog);
+
+ if (isr == 0x1)
+ DRM_DEBUG_DP("PSR frame update done\n");
+ else if (isr == 0x10)
+ DRM_DEBUG_DP("PSR exit done\n");
+
+ complete(&ctrl->psr_op_comp);
+ }
+
isr = dp_catalog_ctrl_get_interrupt(ctrl->catalog);
if (isr & DP_CTRL_INTR_READY_FOR_VIDEO) {
@@ -2010,6 +2072,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
dev_err(dev, "failed to add DP OPP table\n");
init_completion(&ctrl->idle_comp);
+ init_completion(&ctrl->psr_op_comp);
init_completion(&ctrl->video_comp);
/* in parameters */
diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
index 2363a2d..f623035 100644
--- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
+++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
@@ -34,4 +34,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
struct dp_power *power, struct dp_catalog *catalog,
struct dp_parser *parser);
+void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enable);
+void dp_ctrl_config_psr(struct dp_ctrl *dp_ctrl);
+
#endif /* _DP_CTRL_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 5d314e6..c8d02fb 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -347,6 +347,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;
+
dp->audio_supported = drm_detect_monitor_audio(edid);
dp_panel_handle_sink_request(dp->panel);
@@ -871,6 +873,10 @@ static int dp_display_post_enable(struct msm_dp *dp_display)
/* signal the connect event late to synchronize video and display */
dp_display_handle_plugged_change(dp_display, true);
+
+ if (dp_display->psr_supported)
+ dp_ctrl_config_psr(dp->ctrl);
+
return 0;
}
@@ -1037,6 +1043,14 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
enable_irq(dp->irq);
}
+void msm_dp_display_set_psr(struct msm_dp *dp_display, bool enter)
+{
+ struct dp_display_private *dp;
+
+ dp = container_of(dp_display, struct dp_display_private, dp_display);
+ dp_ctrl_set_psr(dp->ctrl, enter);
+}
+
static int hpd_event_thread(void *data)
{
struct dp_display_private *dp_priv;
diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
index e3adcd5..6f512f3 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.h
+++ b/drivers/gpu/drm/msm/dp/dp_display.h
@@ -28,6 +28,7 @@ struct msm_dp {
u32 max_dp_lanes;
struct dp_audio *dp_audio;
+ bool psr_supported;
};
int dp_display_set_plugged_cb(struct msm_dp *dp_display,
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 2436329..d26ca6a 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -142,6 +142,35 @@ static enum drm_mode_status edp_connector_mode_valid(
return MODE_OK;
}
+
+
+static int edp_connector_atomic_check(struct drm_connector *connector,
+ struct drm_atomic_state *state)
+{
+ struct msm_dp *dp;
+ struct drm_connector_state *conn_state;
+ struct drm_crtc_state *crtc_state;
+
+ dp = to_dp_connector(connector)->dp_display;
+ conn_state = drm_atomic_get_new_connector_state(state, connector);
+ if (WARN_ON(!conn_state))
+ return -ENODEV;
+
+ conn_state->self_refresh_aware = true;
+
+ if (!conn_state->crtc)
+ return 0;
+
+ crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+ if (!crtc_state)
+ return 0;
+
+ if (crtc_state->self_refresh_active && !dp->psr_supported)
+ return -EINVAL;
+
+ return 0;
+}
+
static const struct drm_connector_funcs dp_connector_funcs = {
.detect = dp_connector_detect,
.fill_modes = drm_helper_probe_single_connector_modes,
@@ -151,6 +180,11 @@ static const struct drm_connector_funcs dp_connector_funcs = {
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
};
+static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
+ .get_modes = dp_connector_get_modes,
+ .mode_valid = dp_connector_mode_valid,
+};
+
static const struct drm_connector_funcs edp_connector_funcs = {
.fill_modes = drm_helper_probe_single_connector_modes,
.destroy = drm_connector_cleanup,
@@ -159,12 +193,8 @@ static const struct drm_connector_funcs edp_connector_funcs = {
.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
};
-static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
- .get_modes = dp_connector_get_modes,
- .mode_valid = dp_connector_mode_valid,
-};
-
static const struct drm_connector_helper_funcs edp_connector_helper_funcs = {
+ .atomic_check = edp_connector_atomic_check,
.get_modes = edp_connector_get_modes,
.mode_valid = edp_connector_mode_valid,
};
@@ -258,6 +288,130 @@ static void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
msm_dp_display_disable(dp_display, drm_bridge->encoder);
}
+static struct drm_crtc *dp_bridge_get_old_connector_crtc(struct msm_dp *dp,
+ struct drm_atomic_state *state)
+{
+ struct drm_encoder *encoder = dp->encoder;
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state;
+
+ connector = drm_atomic_get_old_connector_for_encoder(state, encoder);
+ if (!connector)
+ return NULL;
+
+ conn_state = drm_atomic_get_old_connector_state(state, connector);
+ if (!conn_state)
+ return NULL;
+
+ return conn_state->crtc;
+}
+
+static struct drm_crtc *dp_bridge_get_new_connector_crtc(struct msm_dp *dp,
+ struct drm_atomic_state *state)
+{
+ struct drm_encoder *encoder = dp->encoder;
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state;
+
+ connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
+ if (!connector)
+ return NULL;
+
+ conn_state = drm_atomic_get_new_connector_state(state, connector);
+ if (!conn_state)
+ return NULL;
+
+ return conn_state->crtc;
+}
+
+static void edp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state)
+{
+ struct drm_atomic_state *old_state = old_bridge_state->base.state;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state;
+ struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
+ struct msm_dp *dp_display = dp_bridge->dp_display;
+
+ crtc = dp_bridge_get_new_connector_crtc(dp_display, old_state);
+ if (!crtc)
+ return;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
+
+ /* Exit from self refresh mode */
+ if (old_crtc_state && old_crtc_state->self_refresh_active) {
+ msm_dp_display_set_psr(dp_display, false);
+ return;
+ }
+
+ msm_dp_display_enable(dp_display, drm_bridge->encoder);
+}
+
+static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state)
+{
+ struct drm_atomic_state *old_state = old_bridge_state->base.state;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state = NULL;
+ struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
+ struct msm_dp *dp_display = dp_bridge->dp_display;
+
+ crtc = dp_bridge_get_old_connector_crtc(dp_display, old_state);
+ if (!crtc)
+ goto out;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
+ if (!new_crtc_state)
+ goto out;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
+ if (!old_crtc_state)
+ goto out;
+
+ /*
+ * Set self refresh mode if current crtc state is active.
+ * If old crtc state is active, exit psr before disabling
+ * the controller. Observed sink stuck in self refresh
+ * if psr exit is skipped when screen off occurs with
+ * sink in psr mode.
+ */
+ if (new_crtc_state->self_refresh_active) {
+ msm_dp_display_set_psr(dp_display, true);
+ return;
+ } else if (old_crtc_state->self_refresh_active) {
+ msm_dp_display_set_psr(dp_display, false);
+ return;
+ }
+
+out:
+ msm_dp_display_pre_disable(dp_display, drm_bridge->encoder);
+}
+
+static void edp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state)
+{
+ struct drm_atomic_state *old_state = old_bridge_state->base.state;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *new_crtc_state = NULL;
+ struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
+ struct msm_dp *dp_display = dp_bridge->dp_display;
+
+ crtc = dp_bridge_get_old_connector_crtc(dp_display, old_state);
+ if (!crtc)
+ return;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
+ if (!new_crtc_state)
+ return;
+
+ /* Self refresh mode is set in dp_bridge_disable. Skip disable */
+ if (new_crtc_state->self_refresh_active)
+ return;
+
+ msm_dp_display_disable(dp_display, drm_bridge->encoder);
+}
+
static const struct drm_bridge_funcs dp_bridge_ops = {
.enable = dp_bridge_enable,
.disable = dp_bridge_disable,
@@ -265,6 +419,16 @@ static const struct drm_bridge_funcs dp_bridge_ops = {
.mode_set = dp_bridge_mode_set,
};
+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,
+ .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,
+};
+
struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
struct drm_encoder *encoder)
{
@@ -279,7 +443,8 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi
dp_bridge->dp_display = dp_display;
bridge = &dp_bridge->bridge;
- bridge->funcs = &dp_bridge_ops;
+ bridge->funcs = (dp_display->connector_type == DRM_MODE_CONNECTOR_eDP) ?
+ &edp_bridge_ops : &dp_bridge_ops;
bridge->encoder = encoder;
rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
index d4d31e5..5503c29 100644
--- a/drivers/gpu/drm/msm/dp/dp_link.c
+++ b/drivers/gpu/drm/msm/dp/dp_link.c
@@ -924,6 +924,26 @@ static int dp_link_process_phy_test_pattern_request(
return 0;
}
+static int dp_link_psr_status(struct dp_link_private *link)
+{
+ u8 status[2];
+
+ drm_dp_dpcd_read(link->aux, DP_PSR_ERROR_STATUS, status, 2);
+
+ if (status[0] & DP_PSR_LINK_CRC_ERROR)
+ DRM_ERROR("PSR LINK CRC ERROR\n");
+ else if (status[0] & DP_PSR_RFB_STORAGE_ERROR)
+ DRM_ERROR("PSR RFB STORAGE ERROR\n");
+ else if (status[0] & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
+ DRM_ERROR("PSR VSC SDP UNCORRECTABLE ERROR\n");
+ else if (status[1] & DP_PSR_CAPS_CHANGE)
+ DRM_INFO("PSR Capability Change\n");
+ else
+ return 0;
+
+ return 1;
+}
+
static u8 get_link_status(const u8 link_status[DP_LINK_STATUS_SIZE], int r)
{
return link_status[r - DP_LANE0_1_STATUS];
@@ -1042,6 +1062,8 @@ int dp_link_process_request(struct dp_link *dp_link)
dp_link->sink_request |= DP_TEST_LINK_TRAINING;
} else if (!dp_link_process_phy_test_pattern_request(link)) {
dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
+ } else if (dp_link_psr_status(link)) {
+ DRM_INFO("PSR IRQ_HPD received\n");
} else {
ret = dp_link_process_link_status_update(link);
if (!ret) {
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
index 71db10c..e128d73 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.c
+++ b/drivers/gpu/drm/msm/dp/dp_panel.c
@@ -19,6 +19,26 @@ struct dp_panel_private {
bool aux_cfg_update_done;
};
+static void dp_panel_read_psr_cap(struct dp_panel_private *panel)
+{
+ ssize_t rlen;
+ struct dp_panel *dp_panel;
+
+ dp_panel = &panel->dp_panel;
+
+ /* edp sink */
+ if (dp_panel->dpcd[DP_EDP_CONFIGURATION_CAP]) {
+ rlen = drm_dp_dpcd_read(panel->aux, DP_PSR_SUPPORT,
+ &dp_panel->psr_cap, 2);
+ if (rlen == 2) {
+ DRM_DEBUG_DP("psr version: 0x%x, psr_cap: 0x%x\n",
+ dp_panel->psr_cap.version,
+ dp_panel->psr_cap.capabilities);
+ } else
+ DRM_ERROR("failed to read psr info, rlen=%zd\n", rlen);
+ }
+}
+
static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
{
int rc = 0;
@@ -104,6 +124,7 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
}
}
+ dp_panel_read_psr_cap(panel);
end:
return rc;
}
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 9023e5b..631657a 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -34,6 +34,11 @@ struct dp_panel_in {
struct dp_catalog *catalog;
};
+struct dp_panel_psr {
+ u8 version;
+ u8 capabilities;
+};
+
struct dp_panel {
/* dpcd raw data */
u8 dpcd[DP_RECEIVER_CAP_SIZE + 1];
@@ -46,6 +51,7 @@ struct dp_panel {
struct edid *edid;
struct drm_connector *connector;
struct dp_display_mode dp_mode;
+ struct dp_panel_psr psr_cap;
bool video_test;
u32 vic;
diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h b/drivers/gpu/drm/msm/dp/dp_reg.h
index 2686028..7a0b052 100644
--- a/drivers/gpu/drm/msm/dp/dp_reg.h
+++ b/drivers/gpu/drm/msm/dp/dp_reg.h
@@ -22,6 +22,20 @@
#define REG_DP_INTR_STATUS2 (0x00000024)
#define REG_DP_INTR_STATUS3 (0x00000028)
+#define REG_DP_INTR_STATUS4 (0x0000002C)
+#define PSR_UPDATE_INT (0x00000001)
+#define PSR_CAPTURE_INT (0x00000004)
+#define PSR_EXIT_INT (0x00000010)
+#define PSR_UPDATE_ERROR_INT (0x00000040)
+#define PSR_WAKE_ERROR_INT (0x00000100)
+
+#define REG_DP_INTR_MASK4 (0x00000030)
+#define PSR_UPDATE_MASK (0x00000001)
+#define PSR_CAPTURE_MASK (0x00000002)
+#define PSR_EXIT_MASK (0x00000004)
+#define PSR_UPDATE_ERROR_MASK (0x00000008)
+#define PSR_WAKE_ERROR_MASK (0x00000010)
+
#define REG_DP_DP_HPD_CTRL (0x00000000)
#define DP_DP_HPD_CTRL_HPD_EN (0x00000001)
@@ -164,6 +178,9 @@
#define MMSS_DP_AUDIO_TIMING_RBR_48 (0x00000094)
#define MMSS_DP_AUDIO_TIMING_HBR_48 (0x00000098)
+#define REG_PSR_CONFIG (0x00000100)
+#define REG_PSR_CMD (0x00000110)
+
#define MMSS_DP_PSR_CRC_RG (0x00000154)
#define MMSS_DP_PSR_CRC_B (0x00000158)
@@ -184,6 +201,8 @@
#define MMSS_DP_AUDIO_STREAM_0 (0x00000240)
#define MMSS_DP_AUDIO_STREAM_1 (0x00000244)
+#define MMSS_DP_SDP_CFG3 (0x0000024c)
+
#define MMSS_DP_EXTENSION_0 (0x00000250)
#define MMSS_DP_EXTENSION_1 (0x00000254)
#define MMSS_DP_EXTENSION_2 (0x00000258)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index ae52412..254fd07 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -400,6 +400,8 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_displa
void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
+void msm_dp_display_set_psr(struct msm_dp *dp, bool enter);
+
#else
static inline int __init msm_dp_register(void)
{
@@ -449,6 +451,10 @@ static inline void msm_dp_debugfs_init(struct msm_dp *dp_display,
{
}
+static inline void msm_dp_display_set_psr(struct msm_dp *dp, bool enter)
+{
+}
+
#endif
void __init msm_mdp_register(void);
--
2.7.4
On Mon, 21 Feb 2022 at 17:52, Vinod Polimera <[email protected]> wrote:
>
> Use atomic enable/disable for bridge callbacks to access certain
> states like self-refresh.
>
> This change avoids panel prepare/unprepare based on self-refresh
> state.
Please split this into two patches:
- change to atomic_* callbacks
- introduction of PSR support.
>
> Signed-off-by: Sankeerth Billakanti <[email protected]>
> Signed-off-by: Kalyan Thota <[email protected]>
> Signed-off-by: Vinod Polimera <[email protected]>
>
> Changes in V2:
> - As per review suggestion by Dmitry.
> ---
> drivers/gpu/drm/bridge/panel.c | 102 +++++++++++++++++++++++++++++++++++++----
> 1 file changed, 94 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index b32295a..5c7dc82 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -41,6 +41,40 @@ static int panel_bridge_connector_get_modes(struct drm_connector *connector)
> return drm_panel_get_modes(panel_bridge->panel, connector);
> }
>
> +static struct drm_crtc *bridge_drm_get_old_connector_crtc(struct drm_encoder *encoder,
> + struct drm_atomic_state *state)
> +{
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> +
> + connector = drm_atomic_get_old_connector_for_encoder(state, encoder);
> + if (!connector)
> + return NULL;
> +
> + conn_state = drm_atomic_get_old_connector_state(state, connector);
> + if (!conn_state)
> + return NULL;
> +
> + return conn_state->crtc;
> +}
> +
> +static struct drm_crtc *bridge_drm_get_new_connector_crtc(struct drm_encoder *encoder,
> + struct drm_atomic_state *state)
> +{
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> +
> + connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
> + if (!connector)
> + return NULL;
> +
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
> + if (!conn_state)
> + return NULL;
> +
> + return conn_state->crtc;
> +}
> +
> static const struct drm_connector_helper_funcs
> panel_bridge_connector_helper_funcs = {
> .get_modes = panel_bridge_connector_get_modes,
> @@ -102,30 +136,82 @@ static void panel_bridge_detach(struct drm_bridge *bridge)
> drm_connector_cleanup(connector);
> }
>
> -static void panel_bridge_pre_enable(struct drm_bridge *bridge)
> +static void panel_bridge_pre_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
> + struct drm_encoder *encoder = bridge->encoder;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *old_crtc_state;
> +
> + crtc = bridge_drm_get_new_connector_crtc(encoder, old_state);
> + if (!crtc)
> + return;
> +
> + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
> + if (old_crtc_state && old_crtc_state->self_refresh_active)
> + return;
>
> drm_panel_prepare(panel_bridge->panel);
> }
>
> -static void panel_bridge_enable(struct drm_bridge *bridge)
> +static void panel_bridge_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
> + struct drm_encoder *encoder = bridge->encoder;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *old_crtc_state;
> +
> + crtc = bridge_drm_get_new_connector_crtc(encoder, old_state);
> + if (!crtc)
> + return;
> +
> + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
> + if (old_crtc_state && old_crtc_state->self_refresh_active)
> + return;
>
> drm_panel_enable(panel_bridge->panel);
> }
>
> -static void panel_bridge_disable(struct drm_bridge *bridge)
> +static void panel_bridge_disable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
> + struct drm_encoder *encoder = bridge->encoder;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *new_crtc_state;
> +
> + crtc = bridge_drm_get_old_connector_crtc(encoder, old_state);
> + if (!crtc)
> + return;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
> + if (new_crtc_state && new_crtc_state->self_refresh_active)
> + return;
>
> drm_panel_disable(panel_bridge->panel);
> }
>
> -static void panel_bridge_post_disable(struct drm_bridge *bridge)
> +static void panel_bridge_post_disable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
> + struct drm_encoder *encoder = bridge->encoder;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *new_crtc_state;
> +
> + crtc = bridge_drm_get_old_connector_crtc(encoder, old_state);
> + if (!crtc)
> + return;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
> + if (new_crtc_state && new_crtc_state->self_refresh_active)
> + return;
>
> drm_panel_unprepare(panel_bridge->panel);
> }
> @@ -141,10 +227,10 @@ static int panel_bridge_get_modes(struct drm_bridge *bridge,
> static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
> .attach = panel_bridge_attach,
> .detach = panel_bridge_detach,
> - .pre_enable = panel_bridge_pre_enable,
> - .enable = panel_bridge_enable,
> - .disable = panel_bridge_disable,
> - .post_disable = panel_bridge_post_disable,
> + .atomic_pre_enable = panel_bridge_pre_enable,
> + .atomic_enable = panel_bridge_enable,
> + .atomic_disable = panel_bridge_disable,
> + .atomic_post_disable = panel_bridge_post_disable,
Please change callback names to reflect that we are using atomic_ interfaces.
> .get_modes = panel_bridge_get_modes,
> .atomic_reset = drm_atomic_helper_bridge_reset,
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> --
> 2.7.4
>
--
With best wishes
Dmitry
On Mon, 21 Feb 2022 at 17:52, Vinod Polimera <[email protected]> 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 <[email protected]>
>
> Changes in v2:
> - Use dp bridge to set psr entry/exit instead of dpu_enocder
> - Don't modify whitespaces
> - set self refresh aware from atomic_check
> - set self refresh aware only if psr is supported
> - provide a stub for msm_dp_display_set_psr
> ---
> 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 | 63 +++++++++++++
> 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 | 1 +
> drivers/gpu/drm/msm/dp/dp_drm.c | 177 ++++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/msm/dp/dp_link.c | 22 +++++
> drivers/gpu/drm/msm/dp/dp_panel.c | 21 +++++
> drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++
> drivers/gpu/drm/msm/dp/dp_reg.h | 19 ++++
> drivers/gpu/drm/msm/msm_drv.h | 6 ++
> 12 files changed, 411 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 8a6d3ea..3cd223d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -45,6 +45,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 dp_io *io;
> @@ -343,6 +351,20 @@ 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 mainlink_ctrl;
> + struct dp_catalog_private *catalog = container_of(dp_catalog,
> + struct dp_catalog_private, dp_catalog);
> +
> + mainlink_ctrl = dp_read_link(catalog, REG_DP_MAINLINK_CTRL);
> + mainlink_ctrl &= ~DP_MAINLINK_CTRL_ENABLE;
> + mainlink_ctrl |= (enable & DP_MAINLINK_CTRL_ENABLE);
> +
> + dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl);
> +}
> +
> void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog,
> bool enable)
> {
> @@ -581,6 +603,51 @@ 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, 0x1);
> + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x0);
> +}
> +
> +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 psr_config;
> +
> + /* enable PSR1 function */
> + psr_config = dp_read_link(catalog, REG_PSR_CONFIG);
> + psr_config |= BIT(0);
> + dp_write_link(catalog, REG_PSR_CONFIG, psr_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 psr_cmd;
> +
> + psr_cmd = dp_read_link(catalog, REG_PSR_CMD);
> +
> + /*
> + * BIT(0) - send psr entry SDP
> + * BIT(1) - sned psr exit SDP
> + */
> + psr_cmd &= ~(BIT(0) | BIT(1));
> +
> + if (enter)
> + psr_cmd |= BIT(0);
> + else
> + psr_cmd |= BIT(1);
> +
> + dp_catalog_enable_sdp(catalog);
> + dp_write_link(catalog, REG_PSR_CMD, psr_cmd);
> +}
> +
> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
> {
> struct dp_catalog_private *catalog = container_of(dp_catalog,
> @@ -608,6 +675,20 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
> return isr;
> }
>
> +int dp_catalog_ctrl_get_psr_interrupt(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 a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 6965afa..9b1b199 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -91,6 +91,7 @@ void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 state);
> void dp_catalog_ctrl_config_ctrl(struct dp_catalog *dp_catalog, u32 config);
> void dp_catalog_ctrl_lane_mapping(struct dp_catalog *dp_catalog);
> void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog, bool enable);
> +void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog *dp_catalog, bool enable);
> void dp_catalog_ctrl_config_misc(struct dp_catalog *dp_catalog, u32 cc, u32 tb);
> void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32 rate,
> u32 stream_rate_khz, bool fixed_nvid);
> @@ -101,12 +102,15 @@ void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool enable);
> void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
> u32 intr_mask, bool en);
> void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog);
> +void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog);
> +void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter);
> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog);
> u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog);
> void dp_catalog_ctrl_phy_reset(struct dp_catalog *dp_catalog);
> int dp_catalog_ctrl_update_vx_px(struct dp_catalog *dp_catalog, u8 v_level,
> u8 p_level);
> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog);
> +int dp_catalog_ctrl_get_psr_interrupt(struct dp_catalog *dp_catalog);
> void dp_catalog_ctrl_update_transfer_unit(struct dp_catalog *dp_catalog,
> u32 dp_tu, u32 valid_boundary,
> u32 valid_boundary2);
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 88ca6c3..ba828ea 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -21,6 +21,7 @@
>
> #define DP_KHZ_TO_HZ 1000
> #define IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES (30 * HZ / 1000) /* 30 ms */
> +#define PSR_OPERATION_COMPLETION_TIMEOUT_JIFFIES (300 * HZ / 1000) /* 300 ms */
> #define WAIT_FOR_VIDEO_READY_TIMEOUT_JIFFIES (HZ / 2)
>
> #define DP_CTRL_INTR_READY_FOR_VIDEO BIT(0)
> @@ -78,6 +79,7 @@ struct dp_ctrl_private {
> struct dp_catalog *catalog;
>
> struct completion idle_comp;
> + struct completion psr_op_comp;
> struct completion video_comp;
> };
>
> @@ -151,6 +153,9 @@ static void dp_ctrl_config_ctrl(struct dp_ctrl_private *ctrl)
> config |= DP_CONFIGURATION_CTRL_STATIC_DYNAMIC_CN;
> config |= DP_CONFIGURATION_CTRL_SYNC_ASYNC_CLK;
>
> + if (ctrl->panel->psr_cap.version)
> + config |= DP_CONFIGURATION_CTRL_SEND_VSC;
> +
> dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
> }
>
> @@ -1365,6 +1370,52 @@ static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl)
> return ret;
> }
>
> +void dp_ctrl_config_psr(struct dp_ctrl *dp_ctrl)
> +{
> + struct dp_ctrl_private *ctrl;
> + u8 psr_config;
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> + if (!ctrl->panel->psr_cap.version)
> + return;
> +
> + dp_catalog_ctrl_config_psr(ctrl->catalog);
> +
> + psr_config = DP_PSR_ENABLE;
> + drm_dp_dpcd_write(ctrl->aux, DP_PSR_EN_CFG, &psr_config, 1);
> +}
> +
> +void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enter)
> +{
> + struct dp_ctrl_private *ctrl;
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> +
> + if (!ctrl->panel->psr_cap.version)
> + return;
> +
> + 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);
> + }
> +}
> +
> int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
> {
> struct dp_ctrl_private *ctrl;
> @@ -1964,6 +2015,17 @@ void dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
>
> ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>
> + if (ctrl->panel->psr_cap.version) {
> + isr = dp_catalog_ctrl_get_psr_interrupt(ctrl->catalog);
> +
> + if (isr == 0x1)
> + DRM_DEBUG_DP("PSR frame update done\n");
> + else if (isr == 0x10)
> + DRM_DEBUG_DP("PSR exit done\n");
> +
> + complete(&ctrl->psr_op_comp);
> + }
> +
> isr = dp_catalog_ctrl_get_interrupt(ctrl->catalog);
>
> if (isr & DP_CTRL_INTR_READY_FOR_VIDEO) {
> @@ -2010,6 +2072,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
> dev_err(dev, "failed to add DP OPP table\n");
>
> init_completion(&ctrl->idle_comp);
> + init_completion(&ctrl->psr_op_comp);
> init_completion(&ctrl->video_comp);
>
> /* in parameters */
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index 2363a2d..f623035 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -34,4 +34,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
> struct dp_power *power, struct dp_catalog *catalog,
> struct dp_parser *parser);
>
> +void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enable);
> +void dp_ctrl_config_psr(struct dp_ctrl *dp_ctrl);
> +
> #endif /* _DP_CTRL_H_ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5d314e6..c8d02fb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -347,6 +347,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;
> +
> dp->audio_supported = drm_detect_monitor_audio(edid);
> dp_panel_handle_sink_request(dp->panel);
>
> @@ -871,6 +873,10 @@ static int dp_display_post_enable(struct msm_dp *dp_display)
>
> /* signal the connect event late to synchronize video and display */
> dp_display_handle_plugged_change(dp_display, true);
> +
> + if (dp_display->psr_supported)
> + dp_ctrl_config_psr(dp->ctrl);
> +
> return 0;
> }
>
> @@ -1037,6 +1043,14 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
> enable_irq(dp->irq);
> }
>
> +void msm_dp_display_set_psr(struct msm_dp *dp_display, bool enter)
If this function is used only by the dp code, there is no need to
pollute the msm_dp_* namespace.
Please rename to dp_display_set_psr() and remove declaration from msm_drv.h.
> +{
> + struct dp_display_private *dp;
> +
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> + dp_ctrl_set_psr(dp->ctrl, enter);
> +}
> +
> static int hpd_event_thread(void *data)
> {
> struct dp_display_private *dp_priv;
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index e3adcd5..6f512f3 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -28,6 +28,7 @@ struct msm_dp {
>
> u32 max_dp_lanes;
> struct dp_audio *dp_audio;
> + bool psr_supported;
> };
>
> int dp_display_set_plugged_cb(struct msm_dp *dp_display,
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> index 2436329..d26ca6a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -142,6 +142,35 @@ static enum drm_mode_status edp_connector_mode_valid(
> return MODE_OK;
> }
>
> +
> +
Extra empty lines.
> +static int edp_connector_atomic_check(struct drm_connector *connector,
> + struct drm_atomic_state *state)
> +{
> + struct msm_dp *dp;
> + struct drm_connector_state *conn_state;
> + struct drm_crtc_state *crtc_state;
> +
> + dp = to_dp_connector(connector)->dp_display;
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
> + if (WARN_ON(!conn_state))
> + return -ENODEV;
> +
> + conn_state->self_refresh_aware = true;
I see that analogix bridge does the same. However I think it would be
better to set self_refresh_aware to true only if the actual attached
panel supports PSR.
> +
> + if (!conn_state->crtc)
> + return 0;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> + if (!crtc_state)
> + return 0;
> +
> + if (crtc_state->self_refresh_active && !dp->psr_supported)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static const struct drm_connector_funcs dp_connector_funcs = {
> .detect = dp_connector_detect,
> .fill_modes = drm_helper_probe_single_connector_modes,
> @@ -151,6 +180,11 @@ static const struct drm_connector_funcs dp_connector_funcs = {
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> };
>
> +static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
> + .get_modes = dp_connector_get_modes,
> + .mode_valid = dp_connector_mode_valid,
> +};
> +
Unnecessary movement.
> static const struct drm_connector_funcs edp_connector_funcs = {
> .fill_modes = drm_helper_probe_single_connector_modes,
> .destroy = drm_connector_cleanup,
> @@ -159,12 +193,8 @@ static const struct drm_connector_funcs edp_connector_funcs = {
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> };
>
> -static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
> - .get_modes = dp_connector_get_modes,
> - .mode_valid = dp_connector_mode_valid,
> -};
> -
> static const struct drm_connector_helper_funcs edp_connector_helper_funcs = {
> + .atomic_check = edp_connector_atomic_check,
> .get_modes = edp_connector_get_modes,
> .mode_valid = edp_connector_mode_valid,
> };
> @@ -258,6 +288,130 @@ static void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
> msm_dp_display_disable(dp_display, drm_bridge->encoder);
> }
>
> +static struct drm_crtc *dp_bridge_get_old_connector_crtc(struct msm_dp *dp,
> + struct drm_atomic_state *state)
> +{
> + struct drm_encoder *encoder = dp->encoder;
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> +
> + connector = drm_atomic_get_old_connector_for_encoder(state, encoder);
> + if (!connector)
> + return NULL;
> +
> + conn_state = drm_atomic_get_old_connector_state(state, connector);
> + if (!conn_state)
> + return NULL;
> +
> + return conn_state->crtc;
> +}
> +
> +static struct drm_crtc *dp_bridge_get_new_connector_crtc(struct msm_dp *dp,
> + struct drm_atomic_state *state)
> +{
> + struct drm_encoder *encoder = dp->encoder;
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> +
> + connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
> + if (!connector)
> + return NULL;
> +
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
> + if (!conn_state)
> + return NULL;
> +
> + return conn_state->crtc;
> +}
This code being repeated over and over again deserves to become a new
atomic helper.
Please move it to drm_atomic.c or .h
> +
> +static void edp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
> + struct drm_bridge_state *old_bridge_state)
> +{
> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *old_crtc_state;
> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
> + struct msm_dp *dp_display = dp_bridge->dp_display;
> +
> + crtc = dp_bridge_get_new_connector_crtc(dp_display, old_state);
> + if (!crtc)
> + return;
> +
> + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
Generic remark to this and the following functions. I don't quite like
the idea of the bridge drivers poking into the crtc state. This looks
like a breach of layering.
The code looks similar to analogix'es one. Maybe we should consider
moving it into common helpers? This would require adding
enable/disable_srr as drm_bridge ops.
Or to introduce drm_bridge_helper_funcs.
> +
> + /* Exit from self refresh mode */
> + if (old_crtc_state && old_crtc_state->self_refresh_active) {
> + msm_dp_display_set_psr(dp_display, false);
> + return;
> + }
> +
> + msm_dp_display_enable(dp_display, drm_bridge->encoder);
> +}
> +
> +static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
> + struct drm_bridge_state *old_bridge_state)
> +{
> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state = NULL;
> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
> + struct msm_dp *dp_display = dp_bridge->dp_display;
> +
> + crtc = dp_bridge_get_old_connector_crtc(dp_display, old_state);
> + if (!crtc)
> + goto out;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
> + if (!new_crtc_state)
> + goto out;
> +
> + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
> + if (!old_crtc_state)
> + goto out;
> +
> + /*
> + * Set self refresh mode if current crtc state is active.
> + * If old crtc state is active, exit psr before disabling
> + * the controller. Observed sink stuck in self refresh
> + * if psr exit is skipped when screen off occurs with
> + * sink in psr mode.
> + */
> + if (new_crtc_state->self_refresh_active) {
> + msm_dp_display_set_psr(dp_display, true);
> + return;
> + } else if (old_crtc_state->self_refresh_active) {
> + msm_dp_display_set_psr(dp_display, false);
> + return;
I think the return here is wrong. Where do we support the case when
the old state was the PSR, but the new states tells us to disable the
crtc? If I understand the drm code correctly, it's a legit case.
> + }
> +
> +out:
> + msm_dp_display_pre_disable(dp_display, drm_bridge->encoder);
> +}
> +
> +static void edp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
> + struct drm_bridge_state *old_bridge_state)
> +{
> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *new_crtc_state = NULL;
> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
> + struct msm_dp *dp_display = dp_bridge->dp_display;
> +
> + crtc = dp_bridge_get_old_connector_crtc(dp_display, old_state);
> + if (!crtc)
> + return;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
> + if (!new_crtc_state)
> + return;
> +
> + /* Self refresh mode is set in dp_bridge_disable. Skip disable */
edp_bridge_atomic_disable?
> + if (new_crtc_state->self_refresh_active)
> + return;
> +
> + msm_dp_display_disable(dp_display, drm_bridge->encoder);
> +}
> +
> static const struct drm_bridge_funcs dp_bridge_ops = {
> .enable = dp_bridge_enable,
> .disable = dp_bridge_disable,
> @@ -265,6 +419,16 @@ static const struct drm_bridge_funcs dp_bridge_ops = {
> .mode_set = dp_bridge_mode_set,
> };
>
> +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,
> + .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,
> +};
> +
> struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
> struct drm_encoder *encoder)
> {
> @@ -279,7 +443,8 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi
> dp_bridge->dp_display = dp_display;
>
> bridge = &dp_bridge->bridge;
> - bridge->funcs = &dp_bridge_ops;
> + bridge->funcs = (dp_display->connector_type == DRM_MODE_CONNECTOR_eDP) ?
> + &edp_bridge_ops : &dp_bridge_ops;
> bridge->encoder = encoder;
>
> rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
> index d4d31e5..5503c29 100644
> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> @@ -924,6 +924,26 @@ static int dp_link_process_phy_test_pattern_request(
> return 0;
> }
>
> +static int dp_link_psr_status(struct dp_link_private *link)
> +{
> + u8 status[2];
> +
> + drm_dp_dpcd_read(link->aux, DP_PSR_ERROR_STATUS, status, 2);
> +
> + if (status[0] & DP_PSR_LINK_CRC_ERROR)
> + DRM_ERROR("PSR LINK CRC ERROR\n");
> + else if (status[0] & DP_PSR_RFB_STORAGE_ERROR)
> + DRM_ERROR("PSR RFB STORAGE ERROR\n");
> + else if (status[0] & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
> + DRM_ERROR("PSR VSC SDP UNCORRECTABLE ERROR\n");
> + else if (status[1] & DP_PSR_CAPS_CHANGE)
> + DRM_INFO("PSR Capability Change\n");
DRM_DEBUG_DP
> + else
> + return 0;
> +
> + return 1;
> +}
> +
> static u8 get_link_status(const u8 link_status[DP_LINK_STATUS_SIZE], int r)
> {
> return link_status[r - DP_LANE0_1_STATUS];
> @@ -1042,6 +1062,8 @@ int dp_link_process_request(struct dp_link *dp_link)
> dp_link->sink_request |= DP_TEST_LINK_TRAINING;
> } else if (!dp_link_process_phy_test_pattern_request(link)) {
> dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
> + } else if (dp_link_psr_status(link)) {
> + DRM_INFO("PSR IRQ_HPD received\n");
DRM_DEBUG_DP
> } else {
> ret = dp_link_process_link_status_update(link);
> if (!ret) {
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 71db10c..e128d73 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -19,6 +19,26 @@ struct dp_panel_private {
> bool aux_cfg_update_done;
> };
>
> +static void dp_panel_read_psr_cap(struct dp_panel_private *panel)
> +{
> + ssize_t rlen;
> + struct dp_panel *dp_panel;
> +
> + dp_panel = &panel->dp_panel;
> +
> + /* edp sink */
> + if (dp_panel->dpcd[DP_EDP_CONFIGURATION_CAP]) {
> + rlen = drm_dp_dpcd_read(panel->aux, DP_PSR_SUPPORT,
> + &dp_panel->psr_cap, 2);
> + if (rlen == 2) {
> + DRM_DEBUG_DP("psr version: 0x%x, psr_cap: 0x%x\n",
> + dp_panel->psr_cap.version,
> + dp_panel->psr_cap.capabilities);
> + } else
> + DRM_ERROR("failed to read psr info, rlen=%zd\n", rlen);
> + }
> +}
> +
> static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
> {
> int rc = 0;
> @@ -104,6 +124,7 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
> }
> }
>
> + dp_panel_read_psr_cap(panel);
> end:
> return rc;
> }
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
> index 9023e5b..631657a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> @@ -34,6 +34,11 @@ struct dp_panel_in {
> struct dp_catalog *catalog;
> };
>
> +struct dp_panel_psr {
> + u8 version;
> + u8 capabilities;
> +};
> +
> struct dp_panel {
> /* dpcd raw data */
> u8 dpcd[DP_RECEIVER_CAP_SIZE + 1];
> @@ -46,6 +51,7 @@ struct dp_panel {
> struct edid *edid;
> struct drm_connector *connector;
> struct dp_display_mode dp_mode;
> + struct dp_panel_psr psr_cap;
> bool video_test;
>
> u32 vic;
> diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h b/drivers/gpu/drm/msm/dp/dp_reg.h
> index 2686028..7a0b052 100644
> --- a/drivers/gpu/drm/msm/dp/dp_reg.h
> +++ b/drivers/gpu/drm/msm/dp/dp_reg.h
> @@ -22,6 +22,20 @@
> #define REG_DP_INTR_STATUS2 (0x00000024)
> #define REG_DP_INTR_STATUS3 (0x00000028)
>
> +#define REG_DP_INTR_STATUS4 (0x0000002C)
> +#define PSR_UPDATE_INT (0x00000001)
> +#define PSR_CAPTURE_INT (0x00000004)
> +#define PSR_EXIT_INT (0x00000010)
> +#define PSR_UPDATE_ERROR_INT (0x00000040)
> +#define PSR_WAKE_ERROR_INT (0x00000100)
> +
> +#define REG_DP_INTR_MASK4 (0x00000030)
> +#define PSR_UPDATE_MASK (0x00000001)
> +#define PSR_CAPTURE_MASK (0x00000002)
> +#define PSR_EXIT_MASK (0x00000004)
> +#define PSR_UPDATE_ERROR_MASK (0x00000008)
> +#define PSR_WAKE_ERROR_MASK (0x00000010)
> +
> #define REG_DP_DP_HPD_CTRL (0x00000000)
> #define DP_DP_HPD_CTRL_HPD_EN (0x00000001)
>
> @@ -164,6 +178,9 @@
> #define MMSS_DP_AUDIO_TIMING_RBR_48 (0x00000094)
> #define MMSS_DP_AUDIO_TIMING_HBR_48 (0x00000098)
>
> +#define REG_PSR_CONFIG (0x00000100)
> +#define REG_PSR_CMD (0x00000110)
> +
> #define MMSS_DP_PSR_CRC_RG (0x00000154)
> #define MMSS_DP_PSR_CRC_B (0x00000158)
>
> @@ -184,6 +201,8 @@
> #define MMSS_DP_AUDIO_STREAM_0 (0x00000240)
> #define MMSS_DP_AUDIO_STREAM_1 (0x00000244)
>
> +#define MMSS_DP_SDP_CFG3 (0x0000024c)
> +
> #define MMSS_DP_EXTENSION_0 (0x00000250)
> #define MMSS_DP_EXTENSION_1 (0x00000254)
> #define MMSS_DP_EXTENSION_2 (0x00000258)
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index ae52412..254fd07 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -400,6 +400,8 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_displa
>
> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
>
> +void msm_dp_display_set_psr(struct msm_dp *dp, bool enter);
> +
> #else
> static inline int __init msm_dp_register(void)
> {
> @@ -449,6 +451,10 @@ static inline void msm_dp_debugfs_init(struct msm_dp *dp_display,
> {
> }
>
> +static inline void msm_dp_display_set_psr(struct msm_dp *dp, bool enter)
> +{
> +}
> +
> #endif
>
> void __init msm_mdp_register(void);
> --
> 2.7.4
>
--
With best wishes
Dmitry
Enable PSR on eDP interface using drm self-refresh librabry.
This patch uses a trigger from self-refresh library to enter/exit
into PSR, when there are no updates from framework.
Changes in V2:
- Move dp functions to bridge code.
- As per Dmitry review suggestions.
Signed-off-by: Kalyan Thota <[email protected]>
Signed-off-by: Vinod Polimera <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 31 +++++++++++++++++++++++++----
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 30 +++++++++++++++++++++++++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +-
3 files changed, 57 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index e7c9fe1..ba3240c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -18,6 +18,7 @@
#include <drm/drm_probe_helper.h>
#include <drm/drm_rect.h>
#include <drm/drm_vblank.h>
+#include <drm/drm_self_refresh_helper.h>
#include "dpu_kms.h"
#include "dpu_hw_lm.h"
@@ -951,6 +952,14 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
+ if (old_crtc_state->self_refresh_active) {
+ drm_for_each_encoder_mask(encoder, crtc->dev,
+ old_crtc_state->encoder_mask) {
+ dpu_encoder_assign_crtc(encoder, NULL);
+ }
+ return;
+ }
+
/* Disable/save vblank irq handling */
drm_crtc_vblank_off(crtc);
@@ -962,7 +971,12 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
*/
if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
release_bandwidth = true;
- dpu_encoder_assign_crtc(encoder, NULL);
+ /* If disable is triggered during psr active(e.g: screen dim in PSR),
+ * we will need encoder->crtc connection to process the device sleep &
+ * preserve it during psr sequence.
+ */
+ if (!crtc->state->self_refresh_active)
+ dpu_encoder_assign_crtc(encoder, NULL);
}
/* wait for frame_event_done completion */
@@ -1010,6 +1024,8 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
struct drm_encoder *encoder;
bool request_bandwidth = false;
+ struct drm_crtc_state *old_crtc_state =
+ drm_atomic_get_old_crtc_state(state, crtc);
pm_runtime_get_sync(crtc->dev->dev);
@@ -1032,8 +1048,10 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
dpu_crtc->enabled = true;
- drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
- dpu_encoder_assign_crtc(encoder, crtc);
+ if (!old_crtc_state->self_refresh_active) {
+ drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
+ dpu_encoder_assign_crtc(encoder, crtc);
+ }
/* Enable/restore vblank irq handling */
drm_crtc_vblank_on(crtc);
@@ -1497,7 +1515,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
{
struct drm_crtc *crtc = NULL;
struct dpu_crtc *dpu_crtc = NULL;
- int i;
+ int i, ret;
dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL);
if (!dpu_crtc)
@@ -1534,6 +1552,11 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
/* initialize event handling */
spin_lock_init(&dpu_crtc->event_lock);
+ ret = drm_self_refresh_helper_init(crtc);
+ if (ret)
+ DPU_ERROR("Failed to initialize %s with SR helpers %d\n",
+ crtc->name, ret);
+
DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc->name);
return crtc;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 6eac417..ba9d8ea 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -217,6 +217,14 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
};
+static inline bool is_self_refresh_active(struct drm_crtc_state *state)
+{
+ if (state && state->self_refresh_active)
+ return true;
+
+ return false;
+}
+
static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
{
struct dpu_hw_dither_cfg dither_cfg = { 0 };
@@ -629,7 +637,8 @@ static int dpu_encoder_virt_atomic_check(
if (drm_atomic_crtc_needs_modeset(crtc_state)) {
dpu_rm_release(global_state, drm_enc);
- if (!crtc_state->active_changed || crtc_state->active)
+ if (!crtc_state->active_changed || crtc_state->active ||
+ crtc_state->self_refresh_active)
ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
drm_enc, crtc_state, topology);
}
@@ -1182,11 +1191,30 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc,
{
struct dpu_encoder_virt *dpu_enc = NULL;
struct msm_drm_private *priv;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_state;
int i = 0;
dpu_enc = to_dpu_encoder_virt(drm_enc);
DPU_DEBUG_ENC(dpu_enc, "\n");
+ if (!drm_enc) {
+ DPU_ERROR("invalid encoder\n");
+ return;
+ }
+ dpu_enc = to_dpu_encoder_virt(drm_enc);
+
+ crtc = dpu_enc->crtc;
+
+ old_state = drm_atomic_get_old_crtc_state(state, crtc);
+
+ /*
+ * The encoder turn off already occurred when self refresh mode
+ * was set earlier, in the old_state for the corresponding crtc.
+ */
+ if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && is_self_refresh_active(old_state))
+ return;
+
mutex_lock(&dpu_enc->enc_lock);
dpu_enc->enabled = false;
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 47fe11a..d550f90 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -495,7 +495,7 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms,
return;
}
- if (!crtc->state->active) {
+ if (!drm_atomic_crtc_effectively_active(crtc->state)) {
DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id);
return;
}
--
2.7.4
Hi,
On Tue, Feb 22, 2022 at 1:23 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> On 22/02/2022 22:25, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Feb 21, 2022 at 7:12 PM Dmitry Baryshkov
> > <[email protected]> wrote:
> >>
> >>> +static int dp_link_psr_status(struct dp_link_private *link)
> >>> +{
> >>> + u8 status[2];
> >>> +
> >>> + drm_dp_dpcd_read(link->aux, DP_PSR_ERROR_STATUS, status, 2);
> >>> +
> >>> + if (status[0] & DP_PSR_LINK_CRC_ERROR)
> >>> + DRM_ERROR("PSR LINK CRC ERROR\n");
> >>> + else if (status[0] & DP_PSR_RFB_STORAGE_ERROR)
> >>> + DRM_ERROR("PSR RFB STORAGE ERROR\n");
> >>> + else if (status[0] & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
> >>> + DRM_ERROR("PSR VSC SDP UNCORRECTABLE ERROR\n");
> >>> + else if (status[1] & DP_PSR_CAPS_CHANGE)
> >>> + DRM_INFO("PSR Capability Change\n");
> >>
> >> DRM_DEBUG_DP
> >
> > Not sure I'll have time to go back and review the series, but one
> > thing that caught my eye as this flashed through my inbox is that I
> > think all of these "shouting" are deprecated. It's even officially
> > documented now as of commit d2f0a8afc1be ("UPSTREAM: drm/print: Add
> > deprecation notes to DRM_...() functions").
>
> Agreed. But not the DRM_INFO too.
You're saying that DRM_INFO _isn't_ deprecated? I was pretty sure that
it was, too. If not, can you please submit a patch to `drm_print.h`
clarifying since my patch (which folks Acked) marked it as deprecated:
+/* NOTE: this is deprecated in favor of pr_info(). */
#define DRM_INFO(fmt, ...) \
_DRM_PRINTK(, INFO, fmt, ##__VA_ARGS__)
My understanding (also in the description of my patch) was that
DRM_INFO() by itself didn't add much so we should just use the
standard pr_info(). If pr_info() wasn't to your liking then it was
better to do drm_info(drmdev, ...) or drm_info(NULL, ...);
-Doug
Hi,
On Mon, Feb 21, 2022 at 7:12 PM Dmitry Baryshkov
<[email protected]> wrote:
>
> > +static int dp_link_psr_status(struct dp_link_private *link)
> > +{
> > + u8 status[2];
> > +
> > + drm_dp_dpcd_read(link->aux, DP_PSR_ERROR_STATUS, status, 2);
> > +
> > + if (status[0] & DP_PSR_LINK_CRC_ERROR)
> > + DRM_ERROR("PSR LINK CRC ERROR\n");
> > + else if (status[0] & DP_PSR_RFB_STORAGE_ERROR)
> > + DRM_ERROR("PSR RFB STORAGE ERROR\n");
> > + else if (status[0] & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
> > + DRM_ERROR("PSR VSC SDP UNCORRECTABLE ERROR\n");
> > + else if (status[1] & DP_PSR_CAPS_CHANGE)
> > + DRM_INFO("PSR Capability Change\n");
>
> DRM_DEBUG_DP
Not sure I'll have time to go back and review the series, but one
thing that caught my eye as this flashed through my inbox is that I
think all of these "shouting" are deprecated. It's even officially
documented now as of commit d2f0a8afc1be ("UPSTREAM: drm/print: Add
deprecation notes to DRM_...() functions").
-Doug
On 22/02/2022 22:25, Doug Anderson wrote:
> Hi,
>
> On Mon, Feb 21, 2022 at 7:12 PM Dmitry Baryshkov
> <[email protected]> wrote:
>>
>>> +static int dp_link_psr_status(struct dp_link_private *link)
>>> +{
>>> + u8 status[2];
>>> +
>>> + drm_dp_dpcd_read(link->aux, DP_PSR_ERROR_STATUS, status, 2);
>>> +
>>> + if (status[0] & DP_PSR_LINK_CRC_ERROR)
>>> + DRM_ERROR("PSR LINK CRC ERROR\n");
>>> + else if (status[0] & DP_PSR_RFB_STORAGE_ERROR)
>>> + DRM_ERROR("PSR RFB STORAGE ERROR\n");
>>> + else if (status[0] & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
>>> + DRM_ERROR("PSR VSC SDP UNCORRECTABLE ERROR\n");
>>> + else if (status[1] & DP_PSR_CAPS_CHANGE)
>>> + DRM_INFO("PSR Capability Change\n");
>>
>> DRM_DEBUG_DP
>
> Not sure I'll have time to go back and review the series, but one
> thing that caught my eye as this flashed through my inbox is that I
> think all of these "shouting" are deprecated. It's even officially
> documented now as of commit d2f0a8afc1be ("UPSTREAM: drm/print: Add
> deprecation notes to DRM_...() functions").
Agreed. But not the DRM_INFO too.
--
With best wishes
Dmitry
Quoting Vinod Polimera (2022-02-21 06:51:26)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index e7c9fe1..ba3240c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -951,6 +952,14 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
>
> DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
>
> + if (old_crtc_state->self_refresh_active) {
> + drm_for_each_encoder_mask(encoder, crtc->dev,
> + old_crtc_state->encoder_mask) {
> + dpu_encoder_assign_crtc(encoder, NULL);
> + }
> + return;
> + }
> +
> /* Disable/save vblank irq handling */
> drm_crtc_vblank_off(crtc);
>
> @@ -962,7 +971,12 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> */
> if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
> release_bandwidth = true;
> - dpu_encoder_assign_crtc(encoder, NULL);
> + /* If disable is triggered during psr active(e.g: screen dim in PSR),
Multiline comments start with /* on a line by itself
/*
* If disable is triggered ...
> + * we will need encoder->crtc connection to process the device sleep &
> + * preserve it during psr sequence.
> + */
> + if (!crtc->state->self_refresh_active)
> + dpu_encoder_assign_crtc(encoder, NULL);
> }
>
> /* wait for frame_event_done completion */
> @@ -1010,6 +1024,8 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
> struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> struct drm_encoder *encoder;
> bool request_bandwidth = false;
> + struct drm_crtc_state *old_crtc_state =
> + drm_atomic_get_old_crtc_state(state, crtc);
Use two lines
struct drm_crtc_state *old_crtc_state;
old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
>
> pm_runtime_get_sync(crtc->dev->dev);
>
> @@ -1032,8 +1048,10 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
> trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
> dpu_crtc->enabled = true;
>
> - drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
> - dpu_encoder_assign_crtc(encoder, crtc);
> + if (!old_crtc_state->self_refresh_active) {
> + drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state->encoder_mask)
> + dpu_encoder_assign_crtc(encoder, crtc);
Make these two lines above into a function with a meaningful name?
dpu_encoder_assign_crtcs()? And then push the encoder mask iteration
into the loop by passing the mask as a function argument. I see
dpu_encoder_assign_crtc() takes a spinlock, so we could probably take
that lock out too and push it into this new function to avoid grabbing
and dropping the spinlock multiple times.
> + }
>
> /* Enable/restore vblank irq handling */
> drm_crtc_vblank_on(crtc);
> @@ -1497,7 +1515,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
> {
> struct drm_crtc *crtc = NULL;
> struct dpu_crtc *dpu_crtc = NULL;
> - int i;
> + int i, ret;
>
> dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL);
> if (!dpu_crtc)
> @@ -1534,6 +1552,11 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
> /* initialize event handling */
> spin_lock_init(&dpu_crtc->event_lock);
>
> + ret = drm_self_refresh_helper_init(crtc);
> + if (ret)
> + DPU_ERROR("Failed to initialize %s with SR helpers %d\n",
What is SR? Write self-refresh?
> + crtc->name, ret);
> +
> DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc->name);
> return crtc;
> }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 6eac417..ba9d8ea 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -217,6 +217,14 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
> };
>
> +static inline bool is_self_refresh_active(struct drm_crtc_state *state)
const drm_crtc_state?
> +{
> + if (state && state->self_refresh_active)
> + return true;
> +
> + return false;
return state && state->self_refresh_active;
> +}
> +
> static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
> {
> struct dpu_hw_dither_cfg dither_cfg = { 0 };
> @@ -629,7 +637,8 @@ static int dpu_encoder_virt_atomic_check(
> if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> dpu_rm_release(global_state, drm_enc);
>
> - if (!crtc_state->active_changed || crtc_state->active)
> + if (!crtc_state->active_changed || crtc_state->active ||
> + crtc_state->self_refresh_active)
> ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> drm_enc, crtc_state, topology);
> }
> @@ -1182,11 +1191,30 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc,
> {
> struct dpu_encoder_virt *dpu_enc = NULL;
> struct msm_drm_private *priv;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *old_state;
> int i = 0;
>
> dpu_enc = to_dpu_encoder_virt(drm_enc);
> DPU_DEBUG_ENC(dpu_enc, "\n");
Presumably this print wants a valid 'dpu_enc' pointer.
>
> + if (!drm_enc) {
So this check for !drm_enc is impossible? Please remove it.
> + DPU_ERROR("invalid encoder\n");
> + return;
> + }
> + dpu_enc = to_dpu_encoder_virt(drm_enc);
We got it again?
> +
> + crtc = dpu_enc->crtc;
> +
> + old_state = drm_atomic_get_old_crtc_state(state, crtc);
> +
> + /*
> + * The encoder turn off already occurred when self refresh mode
s/turn off/disable/
> + * was set earlier, in the old_state for the corresponding crtc.
> + */
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && is_self_refresh_active(old_state))
> + return;
> +
> mutex_lock(&dpu_enc->enc_lock);
> dpu_enc->enabled = false;
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 47fe11a..d550f90 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -495,7 +495,7 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms,
> return;
> }
>
> - if (!crtc->state->active) {
> + if (!drm_atomic_crtc_effectively_active(crtc->state)) {
> DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id);
> return;
> }
> --
> 2.7.4
>
On 23/02/2022 00:32, Doug Anderson wrote:
> Hi,
>
> On Tue, Feb 22, 2022 at 1:23 PM Dmitry Baryshkov
> <[email protected]> wrote:
>>
>> On 22/02/2022 22:25, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Feb 21, 2022 at 7:12 PM Dmitry Baryshkov
>>> <[email protected]> wrote:
>>>>
>>>>> +static int dp_link_psr_status(struct dp_link_private *link)
>>>>> +{
>>>>> + u8 status[2];
>>>>> +
>>>>> + drm_dp_dpcd_read(link->aux, DP_PSR_ERROR_STATUS, status, 2);
>>>>> +
>>>>> + if (status[0] & DP_PSR_LINK_CRC_ERROR)
>>>>> + DRM_ERROR("PSR LINK CRC ERROR\n");
>>>>> + else if (status[0] & DP_PSR_RFB_STORAGE_ERROR)
>>>>> + DRM_ERROR("PSR RFB STORAGE ERROR\n");
>>>>> + else if (status[0] & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
>>>>> + DRM_ERROR("PSR VSC SDP UNCORRECTABLE ERROR\n");
>>>>> + else if (status[1] & DP_PSR_CAPS_CHANGE)
>>>>> + DRM_INFO("PSR Capability Change\n");
>>>>
>>>> DRM_DEBUG_DP
>>>
>>> Not sure I'll have time to go back and review the series, but one
>>> thing that caught my eye as this flashed through my inbox is that I
>>> think all of these "shouting" are deprecated. It's even officially
>>> documented now as of commit d2f0a8afc1be ("UPSTREAM: drm/print: Add
>>> deprecation notes to DRM_...() functions").
>>
>> Agreed. But not the DRM_INFO too.
>
> You're saying that DRM_INFO _isn't_ deprecated? I was pretty sure that
> it was, too. If not, can you please submit a patch to `drm_print.h`
> clarifying since my patch (which folks Acked) marked it as deprecated:
It is deprecated
> My understanding (also in the description of my patch) was that
> DRM_INFO() by itself didn't add much so we should just use the
> standard pr_info(). If pr_info() wasn't to your liking then it was
> better to do drm_info(drmdev, ...) or drm_info(NULL, ...);
I don't think we should get this message at all, unless debugging is
enabled. Thus I asked to change DRM_INFO to DEBUG.
Regarding your point, I'm fine with either of them. Hopefully when
Kuogee's patches are in, we can use drm_dbg_db w/o any issues.
--
With best wishes
Dmitry
On Mon 21 Feb 06:51 PST 2022, 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 <[email protected]>
>
> Changes in v2:
> - Use dp bridge to set psr entry/exit instead of dpu_enocder
> - Don't modify whitespaces
> - set self refresh aware from atomic_check
> - set self refresh aware only if psr is supported
> - provide a stub for msm_dp_display_set_psr
> ---
> 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 | 63 +++++++++++++
> 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 | 1 +
> drivers/gpu/drm/msm/dp/dp_drm.c | 177 ++++++++++++++++++++++++++++++++++--
> drivers/gpu/drm/msm/dp/dp_link.c | 22 +++++
> drivers/gpu/drm/msm/dp/dp_panel.c | 21 +++++
> drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++
> drivers/gpu/drm/msm/dp/dp_reg.h | 19 ++++
> drivers/gpu/drm/msm/msm_drv.h | 6 ++
> 12 files changed, 411 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 8a6d3ea..3cd223d 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -45,6 +45,14 @@
> #define DP_INTERRUPT_STATUS2_MASK \
> (DP_INTERRUPT_STATUS2 << DP_INTERRUPT_STATUS_MASK_SHIFT)
>
> +#define DP_INTERRUPT_STATUS4 \
Is this group of interrupt bits really called "status4", what is 4?
Seems more like the "PSR interrupt status bits" to me.
> + (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 dp_io *io;
> @@ -343,6 +351,20 @@ 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 mainlink_ctrl;
> + struct dp_catalog_private *catalog = container_of(dp_catalog,
> + struct dp_catalog_private, dp_catalog);
> +
> + mainlink_ctrl = dp_read_link(catalog, REG_DP_MAINLINK_CTRL);
> + mainlink_ctrl &= ~DP_MAINLINK_CTRL_ENABLE;
> + mainlink_ctrl |= (enable & DP_MAINLINK_CTRL_ENABLE);
Masking a boolean with constant is different...Please don't do that.
if (enable)
mainlink_ctrl |= DP_MAINLINK_CTRL_ENABLE;
else
mainlink_ctrl &= ~DP_MAINLINK_CTRL_ENABLE;
Is way less magical.
Not also, that you don't have to name your "only" local variable
"mainline_ctrl", there's no risk for collisions with anything else.
Name it "val" or "ctrl".
> +
> + dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl);
> +}
> +
> void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog,
> bool enable)
> {
> @@ -581,6 +603,51 @@ 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, 0x1);
> + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x0);
Can 1 and 0 be defined here?
> +}
> +
> +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 psr_config;
Again, a single local variable. Name it "config".
> +
> + /* enable PSR1 function */
> + psr_config = dp_read_link(catalog, REG_PSR_CONFIG);
> + psr_config |= BIT(0);
Add a define for BIT(0) in this register and the code will be
self-explanatory, no need for a separate comment to help future readers
to guess that BIT(0) enables something...
> + dp_write_link(catalog, REG_PSR_CONFIG, psr_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 psr_cmd;
"cmd" is sufficient.
> +
> + psr_cmd = dp_read_link(catalog, REG_PSR_CMD);
> +
> + /*
> + * BIT(0) - send psr entry SDP
> + * BIT(1) - sned psr exit SDP
> + */
> + psr_cmd &= ~(BIT(0) | BIT(1));
> +
> + if (enter)
> + psr_cmd |= BIT(0);
> + else
> + psr_cmd |= BIT(1);
As above, defines for BIT(0) and BIT(1), and drop the comment, please.
> +
> + dp_catalog_enable_sdp(catalog);
> + dp_write_link(catalog, REG_PSR_CMD, psr_cmd);
> +}
> +
> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
> {
> struct dp_catalog_private *catalog = container_of(dp_catalog,
> @@ -608,6 +675,20 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog)
> return isr;
> }
>
> +int dp_catalog_ctrl_get_psr_interrupt(struct dp_catalog *dp_catalog)
"Getting an interrupt" is something we do with e.g. platform_get_irq().
dp_catalog_ctrl_read_psr_interrupt_status() would better represent the
purpose of this function.
> +{
> + 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 a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 6965afa..9b1b199 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -91,6 +91,7 @@ void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 state);
> void dp_catalog_ctrl_config_ctrl(struct dp_catalog *dp_catalog, u32 config);
> void dp_catalog_ctrl_lane_mapping(struct dp_catalog *dp_catalog);
> void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog, bool enable);
> +void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog *dp_catalog, bool enable);
> void dp_catalog_ctrl_config_misc(struct dp_catalog *dp_catalog, u32 cc, u32 tb);
> void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog, u32 rate,
> u32 stream_rate_khz, bool fixed_nvid);
> @@ -101,12 +102,15 @@ void dp_catalog_ctrl_enable_irq(struct dp_catalog *dp_catalog, bool enable);
> void dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
> u32 intr_mask, bool en);
> void dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog);
> +void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog);
> +void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter);
> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog);
> u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog);
> void dp_catalog_ctrl_phy_reset(struct dp_catalog *dp_catalog);
> int dp_catalog_ctrl_update_vx_px(struct dp_catalog *dp_catalog, u8 v_level,
> u8 p_level);
> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog);
> +int dp_catalog_ctrl_get_psr_interrupt(struct dp_catalog *dp_catalog);
> void dp_catalog_ctrl_update_transfer_unit(struct dp_catalog *dp_catalog,
> u32 dp_tu, u32 valid_boundary,
> u32 valid_boundary2);
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> index 88ca6c3..ba828ea 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> @@ -21,6 +21,7 @@
>
> #define DP_KHZ_TO_HZ 1000
> #define IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES (30 * HZ / 1000) /* 30 ms */
> +#define PSR_OPERATION_COMPLETION_TIMEOUT_JIFFIES (300 * HZ / 1000) /* 300 ms */
> #define WAIT_FOR_VIDEO_READY_TIMEOUT_JIFFIES (HZ / 2)
>
> #define DP_CTRL_INTR_READY_FOR_VIDEO BIT(0)
> @@ -78,6 +79,7 @@ struct dp_ctrl_private {
> struct dp_catalog *catalog;
>
> struct completion idle_comp;
> + struct completion psr_op_comp;
> struct completion video_comp;
> };
>
> @@ -151,6 +153,9 @@ static void dp_ctrl_config_ctrl(struct dp_ctrl_private *ctrl)
> config |= DP_CONFIGURATION_CTRL_STATIC_DYNAMIC_CN;
> config |= DP_CONFIGURATION_CTRL_SYNC_ASYNC_CLK;
>
> + if (ctrl->panel->psr_cap.version)
> + config |= DP_CONFIGURATION_CTRL_SEND_VSC;
> +
> dp_catalog_ctrl_config_ctrl(ctrl->catalog, config);
> }
>
> @@ -1365,6 +1370,52 @@ static int dp_ctrl_enable_stream_clocks(struct dp_ctrl_private *ctrl)
> return ret;
> }
>
> +void dp_ctrl_config_psr(struct dp_ctrl *dp_ctrl)
> +{
> + struct dp_ctrl_private *ctrl;
> + u8 psr_config;
"config" - or "cfg' per the register name.
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
Join this with the declaration of ctrl.
> +
> + if (!ctrl->panel->psr_cap.version)
> + return;
> +
> + dp_catalog_ctrl_config_psr(ctrl->catalog);
> +
> + psr_config = DP_PSR_ENABLE;
> + drm_dp_dpcd_write(ctrl->aux, DP_PSR_EN_CFG, &psr_config, 1);
> +}
> +
> +void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enter)
> +{
> + struct dp_ctrl_private *ctrl;
> +
> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
As above, please join this with the previous line.
> +
> + if (!ctrl->panel->psr_cap.version)
> + return;
> +
> + 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);
So to enter psr we disable psr_mainline...
> + } else {
> + dp_catalog_ctrl_psr_mainlink_enable(ctrl->catalog, true);
and to exit psr we enable psr_mainline?
I certainly would not mind having a comment above this section
describing how this is supposed to work.
> +
> + dp_catalog_ctrl_set_psr(ctrl->catalog, false);
> + }
> +}
> +
> int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
> {
> struct dp_ctrl_private *ctrl;
> @@ -1964,6 +2015,17 @@ void dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
>
> ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>
> + if (ctrl->panel->psr_cap.version) {
> + isr = dp_catalog_ctrl_get_psr_interrupt(ctrl->catalog);
> +
> + if (isr == 0x1)
> + DRM_DEBUG_DP("PSR frame update done\n");
> + else if (isr == 0x10)
Again, the error messages gives a good indication what these bits might
be, but please provide defines to make it clearer.
> + DRM_DEBUG_DP("PSR exit done\n");
> +
> + complete(&ctrl->psr_op_comp);
> + }
> +
> isr = dp_catalog_ctrl_get_interrupt(ctrl->catalog);
>
> if (isr & DP_CTRL_INTR_READY_FOR_VIDEO) {
> @@ -2010,6 +2072,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
> dev_err(dev, "failed to add DP OPP table\n");
>
> init_completion(&ctrl->idle_comp);
> + init_completion(&ctrl->psr_op_comp);
> init_completion(&ctrl->video_comp);
>
> /* in parameters */
> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> index 2363a2d..f623035 100644
> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
> @@ -34,4 +34,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
> struct dp_power *power, struct dp_catalog *catalog,
> struct dp_parser *parser);
>
> +void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enable);
> +void dp_ctrl_config_psr(struct dp_ctrl *dp_ctrl);
> +
> #endif /* _DP_CTRL_H_ */
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5d314e6..c8d02fb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -347,6 +347,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;
> +
> dp->audio_supported = drm_detect_monitor_audio(edid);
> dp_panel_handle_sink_request(dp->panel);
>
> @@ -871,6 +873,10 @@ static int dp_display_post_enable(struct msm_dp *dp_display)
>
> /* signal the connect event late to synchronize video and display */
> dp_display_handle_plugged_change(dp_display, true);
> +
> + if (dp_display->psr_supported)
> + dp_ctrl_config_psr(dp->ctrl);
> +
> return 0;
> }
>
> @@ -1037,6 +1043,14 @@ static void dp_display_config_hpd(struct dp_display_private *dp)
> enable_irq(dp->irq);
> }
>
> +void msm_dp_display_set_psr(struct msm_dp *dp_display, bool enter)
> +{
> + struct dp_display_private *dp;
> +
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> + dp_ctrl_set_psr(dp->ctrl, enter);
> +}
> +
> static int hpd_event_thread(void *data)
> {
> struct dp_display_private *dp_priv;
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index e3adcd5..6f512f3 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -28,6 +28,7 @@ struct msm_dp {
>
> u32 max_dp_lanes;
> struct dp_audio *dp_audio;
> + bool psr_supported;
> };
>
> int dp_display_set_plugged_cb(struct msm_dp *dp_display,
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> index 2436329..d26ca6a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -142,6 +142,35 @@ static enum drm_mode_status edp_connector_mode_valid(
> return MODE_OK;
> }
>
> +
> +
> +static int edp_connector_atomic_check(struct drm_connector *connector,
> + struct drm_atomic_state *state)
> +{
> + struct msm_dp *dp;
> + struct drm_connector_state *conn_state;
> + struct drm_crtc_state *crtc_state;
> +
> + dp = to_dp_connector(connector)->dp_display;
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
> + if (WARN_ON(!conn_state))
> + return -ENODEV;
> +
> + conn_state->self_refresh_aware = true;
> +
> + if (!conn_state->crtc)
> + return 0;
> +
> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> + if (!crtc_state)
> + return 0;
> +
> + if (crtc_state->self_refresh_active && !dp->psr_supported)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> static const struct drm_connector_funcs dp_connector_funcs = {
> .detect = dp_connector_detect,
> .fill_modes = drm_helper_probe_single_connector_modes,
> @@ -151,6 +180,11 @@ static const struct drm_connector_funcs dp_connector_funcs = {
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> };
>
> +static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
> + .get_modes = dp_connector_get_modes,
> + .mode_valid = dp_connector_mode_valid,
> +};
> +
> static const struct drm_connector_funcs edp_connector_funcs = {
> .fill_modes = drm_helper_probe_single_connector_modes,
> .destroy = drm_connector_cleanup,
> @@ -159,12 +193,8 @@ static const struct drm_connector_funcs edp_connector_funcs = {
> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> };
>
> -static const struct drm_connector_helper_funcs dp_connector_helper_funcs = {
> - .get_modes = dp_connector_get_modes,
> - .mode_valid = dp_connector_mode_valid,
> -};
> -
> static const struct drm_connector_helper_funcs edp_connector_helper_funcs = {
> + .atomic_check = edp_connector_atomic_check,
> .get_modes = edp_connector_get_modes,
> .mode_valid = edp_connector_mode_valid,
> };
> @@ -258,6 +288,130 @@ static void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
> msm_dp_display_disable(dp_display, drm_bridge->encoder);
> }
>
> +static struct drm_crtc *dp_bridge_get_old_connector_crtc(struct msm_dp *dp,
> + struct drm_atomic_state *state)
> +{
> + struct drm_encoder *encoder = dp->encoder;
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> +
> + connector = drm_atomic_get_old_connector_for_encoder(state, encoder);
> + if (!connector)
> + return NULL;
> +
> + conn_state = drm_atomic_get_old_connector_state(state, connector);
> + if (!conn_state)
> + return NULL;
> +
> + return conn_state->crtc;
> +}
> +
> +static struct drm_crtc *dp_bridge_get_new_connector_crtc(struct msm_dp *dp,
> + struct drm_atomic_state *state)
> +{
> + struct drm_encoder *encoder = dp->encoder;
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state;
> +
> + connector = drm_atomic_get_new_connector_for_encoder(state, encoder);
> + if (!connector)
> + return NULL;
> +
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
> + if (!conn_state)
> + return NULL;
> +
> + return conn_state->crtc;
> +}
> +
> +static void edp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
> + struct drm_bridge_state *old_bridge_state)
> +{
> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *old_crtc_state;
> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
> + struct msm_dp *dp_display = dp_bridge->dp_display;
> +
> + crtc = dp_bridge_get_new_connector_crtc(dp_display, old_state);
> + if (!crtc)
> + return;
> +
> + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
> +
> + /* Exit from self refresh mode */
> + if (old_crtc_state && old_crtc_state->self_refresh_active) {
> + msm_dp_display_set_psr(dp_display, false);
> + return;
> + }
> +
> + msm_dp_display_enable(dp_display, drm_bridge->encoder);
> +}
> +
> +static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
> + struct drm_bridge_state *old_bridge_state)
> +{
> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state = NULL;
> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
> + struct msm_dp *dp_display = dp_bridge->dp_display;
> +
> + crtc = dp_bridge_get_old_connector_crtc(dp_display, old_state);
> + if (!crtc)
> + goto out;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
> + if (!new_crtc_state)
> + goto out;
> +
> + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
> + if (!old_crtc_state)
> + goto out;
> +
> + /*
> + * Set self refresh mode if current crtc state is active.
> + * If old crtc state is active, exit psr before disabling
> + * the controller. Observed sink stuck in self refresh
> + * if psr exit is skipped when screen off occurs with
> + * sink in psr mode.
> + */
> + if (new_crtc_state->self_refresh_active) {
> + msm_dp_display_set_psr(dp_display, true);
> + return;
> + } else if (old_crtc_state->self_refresh_active) {
> + msm_dp_display_set_psr(dp_display, false);
> + return;
> + }
> +
> +out:
> + msm_dp_display_pre_disable(dp_display, drm_bridge->encoder);
> +}
> +
> +static void edp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
> + struct drm_bridge_state *old_bridge_state)
> +{
> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *new_crtc_state = NULL;
> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
> + struct msm_dp *dp_display = dp_bridge->dp_display;
> +
> + crtc = dp_bridge_get_old_connector_crtc(dp_display, old_state);
> + if (!crtc)
> + return;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
> + if (!new_crtc_state)
> + return;
> +
> + /* Self refresh mode is set in dp_bridge_disable. Skip disable */
> + if (new_crtc_state->self_refresh_active)
> + return;
> +
> + msm_dp_display_disable(dp_display, drm_bridge->encoder);
> +}
> +
> static const struct drm_bridge_funcs dp_bridge_ops = {
> .enable = dp_bridge_enable,
> .disable = dp_bridge_disable,
> @@ -265,6 +419,16 @@ static const struct drm_bridge_funcs dp_bridge_ops = {
> .mode_set = dp_bridge_mode_set,
> };
>
> +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,
> + .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,
> +};
> +
> struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
> struct drm_encoder *encoder)
> {
> @@ -279,7 +443,8 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display, struct drm_devi
> dp_bridge->dp_display = dp_display;
>
> bridge = &dp_bridge->bridge;
> - bridge->funcs = &dp_bridge_ops;
> + bridge->funcs = (dp_display->connector_type == DRM_MODE_CONNECTOR_eDP) ?
> + &edp_bridge_ops : &dp_bridge_ops;
> bridge->encoder = encoder;
>
> rc = drm_bridge_attach(encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c b/drivers/gpu/drm/msm/dp/dp_link.c
> index d4d31e5..5503c29 100644
> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> @@ -924,6 +924,26 @@ static int dp_link_process_phy_test_pattern_request(
> return 0;
> }
>
> +static int dp_link_psr_status(struct dp_link_private *link)
The implementation and caller indicates that this would better return a
bool. That said "status" isn't a boolean property, and given that you
return true when capabilities changes, perhaps this is
dp_link_psr_cap_changed() ?
> +{
> + u8 status[2];
> +
> + drm_dp_dpcd_read(link->aux, DP_PSR_ERROR_STATUS, status, 2);
> +
> + if (status[0] & DP_PSR_LINK_CRC_ERROR)
> + DRM_ERROR("PSR LINK CRC ERROR\n");
> + else if (status[0] & DP_PSR_RFB_STORAGE_ERROR)
> + DRM_ERROR("PSR RFB STORAGE ERROR\n");
> + else if (status[0] & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
> + DRM_ERROR("PSR VSC SDP UNCORRECTABLE ERROR\n");
> + else if (status[1] & DP_PSR_CAPS_CHANGE)
> + DRM_INFO("PSR Capability Change\n");
DEBUG instead?
> + else
> + return 0;
> +
> + return 1;
> +}
> +
> static u8 get_link_status(const u8 link_status[DP_LINK_STATUS_SIZE], int r)
> {
> return link_status[r - DP_LANE0_1_STATUS];
> @@ -1042,6 +1062,8 @@ int dp_link_process_request(struct dp_link *dp_link)
> dp_link->sink_request |= DP_TEST_LINK_TRAINING;
> } else if (!dp_link_process_phy_test_pattern_request(link)) {
> dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
> + } else if (dp_link_psr_status(link)) {
> + DRM_INFO("PSR IRQ_HPD received\n");
> } else {
> ret = dp_link_process_link_status_update(link);
> if (!ret) {
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c b/drivers/gpu/drm/msm/dp/dp_panel.c
> index 71db10c..e128d73 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
> @@ -19,6 +19,26 @@ struct dp_panel_private {
> bool aux_cfg_update_done;
> };
>
> +static void dp_panel_read_psr_cap(struct dp_panel_private *panel)
> +{
> + ssize_t rlen;
> + struct dp_panel *dp_panel;
> +
> + dp_panel = &panel->dp_panel;
> +
> + /* edp sink */
> + if (dp_panel->dpcd[DP_EDP_CONFIGURATION_CAP]) {
> + rlen = drm_dp_dpcd_read(panel->aux, DP_PSR_SUPPORT,
> + &dp_panel->psr_cap, 2);
> + if (rlen == 2) {
sizeof(dp_panel->psr_cap) perhaps?
> + DRM_DEBUG_DP("psr version: 0x%x, psr_cap: 0x%x\n",
> + dp_panel->psr_cap.version,
> + dp_panel->psr_cap.capabilities);
> + } else
> + DRM_ERROR("failed to read psr info, rlen=%zd\n", rlen);
> + }
> +}
> +
> static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
> {
> int rc = 0;
> @@ -104,6 +124,7 @@ static int dp_panel_read_dpcd(struct dp_panel *dp_panel)
> }
> }
>
> + dp_panel_read_psr_cap(panel);
> end:
> return rc;
> }
> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
> index 9023e5b..631657a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
> @@ -34,6 +34,11 @@ struct dp_panel_in {
> struct dp_catalog *catalog;
> };
>
> +struct dp_panel_psr {
> + u8 version;
> + u8 capabilities;
> +};
> +
> struct dp_panel {
> /* dpcd raw data */
> u8 dpcd[DP_RECEIVER_CAP_SIZE + 1];
> @@ -46,6 +51,7 @@ struct dp_panel {
> struct edid *edid;
> struct drm_connector *connector;
> struct dp_display_mode dp_mode;
> + struct dp_panel_psr psr_cap;
> bool video_test;
>
> u32 vic;
> diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h b/drivers/gpu/drm/msm/dp/dp_reg.h
> index 2686028..7a0b052 100644
> --- a/drivers/gpu/drm/msm/dp/dp_reg.h
> +++ b/drivers/gpu/drm/msm/dp/dp_reg.h
> @@ -22,6 +22,20 @@
> #define REG_DP_INTR_STATUS2 (0x00000024)
> #define REG_DP_INTR_STATUS3 (0x00000028)
>
> +#define REG_DP_INTR_STATUS4 (0x0000002C)
> +#define PSR_UPDATE_INT (0x00000001)
> +#define PSR_CAPTURE_INT (0x00000004)
> +#define PSR_EXIT_INT (0x00000010)
> +#define PSR_UPDATE_ERROR_INT (0x00000040)
> +#define PSR_WAKE_ERROR_INT (0x00000100)
> +
> +#define REG_DP_INTR_MASK4 (0x00000030)
> +#define PSR_UPDATE_MASK (0x00000001)
> +#define PSR_CAPTURE_MASK (0x00000002)
> +#define PSR_EXIT_MASK (0x00000004)
> +#define PSR_UPDATE_ERROR_MASK (0x00000008)
> +#define PSR_WAKE_ERROR_MASK (0x00000010)
> +
> #define REG_DP_DP_HPD_CTRL (0x00000000)
> #define DP_DP_HPD_CTRL_HPD_EN (0x00000001)
>
> @@ -164,6 +178,9 @@
> #define MMSS_DP_AUDIO_TIMING_RBR_48 (0x00000094)
> #define MMSS_DP_AUDIO_TIMING_HBR_48 (0x00000098)
>
> +#define REG_PSR_CONFIG (0x00000100)
> +#define REG_PSR_CMD (0x00000110)
> +
> #define MMSS_DP_PSR_CRC_RG (0x00000154)
> #define MMSS_DP_PSR_CRC_B (0x00000158)
>
> @@ -184,6 +201,8 @@
> #define MMSS_DP_AUDIO_STREAM_0 (0x00000240)
> #define MMSS_DP_AUDIO_STREAM_1 (0x00000244)
>
> +#define MMSS_DP_SDP_CFG3 (0x0000024c)
> +
> #define MMSS_DP_EXTENSION_0 (0x00000250)
> #define MMSS_DP_EXTENSION_1 (0x00000254)
> #define MMSS_DP_EXTENSION_2 (0x00000258)
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index ae52412..254fd07 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -400,6 +400,8 @@ void msm_dp_snapshot(struct msm_disp_state *disp_state, struct msm_dp *dp_displa
>
> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor);
>
> +void msm_dp_display_set_psr(struct msm_dp *dp, bool enter);
> +
> #else
> static inline int __init msm_dp_register(void)
> {
> @@ -449,6 +451,10 @@ static inline void msm_dp_debugfs_init(struct msm_dp *dp_display,
> {
> }
>
> +static inline void msm_dp_display_set_psr(struct msm_dp *dp, bool enter)
Perhaps I'm missing it, but I don't see that this is called from outside
the DP driver. So why does it need to be in msm_drv.h and why do you
need a stub?
Thanks,
Bjorn
> +{
> +}
> +
> #endif
>
> void __init msm_mdp_register(void);
> --
> 2.7.4
>
Quoting Vinod Polimera (2022-02-21 06:51:24)
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index b32295a..5c7dc82 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -102,30 +136,82 @@ static void panel_bridge_detach(struct drm_bridge *bridge)
> drm_connector_cleanup(connector);
> }
>
> -static void panel_bridge_pre_enable(struct drm_bridge *bridge)
> +static void panel_bridge_pre_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
> + struct drm_encoder *encoder = bridge->encoder;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *old_crtc_state;
> +
> + crtc = bridge_drm_get_new_connector_crtc(encoder, old_state);
> + if (!crtc)
> + return;
> +
> + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
> + if (old_crtc_state && old_crtc_state->self_refresh_active)
Can you please add some comment about why self_refresh_active means we
should bail out from this function? Such a comment would be helpful to
understand the code quickly vs. having to figure out what the intention
of the logic is. The analogix driver has
/* Don't touch the panel if we're coming back from PSR */
so even mentioning PSR here would be helpful.
Hi Dmitry,
>On Mon, 21 Feb 2022 at 17:52, Vinod Polimera <[email protected]>
>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 <[email protected]>
>>
>> Changes in v2:
>> - Use dp bridge to set psr entry/exit instead of dpu_enocder
>> - Don't modify whitespaces
>> - set self refresh aware from atomic_check
>> - set self refresh aware only if psr is supported
>> - provide a stub for msm_dp_display_set_psr
>> ---
>> 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 | 63 +++++++++++++
>> 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 | 1 +
>> drivers/gpu/drm/msm/dp/dp_drm.c | 177
>++++++++++++++++++++++++++++++++++--
>> drivers/gpu/drm/msm/dp/dp_link.c | 22 +++++
>> drivers/gpu/drm/msm/dp/dp_panel.c | 21 +++++
>> drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++
>> drivers/gpu/drm/msm/dp/dp_reg.h | 19 ++++
>> drivers/gpu/drm/msm/msm_drv.h | 6 ++
>> 12 files changed, 411 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 8a6d3ea..3cd223d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -45,6 +45,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 dp_io *io;
>> @@ -343,6 +351,20 @@ 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 mainlink_ctrl;
>> + struct dp_catalog_private *catalog = container_of(dp_catalog,
>> + struct dp_catalog_private,
>> +dp_catalog);
>> +
>> + mainlink_ctrl = dp_read_link(catalog, REG_DP_MAINLINK_CTRL);
>> + mainlink_ctrl &= ~DP_MAINLINK_CTRL_ENABLE;
>> + mainlink_ctrl |= (enable & DP_MAINLINK_CTRL_ENABLE);
>> +
>> + dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); }
>> +
>> void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog,
>> bool enable) { @@
>> -581,6 +603,51 @@ 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, 0x1);
>> + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x0); }
>> +
>> +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 psr_config;
>> +
>> + /* enable PSR1 function */
>> + psr_config = dp_read_link(catalog, REG_PSR_CONFIG);
>> + psr_config |= BIT(0);
>> + dp_write_link(catalog, REG_PSR_CONFIG, psr_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 psr_cmd;
>> +
>> + psr_cmd = dp_read_link(catalog, REG_PSR_CMD);
>> +
>> + /*
>> + * BIT(0) - send psr entry SDP
>> + * BIT(1) - sned psr exit SDP
>> + */
>> + psr_cmd &= ~(BIT(0) | BIT(1));
>> +
>> + if (enter)
>> + psr_cmd |= BIT(0);
>> + else
>> + psr_cmd |= BIT(1);
>> +
>> + dp_catalog_enable_sdp(catalog);
>> + dp_write_link(catalog, REG_PSR_CMD, psr_cmd); }
>> +
>> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog) {
>> struct dp_catalog_private *catalog = container_of(dp_catalog,
>> @@ -608,6 +675,20 @@ u32 dp_catalog_hpd_get_intr_status(struct
>dp_catalog *dp_catalog)
>> return isr;
>> }
>>
>> +int dp_catalog_ctrl_get_psr_interrupt(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 a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> index 6965afa..9b1b199 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> @@ -91,6 +91,7 @@ void dp_catalog_ctrl_state_ctrl(struct dp_catalog
>> *dp_catalog, u32 state); void dp_catalog_ctrl_config_ctrl(struct
>> dp_catalog *dp_catalog, u32 config); void
>> dp_catalog_ctrl_lane_mapping(struct dp_catalog *dp_catalog); void
>> dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog, bool
>> enable);
>> +void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog
>> +*dp_catalog, bool enable);
>> void dp_catalog_ctrl_config_misc(struct dp_catalog *dp_catalog, u32
>> cc, u32 tb); void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog,
>u32 rate,
>> u32 stream_rate_khz, bool fixed_nvid);
>> @@ -101,12 +102,15 @@ void dp_catalog_ctrl_enable_irq(struct
>> dp_catalog *dp_catalog, bool enable); void
>dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
>> u32 intr_mask, bool en); void
>> dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog);
>> +void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog); void
>> +dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter);
>> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog);
>> u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog);
>> void dp_catalog_ctrl_phy_reset(struct dp_catalog *dp_catalog); int
>> dp_catalog_ctrl_update_vx_px(struct dp_catalog *dp_catalog, u8 v_level,
>> u8 p_level); int
>> dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog);
>> +int dp_catalog_ctrl_get_psr_interrupt(struct dp_catalog *dp_catalog);
>> void dp_catalog_ctrl_update_transfer_unit(struct dp_catalog *dp_catalog,
>> u32 dp_tu, u32 valid_boundary,
>> u32 valid_boundary2); diff --git
>> a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 88ca6c3..ba828ea 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -21,6 +21,7 @@
>>
>> #define DP_KHZ_TO_HZ 1000
>> #define IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES (30 * HZ /
>1000) /* 30 ms */
>> +#define PSR_OPERATION_COMPLETION_TIMEOUT_JIFFIES (300 * HZ /
>1000) /* 300 ms */
>> #define WAIT_FOR_VIDEO_READY_TIMEOUT_JIFFIES (HZ / 2)
>>
>> #define DP_CTRL_INTR_READY_FOR_VIDEO BIT(0)
>> @@ -78,6 +79,7 @@ struct dp_ctrl_private {
>> struct dp_catalog *catalog;
>>
>> struct completion idle_comp;
>> + struct completion psr_op_comp;
>> struct completion video_comp;
>> };
>>
>> @@ -151,6 +153,9 @@ static void dp_ctrl_config_ctrl(struct dp_ctrl_private
>*ctrl)
>> config |= DP_CONFIGURATION_CTRL_STATIC_DYNAMIC_CN;
>> config |= DP_CONFIGURATION_CTRL_SYNC_ASYNC_CLK;
>>
>> + if (ctrl->panel->psr_cap.version)
>> + config |= DP_CONFIGURATION_CTRL_SEND_VSC;
>> +
>> dp_catalog_ctrl_config_ctrl(ctrl->catalog, config); }
>>
>> @@ -1365,6 +1370,52 @@ static int dp_ctrl_enable_stream_clocks(struct
>dp_ctrl_private *ctrl)
>> return ret;
>> }
>>
>> +void dp_ctrl_config_psr(struct dp_ctrl *dp_ctrl) {
>> + struct dp_ctrl_private *ctrl;
>> + u8 psr_config;
>> +
>> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> +
>> + if (!ctrl->panel->psr_cap.version)
>> + return;
>> +
>> + dp_catalog_ctrl_config_psr(ctrl->catalog);
>> +
>> + psr_config = DP_PSR_ENABLE;
>> + drm_dp_dpcd_write(ctrl->aux, DP_PSR_EN_CFG, &psr_config, 1); }
>> +
>> +void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enter) {
>> + struct dp_ctrl_private *ctrl;
>> +
>> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>> +
>> + if (!ctrl->panel->psr_cap.version)
>> + return;
>> +
>> + 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);
>> + }
>> +}
>> +
>> int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
>> {
>> struct dp_ctrl_private *ctrl;
>> @@ -1964,6 +2015,17 @@ void dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
>>
>> ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>>
>> + if (ctrl->panel->psr_cap.version) {
>> + isr =
>> + dp_catalog_ctrl_get_psr_interrupt(ctrl->catalog);
>> +
>> + if (isr == 0x1)
>> + DRM_DEBUG_DP("PSR frame update done\n");
>> + else if (isr == 0x10)
>> + DRM_DEBUG_DP("PSR exit done\n");
>> +
>> + complete(&ctrl->psr_op_comp);
>> + }
>> +
>> isr = dp_catalog_ctrl_get_interrupt(ctrl->catalog);
>>
>> if (isr & DP_CTRL_INTR_READY_FOR_VIDEO) { @@ -2010,6 +2072,7
>> @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
>> dev_err(dev, "failed to add DP OPP table\n");
>>
>> init_completion(&ctrl->idle_comp);
>> + init_completion(&ctrl->psr_op_comp);
>> init_completion(&ctrl->video_comp);
>>
>> /* in parameters */
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> index 2363a2d..f623035 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> @@ -34,4 +34,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct
>dp_link *link,
>> struct dp_power *power, struct dp_catalog *catalog,
>> struct dp_parser *parser);
>>
>> +void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enable); void
>> +dp_ctrl_config_psr(struct dp_ctrl *dp_ctrl);
>> +
>> #endif /* _DP_CTRL_H_ */
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 5d314e6..c8d02fb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -347,6 +347,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;
>> +
>> dp->audio_supported = drm_detect_monitor_audio(edid);
>> dp_panel_handle_sink_request(dp->panel);
>>
>> @@ -871,6 +873,10 @@ static int dp_display_post_enable(struct msm_dp
>> *dp_display)
>>
>> /* signal the connect event late to synchronize video and display */
>> dp_display_handle_plugged_change(dp_display, true);
>> +
>> + if (dp_display->psr_supported)
>> + dp_ctrl_config_psr(dp->ctrl);
>> +
>> return 0;
>> }
>>
>> @@ -1037,6 +1043,14 @@ static void dp_display_config_hpd(struct
>dp_display_private *dp)
>> enable_irq(dp->irq);
>> }
>>
>> +void msm_dp_display_set_psr(struct msm_dp *dp_display, bool enter)
>
>If this function is used only by the dp code, there is no need to pollute the
>msm_dp_* namespace.
>Please rename to dp_display_set_psr() and remove declaration from
>msm_drv.h.
>
Okay. Will change it.
>> +{
>> + struct dp_display_private *dp;
>> +
>> + dp = container_of(dp_display, struct dp_display_private, dp_display);
>> + dp_ctrl_set_psr(dp->ctrl, enter); }
>> +
>> static int hpd_event_thread(void *data) {
>> struct dp_display_private *dp_priv; diff --git
>> a/drivers/gpu/drm/msm/dp/dp_display.h
>> b/drivers/gpu/drm/msm/dp/dp_display.h
>> index e3adcd5..6f512f3 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -28,6 +28,7 @@ struct msm_dp {
>>
>> u32 max_dp_lanes;
>> struct dp_audio *dp_audio;
>> + bool psr_supported;
>> };
>>
>> int dp_display_set_plugged_cb(struct msm_dp *dp_display, diff --git
>> a/drivers/gpu/drm/msm/dp/dp_drm.c
>b/drivers/gpu/drm/msm/dp/dp_drm.c
>> index 2436329..d26ca6a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>> @@ -142,6 +142,35 @@ static enum drm_mode_status
>edp_connector_mode_valid(
>> return MODE_OK;
>> }
>>
>> +
>> +
>
>Extra empty lines.
>
Will remove it
>> +static int edp_connector_atomic_check(struct drm_connector *connector,
>> + struct drm_atomic_state *state) {
>> + struct msm_dp *dp;
>> + struct drm_connector_state *conn_state;
>> + struct drm_crtc_state *crtc_state;
>> +
>> + dp = to_dp_connector(connector)->dp_display;
>> + conn_state = drm_atomic_get_new_connector_state(state,
>connector);
>> + if (WARN_ON(!conn_state))
>> + return -ENODEV;
>> +
>> + conn_state->self_refresh_aware = true;
>
>I see that analogix bridge does the same. However I think it would be better
>to set self_refresh_aware to true only if the actual attached panel supports
>PSR.
>
Okay. Will add that check.
>> +
>> + if (!conn_state->crtc)
>> + return 0;
>> +
>> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state-
>>crtc);
>> + if (!crtc_state)
>> + return 0;
>> +
>> + if (crtc_state->self_refresh_active && !dp->psr_supported)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> static const struct drm_connector_funcs dp_connector_funcs = {
>> .detect = dp_connector_detect,
>> .fill_modes = drm_helper_probe_single_connector_modes,
>> @@ -151,6 +180,11 @@ static const struct drm_connector_funcs
>dp_connector_funcs = {
>> .atomic_destroy_state =
>> drm_atomic_helper_connector_destroy_state,
>> };
>>
>> +static const struct drm_connector_helper_funcs
>dp_connector_helper_funcs = {
>> + .get_modes = dp_connector_get_modes,
>> + .mode_valid = dp_connector_mode_valid, };
>> +
>
>Unnecessary movement.
>
This will be removed.
>> static const struct drm_connector_funcs edp_connector_funcs = {
>> .fill_modes = drm_helper_probe_single_connector_modes,
>> .destroy = drm_connector_cleanup, @@ -159,12 +193,8 @@ static
>> const struct drm_connector_funcs edp_connector_funcs = {
>> .atomic_destroy_state =
>> drm_atomic_helper_connector_destroy_state,
>> };
>>
>> -static const struct drm_connector_helper_funcs
>dp_connector_helper_funcs = {
>> - .get_modes = dp_connector_get_modes,
>> - .mode_valid = dp_connector_mode_valid,
>> -};
>> -
>> static const struct drm_connector_helper_funcs
>> edp_connector_helper_funcs = {
>> + .atomic_check = edp_connector_atomic_check,
>> .get_modes = edp_connector_get_modes,
>> .mode_valid = edp_connector_mode_valid, }; @@ -258,6 +288,130
>> @@ static void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
>> msm_dp_display_disable(dp_display, drm_bridge->encoder); }
>>
>> +static struct drm_crtc *dp_bridge_get_old_connector_crtc(struct msm_dp
>*dp,
>> + struct drm_atomic_state
>> +*state) {
>> + struct drm_encoder *encoder = dp->encoder;
>> + struct drm_connector *connector;
>> + struct drm_connector_state *conn_state;
>> +
>> + connector = drm_atomic_get_old_connector_for_encoder(state,
>encoder);
>> + if (!connector)
>> + return NULL;
>> +
>> + conn_state = drm_atomic_get_old_connector_state(state, connector);
>> + if (!conn_state)
>> + return NULL;
>> +
>> + return conn_state->crtc;
>> +}
>> +
>> +static struct drm_crtc *dp_bridge_get_new_connector_crtc(struct
>msm_dp *dp,
>> + struct drm_atomic_state
>> +*state) {
>> + struct drm_encoder *encoder = dp->encoder;
>> + struct drm_connector *connector;
>> + struct drm_connector_state *conn_state;
>> +
>> + connector = drm_atomic_get_new_connector_for_encoder(state,
>encoder);
>> + if (!connector)
>> + return NULL;
>> +
>> + conn_state = drm_atomic_get_new_connector_state(state,
>connector);
>> + if (!conn_state)
>> + return NULL;
>> +
>> + return conn_state->crtc;
>> +}
>
>This code being repeated over and over again deserves to become a new
>atomic helper.
>Please move it to drm_atomic.c or .h
>
It is being done in the same way for the analogix driver also. Moving this logic to a different place will need changes in other chipset drivers also. This needs to be a separate effort.
>> +
>> +static void edp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>> + struct drm_bridge_state
>> +*old_bridge_state) {
>> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> + struct drm_crtc *crtc;
>> + struct drm_crtc_state *old_crtc_state;
>> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
>> + struct msm_dp *dp_display = dp_bridge->dp_display;
>> +
>> + crtc = dp_bridge_get_new_connector_crtc(dp_display, old_state);
>> + if (!crtc)
>> + return;
>> +
>> + old_crtc_state = drm_atomic_get_old_crtc_state(old_state,
>> + crtc);
>
>Generic remark to this and the following functions. I don't quite like the idea
>of the bridge drivers poking into the crtc state. This looks like a breach of
>layering.
>The code looks similar to analogix'es one. Maybe we should consider moving it
>into common helpers? This would require adding enable/disable_srr as
>drm_bridge ops.
>Or to introduce drm_bridge_helper_funcs.
>
This sounds like a good option. Implementing this suggestion will require changes in the drivers of other chipsets and needs to be a separate effort.
>> +
>> + /* Exit from self refresh mode */
>> + if (old_crtc_state && old_crtc_state->self_refresh_active) {
>> + msm_dp_display_set_psr(dp_display, false);
>> + return;
>> + }
>> +
>> + msm_dp_display_enable(dp_display, drm_bridge->encoder); }
>> +
>> +static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
>> + struct drm_bridge_state
>> +*old_bridge_state) {
>> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> + struct drm_crtc *crtc;
>> + struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state =
>NULL;
>> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
>> + struct msm_dp *dp_display = dp_bridge->dp_display;
>> +
>> + crtc = dp_bridge_get_old_connector_crtc(dp_display, old_state);
>> + if (!crtc)
>> + goto out;
>> +
>> + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
>> + if (!new_crtc_state)
>> + goto out;
>> +
>> + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
>> + if (!old_crtc_state)
>> + goto out;
>> +
>> + /*
>> + * Set self refresh mode if current crtc state is active.
>> + * If old crtc state is active, exit psr before disabling
>> + * the controller. Observed sink stuck in self refresh
>> + * if psr exit is skipped when screen off occurs with
>> + * sink in psr mode.
>> + */
>> + if (new_crtc_state->self_refresh_active) {
>> + msm_dp_display_set_psr(dp_display, true);
>> + return;
>> + } else if (old_crtc_state->self_refresh_active) {
>> + msm_dp_display_set_psr(dp_display, false);
>> + return;
>
>I think the return here is wrong. Where do we support the case when the old
>state was the PSR, but the new states tells us to disable the crtc? If I
>understand the drm code correctly, it's a legit case.
>
The edp_bridge_atomic_post_disable will handle it. We have to exit PSR in that case and gracefully turn off the display interface.
} else if (old_crtc_state->self_refresh_active) {
msm_dp_display_set_psr(dp_display, false);
return;
The above code will exit PSR in edp_bridge_atomic_disable. The display interface off is done in edp_bridge_atomic_post_disable
>> + }
>> +
>> +out:
>> + msm_dp_display_pre_disable(dp_display, drm_bridge->encoder); }
>> +
>> +static void edp_bridge_atomic_post_disable(struct drm_bridge
>*drm_bridge,
>> + struct drm_bridge_state
>> +*old_bridge_state) {
>> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> + struct drm_crtc *crtc;
>> + struct drm_crtc_state *new_crtc_state = NULL;
>> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
>> + struct msm_dp *dp_display = dp_bridge->dp_display;
>> +
>> + crtc = dp_bridge_get_old_connector_crtc(dp_display, old_state);
>> + if (!crtc)
>> + return;
>> +
>> + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
>> + if (!new_crtc_state)
>> + return;
>> +
>> + /* Self refresh mode is set in dp_bridge_disable. Skip disable
>> + */
>
>edp_bridge_atomic_disable?
>
Yes, will put the correct function in the comment.
>> + if (new_crtc_state->self_refresh_active)
>> + return;
>> +
>> + msm_dp_display_disable(dp_display, drm_bridge->encoder); }
>> +
>> static const struct drm_bridge_funcs dp_bridge_ops = {
>> .enable = dp_bridge_enable,
>> .disable = dp_bridge_disable,
>> @@ -265,6 +419,16 @@ static const struct drm_bridge_funcs
>dp_bridge_ops = {
>> .mode_set = dp_bridge_mode_set,
>> };
>>
>> +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,
>> + .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,
>> +};
>> +
>> struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display,
>struct drm_device *dev,
>> struct drm_encoder *encoder) { @@ -279,7
>> +443,8 @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp
>*dp_display, struct drm_devi
>> dp_bridge->dp_display = dp_display;
>>
>> bridge = &dp_bridge->bridge;
>> - bridge->funcs = &dp_bridge_ops;
>> + bridge->funcs = (dp_display->connector_type ==
>DRM_MODE_CONNECTOR_eDP) ?
>> + &edp_bridge_ops : &dp_bridge_ops;
>> bridge->encoder = encoder;
>>
>> rc = drm_bridge_attach(encoder, bridge, NULL,
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR); diff --git
>> a/drivers/gpu/drm/msm/dp/dp_link.c
>b/drivers/gpu/drm/msm/dp/dp_link.c
>> index d4d31e5..5503c29 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_link.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
>> @@ -924,6 +924,26 @@ static int
>dp_link_process_phy_test_pattern_request(
>> return 0;
>> }
>>
>> +static int dp_link_psr_status(struct dp_link_private *link) {
>> + u8 status[2];
>> +
>> + drm_dp_dpcd_read(link->aux, DP_PSR_ERROR_STATUS, status, 2);
>> +
>> + if (status[0] & DP_PSR_LINK_CRC_ERROR)
>> + DRM_ERROR("PSR LINK CRC ERROR\n");
>> + else if (status[0] & DP_PSR_RFB_STORAGE_ERROR)
>> + DRM_ERROR("PSR RFB STORAGE ERROR\n");
>> + else if (status[0] & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
>> + DRM_ERROR("PSR VSC SDP UNCORRECTABLE ERROR\n");
>> + else if (status[1] & DP_PSR_CAPS_CHANGE)
>> + DRM_INFO("PSR Capability Change\n");
>
>DRM_DEBUG_DP
>
Will change it
>> + else
>> + return 0;
>> +
>> + return 1;
>> +}
>> +
>> static u8 get_link_status(const u8 link_status[DP_LINK_STATUS_SIZE],
>> int r) {
>> return link_status[r - DP_LANE0_1_STATUS]; @@ -1042,6 +1062,8
>> @@ int dp_link_process_request(struct dp_link *dp_link)
>> dp_link->sink_request |= DP_TEST_LINK_TRAINING;
>> } else if (!dp_link_process_phy_test_pattern_request(link)) {
>> dp_link->sink_request |=
>> DP_TEST_LINK_PHY_TEST_PATTERN;
>> + } else if (dp_link_psr_status(link)) {
>> + DRM_INFO("PSR IRQ_HPD received\n");
>
>DRM_DEBUG_DP
>
Will change it
>> } else {
>> ret = dp_link_process_link_status_update(link);
>> if (!ret) {
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>> index 71db10c..e128d73 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>> @@ -19,6 +19,26 @@ struct dp_panel_private {
>> bool aux_cfg_update_done;
>> };
>>
>> +static void dp_panel_read_psr_cap(struct dp_panel_private *panel) {
>> + ssize_t rlen;
>> + struct dp_panel *dp_panel;
>> +
>> + dp_panel = &panel->dp_panel;
>> +
>> + /* edp sink */
>> + if (dp_panel->dpcd[DP_EDP_CONFIGURATION_CAP]) {
>> + rlen = drm_dp_dpcd_read(panel->aux, DP_PSR_SUPPORT,
>> + &dp_panel->psr_cap, 2);
>> + if (rlen == 2) {
>> + DRM_DEBUG_DP("psr version: 0x%x, psr_cap: 0x%x\n",
>> + dp_panel->psr_cap.version,
>> + dp_panel->psr_cap.capabilities);
>> + } else
>> + DRM_ERROR("failed to read psr info, rlen=%zd\n", rlen);
>> + }
>> +}
>> +
>> static int dp_panel_read_dpcd(struct dp_panel *dp_panel) {
>> int rc = 0;
>> @@ -104,6 +124,7 @@ static int dp_panel_read_dpcd(struct dp_panel
>*dp_panel)
>> }
>> }
>>
>> + dp_panel_read_psr_cap(panel);
>> end:
>> return rc;
>> }
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
>> b/drivers/gpu/drm/msm/dp/dp_panel.h
>> index 9023e5b..631657a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>> @@ -34,6 +34,11 @@ struct dp_panel_in {
>> struct dp_catalog *catalog;
>> };
>>
>> +struct dp_panel_psr {
>> + u8 version;
>> + u8 capabilities;
>> +};
>> +
>> struct dp_panel {
>> /* dpcd raw data */
>> u8 dpcd[DP_RECEIVER_CAP_SIZE + 1]; @@ -46,6 +51,7 @@ struct
>> dp_panel {
>> struct edid *edid;
>> struct drm_connector *connector;
>> struct dp_display_mode dp_mode;
>> + struct dp_panel_psr psr_cap;
>> bool video_test;
>>
>> u32 vic;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h
>> b/drivers/gpu/drm/msm/dp/dp_reg.h index 2686028..7a0b052 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_reg.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_reg.h
>> @@ -22,6 +22,20 @@
>> #define REG_DP_INTR_STATUS2 (0x00000024)
>> #define REG_DP_INTR_STATUS3 (0x00000028)
>>
>> +#define REG_DP_INTR_STATUS4 (0x0000002C)
>> +#define PSR_UPDATE_INT (0x00000001)
>> +#define PSR_CAPTURE_INT (0x00000004)
>> +#define PSR_EXIT_INT (0x00000010)
>> +#define PSR_UPDATE_ERROR_INT (0x00000040)
>> +#define PSR_WAKE_ERROR_INT (0x00000100)
>> +
>> +#define REG_DP_INTR_MASK4 (0x00000030)
>> +#define PSR_UPDATE_MASK (0x00000001)
>> +#define PSR_CAPTURE_MASK (0x00000002)
>> +#define PSR_EXIT_MASK (0x00000004)
>> +#define PSR_UPDATE_ERROR_MASK (0x00000008)
>> +#define PSR_WAKE_ERROR_MASK (0x00000010)
>> +
>> #define REG_DP_DP_HPD_CTRL (0x00000000)
>> #define DP_DP_HPD_CTRL_HPD_EN (0x00000001)
>>
>> @@ -164,6 +178,9 @@
>> #define MMSS_DP_AUDIO_TIMING_RBR_48 (0x00000094)
>> #define MMSS_DP_AUDIO_TIMING_HBR_48 (0x00000098)
>>
>> +#define REG_PSR_CONFIG (0x00000100)
>> +#define REG_PSR_CMD (0x00000110)
>> +
>> #define MMSS_DP_PSR_CRC_RG (0x00000154)
>> #define MMSS_DP_PSR_CRC_B (0x00000158)
>>
>> @@ -184,6 +201,8 @@
>> #define MMSS_DP_AUDIO_STREAM_0 (0x00000240)
>> #define MMSS_DP_AUDIO_STREAM_1 (0x00000244)
>>
>> +#define MMSS_DP_SDP_CFG3 (0x0000024c)
>> +
>> #define MMSS_DP_EXTENSION_0 (0x00000250)
>> #define MMSS_DP_EXTENSION_1 (0x00000254)
>> #define MMSS_DP_EXTENSION_2 (0x00000258)
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h
>> b/drivers/gpu/drm/msm/msm_drv.h index ae52412..254fd07 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -400,6 +400,8 @@ void msm_dp_snapshot(struct msm_disp_state
>> *disp_state, struct msm_dp *dp_displa
>>
>> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor
>> *minor);
>>
>> +void msm_dp_display_set_psr(struct msm_dp *dp, bool enter);
>> +
>> #else
>> static inline int __init msm_dp_register(void) { @@ -449,6 +451,10
>> @@ static inline void msm_dp_debugfs_init(struct msm_dp *dp_display,
>> { }
>>
>> +static inline void msm_dp_display_set_psr(struct msm_dp *dp, bool
>> +enter) { }
>> +
>> #endif
>>
>> void __init msm_mdp_register(void);
>> --
>> 2.7.4
>>
>
>
>--
>With best wishes
>Dmitry
Thank you,
Sankeerth
> -----Original Message-----
> From: Stephen Boyd <[email protected]>
> Sent: Wednesday, February 23, 2022 2:59 AM
> To: quic_vpolimer <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; dri-
> [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Cc: quic_kalyant <[email protected]>; Sankeerth Billakanti (QUIC)
> <[email protected]>; quic_vproddut <[email protected]>
> Subject: Re: [PATCH v2 4/4] drm/msm/disp/dpu1: add PSR support for eDP
> interface in dpu driver
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> Quoting Vinod Polimera (2022-02-21 06:51:26)
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index e7c9fe1..ba3240c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -951,6 +952,14 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> >
> > DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
> >
> > + if (old_crtc_state->self_refresh_active) {
> > + drm_for_each_encoder_mask(encoder, crtc->dev,
> > + old_crtc_state->encoder_mask) {
> > + dpu_encoder_assign_crtc(encoder, NULL);
> > + }
> > + return;
> > + }
> > +
> > /* Disable/save vblank irq handling */
> > drm_crtc_vblank_off(crtc);
> >
> > @@ -962,7 +971,12 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> > */
> > if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
> > release_bandwidth = true;
> > - dpu_encoder_assign_crtc(encoder, NULL);
> > + /* If disable is triggered during psr active(e.g: screen dim in PSR),
>
> Multiline comments start with /* on a line by itself
>
> /*
> * If disable is triggered ...
>
> > + * we will need encoder->crtc connection to process the device
> sleep &
> > + * preserve it during psr sequence.
> > + */
> > + if (!crtc->state->self_refresh_active)
> > + dpu_encoder_assign_crtc(encoder, NULL);
> > }
> >
> > /* wait for frame_event_done completion */
> > @@ -1010,6 +1024,8 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
> > struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> > struct drm_encoder *encoder;
> > bool request_bandwidth = false;
> > + struct drm_crtc_state *old_crtc_state =
> > + drm_atomic_get_old_crtc_state(state, crtc);
>
> Use two lines
>
> struct drm_crtc_state *old_crtc_state;
>
> old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
>
> >
> > pm_runtime_get_sync(crtc->dev->dev);
> >
> > @@ -1032,8 +1048,10 @@ static void dpu_crtc_enable(struct drm_crtc
> *crtc,
> > trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
> > dpu_crtc->enabled = true;
> >
> > - drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state-
> >encoder_mask)
> > - dpu_encoder_assign_crtc(encoder, crtc);
> > + if (!old_crtc_state->self_refresh_active) {
> > + drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state-
> >encoder_mask)
> > + dpu_encoder_assign_crtc(encoder, crtc);
>
> Make these two lines above into a function with a meaningful name?
> dpu_encoder_assign_crtcs()? And then push the encoder mask iteration
> into the loop by passing the mask as a function argument. I see
> dpu_encoder_assign_crtc() takes a spinlock, so we could probably take
> that lock out too and push it into this new function to avoid grabbing
> and dropping the spinlock multiple times.
enc spinlock is specific to each dpu_enc which will grab the lock for each crtc assign.
>
> > + }
> >
> > /* Enable/restore vblank irq handling */
> > drm_crtc_vblank_on(crtc);
> > @@ -1497,7 +1515,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device
> *dev, struct drm_plane *plane,
> > {
> > struct drm_crtc *crtc = NULL;
> > struct dpu_crtc *dpu_crtc = NULL;
> > - int i;
> > + int i, ret;
> >
> > dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL);
> > if (!dpu_crtc)
> > @@ -1534,6 +1552,11 @@ struct drm_crtc *dpu_crtc_init(struct
> drm_device *dev, struct drm_plane *plane,
> > /* initialize event handling */
> > spin_lock_init(&dpu_crtc->event_lock);
> >
> > + ret = drm_self_refresh_helper_init(crtc);
> > + if (ret)
> > + DPU_ERROR("Failed to initialize %s with SR helpers %d\n",
>
> What is SR? Write self-refresh?
>
> > + crtc->name, ret);
> > +
> > DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc-
> >name);
> > return crtc;
> > }
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 6eac417..ba9d8ea 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -217,6 +217,14 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
> > 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
> > };
> >
> > +static inline bool is_self_refresh_active(struct drm_crtc_state *state)
>
> const drm_crtc_state?
>
> > +{
> > + if (state && state->self_refresh_active)
> > + return true;
> > +
> > + return false;
>
> return state && state->self_refresh_active;
>
> > +}
> > +
> > static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong
> *hw_pp, unsigned bpc)
> > {
> > struct dpu_hw_dither_cfg dither_cfg = { 0 };
> > @@ -629,7 +637,8 @@ static int dpu_encoder_virt_atomic_check(
> > if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> > dpu_rm_release(global_state, drm_enc);
> >
> > - if (!crtc_state->active_changed || crtc_state->active)
> > + if (!crtc_state->active_changed || crtc_state->active ||
> > + crtc_state->self_refresh_active)
> > ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> > drm_enc, crtc_state, topology);
> > }
> > @@ -1182,11 +1191,30 @@ static void dpu_encoder_virt_disable(struct
> drm_encoder *drm_enc,
> > {
> > struct dpu_encoder_virt *dpu_enc = NULL;
> > struct msm_drm_private *priv;
> > + struct drm_crtc *crtc;
> > + struct drm_crtc_state *old_state;
> > int i = 0;
> >
> > dpu_enc = to_dpu_encoder_virt(drm_enc);
> > DPU_DEBUG_ENC(dpu_enc, "\n");
>
> Presumably this print wants a valid 'dpu_enc' pointer.
>
> >
> > + if (!drm_enc) {
>
> So this check for !drm_enc is impossible? Please remove it.
>
> > + DPU_ERROR("invalid encoder\n");
> > + return;
> > + }
> > + dpu_enc = to_dpu_encoder_virt(drm_enc);
>
> We got it again?
>
> > +
> > + crtc = dpu_enc->crtc;
> > +
> > + old_state = drm_atomic_get_old_crtc_state(state, crtc);
> > +
> > + /*
> > + * The encoder turn off already occurred when self refresh mode
>
> s/turn off/disable/
>
> > + * was set earlier, in the old_state for the corresponding crtc.
> > + */
> > + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS &&
> is_self_refresh_active(old_state))
> > + return;
> > +
> > mutex_lock(&dpu_enc->enc_lock);
> > dpu_enc->enabled = false;
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > index 47fe11a..d550f90 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -495,7 +495,7 @@ static void dpu_kms_wait_for_commit_done(struct
> msm_kms *kms,
> > return;
> > }
> >
> > - if (!crtc->state->active) {
> > + if (!drm_atomic_crtc_effectively_active(crtc->state)) {
> > DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id);
> > return;
> > }
> > --
> > 2.7.4
> >
Hi Bjorn,
>> 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 <[email protected]>
>>
>> Changes in v2:
>> - Use dp bridge to set psr entry/exit instead of dpu_enocder
>> - Don't modify whitespaces
>> - set self refresh aware from atomic_check
>> - set self refresh aware only if psr is supported
>> - provide a stub for msm_dp_display_set_psr
>> ---
>> 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 | 63 +++++++++++++
>> 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 | 1 +
>> drivers/gpu/drm/msm/dp/dp_drm.c | 177
>++++++++++++++++++++++++++++++++++--
>> drivers/gpu/drm/msm/dp/dp_link.c | 22 +++++
>> drivers/gpu/drm/msm/dp/dp_panel.c | 21 +++++
>> drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++
>> drivers/gpu/drm/msm/dp/dp_reg.h | 19 ++++
>> drivers/gpu/drm/msm/msm_drv.h | 6 ++
>> 12 files changed, 411 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 8a6d3ea..3cd223d 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -45,6 +45,14 @@
>> #define DP_INTERRUPT_STATUS2_MASK \
>> (DP_INTERRUPT_STATUS2 << DP_INTERRUPT_STATUS_MASK_SHIFT)
>>
>> +#define DP_INTERRUPT_STATUS4 \
>
>Is this group of interrupt bits really called "status4", what is 4?
>Seems more like the "PSR interrupt status bits" to me.
>
The name of the interrupt itself is DP_INTERRUPT_STATUS4
It has PSR status bits. I defined them in dp_reg.h
>> + (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 dp_io *io;
>> @@ -343,6 +351,20 @@ 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 mainlink_ctrl;
>> + struct dp_catalog_private *catalog = container_of(dp_catalog,
>> + struct dp_catalog_private, dp_catalog);
>> +
>> + mainlink_ctrl = dp_read_link(catalog, REG_DP_MAINLINK_CTRL);
>> + mainlink_ctrl &= ~DP_MAINLINK_CTRL_ENABLE;
>> + mainlink_ctrl |= (enable & DP_MAINLINK_CTRL_ENABLE);
>
>Masking a boolean with constant is different...Please don't do that.
>
>if (enable)
> mainlink_ctrl |= DP_MAINLINK_CTRL_ENABLE; else
> mainlink_ctrl &= ~DP_MAINLINK_CTRL_ENABLE;
>
>Is way less magical.
>
>
>Not also, that you don't have to name your "only" local variable
>"mainline_ctrl", there's no risk for collisions with anything else.
>
>Name it "val" or "ctrl".
>
Okay. Changed it.
>> +
>> + dp_write_link(catalog, REG_DP_MAINLINK_CTRL, mainlink_ctrl); }
>> +
>> void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog,
>> bool enable) { @@
>> -581,6 +603,51 @@ 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, 0x1);
>> + dp_write_link(catalog, MMSS_DP_SDP_CFG3, 0x0);
>
>Can 1 and 0 be defined here?
>
Okay
>> +}
>> +
>> +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 psr_config;
>
>Again, a single local variable. Name it "config".
>
Done
>> +
>> + /* enable PSR1 function */
>> + psr_config = dp_read_link(catalog, REG_PSR_CONFIG);
>> + psr_config |= BIT(0);
>
>Add a define for BIT(0) in this register and the code will be self-explanatory,
>no need for a separate comment to help future readers to guess that BIT(0)
>enables something...
>
Okay
>> + dp_write_link(catalog, REG_PSR_CONFIG, psr_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 psr_cmd;
>
>"cmd" is sufficient.
>
Okay
>> +
>> + psr_cmd = dp_read_link(catalog, REG_PSR_CMD);
>> +
>> + /*
>> + * BIT(0) - send psr entry SDP
>> + * BIT(1) - sned psr exit SDP
>> + */
>> + psr_cmd &= ~(BIT(0) | BIT(1));
>> +
>> + if (enter)
>> + psr_cmd |= BIT(0);
>> + else
>> + psr_cmd |= BIT(1);
>
>As above, defines for BIT(0) and BIT(1), and drop the comment, please.
>
Okay. Will do it
>> +
>> + dp_catalog_enable_sdp(catalog);
>> + dp_write_link(catalog, REG_PSR_CMD, psr_cmd); }
>> +
>> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog) {
>> struct dp_catalog_private *catalog = container_of(dp_catalog, @@
>> -608,6 +675,20 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog
>*dp_catalog)
>> return isr;
>> }
>>
>> +int dp_catalog_ctrl_get_psr_interrupt(struct dp_catalog *dp_catalog)
>
>"Getting an interrupt" is something we do with e.g. platform_get_irq().
>
>dp_catalog_ctrl_read_psr_interrupt_status() would better represent the
>purpose of this function.
>
Will change it
>> +{
>> + 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 a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> index 6965afa..9b1b199 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
>> @@ -91,6 +91,7 @@ void dp_catalog_ctrl_state_ctrl(struct dp_catalog
>> *dp_catalog, u32 state); void dp_catalog_ctrl_config_ctrl(struct
>> dp_catalog *dp_catalog, u32 config); void
>> dp_catalog_ctrl_lane_mapping(struct dp_catalog *dp_catalog); void
>> dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog, bool
>> enable);
>> +void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog
>> +*dp_catalog, bool enable);
>> void dp_catalog_ctrl_config_misc(struct dp_catalog *dp_catalog, u32
>> cc, u32 tb); void dp_catalog_ctrl_config_msa(struct dp_catalog *dp_catalog,
>u32 rate,
>> u32 stream_rate_khz, bool fixed_nvid);
>> @@ -101,12 +102,15 @@ void dp_catalog_ctrl_enable_irq(struct
>> dp_catalog *dp_catalog, bool enable); void
>dp_catalog_hpd_config_intr(struct dp_catalog *dp_catalog,
>> u32 intr_mask, bool en); void
>> dp_catalog_ctrl_hpd_config(struct dp_catalog *dp_catalog);
>> +void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog); void
>> +dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool enter);
>> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog);
>> u32 dp_catalog_hpd_get_intr_status(struct dp_catalog *dp_catalog);
>> void dp_catalog_ctrl_phy_reset(struct dp_catalog *dp_catalog); int
>> dp_catalog_ctrl_update_vx_px(struct dp_catalog *dp_catalog, u8 v_level,
>> u8 p_level); int
>> dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog);
>> +int dp_catalog_ctrl_get_psr_interrupt(struct dp_catalog *dp_catalog);
>> void dp_catalog_ctrl_update_transfer_unit(struct dp_catalog *dp_catalog,
>> u32 dp_tu, u32 valid_boundary,
>> u32 valid_boundary2); diff --git
>> a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> index 88ca6c3..ba828ea 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
>> @@ -21,6 +21,7 @@
>>
>> #define DP_KHZ_TO_HZ 1000
>> #define IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES (30 * HZ / 1000)
>/* 30 ms */
>> +#define PSR_OPERATION_COMPLETION_TIMEOUT_JIFFIES (300 * HZ /
>1000) /* 300 ms */
>> #define WAIT_FOR_VIDEO_READY_TIMEOUT_JIFFIES (HZ / 2)
>>
>> #define DP_CTRL_INTR_READY_FOR_VIDEO BIT(0)
>> @@ -78,6 +79,7 @@ struct dp_ctrl_private {
>> struct dp_catalog *catalog;
>>
>> struct completion idle_comp;
>> + struct completion psr_op_comp;
>> struct completion video_comp;
>> };
>>
>> @@ -151,6 +153,9 @@ static void dp_ctrl_config_ctrl(struct dp_ctrl_private
>*ctrl)
>> config |= DP_CONFIGURATION_CTRL_STATIC_DYNAMIC_CN;
>> config |= DP_CONFIGURATION_CTRL_SYNC_ASYNC_CLK;
>>
>> + if (ctrl->panel->psr_cap.version)
>> + config |= DP_CONFIGURATION_CTRL_SEND_VSC;
>> +
>> dp_catalog_ctrl_config_ctrl(ctrl->catalog, config); }
>>
>> @@ -1365,6 +1370,52 @@ static int dp_ctrl_enable_stream_clocks(struct
>dp_ctrl_private *ctrl)
>> return ret;
>> }
>>
>> +void dp_ctrl_config_psr(struct dp_ctrl *dp_ctrl) {
>> + struct dp_ctrl_private *ctrl;
>> + u8 psr_config;
>
>"config" - or "cfg' per the register name.
>
Okay
>> +
>> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>
>Join this with the declaration of ctrl.
>
Okay
>> +
>> + if (!ctrl->panel->psr_cap.version)
>> + return;
>> +
>> + dp_catalog_ctrl_config_psr(ctrl->catalog);
>> +
>> + psr_config = DP_PSR_ENABLE;
>> + drm_dp_dpcd_write(ctrl->aux, DP_PSR_EN_CFG, &psr_config, 1); }
>> +
>> +void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enter) {
>> + struct dp_ctrl_private *ctrl;
>> +
>> + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>
>As above, please join this with the previous line.
>
Done
>> +
>> + if (!ctrl->panel->psr_cap.version)
>> + return;
>> +
>> + 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);
>
>So to enter psr we disable psr_mainline...
>
>> + } else {
>> + dp_catalog_ctrl_psr_mainlink_enable(ctrl->catalog,
>> + true);
>
>and to exit psr we enable psr_mainline?
>
>I certainly would not mind having a comment above this section describing
>how this is supposed to work.
>
Added the comments in the new version
>> +
>> + dp_catalog_ctrl_set_psr(ctrl->catalog, false);
>> + }
>> +}
>> +
>> int dp_ctrl_host_init(struct dp_ctrl *dp_ctrl, bool flip, bool reset)
>> {
>> struct dp_ctrl_private *ctrl;
>> @@ -1964,6 +2015,17 @@ void dp_ctrl_isr(struct dp_ctrl *dp_ctrl)
>>
>> ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
>>
>> + if (ctrl->panel->psr_cap.version) {
>> + isr = dp_catalog_ctrl_get_psr_interrupt(ctrl->catalog);
>> +
>> + if (isr == 0x1)
>> + DRM_DEBUG_DP("PSR frame update done\n");
>> + else if (isr == 0x10)
>
>Again, the error messages gives a good indication what these bits might be,
>but please provide defines to make it clearer.
>
Okay
>> + DRM_DEBUG_DP("PSR exit done\n");
>> +
>> + complete(&ctrl->psr_op_comp);
>> + }
>> +
>> isr = dp_catalog_ctrl_get_interrupt(ctrl->catalog);
>>
>> if (isr & DP_CTRL_INTR_READY_FOR_VIDEO) { @@ -2010,6 +2072,7 @@
>> struct dp_ctrl *dp_ctrl_get(struct device *dev, struct dp_link *link,
>> dev_err(dev, "failed to add DP OPP table\n");
>>
>> init_completion(&ctrl->idle_comp);
>> + init_completion(&ctrl->psr_op_comp);
>> init_completion(&ctrl->video_comp);
>>
>> /* in parameters */
>> diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> index 2363a2d..f623035 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.h
>> @@ -34,4 +34,7 @@ struct dp_ctrl *dp_ctrl_get(struct device *dev, struct
>dp_link *link,
>> struct dp_power *power, struct dp_catalog *catalog,
>> struct dp_parser *parser);
>>
>> +void dp_ctrl_set_psr(struct dp_ctrl *dp_ctrl, bool enable); void
>> +dp_ctrl_config_psr(struct dp_ctrl *dp_ctrl);
>> +
>> #endif /* _DP_CTRL_H_ */
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index 5d314e6..c8d02fb 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>> @@ -347,6 +347,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;
>> +
>> dp->audio_supported = drm_detect_monitor_audio(edid);
>> dp_panel_handle_sink_request(dp->panel);
>>
>> @@ -871,6 +873,10 @@ static int dp_display_post_enable(struct msm_dp
>> *dp_display)
>>
>> /* signal the connect event late to synchronize video and display */
>> dp_display_handle_plugged_change(dp_display, true);
>> +
>> + if (dp_display->psr_supported)
>> + dp_ctrl_config_psr(dp->ctrl);
>> +
>> return 0;
>> }
>>
>> @@ -1037,6 +1043,14 @@ static void dp_display_config_hpd(struct
>dp_display_private *dp)
>> enable_irq(dp->irq);
>> }
>>
>> +void msm_dp_display_set_psr(struct msm_dp *dp_display, bool enter) {
>> + struct dp_display_private *dp;
>> +
>> + dp = container_of(dp_display, struct dp_display_private, dp_display);
>> + dp_ctrl_set_psr(dp->ctrl, enter); }
>> +
>> static int hpd_event_thread(void *data) {
>> struct dp_display_private *dp_priv; diff --git
>> a/drivers/gpu/drm/msm/dp/dp_display.h
>> b/drivers/gpu/drm/msm/dp/dp_display.h
>> index e3adcd5..6f512f3 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
>> @@ -28,6 +28,7 @@ struct msm_dp {
>>
>> u32 max_dp_lanes;
>> struct dp_audio *dp_audio;
>> + bool psr_supported;
>> };
>>
>> int dp_display_set_plugged_cb(struct msm_dp *dp_display, diff --git
>> a/drivers/gpu/drm/msm/dp/dp_drm.c
>b/drivers/gpu/drm/msm/dp/dp_drm.c
>> index 2436329..d26ca6a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
>> @@ -142,6 +142,35 @@ static enum drm_mode_status
>edp_connector_mode_valid(
>> return MODE_OK;
>> }
>>
>> +
>> +
>> +static int edp_connector_atomic_check(struct drm_connector *connector,
>> + struct drm_atomic_state *state) {
>> + struct msm_dp *dp;
>> + struct drm_connector_state *conn_state;
>> + struct drm_crtc_state *crtc_state;
>> +
>> + dp = to_dp_connector(connector)->dp_display;
>> + conn_state = drm_atomic_get_new_connector_state(state,
>connector);
>> + if (WARN_ON(!conn_state))
>> + return -ENODEV;
>> +
>> + conn_state->self_refresh_aware = true;
>> +
>> + if (!conn_state->crtc)
>> + return 0;
>> +
>> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
>> + if (!crtc_state)
>> + return 0;
>> +
>> + if (crtc_state->self_refresh_active && !dp->psr_supported)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> static const struct drm_connector_funcs dp_connector_funcs = {
>> .detect = dp_connector_detect,
>> .fill_modes = drm_helper_probe_single_connector_modes,
>> @@ -151,6 +180,11 @@ static const struct drm_connector_funcs
>dp_connector_funcs = {
>> .atomic_destroy_state =
>> drm_atomic_helper_connector_destroy_state,
>> };
>>
>> +static const struct drm_connector_helper_funcs
>dp_connector_helper_funcs = {
>> + .get_modes = dp_connector_get_modes,
>> + .mode_valid = dp_connector_mode_valid, };
>> +
>> static const struct drm_connector_funcs edp_connector_funcs = {
>> .fill_modes = drm_helper_probe_single_connector_modes,
>> .destroy = drm_connector_cleanup, @@ -159,12 +193,8 @@ static
>> const struct drm_connector_funcs edp_connector_funcs = {
>> .atomic_destroy_state =
>> drm_atomic_helper_connector_destroy_state,
>> };
>>
>> -static const struct drm_connector_helper_funcs
>dp_connector_helper_funcs = {
>> - .get_modes = dp_connector_get_modes,
>> - .mode_valid = dp_connector_mode_valid,
>> -};
>> -
>> static const struct drm_connector_helper_funcs
>> edp_connector_helper_funcs = {
>> + .atomic_check = edp_connector_atomic_check,
>> .get_modes = edp_connector_get_modes,
>> .mode_valid = edp_connector_mode_valid, }; @@ -258,6 +288,130
>> @@ static void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
>> msm_dp_display_disable(dp_display, drm_bridge->encoder); }
>>
>> +static struct drm_crtc *dp_bridge_get_old_connector_crtc(struct msm_dp
>*dp,
>> + struct drm_atomic_state
>> +*state) {
>> + struct drm_encoder *encoder = dp->encoder;
>> + struct drm_connector *connector;
>> + struct drm_connector_state *conn_state;
>> +
>> + connector = drm_atomic_get_old_connector_for_encoder(state,
>encoder);
>> + if (!connector)
>> + return NULL;
>> +
>> + conn_state = drm_atomic_get_old_connector_state(state, connector);
>> + if (!conn_state)
>> + return NULL;
>> +
>> + return conn_state->crtc;
>> +}
>> +
>> +static struct drm_crtc *dp_bridge_get_new_connector_crtc(struct
>msm_dp *dp,
>> + struct drm_atomic_state
>> +*state) {
>> + struct drm_encoder *encoder = dp->encoder;
>> + struct drm_connector *connector;
>> + struct drm_connector_state *conn_state;
>> +
>> + connector = drm_atomic_get_new_connector_for_encoder(state,
>encoder);
>> + if (!connector)
>> + return NULL;
>> +
>> + conn_state = drm_atomic_get_new_connector_state(state,
>connector);
>> + if (!conn_state)
>> + return NULL;
>> +
>> + return conn_state->crtc;
>> +}
>> +
>> +static void edp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>> + struct drm_bridge_state
>> +*old_bridge_state) {
>> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> + struct drm_crtc *crtc;
>> + struct drm_crtc_state *old_crtc_state;
>> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
>> + struct msm_dp *dp_display = dp_bridge->dp_display;
>> +
>> + crtc = dp_bridge_get_new_connector_crtc(dp_display, old_state);
>> + if (!crtc)
>> + return;
>> +
>> + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
>> +
>> + /* Exit from self refresh mode */
>> + if (old_crtc_state && old_crtc_state->self_refresh_active) {
>> + msm_dp_display_set_psr(dp_display, false);
>> + return;
>> + }
>> +
>> + msm_dp_display_enable(dp_display, drm_bridge->encoder); }
>> +
>> +static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
>> + struct drm_bridge_state
>> +*old_bridge_state) {
>> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> + struct drm_crtc *crtc;
>> + struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state = NULL;
>> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
>> + struct msm_dp *dp_display = dp_bridge->dp_display;
>> +
>> + crtc = dp_bridge_get_old_connector_crtc(dp_display, old_state);
>> + if (!crtc)
>> + goto out;
>> +
>> + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
>> + if (!new_crtc_state)
>> + goto out;
>> +
>> + old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
>> + if (!old_crtc_state)
>> + goto out;
>> +
>> + /*
>> + * Set self refresh mode if current crtc state is active.
>> + * If old crtc state is active, exit psr before disabling
>> + * the controller. Observed sink stuck in self refresh
>> + * if psr exit is skipped when screen off occurs with
>> + * sink in psr mode.
>> + */
>> + if (new_crtc_state->self_refresh_active) {
>> + msm_dp_display_set_psr(dp_display, true);
>> + return;
>> + } else if (old_crtc_state->self_refresh_active) {
>> + msm_dp_display_set_psr(dp_display, false);
>> + return;
>> + }
>> +
>> +out:
>> + msm_dp_display_pre_disable(dp_display, drm_bridge->encoder); }
>> +
>> +static void edp_bridge_atomic_post_disable(struct drm_bridge
>*drm_bridge,
>> + struct drm_bridge_state
>> +*old_bridge_state) {
>> + struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> + struct drm_crtc *crtc;
>> + struct drm_crtc_state *new_crtc_state = NULL;
>> + struct msm_dp_bridge *dp_bridge = to_dp_display(drm_bridge);
>> + struct msm_dp *dp_display = dp_bridge->dp_display;
>> +
>> + crtc = dp_bridge_get_old_connector_crtc(dp_display, old_state);
>> + if (!crtc)
>> + return;
>> +
>> + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
>> + if (!new_crtc_state)
>> + return;
>> +
>> + /* Self refresh mode is set in dp_bridge_disable. Skip disable */
>> + if (new_crtc_state->self_refresh_active)
>> + return;
>> +
>> + msm_dp_display_disable(dp_display, drm_bridge->encoder); }
>> +
>> static const struct drm_bridge_funcs dp_bridge_ops = {
>> .enable = dp_bridge_enable,
>> .disable = dp_bridge_disable,
>> @@ -265,6 +419,16 @@ static const struct drm_bridge_funcs
>dp_bridge_ops = {
>> .mode_set = dp_bridge_mode_set,
>> };
>>
>> +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,
>> + .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,
>> +};
>> +
>> struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display,
>struct drm_device *dev,
>> struct drm_encoder *encoder) { @@ -279,7 +443,8
>> @@ struct drm_bridge *msm_dp_bridge_init(struct msm_dp *dp_display,
>struct drm_devi
>> dp_bridge->dp_display = dp_display;
>>
>> bridge = &dp_bridge->bridge;
>> - bridge->funcs = &dp_bridge_ops;
>> + bridge->funcs = (dp_display->connector_type ==
>DRM_MODE_CONNECTOR_eDP) ?
>> + &edp_bridge_ops : &dp_bridge_ops;
>> bridge->encoder = encoder;
>>
>> rc = drm_bridge_attach(encoder, bridge, NULL,
>> DRM_BRIDGE_ATTACH_NO_CONNECTOR); diff --git
>> a/drivers/gpu/drm/msm/dp/dp_link.c
>b/drivers/gpu/drm/msm/dp/dp_link.c
>> index d4d31e5..5503c29 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_link.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
>> @@ -924,6 +924,26 @@ static int
>dp_link_process_phy_test_pattern_request(
>> return 0;
>> }
>>
>> +static int dp_link_psr_status(struct dp_link_private *link)
>
>The implementation and caller indicates that this would better return a bool.
>That said "status" isn't a boolean property, and given that you return true
>when capabilities changes, perhaps this is
>dp_link_psr_cap_changed() ?
>
Okay. I split the function into two.
>> +{
>> + u8 status[2];
>> +
>> + drm_dp_dpcd_read(link->aux, DP_PSR_ERROR_STATUS, status, 2);
>> +
>> + if (status[0] & DP_PSR_LINK_CRC_ERROR)
>> + DRM_ERROR("PSR LINK CRC ERROR\n");
>> + else if (status[0] & DP_PSR_RFB_STORAGE_ERROR)
>> + DRM_ERROR("PSR RFB STORAGE ERROR\n");
>> + else if (status[0] & DP_PSR_VSC_SDP_UNCORRECTABLE_ERROR)
>> + DRM_ERROR("PSR VSC SDP UNCORRECTABLE ERROR\n");
>> + else if (status[1] & DP_PSR_CAPS_CHANGE)
>> + DRM_INFO("PSR Capability Change\n");
>
>DEBUG instead?
>
Okay
>> + else
>> + return 0;
>> +
>> + return 1;
>> +}
>> +
>> static u8 get_link_status(const u8 link_status[DP_LINK_STATUS_SIZE],
>> int r) {
>> return link_status[r - DP_LANE0_1_STATUS]; @@ -1042,6 +1062,8 @@
>> int dp_link_process_request(struct dp_link *dp_link)
>> dp_link->sink_request |= DP_TEST_LINK_TRAINING;
>> } else if (!dp_link_process_phy_test_pattern_request(link)) {
>> dp_link->sink_request |= DP_TEST_LINK_PHY_TEST_PATTERN;
>> + } else if (dp_link_psr_status(link)) {
>> + DRM_INFO("PSR IRQ_HPD received\n");
>> } else {
>> ret = dp_link_process_link_status_update(link);
>> if (!ret) {
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.c
>> b/drivers/gpu/drm/msm/dp/dp_panel.c
>> index 71db10c..e128d73 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.c
>> @@ -19,6 +19,26 @@ struct dp_panel_private {
>> bool aux_cfg_update_done;
>> };
>>
>> +static void dp_panel_read_psr_cap(struct dp_panel_private *panel) {
>> + ssize_t rlen;
>> + struct dp_panel *dp_panel;
>> +
>> + dp_panel = &panel->dp_panel;
>> +
>> + /* edp sink */
>> + if (dp_panel->dpcd[DP_EDP_CONFIGURATION_CAP]) {
>> + rlen = drm_dp_dpcd_read(panel->aux, DP_PSR_SUPPORT,
>> + &dp_panel->psr_cap, 2);
>> + if (rlen == 2) {
>
>sizeof(dp_panel->psr_cap) perhaps?
>
Okay
>> + DRM_DEBUG_DP("psr version: 0x%x, psr_cap: 0x%x\n",
>> + dp_panel->psr_cap.version,
>> + dp_panel->psr_cap.capabilities);
>> + } else
>> + DRM_ERROR("failed to read psr info, rlen=%zd\n", rlen);
>> + }
>> +}
>> +
>> static int dp_panel_read_dpcd(struct dp_panel *dp_panel) {
>> int rc = 0;
>> @@ -104,6 +124,7 @@ static int dp_panel_read_dpcd(struct dp_panel
>*dp_panel)
>> }
>> }
>>
>> + dp_panel_read_psr_cap(panel);
>> end:
>> return rc;
>> }
>> diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h
>> b/drivers/gpu/drm/msm/dp/dp_panel.h
>> index 9023e5b..631657a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_panel.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_panel.h
>> @@ -34,6 +34,11 @@ struct dp_panel_in {
>> struct dp_catalog *catalog;
>> };
>>
>> +struct dp_panel_psr {
>> + u8 version;
>> + u8 capabilities;
>> +};
>> +
>> struct dp_panel {
>> /* dpcd raw data */
>> u8 dpcd[DP_RECEIVER_CAP_SIZE + 1]; @@ -46,6 +51,7 @@ struct
>> dp_panel {
>> struct edid *edid;
>> struct drm_connector *connector;
>> struct dp_display_mode dp_mode;
>> + struct dp_panel_psr psr_cap;
>> bool video_test;
>>
>> u32 vic;
>> diff --git a/drivers/gpu/drm/msm/dp/dp_reg.h
>> b/drivers/gpu/drm/msm/dp/dp_reg.h index 2686028..7a0b052 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_reg.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_reg.h
>> @@ -22,6 +22,20 @@
>> #define REG_DP_INTR_STATUS2 (0x00000024)
>> #define REG_DP_INTR_STATUS3 (0x00000028)
>>
>> +#define REG_DP_INTR_STATUS4 (0x0000002C)
>> +#define PSR_UPDATE_INT (0x00000001)
>> +#define PSR_CAPTURE_INT (0x00000004)
>> +#define PSR_EXIT_INT (0x00000010)
>> +#define PSR_UPDATE_ERROR_INT (0x00000040)
>> +#define PSR_WAKE_ERROR_INT (0x00000100)
>> +
>> +#define REG_DP_INTR_MASK4 (0x00000030)
>> +#define PSR_UPDATE_MASK (0x00000001)
>> +#define PSR_CAPTURE_MASK (0x00000002)
>> +#define PSR_EXIT_MASK (0x00000004)
>> +#define PSR_UPDATE_ERROR_MASK (0x00000008)
>> +#define PSR_WAKE_ERROR_MASK (0x00000010)
>> +
>> #define REG_DP_DP_HPD_CTRL (0x00000000)
>> #define DP_DP_HPD_CTRL_HPD_EN (0x00000001)
>>
>> @@ -164,6 +178,9 @@
>> #define MMSS_DP_AUDIO_TIMING_RBR_48 (0x00000094)
>> #define MMSS_DP_AUDIO_TIMING_HBR_48 (0x00000098)
>>
>> +#define REG_PSR_CONFIG (0x00000100)
>> +#define REG_PSR_CMD (0x00000110)
>> +
>> #define MMSS_DP_PSR_CRC_RG (0x00000154)
>> #define MMSS_DP_PSR_CRC_B (0x00000158)
>>
>> @@ -184,6 +201,8 @@
>> #define MMSS_DP_AUDIO_STREAM_0 (0x00000240)
>> #define MMSS_DP_AUDIO_STREAM_1 (0x00000244)
>>
>> +#define MMSS_DP_SDP_CFG3 (0x0000024c)
>> +
>> #define MMSS_DP_EXTENSION_0 (0x00000250)
>> #define MMSS_DP_EXTENSION_1 (0x00000254)
>> #define MMSS_DP_EXTENSION_2 (0x00000258)
>> diff --git a/drivers/gpu/drm/msm/msm_drv.h
>> b/drivers/gpu/drm/msm/msm_drv.h index ae52412..254fd07 100644
>> --- a/drivers/gpu/drm/msm/msm_drv.h
>> +++ b/drivers/gpu/drm/msm/msm_drv.h
>> @@ -400,6 +400,8 @@ void msm_dp_snapshot(struct msm_disp_state
>> *disp_state, struct msm_dp *dp_displa
>>
>> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor
>> *minor);
>>
>> +void msm_dp_display_set_psr(struct msm_dp *dp, bool enter);
>> +
>> #else
>> static inline int __init msm_dp_register(void) { @@ -449,6 +451,10
>> @@ static inline void msm_dp_debugfs_init(struct msm_dp *dp_display,
>> { }
>>
>> +static inline void msm_dp_display_set_psr(struct msm_dp *dp, bool
>> +enter)
>
>Perhaps I'm missing it, but I don't see that this is called from outside the DP
>driver. So why does it need to be in msm_drv.h and why do you need a stub?
>
Yeah, it was used earlier when DP bridge was not present. It is not needed here anymore. I moved it to dp_display.h
>Thanks,
>Bjorn
>
>> +{
>> +}
>> +
>> #endif
>>
>> void __init msm_mdp_register(void);
>> --
>> 2.7.4
>>
Thank you,
Sankeerth