Thanks @Jani Nikula Addressed all review comments.
Regards, Mitul > -----Original Message----- > From: Jani Nikula <[email protected]> > Sent: Wednesday, November 15, 2023 2:25 PM > To: Golani, Mitulkumar Ajitkumar <[email protected]>; > [email protected] > Cc: Syrjala, Ville <[email protected]> > Subject: Re: [Intel-gfx] [RFC 2/3] drm/i915: Add Enable/Disable for CMRR > based on VRR state > > 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. > > > > Signed-off-by: Mitul Golani <[email protected]> > > --- > > .../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- > [email protected] > > > #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
