Thanks @Jani Nikula
Addressed all review comments.
Regards,
Mitul
> -----Original Message-----
> From: Jani Nikula <jani.nik...@linux.intel.com>
> Sent: Wednesday, November 15, 2023 2:44 PM
> To: Golani, Mitulkumar Ajitkumar <mitulkumar.ajitkumar.gol...@intel.com>;
> intel-gfx@lists.freedesktop.org
> Cc: Syrjala, Ville <ville.syrj...@intel.com>
> Subject: Re: [Intel-gfx] [RFC 3/3] drm/i915: Compute CMRR and calculate
> vtotal
>
> On Wed, 15 Nov 2023, Mitul Golani
> <mitulkumar.ajitkumar.gol...@intel.com> wrote:
> > Compute Fixed Average Vtotal/CMRR with resepect to userspace VRR
> > enablement. Also calculate required parameters in case of CMRR is
> > enabled. During intel_vrr_compute_config, CMRR is getting enabled
> > based on userspace has enabled Adaptive Sync Vtotal mode (Legacy VRR)
> > or not.
> >
> > Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.gol...@intel.com>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 1 +
> > .../drm/i915/display/intel_display_device.h | 1 +
> > drivers/gpu/drm/i915/display/intel_vrr.c | 81 +++++++++++++++++--
> > 3 files changed, 78 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index f5a69309e65a..d61790f8ebb4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5479,6 +5479,7 @@ intel_pipe_config_compare(const struct
> intel_crtc_state *current_config,
> > PIPE_CONF_CHECK_I(vrr.guardband);
> > PIPE_CONF_CHECK_LLI(cmrr.cmrr_m);
> > PIPE_CONF_CHECK_LLI(cmrr.cmrr_n);
> > + PIPE_CONF_CHECK_BOOL(cmrr.enable);
> > }
> >
> > #undef PIPE_CONF_CHECK_X
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h
> > b/drivers/gpu/drm/i915/display/intel_display_device.h
> > index 4299cc452e05..66cbc3a6bbe8 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> > @@ -68,6 +68,7 @@ struct drm_printer;
> > #define HAS_TRANSCODER(i915, trans)
> ((DISPLAY_RUNTIME_INFO(i915)->cpu_transcoder_mask & \
> > BIT(trans)) != 0)
> > #define HAS_VRR(i915) (DISPLAY_VER(i915) >= 11)
> > +#define HAS_CMRR(i915) (DISPLAY_VER(i915) >= 20)
> > #define INTEL_NUM_PIPES(i915)
> (hweight8(DISPLAY_RUNTIME_INFO(i915)->pipe_mask))
> > #define I915_HAS_HOTPLUG(i915) (DISPLAY_INFO(i915)-
> >has_hotplug)
> > #define OVERLAY_NEEDS_PHYSICAL(i915) (DISPLAY_INFO(i915)-
> >overlay_needs_physical)
> > diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c
> > b/drivers/gpu/drm/i915/display/intel_vrr.c
> > index 1e33661062b3..4a056a71b68d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> > @@ -105,6 +105,52 @@ int intel_vrr_vmax_vblank_start(const struct
> intel_crtc_state *crtc_state)
> > return crtc_state->vrr.vmax -
> > intel_vrr_vblank_exit_length(crtc_state);
> > }
> >
> > +static int
> > +is_cmrr_frac_required(struct intel_crtc_state *crtc_state)
>
> Predicate functions need to be bool, not int.
>
> > +{
> > + int target_refresh_k, actual_refresh_k;
> > + struct drm_display_mode *adjusted_mode =
> > +&crtc_state->hw.adjusted_mode;
> > +
> > + target_refresh_k = drm_mode_vrefresh(adjusted_mode) * 1000;
> > + actual_refresh_k = DIV_ROUND_UP(adjusted_mode->crtc_clock *
> 1000,
> > + adjusted_mode->crtc_htotal) * 1000;
> > + actual_refresh_k /= adjusted_mode->crtc_vtotal;
> > +
> > + if (actual_refresh_k == target_refresh_k)
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +static unsigned int
> > +cmrr_get_vtotal(struct intel_crtc_state *crtc_state) {
> > + unsigned int muliplierM = 1, muliplierN = 1, vtotal;
>
> Please no camel case, and please fix the typo in multiplier.
>
> > + unsigned int actual_refresh_rate, desired_refresh_rate;
> > + unsigned long long actual_pixel_rate;
>
> All this unsigned math gives me an uneasy feeling. Please consider using
> signed math.
>
> > + struct drm_display_mode *adjusted_mode =
> > +&crtc_state->hw.adjusted_mode;
> > +
> > + actual_refresh_rate = DIV_ROUND_UP(adjusted_mode->crtc_clock *
> 1000,
> > + adjusted_mode->crtc_htotal) *
> 1000;
> > + actual_refresh_rate /= adjusted_mode->crtc_vtotal;
> > + desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
> > + actual_pixel_rate = actual_refresh_rate * adjusted_mode-
> >crtc_vtotal;
> > + actual_pixel_rate = (actual_pixel_rate * adjusted_mode->crtc_htotal)
> > +/ 1000;
> > +
> > + if (is_cmrr_frac_required(crtc_state)) {
> > + muliplierM = 1001;
> > + muliplierN = 1000;
> > + }
> > +
> > + crtc_state->cmrr.cmrr_n = DIV_ROUND_UP(desired_refresh_rate *
> > + adjusted_mode->crtc_htotal * muliplierN,
> muliplierM) * muliplierN;
> > + vtotal = DIV_ROUND_UP(actual_pixel_rate * muliplierN, crtc_state-
> >cmrr.cmrr_n);
> > + crtc_state->cmrr.cmrr_m =
> > + (actual_pixel_rate * muliplierM) % crtc_state->cmrr.cmrr_n;
>
> You're probably going to need do_div() for the remainder.
>
> > +
> > + return vtotal;
> > +}
> > +
> > void
> > intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
> > struct drm_connector_state *conn_state) @@ -
> 149,6 +195,27 @@
> > intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
> >
> > crtc_state->vrr.flipline = crtc_state->vrr.vmin + 1;
> >
> > + /*
> > + * When panel is VRR capable and userspace has
> > + * not enabled adaptive sync mode then Fixed Average
> > + * Vtotal mode should be enabled.
> > + */
> > + if (crtc_state->uapi.vrr_enabled) {
> > + crtc_state->vrr.enable = true;
> > + if (HAS_CMRR(i915))
> > + crtc_state->cmrr.enable = false;
>
> What would have set this to true?
>
> > + crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> > + } else {
> > + crtc_state->vrr.enable = false;
>
> What would have set this to true?
>
> You could simplify this to
>
> if (crtc_state->uapi.vrr_enabled)
> ...
> else if (HAS_CMRR(i915))
> ...
>
> > + if (HAS_CMRR(i915)) {
> > + crtc_state->cmrr.enable = true;
> > + crtc_state->vrr.vmax = cmrr_get_vtotal(crtc_state);
> > + crtc_state->vrr.vmin = crtc_state->vrr.vmax;
> > + crtc_state->vrr.flipline = crtc_state->vrr.vmin;
> > + crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> > + }
> > + }
> > +
> > /*
> > * For XE_LPD+, we use guardband and pipeline override
> > * is deprecated.
> > @@ -161,11 +228,6 @@ intel_vrr_compute_config(struct intel_crtc_state
> *crtc_state,
> > min(255, crtc_state->vrr.vmin - adjusted_mode-
> >crtc_vblank_start -
> > crtc_state->framestart_delay - 1);
> > }
> > -
> > - if (crtc_state->uapi.vrr_enabled) {
> > - crtc_state->vrr.enable = true;
> > - crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> > - }
> > }
> >
> > static u32 trans_vrr_ctl(const struct intel_crtc_state *crtc_state)
> > @@ -295,6 +357,15 @@ void intel_vrr_get_config(struct intel_crtc_state
> > *crtc_state)
> >
> > trans_vrr_ctl = intel_de_read(dev_priv,
> > TRANS_VRR_CTL(cpu_transcoder));
> >
> > + if (HAS_CMRR(dev_priv)) {
> > + crtc_state->cmrr.enable = (trans_vrr_ctl &
> VRR_CTL_CMRR_ENABLE) &&
> > + (trans_vrr_ctl &
> VRR_CTL_VRR_ENABLE);
> > + crtc_state->vrr.enable = trans_vrr_ctl &
> VRR_CTL_VRR_ENABLE &&
> > + !(trans_vrr_ctl &
> VRR_CTL_CMRR_ENABLE);
> > + } else {
> > + crtc_state->vrr.enable = trans_vrr_ctl &
> VRR_CTL_VRR_ENABLE;
> > + }
> > +
> > 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));
>
> --
> Jani Nikula, Intel
