> -----Original Message-----
> From: Intel-gfx <intel-gfx-boun...@lists.freedesktop.org> On Behalf Of Mitul
> Golani
> Sent: Thursday, May 9, 2024 1:28 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Shankar, Uma <uma.shan...@intel.com>; Nikula, Jani
> <jani.nik...@intel.com>
> Subject: [PATCH v8 2/7] drm/i915: Add Enable/Disable for CMRR based on VRR
> state
>
> Add CMRR/Fixed Average Vtotal mode enable and disable functions based on
> change in VRR mode of operation.
> When Adaptive Sync Vtotal is enabled, Fixed Average Vtotal mode is disabled
> and vice versa. With this commit setting the stage for subsequent CMRR
> enablement.
>
> --v2:
> - Check pipe active state in cmrr enabling. [Jani]
> - Remove usage of bitwise OR on booleans. [Jani]
> - Revert unrelated changes. [Jani]
> - Update intel_vrr_enable, vrr and cmrr enable conditions. [Jani]
> - Simplify whole if-ladder in intel_vrr_enable. [Jani]
> - Revert patch restructuring mistakes in intel_vrr_get_config. [Jani]
>
> --v3:
> - Check pipe active state in cmrr disabling.[Jani]
> - Correct messed up condition in intel_vrr_enable. [Jani]
>
> --v4:
> - Removing RFC tag.
>
> --v5:
> - CMRR handling in co-existatnce of LRR and DRRS.
>
> --v7:
> - Rebase on top of AS SDP merge.
>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.gol...@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 37 ++++++++++++++++---
> drivers/gpu/drm/i915/display/intel_vrr.c | 38 ++++++++++++--------
> 2 files changed, 56 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index 258a78447fba..4a5318ab017d 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1020,6 +1020,18 @@ static bool vrr_enabling(const struct
> intel_crtc_state *old_crtc_state,
> vrr_params_changed(old_crtc_state, new_crtc_state))); }
>
Can the below functions be moved from intel_display.c to intel_vrr.c?
Thanks and Regards,
Arun R Murthy
-------------------
> +static bool cmrr_enabling(const struct intel_crtc_state *old_crtc_state,
> + const struct intel_crtc_state *new_crtc_state) {
> + if (!new_crtc_state->hw.active)
> + return false;
> +
> + return is_enabling(cmrr.enable, old_crtc_state, new_crtc_state) ||
> + (new_crtc_state->cmrr.enable &&
> + (new_crtc_state->update_m_n || new_crtc_state->update_lrr
> ||
> + cmrr_params_changed(old_crtc_state, new_crtc_state))); }
> +
> static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
> const struct intel_crtc_state *new_crtc_state) { @@ -
> 1032,6 +1044,18 @@ static bool vrr_disabling(const struct intel_crtc_state
> *old_crtc_state,
> vrr_params_changed(old_crtc_state, new_crtc_state))); }
>
> +static bool cmrr_disabling(const struct intel_crtc_state *old_crtc_state,
> + const struct intel_crtc_state *new_crtc_state) {
> + if (!old_crtc_state->hw.active)
> + return false;
> +
> + return is_disabling(cmrr.enable, old_crtc_state, new_crtc_state) ||
> + (old_crtc_state->cmrr.enable &&
> + (new_crtc_state->update_m_n || new_crtc_state->update_lrr
> ||
> + cmrr_params_changed(old_crtc_state, new_crtc_state))); }
> +
> static bool audio_enabling(const struct intel_crtc_state *old_crtc_state,
> const struct intel_crtc_state *new_crtc_state) { @@
> -
> 1053,7 +1077,6 @@ static bool audio_disabling(const struct intel_crtc_state
> *old_crtc_state,
> (old_crtc_state->has_audio &&
> memcmp(old_crtc_state->eld, new_crtc_state->eld,
> MAX_ELD_BYTES) != 0); }
> -
> #undef is_disabling
> #undef is_enabling
>
> @@ -1175,7 +1198,8 @@ static void intel_pre_plane_update(struct
> intel_atomic_state *state,
> intel_atomic_get_new_crtc_state(state, crtc);
> enum pipe pipe = crtc->pipe;
>
> - if (vrr_disabling(old_crtc_state, new_crtc_state)) {
> + if (vrr_disabling(old_crtc_state, new_crtc_state) ||
> + cmrr_disabling(old_crtc_state, new_crtc_state)) {
> intel_vrr_disable(old_crtc_state);
> intel_crtc_update_active_timings(old_crtc_state, false);
> }
> @@ -6767,7 +6791,8 @@ static void commit_pipe_post_planes(struct
> intel_atomic_state *state,
> !intel_crtc_needs_modeset(new_crtc_state))
> skl_detach_scalers(new_crtc_state);
>
> - if (vrr_enabling(old_crtc_state, new_crtc_state))
> + if (vrr_enabling(old_crtc_state, new_crtc_state) ||
> + cmrr_enabling(old_crtc_state, new_crtc_state))
> intel_vrr_enable(new_crtc_state);
> }
>
> @@ -6868,9 +6893,11 @@ static void intel_update_crtc(struct
> intel_atomic_state *state,
> * FIXME Should be synchronized with the start of vblank somehow...
> */
> if (vrr_enabling(old_crtc_state, new_crtc_state) ||
> - new_crtc_state->update_m_n || new_crtc_state->update_lrr)
> + new_crtc_state->update_m_n || new_crtc_state->update_lrr ||
> + cmrr_enabling(old_crtc_state, new_crtc_state))
> intel_crtc_update_active_timings(new_crtc_state,
> - new_crtc_state->vrr.enable);
> + new_crtc_state->vrr.enable ||
> + new_crtc_state->cmrr.enable);
>
> /*
> * We usually enable FIFO underrun interrupts as part of the diff --git
> a/drivers/gpu/drm/i915/display/intel_vrr.c
> b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 831554ea46b2..83ae56d22b5f 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -242,7 +242,7 @@ void intel_vrr_send_push(const struct intel_crtc_state
> *crtc_state)
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>
> - if (!crtc_state->vrr.enable)
> + if (!(crtc_state->vrr.enable || crtc_state->cmrr.enable))
> return;
>
> intel_de_write(dev_priv, TRANS_PUSH(cpu_transcoder), @@ -255,7
> +255,7 @@ bool intel_vrr_is_push_sent(const struct intel_crtc_state
> *crtc_state)
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>
> - if (!crtc_state->vrr.enable)
> + if (!(crtc_state->vrr.enable || crtc_state->cmrr.enable))
> return false;
>
> return intel_de_read(dev_priv, TRANS_PUSH(cpu_transcoder)) &
> TRANS_PUSH_SEND; @@ -266,18 +266,28 @@ void intel_vrr_enable(const
> struct intel_crtc_state *crtc_state)
> struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc-
> >dev);
> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>
> - if (!crtc_state->vrr.enable)
> + if (drm_WARN_ON(&dev_priv->drm, crtc_state->vrr.enable &&
> + crtc_state->cmrr.enable))
> return;
>
> - intel_de_write(dev_priv, TRANS_PUSH(cpu_transcoder),
> TRANS_PUSH_EN);
> -
> - if (HAS_AS_SDP(dev_priv))
> - intel_de_write(dev_priv, TRANS_VRR_VSYNC(cpu_transcoder),
> - VRR_VSYNC_END(crtc_state->vrr.vsync_end) |
> - VRR_VSYNC_START(crtc_state->vrr.vsync_start));
> + if (crtc_state->vrr.enable) {
> + intel_de_write(dev_priv,
> + TRANS_PUSH(cpu_transcoder), TRANS_PUSH_EN);
> + if (HAS_AS_SDP(dev_priv))
> + intel_de_write(dev_priv,
> TRANS_VRR_VSYNC(cpu_transcoder),
> + VRR_VSYNC_END(crtc_state-
> >vrr.vsync_end) |
> + VRR_VSYNC_START(crtc_state-
> >vrr.vsync_start));
> + intel_de_write(dev_priv, TRANS_VRR_CTL(cpu_transcoder),
> + VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state));
> + }
>
> - intel_de_write(dev_priv, TRANS_VRR_CTL(cpu_transcoder),
> - VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state));
> + if (crtc_state->cmrr.enable) {
> + intel_de_write(dev_priv,
> + TRANS_PUSH(cpu_transcoder), TRANS_PUSH_EN);
> + intel_de_write(dev_priv, TRANS_VRR_CTL(cpu_transcoder),
> + VRR_CTL_VRR_ENABLE | VRR_CTL_CMRR_ENABLE
> |
> + trans_vrr_ctl(crtc_state));
> + }
> }
>
> void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state) @@
> -286,7
> +296,7 @@ void intel_vrr_disable(const struct intel_crtc_state
> *old_crtc_state)
> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
>
> - if (!old_crtc_state->vrr.enable)
> + if (!(old_crtc_state->vrr.enable || old_crtc_state->cmrr.enable))
> return;
>
> intel_de_write(dev_priv, TRANS_VRR_CTL(cpu_transcoder), @@ -
> 332,10 +342,10 @@ void intel_vrr_get_config(struct intel_crtc_state
> *crtc_state)
> crtc_state->vrr.vmin = intel_de_read(dev_priv,
> TRANS_VRR_VMIN(cpu_transcoder)) + 1;
> }
>
> - if (crtc_state->vrr.enable) {
> + if (crtc_state->vrr.enable || crtc_state->cmrr.enable) {
> crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
>
> - if (HAS_AS_SDP(dev_priv)) {
> + if (HAS_AS_SDP(dev_priv) && crtc_state->vrr.enable) {
> trans_vrr_vsync =
> intel_de_read(dev_priv,
> TRANS_VRR_VSYNC(cpu_transcoder));
> crtc_state->vrr.vsync_start =
> --
> 2.25.1
