On Sat, 12 Feb 2022 at 15:52, Vinod Polimera <quic_vpolimer@xxxxxxxxxxx> wrote:
>
> Enable PSR on eDP interface using drm self-refresh librabry.
> This patch uses a trigger from self-refresh library to enter/exit
> into PSR, when there are no updates from framework.
>
> Signed-off-by: Vinod Polimera <quic_vpolimer@xxxxxxxxxxx>
> Signed-off-by: Kalyan Thota <quic_kalyant@xxxxxxxxxxx>
The S-O-B of the sender should come last. Please change the order of
the S-O-B tags.
> ---
> drivers/gpu/drm/bridge/panel.c | 64 +++++++++++++++++++++-----
This chunk should come in a separate patch. Please do not mix drm/msm
patches with the generic code.
> drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 30 +++++++++---
> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 71 ++++++++++++++++++++++++++---
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 2 +-
> 4 files changed, 142 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index b32295a..c440546 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -102,31 +102,71 @@ static void panel_bridge_detach(struct drm_bridge *bridge)
> drm_connector_cleanup(connector);
> }
>
> -static void panel_bridge_pre_enable(struct drm_bridge *bridge)
> +static void panel_bridge_pre_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> -
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *cstate;
> + int i;
> +
> + if (old_bridge_state->base.state) {
> + for_each_old_crtc_in_state(old_bridge_state->base.state, crtc, cstate, i) {
> + if (cstate->self_refresh_active && cstate->active)
> + return;
> + }
Ugh. No. You will skip panel actions if _any_ of the crtc's has PSR
enabled. There might be other CRTCs in play.
Please see analogix_dp_core.c for a proper way to handle this.
Consider moving common code (e.g. getting the crtc used by the
encoder) pieces to the generic drm code.
> + }
> drm_panel_prepare(panel_bridge->panel);
> }
>
> -static void panel_bridge_enable(struct drm_bridge *bridge)
> +static void panel_bridge_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> -
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *cstate;
> + int i;
> +
> + if (old_bridge_state->base.state) {
> + for_each_old_crtc_in_state(old_bridge_state->base.state, crtc, cstate, i) {
> + if (cstate->self_refresh_active)
> + return;
> + }
> + }
> drm_panel_enable(panel_bridge->panel);
> }
>
> -static void panel_bridge_disable(struct drm_bridge *bridge)
> +static void panel_bridge_disable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> -
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *cstate;
> + int i;
> +
> + if (old_bridge_state->base.state) {
> + for_each_new_crtc_in_state(old_bridge_state->base.state, crtc, cstate, i) {
> + if (cstate->self_refresh_active)
> + return;
> + }
> + }
> drm_panel_disable(panel_bridge->panel);
> }
>
> -static void panel_bridge_post_disable(struct drm_bridge *bridge)
> +static void panel_bridge_post_disable(struct drm_bridge *bridge,
> + struct drm_bridge_state *old_bridge_state)
> {
> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
> -
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *cstate;
> + int i;
> +
> + if (old_bridge_state->base.state) {
> + for_each_new_crtc_in_state(old_bridge_state->base.state, crtc, cstate, i) {
> + if (cstate->self_refresh_active)
> + return;
> + }
> + }
> drm_panel_unprepare(panel_bridge->panel);
> }
>
> @@ -141,10 +181,10 @@ static int panel_bridge_get_modes(struct drm_bridge *bridge,
> static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
> .attach = panel_bridge_attach,
> .detach = panel_bridge_detach,
> - .pre_enable = panel_bridge_pre_enable,
> - .enable = panel_bridge_enable,
> - .disable = panel_bridge_disable,
> - .post_disable = panel_bridge_post_disable,
> + .atomic_pre_enable = panel_bridge_pre_enable,
> + .atomic_enable = panel_bridge_enable,
> + .atomic_disable = panel_bridge_disable,
> + .atomic_post_disable = panel_bridge_post_disable,
> .get_modes = panel_bridge_get_modes,
> .atomic_reset = drm_atomic_helper_bridge_reset,
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index e7c9fe1..90223b8 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -18,6 +18,7 @@
> #include <drm/drm_probe_helper.h>
> #include <drm/drm_rect.h>
> #include <drm/drm_vblank.h>
> +#include <drm/drm_self_refresh_helper.h>
>
> #include "dpu_kms.h"
> #include "dpu_hw_lm.h"
> @@ -457,7 +458,6 @@ static void _dpu_crtc_blend_setup(struct drm_crtc *crtc)
> mixer[i].mixer_op_mode,
> ctl->idx - CTL_0,
> mixer[i].flush_mask);
> -
Unnecessary
> ctl->ops.setup_blendstage(ctl, mixer[i].hw_lm->idx,
> &stage_cfg);
> }
> @@ -951,6 +951,14 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
>
> DRM_DEBUG_KMS("crtc%d\n", crtc->base.id);
>
> + if (old_crtc_state->self_refresh_active) {
> + drm_for_each_encoder_mask(encoder, crtc->dev,
> + old_crtc_state->encoder_mask) {
> + dpu_encoder_assign_crtc(encoder, NULL);
> + }
> + return;
> + }
> +
> /* Disable/save vblank irq handling */
> drm_crtc_vblank_off(crtc);
>
> @@ -962,7 +970,8 @@ 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 (!crtc->state->self_refresh_active)
> + dpu_encoder_assign_crtc(encoder, NULL);
This deserves some comments in the code
> }
>
> /* wait for frame_event_done completion */
> @@ -1010,6 +1019,8 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
> struct dpu_crtc *dpu_crtc = to_dpu_crtc(crtc);
> struct drm_encoder *encoder;
> bool request_bandwidth = false;
> + struct drm_crtc_state *old_crtc_state =
> + drm_atomic_get_old_crtc_state(state, crtc);
>
> pm_runtime_get_sync(crtc->dev->dev);
>
> @@ -1032,8 +1043,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);
> + }
This deserves some comments in the code
>
> /* Enable/restore vblank irq handling */
> drm_crtc_vblank_on(crtc);
> @@ -1069,7 +1082,7 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>
> pstates = kzalloc(sizeof(*pstates) * DPU_STAGE_MAX * 4, GFP_KERNEL);
>
> - if (!crtc_state->enable || !crtc_state->active) {
> + if (!crtc_state->enable || !crtc_state->active || crtc_state->self_refresh_active) {
This does not seem correct to me. According to the docs: "When in self
refresh mode, the crtc_state->active value will be false, since the
CRTC is off."
> DRM_DEBUG_ATOMIC("crtc%d -> enable %d, active %d, skip atomic_check\n",
> crtc->base.id, crtc_state->enable,
> crtc_state->active);
> @@ -1497,7 +1510,7 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
> {
> struct drm_crtc *crtc = NULL;
> struct dpu_crtc *dpu_crtc = NULL;
> - int i;
> + int i, ret;
>
> dpu_crtc = kzalloc(sizeof(*dpu_crtc), GFP_KERNEL);
> if (!dpu_crtc)
> @@ -1534,6 +1547,11 @@ struct drm_crtc *dpu_crtc_init(struct drm_device *dev, struct drm_plane *plane,
> /* initialize event handling */
> spin_lock_init(&dpu_crtc->event_lock);
>
> + ret = drm_self_refresh_helper_init(crtc);
> + if (ret)
> + DPU_ERROR("Failed to initialize %s with SR helpers %d\n",
> + crtc->name, ret);
> +
> DRM_DEBUG_KMS("%s: successfully initialized crtc\n", dpu_crtc->name);
> return crtc;
> }
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1e648db..461fdd1 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -217,6 +217,14 @@ static u32 dither_matrix[DITHER_MATRIX_SZ] = {
> 15, 7, 13, 5, 3, 11, 1, 9, 12, 4, 14, 6, 0, 8, 2, 10
> };
>
> +static inline bool is_self_refresh_active(struct drm_crtc_state *state)
> +{
> + if (state && state->self_refresh_active)
> + return true;
> +
> + return false;
> +}
> +
> static void _dpu_encoder_setup_dither(struct dpu_hw_pingpong *hw_pp, unsigned bpc)
> {
> struct dpu_hw_dither_cfg dither_cfg = { 0 };
> @@ -600,6 +608,9 @@ static int dpu_encoder_virt_atomic_check(
>
> trace_dpu_enc_atomic_check(DRMID(drm_enc));
>
> + if (crtc_state->self_refresh_active)
> + return ret;
> +
Why?
> /* perform atomic check on the first physical encoder (master) */
> for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> @@ -1138,15 +1149,19 @@ 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_enable(struct drm_encoder *drm_enc,
> + struct drm_atomic_state *state)
> {
> struct dpu_encoder_virt *dpu_enc = NULL;
> int ret = 0;
> struct msm_drm_private *priv;
> struct drm_display_mode *cur_mode = NULL;
> + struct drm_crtc_state *old_crtc_state;
> + struct drm_crtc *crtc;
>
> dpu_enc = to_dpu_encoder_virt(drm_enc);
>
> + crtc = dpu_enc->crtc;
> mutex_lock(&dpu_enc->enc_lock);
> cur_mode = &dpu_enc->base.crtc->state->adjusted_mode;
> priv = drm_enc->dev->dev_private;
> @@ -1170,21 +1185,59 @@ static void dpu_encoder_virt_enable(struct drm_encoder *drm_enc)
>
> _dpu_encoder_virt_enable_helper(drm_enc);
>
> - dpu_enc->enabled = true;
> + /* Coming back from self refresh, exit PSR */
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS &&
> + is_self_refresh_active(old_crtc_state))
> + msm_dp_display_set_psr(dpu_enc->dp, false);
>
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS &&
> + !is_self_refresh_active(old_crtc_state)) {
> + ret = msm_dp_display_enable(dpu_enc->dp, drm_enc);
You shouldn't call msm_dp_display_*() directly.
I might be wrong, but I think that these function calls should be
moved into dp_bridge code instead of taking place here.
> + if (ret) {
> + DPU_ERROR_ENC(dpu_enc, "dp display enable failed: %d\n",
> + ret);
> + goto out;
> + }
> + }
> +
> + dpu_enc->enabled = true;
> out:
> mutex_unlock(&dpu_enc->enc_lock);
> }
>
> -static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
> +static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc,
> + struct drm_atomic_state *state)
> {
> struct dpu_encoder_virt *dpu_enc = NULL;
> struct msm_drm_private *priv;
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *old_state;
> int i = 0;
>
> dpu_enc = to_dpu_encoder_virt(drm_enc);
> DPU_DEBUG_ENC(dpu_enc, "\n");
>
> + if (!drm_enc) {
> + DPU_ERROR("invalid encoder\n");
> + return;
> + }
> + dpu_enc = to_dpu_encoder_virt(drm_enc);
> +
> + crtc = dpu_enc->crtc;
> +
> + /* Enter PSR if encoder supports */
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && is_self_refresh_active(crtc->state))
> + msm_dp_display_set_psr(dpu_enc->dp, true);
> +
> + old_state = drm_atomic_get_old_crtc_state(state, crtc);
> +
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && is_self_refresh_active(old_state)) {
> + msm_dp_display_set_psr(dpu_enc->dp, false);
> + if (msm_dp_display_disable(dpu_enc->dp, drm_enc))
> + DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
> + return;
> + }
> +
> mutex_lock(&dpu_enc->enc_lock);
> dpu_enc->enabled = false;
>
> @@ -1194,6 +1247,9 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>
> /* wait for idle */
> dpu_encoder_wait_for_event(drm_enc, MSM_ENC_TX_COMPLETE);
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && !is_self_refresh_active(crtc->state))
> + if (msm_dp_display_pre_disable(dpu_enc->dp, drm_enc))
> + DPU_ERROR_ENC(dpu_enc, "dp display push idle failed\n");
>
> dpu_encoder_resource_control(drm_enc, DPU_ENC_RC_EVENT_PRE_STOP);
>
> @@ -1204,7 +1260,6 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
> phys->ops.disable(phys);
> }
>
> -
> /* after phys waits for frame-done, should be no more frames pending */
> if (atomic_xchg(&dpu_enc->frame_done_timeout_ms, 0)) {
> DPU_ERROR("enc%d timeout pending\n", drm_enc->base.id);
> @@ -1219,6 +1274,10 @@ static void dpu_encoder_virt_disable(struct drm_encoder *drm_enc)
>
> DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
>
> + if (drm_enc->encoder_type == DRM_MODE_ENCODER_TMDS && !is_self_refresh_active(crtc->state))
> + if (msm_dp_display_disable(dpu_enc->dp, drm_enc))
> + DPU_ERROR_ENC(dpu_enc, "dp display disable failed\n");
> +
> mutex_unlock(&dpu_enc->enc_lock);
> }
>
> @@ -2094,8 +2153,8 @@ static void dpu_encoder_frame_done_timeout(struct timer_list *t)
>
> static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
> .mode_set = dpu_encoder_virt_mode_set,
> - .disable = dpu_encoder_virt_disable,
> - .enable = dpu_encoder_virt_enable,
> + .atomic_disable = dpu_encoder_virt_disable,
> + .atomic_enable = dpu_encoder_virt_enable,
Please split this into a separate commit. First you convert
dpu_encoder to use atomic_enable/atomic_disable, then you add PSR
support.
> .atomic_check = dpu_encoder_virt_atomic_check,
> };
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 47fe11a..aed8e09 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -495,7 +495,7 @@ static void dpu_kms_wait_for_commit_done(struct msm_kms *kms,
> return;
> }
>
> - if (!crtc->state->active) {
> + if (!crtc->state->active && !crtc->state->self_refresh_active) {
drm_atomic_crtc_effectively_active() ?
> DPU_DEBUG("[crtc:%d] not active\n", crtc->base.id);
> return;
> }
> --
> 2.7.4
>
--
With best wishes
Dmitry
