On Wed, 15 Nov 2023, Mitul Golani <[email protected]> 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 <[email protected]> > --- > .../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
