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.
- Move dp functions to bridge code.
Changes in v3:
- Change callback names to reflect atomic interfaces.
- Move bridge callback change to separate patch as suggested by Dmitry.
- Remove psr function declaration from msm_drv.h.
- Set self_refresh_aware flag only if psr is supported.
- Modify the variable names to simpler form.
- Define bit fields for PSR settings.
- Add comments explaining the steps to enter/exit psr.
- Change DRM_INFO to drm_dbg_db.
Changes in v4:
- Move the get crtc functions to drm_atomic.
- Add atomic functions for DP bridge too.
- Add ternary operator to choose eDP or DP ops.
- Return true/false instead of 1/0.
- mode_valid missing in the eDP bridge ops.
- Move the functions to get crtc into drm_atomic.c.
- Fix compilation issues.
- Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc.
- Check for crtc state enable while reserving resources.
Changes in v5:
- Move the mode_valid changes into a different patch.
- Complete psr_op_comp only when isr is set.
- Move the DP atomic callback changes to a different patch.
- Get crtc from drm connector state crtc.
- Move to separate patch for check for crtc state enable while
reserving resources.
Changes in v6:
- Remove crtc from dpu_encoder_virt struct.
- fix crtc check during vblank toggle crtc.
- Misc changes.
Changes in v7:
- Add fix for underrun issue on kasan build.
Changes in v8:
- Drop the enc spinlock as it won't serve any purpose in
protetcing conn state.(Dmitry/Doug)
Changes in v9:
- Update commit message and fix alignment using spaces.(Marijn)
- Misc changes.(Marijn)
Changes in v10:
- Get crtc cached in dpu_enc during obj init.(Dmitry)
Changes in v11:
- Remove crtc cached in dpu_enc during obj init.
- Update dpu_enc crtc state on crtc enable/disable during self refresh.
Sankeerth Billakanti (1):
drm/msm/dp: disable self_refresh_aware after entering psr
Vinod Polimera (14):
drm: add helper functions to retrieve old and new crtc
drm/msm/dp: use atomic callbacks for DP bridge ops
drm/msm/dp: Add basic PSR support for eDP
drm/msm/dp: use the eDP bridge ops to validate eDP modes
drm/bridge: use atomic enable/disable callbacks for panel bridge
drm/bridge: add psr support for panel bridge callbacks
drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder
functions
drm/msm/disp/dpu: check for crtc enable rather than crtc active to
release shared resources
drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver
drm/msm/disp/dpu: get timing engine status from intf status register
drm/msm/disp/dpu: wait for extra vsync till timing engine status is
disabled
drm/msm/disp/dpu: reset the datapath after timing engine disable
drm/msm/disp/dpu: clear active interface in the datapath cleanup
drm/msm/disp/dpu: update dpu_enc crtc state on crtc enable/disable
during self refresh
drivers/gpu/drm/bridge/panel.c | 68 ++++++-
drivers/gpu/drm/drm_atomic.c | 60 +++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 42 ++++-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 29 ++-
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 22 +++
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 12 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 8 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +-
drivers/gpu/drm/msm/dp/dp_catalog.c | 80 +++++++++
drivers/gpu/drm/msm/dp/dp_catalog.h | 4 +
drivers/gpu/drm/msm/dp/dp_ctrl.c | 80 +++++++++
drivers/gpu/drm/msm/dp/dp_ctrl.h | 3 +
drivers/gpu/drm/msm/dp/dp_display.c | 36 ++--
drivers/gpu/drm/msm/dp/dp_display.h | 2 +
drivers/gpu/drm/msm/dp/dp_drm.c | 196 ++++++++++++++++++++-
drivers/gpu/drm/msm/dp/dp_drm.h | 9 +-
drivers/gpu/drm/msm/dp/dp_link.c | 36 ++++
drivers/gpu/drm/msm/dp/dp_panel.c | 22 +++
drivers/gpu/drm/msm/dp/dp_panel.h | 6 +
drivers/gpu/drm/msm/dp/dp_reg.h | 27 +++
include/drm/drm_atomic.h | 7 +
22 files changed, 710 insertions(+), 44 deletions(-)
--
2.7.4
Clear interface active register from the datapath for a clean shutdown of
the datapath.
Signed-off-by: Vinod Polimera <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9cf1263..30dee50 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -2085,6 +2085,9 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
if (phys_enc->hw_pp->merge_3d)
intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
+ if (phys_enc->hw_intf)
+ intf_cfg.intf = phys_enc->hw_intf->idx;
+
if (ctl->ops.reset_intf_cfg)
ctl->ops.reset_intf_cfg(ctl, &intf_cfg);
--
2.7.4
From: Sankeerth Billakanti <[email protected]>
Updated frames get queued if self_refresh_aware is set when the
sink is in psr. To support bridge enable and avoid queuing of update
frames, reset the self_refresh_aware state after entering psr.
Signed-off-by: Sankeerth Billakanti <[email protected]>
Signed-off-by: Vinod Polimera <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_drm.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 029e08c..92d1a1b 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -134,6 +134,8 @@ static void edp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
struct drm_crtc_state *old_crtc_state;
struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
struct msm_dp *dp = dp_bridge->dp_display;
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state = NULL;
/*
* Check the old state of the crtc to determine if the panel
@@ -150,10 +152,22 @@ static void edp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
if (old_crtc_state && old_crtc_state->self_refresh_active) {
dp_display_set_psr(dp, false);
- return;
+ goto psr_aware;
}
dp_bridge_atomic_enable(drm_bridge, old_bridge_state);
+
+psr_aware:
+ connector = drm_atomic_get_new_connector_for_encoder(atomic_state,
+ drm_bridge->encoder);
+ if (connector)
+ conn_state = drm_atomic_get_new_connector_state(atomic_state,
+ connector);
+
+ if (conn_state) {
+ conn_state->self_refresh_aware = dp->psr_supported;
+ }
+
}
static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
@@ -164,6 +178,14 @@ static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state = NULL;
struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
struct msm_dp *dp = dp_bridge->dp_display;
+ struct drm_connector *connector;
+ struct drm_connector_state *conn_state = NULL;
+
+ connector = drm_atomic_get_old_connector_for_encoder(atomic_state,
+ drm_bridge->encoder);
+ if (connector)
+ conn_state = drm_atomic_get_new_connector_state(atomic_state,
+ connector);
crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state,
drm_bridge->encoder);
@@ -190,6 +212,9 @@ static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
* when display disable occurs while the sink is in psr state.
*/
if (new_crtc_state->self_refresh_active) {
+ if (conn_state)
+ conn_state->self_refresh_aware = false;
+
dp_display_set_psr(dp, true);
return;
} else if (old_crtc_state->self_refresh_active) {
--
2.7.4
The eDP and DP interfaces shared the bridge operations and
the eDP specific changes were implemented under is_edp check.
To add psr support for eDP, we started using a new set of eDP
bridge ops. We are moving the eDP specific code in the
dp_bridge_mode_valid function to a new eDP function,
edp_bridge_mode_valid under the eDP bridge ops.
Signed-off-by: Sankeerth Billakanti <[email protected]>
Signed-off-by: Vinod Polimera <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_display.c | 8 --------
drivers/gpu/drm/msm/dp/dp_drm.c | 34 +++++++++++++++++++++++++++++++++-
2 files changed, 33 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 86ed80c..ffb21a6 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -996,14 +996,6 @@ enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
return -EINVAL;
}
- /*
- * The eDP controller currently does not have a reliable way of
- * enabling panel power to read sink capabilities. So, we rely
- * on the panel driver to populate only supported modes for now.
- */
- if (dp->is_edp)
- return MODE_OK;
-
if (mode->clock > DP_MAX_PIXEL_CLK_KHZ)
return MODE_CLOCK_HIGH;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 3b38bd9..029e08c 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -226,12 +226,44 @@ static void edp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
dp_bridge_atomic_post_disable(drm_bridge, old_bridge_state);
}
+/**
+ * edp_bridge_mode_valid - callback to determine if specified mode is valid
+ * @bridge: Pointer to drm bridge structure
+ * @info: display info
+ * @mode: Pointer to drm mode structure
+ * Returns: Validity status for specified mode
+ */
+static enum drm_mode_status edp_bridge_mode_valid(struct drm_bridge *bridge,
+ const struct drm_display_info *info,
+ const struct drm_display_mode *mode)
+{
+ struct msm_dp *dp;
+ int mode_pclk_khz = mode->clock;
+
+ dp = to_dp_bridge(bridge)->dp_display;
+
+ if (!dp || !mode_pclk_khz || !dp->connector) {
+ DRM_ERROR("invalid params\n");
+ return -EINVAL;
+ }
+
+ if (mode->clock > DP_MAX_PIXEL_CLK_KHZ)
+ return MODE_CLOCK_HIGH;
+
+ /*
+ * The eDP controller currently does not have a reliable way of
+ * enabling panel power to read sink capabilities. So, we rely
+ * on the panel driver to populate only supported modes for now.
+ */
+ return MODE_OK;
+}
+
static const struct drm_bridge_funcs edp_bridge_ops = {
.atomic_enable = edp_bridge_atomic_enable,
.atomic_disable = edp_bridge_atomic_disable,
.atomic_post_disable = edp_bridge_atomic_post_disable,
.mode_set = dp_bridge_mode_set,
- .mode_valid = dp_bridge_mode_valid,
+ .mode_valid = edp_bridge_mode_valid,
.atomic_reset = drm_atomic_helper_bridge_reset,
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
--
2.7.4
Add new helper functions, drm_atomic_get_old_crtc_for_encoder
and drm_atomic_get_new_crtc_for_encoder to retrieve the
corresponding crtc for the encoder.
Signed-off-by: Sankeerth Billakanti <[email protected]>
Signed-off-by: Vinod Polimera <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/drm_atomic.c | 60 ++++++++++++++++++++++++++++++++++++++++++++
include/drm/drm_atomic.h | 7 ++++++
2 files changed, 67 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 5457c02..7cc39f6 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -985,6 +985,66 @@ drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
EXPORT_SYMBOL(drm_atomic_get_new_connector_for_encoder);
/**
+ * drm_atomic_get_old_crtc_for_encoder - Get old crtc for an encoder
+ * @state: Atomic state
+ * @encoder: The encoder to fetch the crtc state for
+ *
+ * This function finds and returns the crtc that was connected to @encoder
+ * as specified by the @state.
+ *
+ * Returns: The old crtc connected to @encoder, or NULL if the encoder is
+ * not connected.
+ */
+struct drm_crtc *
+drm_atomic_get_old_crtc_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *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;
+}
+EXPORT_SYMBOL(drm_atomic_get_old_crtc_for_encoder);
+
+/**
+ * drm_atomic_get_new_crtc_for_encoder - Get new crtc for an encoder
+ * @state: Atomic state
+ * @encoder: The encoder to fetch the crtc state for
+ *
+ * This function finds and returns the crtc that will be connected to @encoder
+ * as specified by the @state.
+ *
+ * Returns: The new crtc connected to @encoder, or NULL if the encoder is
+ * not connected.
+ */
+struct drm_crtc *
+drm_atomic_get_new_crtc_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *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;
+}
+EXPORT_SYMBOL(drm_atomic_get_new_crtc_for_encoder);
+
+/**
* drm_atomic_get_connector_state - get connector state
* @state: global atomic state object
* @connector: connector to get state object for
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 92586ab..9a022ca 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -528,6 +528,13 @@ struct drm_connector *
drm_atomic_get_new_connector_for_encoder(const struct drm_atomic_state *state,
struct drm_encoder *encoder);
+struct drm_crtc *
+drm_atomic_get_old_crtc_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder);
+struct drm_crtc *
+drm_atomic_get_new_crtc_for_encoder(struct drm_atomic_state *state,
+ struct drm_encoder *encoder);
+
/**
* drm_atomic_get_existing_crtc_state - get CRTC state, if it exists
* @state: global atomic state object
--
2.7.4
Reset the datapath after disabling the timing gen, such that
it can start on a clean slate when the intf is enabled back.
This was a recommended sequence from the DPU HW programming guide.
Signed-off-by: Vinod Polimera <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 0396084..3a37429 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -588,6 +588,7 @@ static void dpu_encoder_phys_vid_disable(struct dpu_encoder_phys *phys_enc)
}
}
+ dpu_encoder_helper_phys_cleanup(phys_enc);
phys_enc->enable_state = DPU_ENC_DISABLED;
}
--
2.7.4
Recommended way of reading the interface timing gen status is via
status register. Timing gen status register will give a reliable status
of the interface especially during ON/OFF transitions. This support was
added from DPU version 5.0.0.
Signed-off-by: Vinod Polimera <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 12 +++++++-----
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 8 +++++++-
3 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 4375e72..0244a7b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -80,7 +80,8 @@
#define INTF_SC7180_MASK BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE)
-#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
+#define INTF_SC7280_MASK \
+ (INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_STATUS_SUPPORTED))
#define IRQ_SDM845_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
BIT(MDP_SSPP_TOP0_INTR2) | \
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
index 978e3bd..79c18fe 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
@@ -213,17 +213,19 @@ enum {
/**
* INTF sub-blocks
- * @DPU_INTF_INPUT_CTRL Supports the setting of pp block from which
- * pixel data arrives to this INTF
- * @DPU_INTF_TE INTF block has TE configuration support
- * @DPU_DATA_HCTL_EN Allows data to be transferred at different rate
- than video timing
+ * @DPU_INTF_INPUT_CTRL Supports the setting of pp block from which
+ * pixel data arrives to this INTF
+ * @DPU_INTF_TE INTF block has TE configuration support
+ * @DPU_DATA_HCTL_EN Allows data to be transferred at different rate
+ * than video timing
+ * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
* @DPU_INTF_MAX
*/
enum {
DPU_INTF_INPUT_CTRL = 0x1,
DPU_INTF_TE,
DPU_DATA_HCTL_EN,
+ DPU_INTF_STATUS_SUPPORTED,
DPU_INTF_MAX
};
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 7ce66bf..84ee2ef 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -62,6 +62,7 @@
#define INTF_LINE_COUNT 0x0B0
#define INTF_MUX 0x25C
+#define INTF_STATUS 0x26C
#define INTF_CFG_ACTIVE_H_EN BIT(29)
#define INTF_CFG_ACTIVE_V_EN BIT(30)
@@ -297,8 +298,13 @@ static void dpu_hw_intf_get_status(
struct intf_status *s)
{
struct dpu_hw_blk_reg_map *c = &intf->hw;
+ unsigned long cap = intf->cap->features;
+
+ if (cap & BIT(DPU_INTF_STATUS_SUPPORTED))
+ s->is_en = DPU_REG_READ(c, INTF_STATUS) & BIT(0);
+ else
+ s->is_en = DPU_REG_READ(c, INTF_TIMING_ENGINE_EN);
- s->is_en = DPU_REG_READ(c, INTF_TIMING_ENGINE_EN);
s->is_prog_fetch_en = !!(DPU_REG_READ(c, INTF_CONFIG) & BIT(31));
if (s->is_en) {
s->frame_count = DPU_REG_READ(c, INTF_FRAME_COUNT);
--
2.7.4
Populate the enocder software structure to reflect the updated
crtc appropriately during crtc enable/disable for a new commit
while taking care of the self refresh transitions when crtc
disable is triggered from the drm self refresh library.
Signed-off-by: Vinod Polimera <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 31 ++++++++++++++++++++++++++-----
1 file changed, 26 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index d513aeb4..e8e456a 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1013,14 +1013,23 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
crtc);
struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
- struct drm_encoder *encoder;
+ struct drm_encoder *encoder = NULL;
unsigned long flags;
bool release_bandwidth = false;
DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
- if (old_crtc_state->self_refresh_active)
+ /* If disable is triggered while in self refresh mode,
+ * reset the encoder software state so that in enable
+ * it won't trigger a warn while assigning crtc.
+ */
+ 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);
@@ -1033,7 +1042,14 @@ 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 */
@@ -1081,6 +1097,9 @@ 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;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
pm_runtime_get_sync(crtc->dev->dev);
@@ -1103,8 +1122,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);
--
2.7.4
There can be a race between timing gen disable and vblank irq. The
wait post timing gen disable may return early but intf disable sequence
might not be completed. Ensure that, intf status is disabled before
we retire the function.
Signed-off-by: Vinod Polimera <[email protected]>
---
.../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 48c4810..0396084 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -523,6 +523,7 @@ static void dpu_encoder_phys_vid_disable(struct dpu_encoder_phys *phys_enc)
{
unsigned long lock_flags;
int ret;
+ struct intf_status intf_status = {0};
if (!phys_enc->parent || !phys_enc->parent->dev) {
DPU_ERROR("invalid encoder/device\n");
@@ -567,6 +568,26 @@ static void dpu_encoder_phys_vid_disable(struct dpu_encoder_phys *phys_enc)
}
}
+ if (phys_enc->hw_intf && phys_enc->hw_intf->ops.get_status)
+ phys_enc->hw_intf->ops.get_status(phys_enc->hw_intf, &intf_status);
+
+ /*
+ * Wait for a vsync if timing en status is on after timing engine
+ * is disabled.
+ */
+ if (intf_status.is_en && dpu_encoder_phys_vid_is_master(phys_enc)) {
+ spin_lock_irqsave(phys_enc->enc_spinlock, lock_flags);
+ dpu_encoder_phys_inc_pending(phys_enc);
+ spin_unlock_irqrestore(phys_enc->enc_spinlock, lock_flags);
+ ret = dpu_encoder_phys_vid_wait_for_vblank(phys_enc);
+ if (ret) {
+ atomic_set(&phys_enc->pending_kickoff_cnt, 0);
+ DRM_ERROR("wait disable failed: id:%u intf:%d ret:%d\n",
+ DRMID(phys_enc->parent),
+ phys_enc->hw_intf->idx - INTF_0, ret);
+ }
+ }
+
phys_enc->enable_state = DPU_ENC_DISABLED;
}
--
2.7.4
On 19/01/2023 16:26, Vinod Polimera wrote:
> From: Sankeerth Billakanti <[email protected]>
>
> Updated frames get queued if self_refresh_aware is set when the
> sink is in psr. To support bridge enable and avoid queuing of update
> frames, reset the self_refresh_aware state after entering psr.
I'm not convinced by this change. E.g. analogix code doesn't do this.
Could you please clarify, why do you need to toggle the
self_refresh_aware flag?
>
> Signed-off-by: Sankeerth Billakanti <[email protected]>
> Signed-off-by: Vinod Polimera <[email protected]>
> ---
> drivers/gpu/drm/msm/dp/dp_drm.c | 27 ++++++++++++++++++++++++++-
> 1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
> index 029e08c..92d1a1b 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -134,6 +134,8 @@ static void edp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
> struct drm_crtc_state *old_crtc_state;
> struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> struct msm_dp *dp = dp_bridge->dp_display;
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state = NULL;
>
> /*
> * Check the old state of the crtc to determine if the panel
> @@ -150,10 +152,22 @@ static void edp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
>
> if (old_crtc_state && old_crtc_state->self_refresh_active) {
> dp_display_set_psr(dp, false);
> - return;
> + goto psr_aware;
> }
>
> dp_bridge_atomic_enable(drm_bridge, old_bridge_state);
> +
> +psr_aware:
> + connector = drm_atomic_get_new_connector_for_encoder(atomic_state,
> + drm_bridge->encoder);
> + if (connector)
> + conn_state = drm_atomic_get_new_connector_state(atomic_state,
> + connector);
> +
> + if (conn_state) {
> + conn_state->self_refresh_aware = dp->psr_supported;
> + }
No need to wrap a single line statement in brackets.
> +
> }
>
> static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
> @@ -164,6 +178,14 @@ static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
> struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state = NULL;
> struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> struct msm_dp *dp = dp_bridge->dp_display;
> + struct drm_connector *connector;
> + struct drm_connector_state *conn_state = NULL;
> +
> + connector = drm_atomic_get_old_connector_for_encoder(atomic_state,
> + drm_bridge->encoder);
> + if (connector)
> + conn_state = drm_atomic_get_new_connector_state(atomic_state,
> + connector);
>
> crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state,
> drm_bridge->encoder);
> @@ -190,6 +212,9 @@ static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
> * when display disable occurs while the sink is in psr state.
> */
> if (new_crtc_state->self_refresh_active) {
> + if (conn_state)
> + conn_state->self_refresh_aware = false;
> +
> dp_display_set_psr(dp, true);
> return;
> } else if (old_crtc_state->self_refresh_active) {
--
With best wishes
Dmitry
On 19/01/2023 16:26, Vinod Polimera wrote:
> Recommended way of reading the interface timing gen status is via
> status register. Timing gen status register will give a reliable status
> of the interface especially during ON/OFF transitions. This support was
> added from DPU version 5.0.0.
5.0.0 is sm8150. I have expected to see INTF_SC7180_MASK to be changed,
while this patch for some reason changes only INTF_SC7280_MASK. Could
you please clarify this?
>
> Signed-off-by: Vinod Polimera <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 ++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 12 +++++++-----
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 8 +++++++-
> 3 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> index 4375e72..0244a7b 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
> @@ -80,7 +80,8 @@
>
> #define INTF_SC7180_MASK BIT(DPU_INTF_INPUT_CTRL) | BIT(DPU_INTF_TE)
>
> -#define INTF_SC7280_MASK INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN)
> +#define INTF_SC7280_MASK \
> + (INTF_SC7180_MASK | BIT(DPU_DATA_HCTL_EN) | BIT(DPU_INTF_STATUS_SUPPORTED))
>
> #define IRQ_SDM845_MASK (BIT(MDP_SSPP_TOP0_INTR) | \
> BIT(MDP_SSPP_TOP0_INTR2) | \
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> index 978e3bd..79c18fe 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
> @@ -213,17 +213,19 @@ enum {
>
> /**
> * INTF sub-blocks
> - * @DPU_INTF_INPUT_CTRL Supports the setting of pp block from which
> - * pixel data arrives to this INTF
> - * @DPU_INTF_TE INTF block has TE configuration support
> - * @DPU_DATA_HCTL_EN Allows data to be transferred at different rate
> - than video timing
> + * @DPU_INTF_INPUT_CTRL Supports the setting of pp block from which
> + * pixel data arrives to this INTF
> + * @DPU_INTF_TE INTF block has TE configuration support
> + * @DPU_DATA_HCTL_EN Allows data to be transferred at different rate
> + * than video timing
> + * @DPU_INTF_STATUS_SUPPORTED INTF block has INTF_STATUS register
> * @DPU_INTF_MAX
> */
> enum {
> DPU_INTF_INPUT_CTRL = 0x1,
> DPU_INTF_TE,
> DPU_DATA_HCTL_EN,
> + DPU_INTF_STATUS_SUPPORTED,
> DPU_INTF_MAX
> };
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> index 7ce66bf..84ee2ef 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
> @@ -62,6 +62,7 @@
> #define INTF_LINE_COUNT 0x0B0
>
> #define INTF_MUX 0x25C
> +#define INTF_STATUS 0x26C
>
> #define INTF_CFG_ACTIVE_H_EN BIT(29)
> #define INTF_CFG_ACTIVE_V_EN BIT(30)
> @@ -297,8 +298,13 @@ static void dpu_hw_intf_get_status(
> struct intf_status *s)
> {
> struct dpu_hw_blk_reg_map *c = &intf->hw;
> + unsigned long cap = intf->cap->features;
> +
> + if (cap & BIT(DPU_INTF_STATUS_SUPPORTED))
> + s->is_en = DPU_REG_READ(c, INTF_STATUS) & BIT(0);
> + else
> + s->is_en = DPU_REG_READ(c, INTF_TIMING_ENGINE_EN);
>
> - s->is_en = DPU_REG_READ(c, INTF_TIMING_ENGINE_EN);
> s->is_prog_fetch_en = !!(DPU_REG_READ(c, INTF_CONFIG) & BIT(31));
> if (s->is_en) {
> s->frame_count = DPU_REG_READ(c, INTF_FRAME_COUNT);
--
With best wishes
Dmitry
On 19/01/2023 16:26, Vinod Polimera wrote:
> There can be a race between timing gen disable and vblank irq. The
> wait post timing gen disable may return early but intf disable sequence
> might not be completed. Ensure that, intf status is disabled before
> we retire the function.
>
> Signed-off-by: Vinod Polimera <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
> ---
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
--
With best wishes
Dmitry
On 19/01/2023 16:26, Vinod Polimera wrote:
> Reset the datapath after disabling the timing gen, such that
> it can start on a clean slate when the intf is enabled back.
> This was a recommended sequence from the DPU HW programming guide.
>
> Signed-off-by: Vinod Polimera <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
On 19/01/2023 16:26, Vinod Polimera wrote:
> Clear interface active register from the datapath for a clean shutdown of
> the datapath.
>
> Signed-off-by: Vinod Polimera <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 9cf1263..30dee50 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -2085,6 +2085,9 @@ void dpu_encoder_helper_phys_cleanup(struct dpu_encoder_phys *phys_enc)
> if (phys_enc->hw_pp->merge_3d)
> intf_cfg.merge_3d = phys_enc->hw_pp->merge_3d->idx;
>
> + if (phys_enc->hw_intf)
> + intf_cfg.intf = phys_enc->hw_intf->idx;
> +
> if (ctl->ops.reset_intf_cfg)
> ctl->ops.reset_intf_cfg(ctl, &intf_cfg);
>
This is already handled as a part of the commit
9811913a6dd0 ("drm/msm/dpu: populate wb or intf before reset_intf_cfg")
--
With best wishes
Dmitry
On 19/01/2023 16:26, Vinod Polimera wrote:
> Populate the enocder software structure to reflect the updated
> crtc appropriately during crtc enable/disable for a new commit
> while taking care of the self refresh transitions when crtc
> disable is triggered from the drm self refresh library.
>
> Signed-off-by: Vinod Polimera <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 31 ++++++++++++++++++++++++++-----
> 1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index d513aeb4..e8e456a 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1013,14 +1013,23 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> crtc);
> struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc->state);
> - struct drm_encoder *encoder;
> + struct drm_encoder *encoder = NULL;
Why is this chunk necessary?
> unsigned long flags;
> bool release_bandwidth = false;
>
> DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
>
> - if (old_crtc_state->self_refresh_active)
> + /* If disable is triggered while in self refresh mode,
> + * reset the encoder software state so that in enable
> + * it won't trigger a warn while assigning crtc.
> + */
> + 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);
> @@ -1033,7 +1042,14 @@ 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 */
> @@ -1081,6 +1097,9 @@ 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;
> +
> + old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
>
> pm_runtime_get_sync(crtc->dev->dev);
>
> @@ -1103,8 +1122,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);
--
With best wishes
Dmitry
On 19/01/2023 16:26, Vinod Polimera wrote:
> 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.
> - Move dp functions to bridge code.
>
> Changes in v3:
> - Change callback names to reflect atomic interfaces.
> - Move bridge callback change to separate patch as suggested by Dmitry.
> - Remove psr function declaration from msm_drv.h.
> - Set self_refresh_aware flag only if psr is supported.
> - Modify the variable names to simpler form.
> - Define bit fields for PSR settings.
> - Add comments explaining the steps to enter/exit psr.
> - Change DRM_INFO to drm_dbg_db.
>
> Changes in v4:
> - Move the get crtc functions to drm_atomic.
> - Add atomic functions for DP bridge too.
> - Add ternary operator to choose eDP or DP ops.
> - Return true/false instead of 1/0.
> - mode_valid missing in the eDP bridge ops.
> - Move the functions to get crtc into drm_atomic.c.
> - Fix compilation issues.
> - Remove dpu_assign_crtc and get crtc from drm_enc instead of dpu_enc.
> - Check for crtc state enable while reserving resources.
>
> Changes in v5:
> - Move the mode_valid changes into a different patch.
> - Complete psr_op_comp only when isr is set.
> - Move the DP atomic callback changes to a different patch.
> - Get crtc from drm connector state crtc.
> - Move to separate patch for check for crtc state enable while
> reserving resources.
>
> Changes in v6:
> - Remove crtc from dpu_encoder_virt struct.
> - fix crtc check during vblank toggle crtc.
> - Misc changes.
>
> Changes in v7:
> - Add fix for underrun issue on kasan build.
>
> Changes in v8:
> - Drop the enc spinlock as it won't serve any purpose in
> protetcing conn state.(Dmitry/Doug)
>
> Changes in v9:
> - Update commit message and fix alignment using spaces.(Marijn)
> - Misc changes.(Marijn)
>
> Changes in v10:
> - Get crtc cached in dpu_enc during obj init.(Dmitry)
>
> Changes in v11:
> - Remove crtc cached in dpu_enc during obj init.
> - Update dpu_enc crtc state on crtc enable/disable during self refresh.
>
> Sankeerth Billakanti (1):
> drm/msm/dp: disable self_refresh_aware after entering psr
>
> Vinod Polimera (14):
> drm: add helper functions to retrieve old and new crtc
> drm/msm/dp: use atomic callbacks for DP bridge ops
> drm/msm/dp: Add basic PSR support for eDP
> drm/msm/dp: use the eDP bridge ops to validate eDP modes
> drm/bridge: use atomic enable/disable callbacks for panel bridge
> drm/bridge: add psr support for panel bridge callbacks
> drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder
> functions
> drm/msm/disp/dpu: check for crtc enable rather than crtc active to
> release shared resources
> drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver
> drm/msm/disp/dpu: get timing engine status from intf status register
> drm/msm/disp/dpu: wait for extra vsync till timing engine status is
> disabled
> drm/msm/disp/dpu: reset the datapath after timing engine disable
> drm/msm/disp/dpu: clear active interface in the datapath cleanup
> drm/msm/disp/dpu: update dpu_enc crtc state on crtc enable/disable
> during self refresh
1) Please move core patches to the beginning of the series, so that it
is possible to pick them up separately
2) Please follow Daniel's comment in
https://lore.kernel.org/all/[email protected]/ and
apply corresponding Reviewed-by tags.
>
> drivers/gpu/drm/bridge/panel.c | 68 ++++++-
> drivers/gpu/drm/drm_atomic.c | 60 +++++++
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 42 ++++-
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 29 ++-
> .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c | 22 +++
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 3 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h | 12 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c | 8 +-
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +-
> drivers/gpu/drm/msm/dp/dp_catalog.c | 80 +++++++++
> drivers/gpu/drm/msm/dp/dp_catalog.h | 4 +
> drivers/gpu/drm/msm/dp/dp_ctrl.c | 80 +++++++++
> drivers/gpu/drm/msm/dp/dp_ctrl.h | 3 +
> drivers/gpu/drm/msm/dp/dp_display.c | 36 ++--
> drivers/gpu/drm/msm/dp/dp_display.h | 2 +
> drivers/gpu/drm/msm/dp/dp_drm.c | 196 ++++++++++++++++++++-
> drivers/gpu/drm/msm/dp/dp_drm.h | 9 +-
> drivers/gpu/drm/msm/dp/dp_link.c | 36 ++++
> drivers/gpu/drm/msm/dp/dp_panel.c | 22 +++
> drivers/gpu/drm/msm/dp/dp_panel.h | 6 +
> drivers/gpu/drm/msm/dp/dp_reg.h | 27 +++
> include/drm/drm_atomic.h | 7 +
> 22 files changed, 710 insertions(+), 44 deletions(-)
>
--
With best wishes
Dmitry
> -----Original Message-----
> From: Dmitry Baryshkov <[email protected]>
> Sent: Tuesday, January 24, 2023 5:52 AM
> To: Vinod Polimera (QUIC) <[email protected]>; dri-
> [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: Sankeerth Billakanti (QUIC) <[email protected]>; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; Kalyan Thota (QUIC) <[email protected]>;
> Kuogee Hsieh (QUIC) <[email protected]>; Vishnuvardhan
> Prodduturi (QUIC) <[email protected]>; Bjorn Andersson (QUIC)
> <[email protected]>; Abhinav Kumar (QUIC)
> <[email protected]>
> Subject: Re: [PATCH Resend v11 05/15] drm/msm/dp: disable
> self_refresh_aware after entering psr
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> On 19/01/2023 16:26, Vinod Polimera wrote:
> > From: Sankeerth Billakanti <[email protected]>
> >
> > Updated frames get queued if self_refresh_aware is set when the
> > sink is in psr. To support bridge enable and avoid queuing of update
> > frames, reset the self_refresh_aware state after entering psr.
>
> I'm not convinced by this change. E.g. analogix code doesn't do this.
> Could you please clarify, why do you need to toggle the
> self_refresh_aware flag?
>
This was done to fix a bug reported by google. The use case is as follows:
CPU was running in a low frequency with debug build.
When self refresh was triggered by the library, due to system latency, the queued work was not scheduled.
There in another commit came and reinitialized the timer for the next PSR trigger.
This sequence happened multiple times and we found there were multiple works which are stuck and yet to be run.
As PSR trigger is guarded by self_refresh_aware, we initialized the variable such that, if we are in PSR then until PSR exit, there cannot be any further PSR entry again.
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/tags/v5.15.90/drivers/gpu/drm/drm_self_refresh_helper.c#105
This has solved few flicker issues during the stress testing.
> >
> > Signed-off-by: Sankeerth Billakanti <[email protected]>
> > Signed-off-by: Vinod Polimera <[email protected]>
> > ---
> > drivers/gpu/drm/msm/dp/dp_drm.c | 27
> ++++++++++++++++++++++++++-
> > 1 file changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
> b/drivers/gpu/drm/msm/dp/dp_drm.c
> > index 029e08c..92d1a1b 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > @@ -134,6 +134,8 @@ static void edp_bridge_atomic_enable(struct
> drm_bridge *drm_bridge,
> > struct drm_crtc_state *old_crtc_state;
> > struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> > struct msm_dp *dp = dp_bridge->dp_display;
> > + struct drm_connector *connector;
> > + struct drm_connector_state *conn_state = NULL;
> >
> > /*
> > * Check the old state of the crtc to determine if the panel
> > @@ -150,10 +152,22 @@ static void edp_bridge_atomic_enable(struct
> drm_bridge *drm_bridge,
> >
> > if (old_crtc_state && old_crtc_state->self_refresh_active) {
> > dp_display_set_psr(dp, false);
> > - return;
> > + goto psr_aware;
> > }
> >
> > dp_bridge_atomic_enable(drm_bridge, old_bridge_state);
> > +
> > +psr_aware:
> > + connector =
> drm_atomic_get_new_connector_for_encoder(atomic_state,
> > + drm_bridge->encoder);
> > + if (connector)
> > + conn_state =
> drm_atomic_get_new_connector_state(atomic_state,
> > + connector);
> > +
> > + if (conn_state) {
> > + conn_state->self_refresh_aware = dp->psr_supported;
> > + }
>
> No need to wrap a single line statement in brackets.
>
> > +
> > }
> >
> > static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
> > @@ -164,6 +178,14 @@ static void edp_bridge_atomic_disable(struct
> drm_bridge *drm_bridge,
> > struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state = NULL;
> > struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> > struct msm_dp *dp = dp_bridge->dp_display;
> > + struct drm_connector *connector;
> > + struct drm_connector_state *conn_state = NULL;
> > +
> > + connector =
> drm_atomic_get_old_connector_for_encoder(atomic_state,
> > + drm_bridge->encoder);
> > + if (connector)
> > + conn_state =
> drm_atomic_get_new_connector_state(atomic_state,
> > + connector);
> >
> > crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state,
> > drm_bridge->encoder);
> > @@ -190,6 +212,9 @@ static void edp_bridge_atomic_disable(struct
> drm_bridge *drm_bridge,
> > * when display disable occurs while the sink is in psr state.
> > */
> > if (new_crtc_state->self_refresh_active) {
> > + if (conn_state)
> > + conn_state->self_refresh_aware = false;
> > +
> > dp_display_set_psr(dp, true);
> > return;
> > } else if (old_crtc_state->self_refresh_active) {
>
> --
> With best wishes
> Dmitry
Thanks,
Vinod P.
On Tue, 24 Jan 2023 at 17:10, Vinod Polimera <[email protected]> wrote:
> > -----Original Message-----
> > From: Dmitry Baryshkov <[email protected]>
> > Sent: Tuesday, January 24, 2023 5:52 AM
> > To: Vinod Polimera (QUIC) <[email protected]>; dri-
> > [email protected]; [email protected];
> > [email protected]; [email protected]
> > Cc: Sankeerth Billakanti (QUIC) <[email protected]>; linux-
> > [email protected]; [email protected]; [email protected];
> > [email protected]; Kalyan Thota (QUIC) <[email protected]>;
> > Kuogee Hsieh (QUIC) <[email protected]>; Vishnuvardhan
> > Prodduturi (QUIC) <[email protected]>; Bjorn Andersson (QUIC)
> > <[email protected]>; Abhinav Kumar (QUIC)
> > <[email protected]>
> > Subject: Re: [PATCH Resend v11 05/15] drm/msm/dp: disable
> > self_refresh_aware after entering psr
> >
> > WARNING: This email originated from outside of Qualcomm. Please be wary
> > of any links or attachments, and do not enable macros.
I hope such headers can be fixed on your side rather than being sent to the ML.
> >
> > On 19/01/2023 16:26, Vinod Polimera wrote:
> > > From: Sankeerth Billakanti <[email protected]>
> > >
> > > Updated frames get queued if self_refresh_aware is set when the
> > > sink is in psr. To support bridge enable and avoid queuing of update
> > > frames, reset the self_refresh_aware state after entering psr.
> >
> > I'm not convinced by this change. E.g. analogix code doesn't do this.
> > Could you please clarify, why do you need to toggle the
> > self_refresh_aware flag?
> >
> This was done to fix a bug reported by google. The use case is as follows:
> CPU was running in a low frequency with debug build.
> When self refresh was triggered by the library, due to system latency, the queued work was not scheduled.
> There in another commit came and reinitialized the timer for the next PSR trigger.
> This sequence happened multiple times and we found there were multiple works which are stuck and yet to be run.
Where were workers stuck? Was it a busy loop around -EDEADLK /
drm_modeset_backoff()? Also, what were ther ewma times for entry/exit
avg times?
I'm asking because the issue that you are describing sounds like a
generic one, not the driver-specific issue. And being generic it
should be handled in a generic fascion, in drm_self_refresh_helper.c.
For example, I can imagine adding a variable to sr_data telling that
the driver is in the process of transitioning to SR. Note: I did not
perform a full research if it is a working solution or not. But from
your description the driver really has to bail out early from
drm_self_refresh_helper_entry_work().
> As PSR trigger is guarded by self_refresh_aware, we initialized the variable such that, if we are in PSR then until PSR exit, there cannot be any further PSR entry again.
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/tags/v5.15.90/drivers/gpu/drm/drm_self_refresh_helper.c#105
Yes, and that's what triggered my attention. We are using a set of
helpers, that depend on the self_refresh_aware being true. And
suddenly under the hood we disable this flag. I'd say that I can not
predict the effect this will have on the helpers library behaviour.
> This has solved few flicker issues during the stress testing.
> > >
> > > Signed-off-by: Sankeerth Billakanti <[email protected]>
> > > Signed-off-by: Vinod Polimera <[email protected]>
> > > ---
> > > drivers/gpu/drm/msm/dp/dp_drm.c | 27
> > ++++++++++++++++++++++++++-
> > > 1 file changed, 26 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
> > b/drivers/gpu/drm/msm/dp/dp_drm.c
> > > index 029e08c..92d1a1b 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > > @@ -134,6 +134,8 @@ static void edp_bridge_atomic_enable(struct
> > drm_bridge *drm_bridge,
> > > struct drm_crtc_state *old_crtc_state;
> > > struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> > > struct msm_dp *dp = dp_bridge->dp_display;
> > > + struct drm_connector *connector;
> > > + struct drm_connector_state *conn_state = NULL;
> > >
> > > /*
> > > * Check the old state of the crtc to determine if the panel
> > > @@ -150,10 +152,22 @@ static void edp_bridge_atomic_enable(struct
> > drm_bridge *drm_bridge,
> > >
> > > if (old_crtc_state && old_crtc_state->self_refresh_active) {
> > > dp_display_set_psr(dp, false);
> > > - return;
> > > + goto psr_aware;
> > > }
> > >
> > > dp_bridge_atomic_enable(drm_bridge, old_bridge_state);
> > > +
> > > +psr_aware:
> > > + connector =
> > drm_atomic_get_new_connector_for_encoder(atomic_state,
> > > + drm_bridge->encoder);
> > > + if (connector)
> > > + conn_state =
> > drm_atomic_get_new_connector_state(atomic_state,
> > > + connector);
> > > +
> > > + if (conn_state) {
> > > + conn_state->self_refresh_aware = dp->psr_supported;
> > > + }
> >
> > No need to wrap a single line statement in brackets.
> >
> > > +
> > > }
> > >
> > > static void edp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
> > > @@ -164,6 +178,14 @@ static void edp_bridge_atomic_disable(struct
> > drm_bridge *drm_bridge,
> > > struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state = NULL;
> > > struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> > > struct msm_dp *dp = dp_bridge->dp_display;
> > > + struct drm_connector *connector;
> > > + struct drm_connector_state *conn_state = NULL;
> > > +
> > > + connector =
> > drm_atomic_get_old_connector_for_encoder(atomic_state,
> > > + drm_bridge->encoder);
> > > + if (connector)
> > > + conn_state =
> > drm_atomic_get_new_connector_state(atomic_state,
> > > + connector);
> > >
> > > crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state,
> > > drm_bridge->encoder);
> > > @@ -190,6 +212,9 @@ static void edp_bridge_atomic_disable(struct
> > drm_bridge *drm_bridge,
> > > * when display disable occurs while the sink is in psr state.
> > > */
> > > if (new_crtc_state->self_refresh_active) {
> > > + if (conn_state)
> > > + conn_state->self_refresh_aware = false;
> > > +
> > > dp_display_set_psr(dp, true);
> > > return;
> > > } else if (old_crtc_state->self_refresh_active) {
> >
> > --
> > With best wishes
> > Dmitry
>
> Thanks,
> Vinod P.
>
--
With best wishes
Dmitry
> -----Original Message-----
> From: Dmitry Baryshkov <[email protected]>
> Sent: Tuesday, January 24, 2023 10:15 PM
> To: Vinod Polimera <[email protected]>
> Cc: Vinod Polimera (QUIC) <[email protected]>; dri-
> [email protected]; [email protected];
> [email protected]; [email protected]; Sankeerth
> Billakanti (QUIC) <[email protected]>; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; Kalyan Thota (QUIC) <[email protected]>;
> Kuogee Hsieh (QUIC) <[email protected]>; Vishnuvardhan
> Prodduturi (QUIC) <[email protected]>; Bjorn Andersson (QUIC)
> <[email protected]>; Abhinav Kumar (QUIC)
> <[email protected]>
> Subject: Re: [PATCH Resend v11 05/15] drm/msm/dp: disable
> self_refresh_aware after entering psr
> >
> On Tue, 24 Jan 2023 at 17:10, Vinod Polimera <[email protected]>
> wrote:
> > > -----Original Message-----
> > > From: Dmitry Baryshkov <[email protected]>
> > > Sent: Tuesday, January 24, 2023 5:52 AM
> > > To: Vinod Polimera (QUIC) <[email protected]>; dri-
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]
> > > Cc: Sankeerth Billakanti (QUIC) <[email protected]>; linux-
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; Kalyan Thota (QUIC)
> <[email protected]>;
> > > Kuogee Hsieh (QUIC) <[email protected]>; Vishnuvardhan
> > > Prodduturi (QUIC) <[email protected]>; Bjorn Andersson
> (QUIC)
> > > <[email protected]>; Abhinav Kumar (QUIC)
> > > <[email protected]>
> > > Subject: Re: [PATCH Resend v11 05/15] drm/msm/dp: disable
> > > self_refresh_aware after entering psr
> > >
> > >
> > > On 19/01/2023 16:26, Vinod Polimera wrote:
> > > > From: Sankeerth Billakanti <[email protected]>
> > > >
> > > > Updated frames get queued if self_refresh_aware is set when the
> > > > sink is in psr. To support bridge enable and avoid queuing of update
> > > > frames, reset the self_refresh_aware state after entering psr.
> > >
> > > I'm not convinced by this change. E.g. analogix code doesn't do this.
> > > Could you please clarify, why do you need to toggle the
> > > self_refresh_aware flag?
> > >
> > This was done to fix a bug reported by google. The use case is as follows:
> > CPU was running in a low frequency with debug build.
> > When self refresh was triggered by the library, due to system latency,
> the queued work was not scheduled.
> > There in another commit came and reinitialized the timer for the next
> PSR trigger.
> > This sequence happened multiple times and we found there were
> multiple works which are stuck and yet to be run.
>
> Where were workers stuck? Was it a busy loop around -EDEADLK /
> drm_modeset_backoff()? Also, what were ther ewma times for entry/exit
> avg times?
>
It is not an EDEADLK and return is successful.
Entry and exit times are proper but the work that is getting scheduled after timer expiry is happening very late.
> I'm asking because the issue that you are describing sounds like a
> generic one, not the driver-specific issue. And being generic it
> should be handled in a generic fascion, in drm_self_refresh_helper.c.
>
> For example, I can imagine adding a variable to sr_data telling that
> the driver is in the process of transitioning to SR. Note: I did not
> perform a full research if it is a working solution or not. But from
> your description the driver really has to bail out early from
> drm_self_refresh_helper_entry_work().
>
> > As PSR trigger is guarded by self_refresh_aware, we initialized the
> variable such that, if we are in PSR then until PSR exit, there cannot be any
> further PSR entry again.
> >
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/ref
> s/tags/v5.15.90/drivers/gpu/drm/drm_self_refresh_helper.c#105
>
> Yes, and that's what triggered my attention. We are using a set of
> helpers, that depend on the self_refresh_aware being true. And
> suddenly under the hood we disable this flag. I'd say that I can not
> predict the effect this will have on the helpers library behaviour.
>
> > This has solved few flicker issues during the stress testing.
> > > >
> > > > Signed-off-by: Sankeerth Billakanti <[email protected]>
> > > > Signed-off-by: Vinod Polimera <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/msm/dp/dp_drm.c | 27
> > > ++++++++++++++++++++++++++-
> > > > 1 file changed, 26 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
> > > b/drivers/gpu/drm/msm/dp/dp_drm.c
> > > > index 029e08c..92d1a1b 100644
> > > > --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> > > > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> > > > @@ -134,6 +134,8 @@ static void edp_bridge_atomic_enable(struct
> > > drm_bridge *drm_bridge,
> > > > struct drm_crtc_state *old_crtc_state;
> > > > struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> > > > struct msm_dp *dp = dp_bridge->dp_display;
> > > > + struct drm_connector *connector;
> > > > + struct drm_connector_state *conn_state = NULL;
> > > >
> > > > /*
> > > > * Check the old state of the crtc to determine if the panel
> > > > @@ -150,10 +152,22 @@ static void edp_bridge_atomic_enable(struct
> > > drm_bridge *drm_bridge,
> > > >
> > > > if (old_crtc_state && old_crtc_state->self_refresh_active) {
> > > > dp_display_set_psr(dp, false);
> > > > - return;
> > > > + goto psr_aware;
> > > > }
> > > >
> > > > dp_bridge_atomic_enable(drm_bridge, old_bridge_state);
> > > > +
> > > > +psr_aware:
> > > > + connector =
> > > drm_atomic_get_new_connector_for_encoder(atomic_state,
> > > > + drm_bridge->encoder);
> > > > + if (connector)
> > > > + conn_state =
> > > drm_atomic_get_new_connector_state(atomic_state,
> > > > + connector);
> > > > +
> > > > + if (conn_state) {
> > > > + conn_state->self_refresh_aware = dp->psr_supported;
> > > > + }
> > >
> > > No need to wrap a single line statement in brackets.
> > >
> > > > +
> > > > }
> > > >
> > > > static void edp_bridge_atomic_disable(struct drm_bridge
> *drm_bridge,
> > > > @@ -164,6 +178,14 @@ static void edp_bridge_atomic_disable(struct
> > > drm_bridge *drm_bridge,
> > > > struct drm_crtc_state *new_crtc_state = NULL, *old_crtc_state =
> NULL;
> > > > struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> > > > struct msm_dp *dp = dp_bridge->dp_display;
> > > > + struct drm_connector *connector;
> > > > + struct drm_connector_state *conn_state = NULL;
> > > > +
> > > > + connector =
> > > drm_atomic_get_old_connector_for_encoder(atomic_state,
> > > > + drm_bridge->encoder);
> > > > + if (connector)
> > > > + conn_state =
> > > drm_atomic_get_new_connector_state(atomic_state,
> > > > + connector);
> > > >
> > > > crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state,
> > > > drm_bridge->encoder);
> > > > @@ -190,6 +212,9 @@ static void edp_bridge_atomic_disable(struct
> > > drm_bridge *drm_bridge,
> > > > * when display disable occurs while the sink is in psr state.
> > > > */
> > > > if (new_crtc_state->self_refresh_active) {
> > > > + if (conn_state)
> > > > + conn_state->self_refresh_aware = false;
> > > > +
> > > > dp_display_set_psr(dp, true);
> > > > return;
> > > > } else if (old_crtc_state->self_refresh_active) {
> > >
> > > --
> > > With best wishes
> > > Dmitry
> >
> > Thanks,
> > Vinod P.
> >
>
>
> --
> With best wishes
> Dmitry
--
Thanks
Vinod P.