Please let me know your thought and review feedback if something is not acceptable, will modify accordingly, as there is no specific way, it can be taken care multiple way.
Regards,
Animesh
>
> >
> > Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_alpm.c | 3 +-
> > drivers/gpu/drm/i915/display/intel_alpm.h | 6 ++-
> > drivers/gpu/drm/i915/display/intel_display.c | 54
> > +++++++++++++++++++- drivers/gpu/drm/i915/display/intel_display.h | 2
> +
> > drivers/gpu/drm/i915/display/intel_dp.c | 20 +++++++-
> > drivers/gpu/drm/i915/display/intel_dp.h | 2 +
> > drivers/gpu/drm/i915/display/intel_psr.c | 12 ++---
> > drivers/gpu/drm/i915/display/intel_vrr.c | 13 -----
> > 8 files changed, 84 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c
> > b/drivers/gpu/drm/i915/display/intel_alpm.c
> > index 866b3b409c4d..021e970d8209 100644
> > --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> > +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> > @@ -266,8 +266,7 @@ bool intel_alpm_compute_params(struct intel_dp
> > *intel_dp, }
> >
> > void intel_alpm_lobf_compute_config(struct intel_dp *intel_dp,
> > - struct intel_crtc_state *crtc_state,
> > - struct drm_connector_state *conn_state)
> > + struct intel_crtc_state *crtc_state)
> > {
> > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > struct drm_display_mode *adjusted_mode =
> > &crtc_state->hw.adjusted_mode; diff --git
> > a/drivers/gpu/drm/i915/display/intel_alpm.h
> > b/drivers/gpu/drm/i915/display/intel_alpm.h
> > index 8c409b10dce6..8abe7bd9bc37 100644
> > --- a/drivers/gpu/drm/i915/display/intel_alpm.h
> > +++ b/drivers/gpu/drm/i915/display/intel_alpm.h
> > @@ -16,9 +16,11 @@ struct intel_connector; void
> > intel_alpm_init_dpcd(struct intel_dp *intel_dp); bool
> > intel_alpm_compute_params(struct intel_dp *intel_dp,
> > const struct intel_crtc_state *crtc_state);
> > +bool intel_alpm_config_valid(struct intel_dp *intel_dp,
> > + const struct intel_crtc_state *crtc_state,
> > + bool aux_less);
> > void intel_alpm_lobf_compute_config(struct intel_dp *intel_dp,
> > - struct intel_crtc_state *crtc_state,
> > - struct drm_connector_state *conn_state);
> > + struct intel_crtc_state *crtc_state);
> > void intel_alpm_configure(struct intel_dp *intel_dp,
> > const struct intel_crtc_state *crtc_state); void
> > intel_alpm_lobf_debugfs_add(struct intel_connector *connector); diff
> > --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index c2c388212e2e..cdab71f81eaa 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -2512,9 +2512,21 @@ static int intel_crtc_compute_pipe_mode(struct
> > intel_crtc_state *crtc_state) static int intel_crtc_compute_config(struct
> intel_atomic_state *state,
> > struct intel_crtc *crtc)
> > {
> > + struct drm_connector *connector;
> > + struct drm_connector_state *connector_state;
> > struct intel_crtc_state *crtc_state =
> > intel_atomic_get_new_crtc_state(state, crtc);
> > - int ret;
> > + int ret, i;
> > +
> > + for_each_new_connector_in_state(&state->base, connector,
> connector_state, i) {
> > + struct intel_encoder *encoder =
> > + to_intel_encoder(connector_state->best_encoder);
> > +
> > + if (connector_state->crtc != &crtc->base)
> > + continue;
> > +
> > + intel_crtc_adjust_vblank_delay(crtc_state, encoder);
> > + }
> >
> > ret = intel_dpll_crtc_compute_clock(state, crtc);
> > if (ret)
> > @@ -3925,6 +3937,46 @@ bool intel_crtc_get_pipe_config(struct
> intel_crtc_state *crtc_state)
> > return true;
> > }
> >
> > +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state *crtc_state,
> > + struct intel_encoder *encoder)
>
> This could be static.
>
> > +{
> > + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > + struct drm_display_mode *adjusted_mode =
> > +&crtc_state->hw.adjusted_mode;
> > +
> > + /*
> > + * wa_14015401596 for display versions 13, 14.
> > + * Program Set Context Latency in TRANS_SET_CONTEXT_LATENCY
> register
> > + * to at least a value of 1 when Panel Replay is enabled with VRR.
> > + * Value for TRANS_SET_CONTEXT_LATENCY is calculated by
> substracting
> > + * crtc_vdisplay from crtc_vblank_start, so incrementing
> crtc_vblank_start
> > + * by 1 if both are equal.
> > + */
> > + if (crtc_state->vrr.enable && crtc_state->has_panel_replay &&
> > + adjusted_mode->crtc_vblank_start == adjusted_mode-
> >crtc_vdisplay &&
> > + IS_DISPLAY_VER(to_i915(crtc->base.dev), 13, 14))
> > + adjusted_mode->crtc_vblank_start += 1;
> > +
> > + if (crtc_state->vrr.enable) {
> > + /*
> > + * For XE_LPD+, we use guardband and pipeline override
> > + * is deprecated.
> > + */
> > + if (DISPLAY_VER(to_i915(crtc->base.dev)) >= 13) {
> > + crtc_state->vrr.guardband =
> > + crtc_state->vrr.vmin + 1 - adjusted_mode-
> >crtc_vblank_start;
> > + } else {
> > + crtc_state->vrr.pipeline_full =
> > + min(255, crtc_state->vrr.vmin - adjusted_mode-
> >crtc_vblank_start -
>
> Misleading indentation (see where you're copy-pasting this from).
>
> > + crtc_state->framestart_delay - 1);
> > + }
>
> I think only intel_vrr.c should be modifying vrr.guardband, vrr.pipeline_full.
>
> > + }
> > +
> > + if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP) ||
> > + intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) {
> > + intel_dp_late_compute_config(encoder, crtc_state);
>
> So, ugh. Naming this intel_dp_late_compute_config() is *very* confusing.
>
> We have intel_modeset_pipe_config() calling encoder->compute_config().
>
> Now you're adding intel_modeset_pipe_config() calling
> intel_crtc_compute_config() calling intel_crtc_adjust_vblank_delay() calling
> intel_dp_late_compute_config().
>
> Yet there's separate intel_modeset_pipe_config_late() and
> encoder->compute_config_late().
>
> In general, it's very surprising to have a function called
> intel_crtc_adjust_vblank_delay() do *much* more than, well, adjust vblank
> delay.
>
> > + }
> > +}
> > +
> > int intel_dotclock_calculate(int link_freq,
> > const struct intel_link_m_n *m_n) { diff --git
> > a/drivers/gpu/drm/i915/display/intel_display.h
> > b/drivers/gpu/drm/i915/display/intel_display.h
> > index b0cf6ca70952..21fd330b8834 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -428,6 +428,8 @@ bool intel_crtc_is_joiner_primary(const struct
> > intel_crtc_state *crtc_state);
> > u8 intel_crtc_joiner_secondary_pipes(const struct intel_crtc_state
> > *crtc_state); struct intel_crtc *intel_primary_crtc(const struct
> > intel_crtc_state *crtc_state); bool intel_crtc_get_pipe_config(struct
> > intel_crtc_state *crtc_state);
> > +void intel_crtc_adjust_vblank_delay(struct intel_crtc_state *crtc_state,
> > + struct intel_encoder *encoder);
> > bool intel_pipe_config_compare(const struct intel_crtc_state
> *current_config,
> > const struct intel_crtc_state *pipe_config,
> > bool fastset);
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 3903f6ead6e6..e01e72ec48b3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -3009,7 +3009,6 @@ intel_dp_compute_config(struct intel_encoder
> *encoder,
> > intel_vrr_compute_config(pipe_config, conn_state);
> > intel_dp_compute_as_sdp(intel_dp, pipe_config);
> > intel_psr_compute_config(intel_dp, pipe_config, conn_state);
> > - intel_alpm_lobf_compute_config(intel_dp, pipe_config, conn_state);
> > intel_dp_drrs_compute_config(connector, pipe_config,
> link_bpp_x16);
> > intel_dp_compute_vsc_sdp(intel_dp, pipe_config, conn_state);
> > intel_dp_compute_hdr_metadata_infoframe_sdp(intel_dp,
> pipe_config,
> > conn_state); @@ -3018,6 +3017,25 @@ intel_dp_compute_config(struct
> intel_encoder *encoder,
> > pipe_config);
> > }
> >
> > +void intel_dp_late_compute_config(struct intel_encoder *encoder,
> > + struct intel_crtc_state *crtc_state) {
> > + struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +
> > + if (crtc_state->has_sel_update &&
> > + !intel_alpm_config_valid(intel_dp, crtc_state, false)) {
> > + crtc_state->enable_psr2_sel_fetch = false;
> > + crtc_state->has_sel_update = false;
> > + }
> > +
> > + if (crtc_state->has_panel_replay && intel_dp_is_edp(intel_dp) &&
> > + !intel_alpm_config_valid(intel_dp, crtc_state, false)) {
> > + crtc_state->has_panel_replay = false;
> > + }
>
> I think only intel_psr.c should be modifying enable_psr2_sel_fetch,
> has_sel_update, has_panel_replay.
>
> > +
> > + intel_alpm_lobf_compute_config(intel_dp, crtc_state); }
> > +
> > void intel_dp_set_link_params(struct intel_dp *intel_dp,
> > int link_rate, int lane_count) { diff --git
> > a/drivers/gpu/drm/i915/display/intel_dp.h
> > b/drivers/gpu/drm/i915/display/intel_dp.h
> > index a0f990a95ecc..cd473f939941 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > @@ -74,6 +74,8 @@ void intel_dp_encoder_flush_work(struct
> drm_encoder
> > *encoder); int intel_dp_compute_config(struct intel_encoder *encoder,
> > struct intel_crtc_state *pipe_config,
> > struct drm_connector_state *conn_state);
> > +void intel_dp_late_compute_config(struct intel_encoder *encoder,
> > + struct intel_crtc_state *crtc_state);
> > int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > struct intel_crtc_state *pipe_config,
> > struct drm_connector_state *conn_state, diff
> --git
> > a/drivers/gpu/drm/i915/display/intel_psr.c
> > b/drivers/gpu/drm/i915/display/intel_psr.c
> > index 9cb1cdaaeefa..4dc917b7f447 100644
> > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > @@ -1344,9 +1344,9 @@ static bool wake_lines_fit_into_vblank(struct
> intel_dp *intel_dp,
> > return true;
> > }
> >
> > -static bool alpm_config_valid(struct intel_dp *intel_dp,
> > - const struct intel_crtc_state *crtc_state,
> > - bool aux_less)
> > +bool intel_alpm_config_valid(struct intel_dp *intel_dp,
> > + const struct intel_crtc_state *crtc_state,
> > + bool aux_less)
> > {
> > struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> >
> > @@ -1442,9 +1442,6 @@ static bool intel_psr2_config_valid(struct intel_dp
> *intel_dp,
> > return false;
> > }
> >
> > - if (!alpm_config_valid(intel_dp, crtc_state, false))
> > - return false;
> > -
>
> The commit message does not explain this.
>
> > if (!crtc_state->enable_psr2_sel_fetch &&
> > (crtc_hdisplay > psr_max_h || crtc_vdisplay > psr_max_v)) {
> > drm_dbg_kms(&dev_priv->drm,
> > @@ -1583,9 +1580,6 @@ _panel_replay_compute_config(struct intel_dp
> *intel_dp,
> > return false;
> > }
> >
> > - if (!alpm_config_valid(intel_dp, crtc_state, true))
> > - return false;
> > -
>
> Ditto.
>
> > return true;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index 5a0da64c7db3..46341367d250 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -242,19 +242,6 @@ intel_vrr_compute_config(struct intel_crtc_state
> *crtc_state,
> > (crtc_state->hw.adjusted_mode.crtc_vtotal -
> > crtc_state->hw.adjusted_mode.vsync_end);
> > }
> > -
> > - /*
> > - * For XE_LPD+, we use guardband and pipeline override
> > - * is deprecated.
> > - */
> > - if (DISPLAY_VER(i915) >= 13) {
> > - crtc_state->vrr.guardband =
> > - crtc_state->vrr.vmin + 1 - adjusted_mode-
> >crtc_vblank_start;
> > - } else {
> > - crtc_state->vrr.pipeline_full =
> > - min(255, crtc_state->vrr.vmin - adjusted_mode-
> >crtc_vblank_start -
> > - crtc_state->framestart_delay - 1);
> > - }
> > }
> >
> > static u32 trans_vrr_ctl(const struct intel_crtc_state *crtc_state)
>
> --
> Jani Nikula, Intel
