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.
Signed-off-by: Sankeerth Billakanti <[email protected]>
Signed-off-by: Kalyan Thota <[email protected]>
Signed-off-by: Vinod Polimera <[email protected]>
Sankeerth Billakanti (1):
drm/msm/dp: disable self_refresh_aware after entering psr
Vinod Polimera (14):
drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector
state instead of dpu_enc
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
drivers/gpu/drm/bridge/panel.c | 68 ++++++-
drivers/gpu/drm/drm_atomic.c | 60 ++++++
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 17 +-
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 64 ++++---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 -
.../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 | 207 ++++++++++++++++++++-
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 +
23 files changed, 706 insertions(+), 77 deletions(-)
--
2.7.4
Update crtc retrieval from dpu_enc to dpu_enc connector state,
since new links get set as part of the dpu enc virt mode set.
The dpu_enc->crtc cache is no more needed, hence cleaning it as
part of this change.
Signed-off-by: Vinod Polimera <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 4 ----
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 35 ++++++++++++-----------------
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 -------
3 files changed, 14 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 13ce321..8ec9a13 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1029,7 +1029,6 @@ 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);
}
/* wait for frame_event_done completion */
@@ -1099,9 +1098,6 @@ 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);
-
/* Enable/restore vblank irq handling */
drm_crtc_vblank_on(crtc);
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 9c6817b..0c7d8b5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -132,11 +132,6 @@ enum dpu_enc_rc_states {
* @intfs_swapped: Whether or not the phys_enc interfaces have been swapped
* for partial update right-only cases, such as pingpong
* split where virtual pingpong does not generate IRQs
- * @crtc: Pointer to the currently assigned crtc. Normally you
- * would use crtc->state->encoder_mask to determine the
- * link between encoder/crtc. However in this case we need
- * to track crtc in the disable() hook which is called
- * _after_ encoder_mask is cleared.
* @connector: If a mode is set, cached pointer to the active connector
* @crtc_kickoff_cb: Callback into CRTC that will flush & start
* all CTL paths
@@ -181,7 +176,6 @@ struct dpu_encoder_virt {
bool intfs_swapped;
- struct drm_crtc *crtc;
struct drm_connector *connector;
struct dentry *debugfs_root;
@@ -1288,6 +1282,7 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
struct dpu_encoder_phys *phy_enc)
{
struct dpu_encoder_virt *dpu_enc = NULL;
+ struct drm_crtc *crtc;
unsigned long lock_flags;
if (!drm_enc || !phy_enc)
@@ -1298,9 +1293,14 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
atomic_inc(&phy_enc->vsync_cnt);
+ if (!dpu_enc->connector || !dpu_enc->connector->state)
+ return;
+
+ crtc = dpu_enc->connector->state->crtc;
+
spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
- if (dpu_enc->crtc)
- dpu_crtc_vblank_callback(dpu_enc->crtc);
+ if (crtc)
+ dpu_crtc_vblank_callback(crtc);
spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
DPU_ATRACE_END("encoder_vblank_callback");
@@ -1324,29 +1324,22 @@ static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
DPU_ATRACE_END("encoder_underrun_callback");
}
-void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct drm_crtc *crtc)
-{
- struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
- unsigned long lock_flags;
-
- spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
- /* crtc should always be cleared before re-assigning */
- WARN_ON(crtc && dpu_enc->crtc);
- dpu_enc->crtc = crtc;
- spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
-}
-
void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc,
struct drm_crtc *crtc, bool enable)
{
struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+ struct drm_crtc *new_crtc;
unsigned long lock_flags;
int i;
trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
+ if (!dpu_enc->connector || !dpu_enc->connector->state)
+ return;
+
+ new_crtc = dpu_enc->connector->state->crtc;
spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
- if (dpu_enc->crtc != crtc) {
+ if (!new_crtc || new_crtc != crtc) {
spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
return;
}
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 9e7236e..eda5cd8 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -40,14 +40,6 @@ struct msm_display_info {
};
/**
- * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
- * @encoder: encoder pointer
- * @crtc: crtc pointer
- */
-void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
- struct drm_crtc *crtc);
-
-/**
* dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on or off if
* the encoder is assigned to the given crtc
* @encoder: encoder pointer
--
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 5f2aae4..e481099 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -984,14 +984,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 a3491d2..3e8912b 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -192,12 +192,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
Use atomic variants for encoder callback functions such that
certain states like self-refresh can be accessed as part of
enable/disable sequence.
Signed-off-by: Kalyan Thota <[email protected]>
Signed-off-by: Vinod Polimera <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 0c7d8b5..9cb2542 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -1167,7 +1167,8 @@ void dpu_encoder_virt_runtime_resume(struct drm_encoder *drm_enc)
mutex_unlock(&dpu_enc->enc_lock);
}
-static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
+static void dpu_encoder_virt_atomic_enable(struct drm_encoder *drm_enc,
+ struct drm_atomic_state *state)
{
struct dpu_encoder_virt *dpu_enc = NULL;
int ret = 0;
@@ -1203,7 +1204,8 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
mutex_unlock(&dpu_enc->enc_lock);
}
-static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
+static void dpu_encoder_virt_atomic_disable(struct drm_encoder *drm_enc,
+ struct drm_atomic_state *state)
{
struct dpu_encoder_virt *dpu_enc = NULL;
int i = 0;
@@ -2387,8 +2389,8 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
.atomic_mode_set = dpu_encoder_virt_atomic_mode_set,
- .disable = dpu_encoder_virt_disable,
- .enable = dpu_encoder_virt_enable,
+ .atomic_disable = dpu_encoder_virt_atomic_disable,
+ .atomic_enable = dpu_encoder_virt_atomic_enable,
.atomic_check = dpu_encoder_virt_atomic_check,
};
--
2.7.4
This change will handle the psr entry exit cases in the panel
bridge atomic callback functions. For example, the panel power
should not turn off if the panel is entering psr.
Signed-off-by: Sankeerth Billakanti <[email protected]>
Signed-off-by: Vinod Polimera <[email protected]>
---
drivers/gpu/drm/bridge/panel.c | 48 ++++++++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index 3558cbf..5e77e38 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -113,6 +113,18 @@ static void panel_bridge_atomic_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 *atomic_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state;
+
+ crtc = drm_atomic_get_new_crtc_for_encoder(atomic_state, encoder);
+ if (!crtc)
+ return;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(atomic_state, crtc);
+ if (old_crtc_state && old_crtc_state->self_refresh_active)
+ return;
drm_panel_prepare(panel_bridge->panel);
}
@@ -121,6 +133,18 @@ static void panel_bridge_atomic_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 *atomic_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *old_crtc_state;
+
+ crtc = drm_atomic_get_new_crtc_for_encoder(atomic_state, encoder);
+ if (!crtc)
+ return;
+
+ old_crtc_state = drm_atomic_get_old_crtc_state(atomic_state, crtc);
+ if (old_crtc_state && old_crtc_state->self_refresh_active)
+ return;
drm_panel_enable(panel_bridge->panel);
}
@@ -129,6 +153,18 @@ static void panel_bridge_atomic_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 *atomic_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *new_crtc_state;
+
+ crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state, encoder);
+ if (!crtc)
+ return;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(atomic_state, crtc);
+ if (new_crtc_state && new_crtc_state->self_refresh_active)
+ return;
drm_panel_disable(panel_bridge->panel);
}
@@ -137,6 +173,18 @@ static void panel_bridge_atomic_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 *atomic_state = old_bridge_state->base.state;
+ struct drm_encoder *encoder = bridge->encoder;
+ struct drm_crtc *crtc;
+ struct drm_crtc_state *new_crtc_state;
+
+ crtc = drm_atomic_get_old_crtc_for_encoder(atomic_state, encoder);
+ if (!crtc)
+ return;
+
+ new_crtc_state = drm_atomic_get_new_crtc_state(atomic_state, crtc);
+ if (new_crtc_state && new_crtc_state->self_refresh_active)
+ return;
drm_panel_unprepare(panel_bridge->panel);
}
--
2.7.4
Use atomic variants for DP bridge callback functions so that
the atomic state can be accessed in the interface drivers.
The atomic state will help the driver find out if the display
is in self refresh state.
Signed-off-by: Sankeerth Billakanti <[email protected]>
Signed-off-by: Vinod Polimera <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/msm/dp/dp_display.c | 9 ++++++---
drivers/gpu/drm/msm/dp/dp_drm.c | 17 ++++++++++-------
drivers/gpu/drm/msm/dp/dp_drm.h | 9 ++++++---
3 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index bfd0aef..bfffc2b 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1636,7 +1636,8 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
return 0;
}
-void dp_bridge_enable(struct drm_bridge *drm_bridge)
+void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
struct msm_dp *dp = dp_bridge->dp_display;
@@ -1691,7 +1692,8 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge)
mutex_unlock(&dp_display->event_mutex);
}
-void dp_bridge_disable(struct drm_bridge *drm_bridge)
+void dp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
struct msm_dp *dp = dp_bridge->dp_display;
@@ -1702,7 +1704,8 @@ void dp_bridge_disable(struct drm_bridge *drm_bridge)
dp_ctrl_push_idle(dp_display->ctrl);
}
-void dp_bridge_post_disable(struct drm_bridge *drm_bridge)
+void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state)
{
struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
struct msm_dp *dp = dp_bridge->dp_display;
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c
index 6df25f7..43ce208 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.c
+++ b/drivers/gpu/drm/msm/dp/dp_drm.c
@@ -61,13 +61,16 @@ static int dp_bridge_get_modes(struct drm_bridge *bridge, struct drm_connector *
}
static const struct drm_bridge_funcs dp_bridge_ops = {
- .enable = dp_bridge_enable,
- .disable = dp_bridge_disable,
- .post_disable = dp_bridge_post_disable,
- .mode_set = dp_bridge_mode_set,
- .mode_valid = dp_bridge_mode_valid,
- .get_modes = dp_bridge_get_modes,
- .detect = dp_bridge_detect,
+ .atomic_enable = dp_bridge_atomic_enable,
+ .atomic_disable = dp_bridge_atomic_disable,
+ .atomic_post_disable = dp_bridge_atomic_post_disable,
+ .mode_set = dp_bridge_mode_set,
+ .mode_valid = dp_bridge_mode_valid,
+ .get_modes = dp_bridge_get_modes,
+ .detect = dp_bridge_detect,
+ .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 *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
diff --git a/drivers/gpu/drm/msm/dp/dp_drm.h b/drivers/gpu/drm/msm/dp/dp_drm.h
index 82035db..e69c0b9 100644
--- a/drivers/gpu/drm/msm/dp/dp_drm.h
+++ b/drivers/gpu/drm/msm/dp/dp_drm.h
@@ -23,9 +23,12 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display, struct dr
struct drm_bridge *dp_bridge_init(struct msm_dp *dp_display, struct drm_device *dev,
struct drm_encoder *encoder);
-void dp_bridge_enable(struct drm_bridge *drm_bridge);
-void dp_bridge_disable(struct drm_bridge *drm_bridge);
-void dp_bridge_post_disable(struct drm_bridge *drm_bridge);
+void dp_bridge_atomic_enable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state);
+void dp_bridge_atomic_disable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state);
+void dp_bridge_atomic_post_disable(struct drm_bridge *drm_bridge,
+ struct drm_bridge_state *old_bridge_state);
enum drm_mode_status dp_bridge_mode_valid(struct drm_bridge *bridge,
const struct drm_display_info *info,
const struct drm_display_mode *mode);
--
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 5a0dc54..aeeb759 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
@@ -590,6 +590,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
On 13/09/2022 20:04, Dmitry Baryshkov wrote:
> On 13/09/2022 17:51, Vinod Polimera wrote:
>> Update crtc retrieval from dpu_enc to dpu_enc connector state,
>> since new links get set as part of the dpu enc virt mode set.
>> The dpu_enc->crtc cache is no more needed, hence cleaning it as
>> part of this change.
>>
>> Signed-off-by: Vinod Polimera <[email protected]>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 4 ----
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 35
>> ++++++++++++-----------------
>> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 -------
>> 3 files changed, 14 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> index 13ce321..8ec9a13 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>> @@ -1029,7 +1029,6 @@ 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);
>> }
>> /* wait for frame_event_done completion */
>> @@ -1099,9 +1098,6 @@ 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);
>> -
>> /* Enable/restore vblank irq handling */
>> drm_crtc_vblank_on(crtc);
>> }
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 9c6817b..0c7d8b5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -132,11 +132,6 @@ enum dpu_enc_rc_states {
>> * @intfs_swapped: Whether or not the phys_enc interfaces have
>> been swapped
>> * for partial update right-only cases, such as pingpong
>> * split where virtual pingpong does not generate IRQs
>> - * @crtc: Pointer to the currently assigned crtc. Normally you
>> - * would use crtc->state->encoder_mask to determine the
>> - * link between encoder/crtc. However in this case we need
>> - * to track crtc in the disable() hook which is called
>> - * _after_ encoder_mask is cleared.
>> * @connector: If a mode is set, cached pointer to the active
>> connector
>> * @crtc_kickoff_cb: Callback into CRTC that will flush & start
>> * all CTL paths
>> @@ -181,7 +176,6 @@ struct dpu_encoder_virt {
>> bool intfs_swapped;
>> - struct drm_crtc *crtc;
>> struct drm_connector *connector;
>> struct dentry *debugfs_root;
>> @@ -1288,6 +1282,7 @@ static void dpu_encoder_vblank_callback(struct
>> drm_encoder *drm_enc,
>> struct dpu_encoder_phys *phy_enc)
>> {
>> struct dpu_encoder_virt *dpu_enc = NULL;
>> + struct drm_crtc *crtc;
>> unsigned long lock_flags;
>> if (!drm_enc || !phy_enc)
>> @@ -1298,9 +1293,14 @@ static void dpu_encoder_vblank_callback(struct
>> drm_encoder *drm_enc,
>> atomic_inc(&phy_enc->vsync_cnt);
>> + if (!dpu_enc->connector || !dpu_enc->connector->state)
>> + return;
>> +
>> + crtc = dpu_enc->connector->state->crtc;
>> +
>> spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
>> - if (dpu_enc->crtc)
>> - dpu_crtc_vblank_callback(dpu_enc->crtc);
>> + if (crtc)
>> + dpu_crtc_vblank_callback(crtc);
>> spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
>
> Stephen's comment about reading the state from outside of the lock and
> then using local variable under spinlock is still valid. Moreover I'm
> not sure that enc_spinlock protects connector's state. I'd say one has
> to take the modesetting lock here.
s/Stephen's/Doug's/
--
With best wishes
Dmitry
On 13/09/2022 17:51, Vinod Polimera wrote:
> Update crtc retrieval from dpu_enc to dpu_enc connector state,
> since new links get set as part of the dpu enc virt mode set.
> The dpu_enc->crtc cache is no more needed, hence cleaning it as
> part of this change.
>
> Signed-off-by: Vinod Polimera <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 4 ----
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 35 ++++++++++++-----------------
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 -------
> 3 files changed, 14 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 13ce321..8ec9a13 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1029,7 +1029,6 @@ 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);
> }
>
> /* wait for frame_event_done completion */
> @@ -1099,9 +1098,6 @@ 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);
> -
> /* Enable/restore vblank irq handling */
> drm_crtc_vblank_on(crtc);
> }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 9c6817b..0c7d8b5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -132,11 +132,6 @@ enum dpu_enc_rc_states {
> * @intfs_swapped: Whether or not the phys_enc interfaces have been swapped
> * for partial update right-only cases, such as pingpong
> * split where virtual pingpong does not generate IRQs
> - * @crtc: Pointer to the currently assigned crtc. Normally you
> - * would use crtc->state->encoder_mask to determine the
> - * link between encoder/crtc. However in this case we need
> - * to track crtc in the disable() hook which is called
> - * _after_ encoder_mask is cleared.
> * @connector: If a mode is set, cached pointer to the active connector
> * @crtc_kickoff_cb: Callback into CRTC that will flush & start
> * all CTL paths
> @@ -181,7 +176,6 @@ struct dpu_encoder_virt {
>
> bool intfs_swapped;
>
> - struct drm_crtc *crtc;
> struct drm_connector *connector;
>
> struct dentry *debugfs_root;
> @@ -1288,6 +1282,7 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
> struct dpu_encoder_phys *phy_enc)
> {
> struct dpu_encoder_virt *dpu_enc = NULL;
> + struct drm_crtc *crtc;
> unsigned long lock_flags;
>
> if (!drm_enc || !phy_enc)
> @@ -1298,9 +1293,14 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,
>
> atomic_inc(&phy_enc->vsync_cnt);
>
> + if (!dpu_enc->connector || !dpu_enc->connector->state)
> + return;
> +
> + crtc = dpu_enc->connector->state->crtc;
> +
> spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> - if (dpu_enc->crtc)
> - dpu_crtc_vblank_callback(dpu_enc->crtc);
> + if (crtc)
> + dpu_crtc_vblank_callback(crtc);
> spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
Stephen's comment about reading the state from outside of the lock and
then using local variable under spinlock is still valid. Moreover I'm
not sure that enc_spinlock protects connector's state. I'd say one has
to take the modesetting lock here.
>
> DPU_ATRACE_END("encoder_vblank_callback");
> @@ -1324,29 +1324,22 @@ static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc,
> DPU_ATRACE_END("encoder_underrun_callback");
> }
>
> -void dpu_encoder_assign_crtc(struct drm_encoder *drm_enc, struct drm_crtc *crtc)
> -{
> - struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> - unsigned long lock_flags;
> -
> - spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> - /* crtc should always be cleared before re-assigning */
> - WARN_ON(crtc && dpu_enc->crtc);
> - dpu_enc->crtc = crtc;
> - spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> -}
> -
> void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc,
> struct drm_crtc *crtc, bool enable)
> {
> struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> + struct drm_crtc *new_crtc;
> unsigned long lock_flags;
> int i;
>
> trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
>
> + if (!dpu_enc->connector || !dpu_enc->connector->state)
> + return;
> +
> + new_crtc = dpu_enc->connector->state->crtc;
> spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> - if (dpu_enc->crtc != crtc) {
> + if (!new_crtc || new_crtc != crtc) {
> spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> return;
> }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 9e7236e..eda5cd8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -40,14 +40,6 @@ struct msm_display_info {
> };
>
> /**
> - * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to
> - * @encoder: encoder pointer
> - * @crtc: crtc pointer
> - */
> -void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
> - struct drm_crtc *crtc);
> -
> -/**
> * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on or off if
> * the encoder is assigned to the given crtc
> * @encoder: encoder pointer
--
With best wishes
Dmitry
> -----Original Message-----
> From: Dmitry Baryshkov <[email protected]>
> Sent: Tuesday, September 13, 2022 10:36 PM
> To: Vinod Polimera (QUIC) <[email protected]>; dri-
> [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: [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]>; Aravind Venkateswaran (QUIC)
> <[email protected]>; Abhinav Kumar (QUIC)
> <[email protected]>; Sankeerth Billakanti (QUIC)
> <[email protected]>
> Subject: Re: [PATCH v7 01/15] drm/msm/disp/dpu: clear dpu_assign_crtc and
> get crtc from connector state instead of dpu_enc
>
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
>
> On 13/09/2022 20:04, Dmitry Baryshkov wrote:
> > On 13/09/2022 17:51, Vinod Polimera wrote:
> >> Update crtc retrieval from dpu_enc to dpu_enc connector state,
> >> since new links get set as part of the dpu enc virt mode set.
> >> The dpu_enc->crtc cache is no more needed, hence cleaning it as
> >> part of this change.
> >>
> >> Signed-off-by: Vinod Polimera <[email protected]>
> >> ---
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 4 ----
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 35
> >> ++++++++++++-----------------
> >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 -------
> >> 3 files changed, 14 insertions(+), 33 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> index 13ce321..8ec9a13 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> >> @@ -1029,7 +1029,6 @@ 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);
> >> }
> >> /* wait for frame_event_done completion */
> >> @@ -1099,9 +1098,6 @@ 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);
> >> -
> >> /* Enable/restore vblank irq handling */
> >> drm_crtc_vblank_on(crtc);
> >> }
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> index 9c6817b..0c7d8b5 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> >> @@ -132,11 +132,6 @@ enum dpu_enc_rc_states {
> >> * @intfs_swapped: Whether or not the phys_enc interfaces have
> >> been swapped
> >> * for partial update right-only cases, such as pingpong
> >> * split where virtual pingpong does not generate IRQs
> >> - * @crtc: Pointer to the currently assigned crtc. Normally you
> >> - * would use crtc->state->encoder_mask to determine the
> >> - * link between encoder/crtc. However in this case we need
> >> - * to track crtc in the disable() hook which is called
> >> - * _after_ encoder_mask is cleared.
> >> * @connector: If a mode is set, cached pointer to the active
> >> connector
> >> * @crtc_kickoff_cb: Callback into CRTC that will flush & start
> >> * all CTL paths
> >> @@ -181,7 +176,6 @@ struct dpu_encoder_virt {
> >> bool intfs_swapped;
> >> - struct drm_crtc *crtc;
> >> struct drm_connector *connector;
> >> struct dentry *debugfs_root;
> >> @@ -1288,6 +1282,7 @@ static void dpu_encoder_vblank_callback(struct
> >> drm_encoder *drm_enc,
> >> struct dpu_encoder_phys *phy_enc)
> >> {
> >> struct dpu_encoder_virt *dpu_enc = NULL;
> >> + struct drm_crtc *crtc;
> >> unsigned long lock_flags;
> >> if (!drm_enc || !phy_enc)
> >> @@ -1298,9 +1293,14 @@ static void
> dpu_encoder_vblank_callback(struct
> >> drm_encoder *drm_enc,
> >> atomic_inc(&phy_enc->vsync_cnt);
> >> + if (!dpu_enc->connector || !dpu_enc->connector->state)
> >> + return;
> >> +
> >> + crtc = dpu_enc->connector->state->crtc;
> >> +
> >> spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> >> - if (dpu_enc->crtc)
> >> - dpu_crtc_vblank_callback(dpu_enc->crtc);
> >> + if (crtc)
> >> + dpu_crtc_vblank_callback(crtc);
> >> spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> >
> > Stephen's comment about reading the state from outside of the lock and
> > then using local variable under spinlock is still valid. Moreover I'm
> > not sure that enc_spinlock protects connector's state. I'd say one has
> > to take the modesetting lock here.
>
> s/Stephen's/Doug's/
As this is in interrupt context , we cannot use modeset lock here and connector does not have the spinlock to protect state.
since state swap happens after commit to hw done, which is after vblank processing, i was thinking it should be okay to drop the spinlock while accessing connector state.
I have updated the patch with the changes in v8, please do review.
>
> --
> With best wishes
> Dmitry
Thanks
Vinod P.