On Wed, 15 Nov 2023, Mitul Golani <mitulkumar.ajitkumar.golani@xxxxxxxxx> wrote:
> 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.
>
Where's the patch changelog?
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@xxxxxxxxx>
> ---
> .../drm/i915/display/intel_crtc_state_dump.c | 4 ++-
> drivers/gpu/drm/i915/display/intel_display.c | 27 +++++++++++++++---
> drivers/gpu/drm/i915/display/intel_vrr.c | 28 +++++++++++++------
> 3 files changed, 46 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> index 2d15e82c0b3d..908a4c4ccb00 100644
> --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> @@ -299,7 +299,9 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config,
> intel_dump_buffer(i915, "ELD: ", pipe_config->eld,
> drm_eld_size(pipe_config->eld));
>
> - drm_dbg_kms(&i915->drm, "vrr: %s, vmin: %d, vmax: %d, pipeline full: %d, guardband: %d flipline: %d, vmin vblank: %d, vmax vblank: %d\n",
> + drm_dbg_kms(&i915->drm,
> + "cmrr: %s, vrr: %s, vmin: %d, vmax: %d, pipeline full: %d, guardband: %d, flipline: %d, vmin vblank: %d, vmax vblank: %d\n",
> + str_yes_no(pipe_config->cmrr.enable),
> str_yes_no(pipe_config->vrr.enable),
> pipe_config->vrr.vmin, pipe_config->vrr.vmax,
> pipe_config->vrr.pipeline_full, pipe_config->vrr.guardband,
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index f99d2de840bc..ae7cc4eca064 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -937,6 +937,15 @@ static bool vrr_enabling(const struct intel_crtc_state *old_crtc_state,
> vrr_params_changed(old_crtc_state, new_crtc_state)));
> }
>
> +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);
> +}
> +
> static bool vrr_disabling(const struct intel_crtc_state *old_crtc_state,
> const struct intel_crtc_state *new_crtc_state)
> {
> @@ -946,6 +955,12 @@ 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)
> +{
And the hw.active check here?
> + return is_disabling(cmrr.enable, old_crtc_state, new_crtc_state);
> +}
> +
> #undef is_disabling
> #undef is_enabling
>
> @@ -1064,7 +1079,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);
> }
> @@ -6754,7 +6770,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);
> }
>
> @@ -6851,9 +6868,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 c889b0aa69a4..6da14c18a3e3 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -224,7 +224,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),
> @@ -237,7 +237,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;
> @@ -248,12 +248,24 @@ 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);
> - intel_de_write(dev_priv, TRANS_VRR_CTL(cpu_transcoder),
> - VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state));
> + if (!crtc_state->vrr.enable && crtc_state->cmrr.enable) {
You already checked that vrr.enable and cmrr.enable can't both be true
at the same time. Now you only check vrr.enable here.
Yeah, the conditions are bogus. Please don't rush it.
> + 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 | trans_vrr_ctl(crtc_state));
> + }
> +
> + if (!crtc_state->vrr.enable && crtc_state->cmrr.enable) {
Ditto here.
> + 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)
> @@ -262,7 +274,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),
> @@ -305,6 +317,6 @@ 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;
> }
--
Jani Nikula, Intel
