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.
>
> 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 | 24 ++++++++++--
> drivers/gpu/drm/i915/display/intel_vrr.c | 37 +++++++++++++------
> 3 files changed, 48 insertions(+), 17 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..f5a69309e65a 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -937,6 +937,12 @@ 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)
> +{
> + 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 +952,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)
> +{
> + return is_disabling(cmrr.enable, old_crtc_state, new_crtc_state);
> +}
> +
See https://patchwork.freedesktop.org/patch/msgid/20231106211915.13406-2-ville.syrjala@xxxxxxxxxxxxxxx
> #undef is_disabling
> #undef is_enabling
>
> @@ -1064,7 +1076,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 +6767,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 +6865,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);
Please don't use bitwise OR on booleans.
>
> /*
> * 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 4aeccbbf1d2a..1e33661062b3 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))
Please don't use bitwise OR on booleans.
> 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))
Please don't use bitwise OR on booleans.
> return false;
>
> return intel_de_read(dev_priv, TRANS_PUSH(cpu_transcoder)) & TRANS_PUSH_SEND;
> @@ -245,15 +245,30 @@ bool intel_vrr_is_push_sent(const struct intel_crtc_state *crtc_state)
>
> void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
> {
> - struct drm_i915_private *dev_priv = to_i915(crtc_state->uapi.crtc->dev);
> + struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> + struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
Unrelated change.
> enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>
> - if (!crtc_state->vrr.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->cmrr.enable && !crtc_state->vrr.enable) {
> + return;
> + } else if (crtc_state->vrr.enable && crtc_state->cmrr.enable) {
> + drm_WARN_ON(&dev_priv->drm, crtc_state->vrr.enable &&
> + crtc_state->cmrr.enable);
Please don't duplicate the if and the drm_WARN_ON() conditions. You'll
want to do this as the first thing with and early return:
if (drm_WARN_ON(...))
return;
Then you can have two independent blocks:
if (crtc_state->vrr.enable)
// handle vrr
if (crtc_state->cmrr.enable)
// handle cmmr
And you can remove the whole complicated if-ladder.
> + } else {
> + if (!crtc_state->vrr.enable && 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));
> + } else {
> + 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));
> + }
> + }
> }
>
> void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
> @@ -262,7 +277,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))
Please don't use bitwise OR on booleans.
> return;
>
> intel_de_write(dev_priv, TRANS_VRR_CTL(cpu_transcoder),
> @@ -280,8 +295,6 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>
> trans_vrr_ctl = intel_de_read(dev_priv, TRANS_VRR_CTL(cpu_transcoder));
>
> - crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE;
> -
Huh?
> if (crtc_state->cmrr.enable) {
> cmrr_n_hi = intel_de_read(dev_priv, TRANS_CMRR_N_HI(cpu_transcoder));
> cmrr_n_lo = intel_de_read(dev_priv, TRANS_CMRR_N_LO(cpu_transcoder));
> @@ -306,6 +319,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)
Please don't use bitwise OR on booleans.
> crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> }
--
Jani Nikula, Intel
