Re: [PATCH v9 3/8] drm/i915: Compute CMRR and calculate vtotal

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 5/24/2024 3:54 PM, Mitul Golani 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 Variable refresh mode
> with VRR timing generator or not. Make CMRR as small subset of
> FAVT mode, when Panel is running on Fixed refresh rate
> and on VRR framework then only enable CMRR to match with
> actual refresh rate.
>
> --v2:
> - Update is_cmrr_frac_required function return as bool, not int. [Jani]
> - Use signed int math instead of unsigned in cmrr_get_vtotal2. [Jani]
> - Fix typo and usage of camel case in cmrr_get_vtotal. [Jani]
> - Use do_div in cmrr_get_vtotalwhile calculating cmrr_m. [ Jani]
> - Simplify cmrr and vrr compute config in intel_vrr_compute_config. [Jani]
> - Correct valiable name usage in is_cmrr_frac_required. [Ville]
>
> --v3:
> - Removing RFC tag.
>
> --v4:
> - Added edp check to address edp usecase for now. (ville)
> - Updated is_cmrr_fraction_required to more simplified calculation.
> - on longterm goal to be worked upon uapi as suggestion from ville.
>
> --v5:
> - Correct vtotal paramas accuracy and add 2 digit precision.
> - Avoid using DIV_ROUND_UP and improve scanline precision.
>
> --v6:
> - Make CMRR a small subset of FAVT mode.
>
> --v7:
> - Update commit message to avoid confusion with Legacy VRR (Ankit).
> - Add cmrr.enable in last, so remove from this patch.
>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@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      | 99 ++++++++++++++++---
>   3 files changed, 89 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 803360fcb0cc..f7e82d1e2bf9 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -5437,6 +5437,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>   		PIPE_CONF_CHECK_I(vrr.vsync_end);
>   		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 17ddf82f0b6e..b372b1acc19b 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
> @@ -71,6 +71,7 @@ struct drm_printer;
>   					  BIT(trans)) != 0)
>   #define HAS_VRR(i915)			(DISPLAY_VER(i915) >= 11)
>   #define HAS_AS_SDP(i915)		(DISPLAY_VER(i915) >= 13)
> +#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 3b250e92af98..3fbedd7366bb 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -12,6 +12,9 @@
>   #include "intel_vrr_reg.h"
>   #include "intel_dp.h"
>   
> +#define FIXED_POINT_PRECISION		100
> +#define CMRR_PRECISION_TOLERANCE	10
> +
>   bool intel_vrr_is_capable(struct intel_connector *connector)
>   {
>   	const struct drm_display_info *info = &connector->base.display_info;
> @@ -107,6 +110,59 @@ 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 bool
> +is_cmrr_frac_required(struct intel_crtc_state *crtc_state, bool is_edp)
> +{
> +	int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line;
> +	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> +
> +	if (!(HAS_CMRR(i915) && is_edp))
> +		return false;
> +
> +	actual_refresh_k =
> +		drm_mode_vrefresh(adjusted_mode) * FIXED_POINT_PRECISION;
> +	pixel_clock_per_line =
> +		adjusted_mode->crtc_clock * 1000 / adjusted_mode->crtc_htotal;
> +	calculated_refresh_k =
> +		pixel_clock_per_line * FIXED_POINT_PRECISION / adjusted_mode->crtc_vtotal;
> +
> +	if ((actual_refresh_k - calculated_refresh_k) < CMRR_PRECISION_TOLERANCE)
> +		return false;
> +
> +	return true;
> +}
> +
> +static unsigned int
> +cmrr_get_vtotal(struct intel_crtc_state *crtc_state)
> +{
> +	int multiplier_m = 1, multiplier_n = 1, vtotal;
> +	int actual_refresh_rate, desired_refresh_rate;
> +	long long actual_pixel_rate, adjusted_pixel_rate, pixel_clock_per_line;
> +	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +	pixel_clock_per_line =
> +		adjusted_mode->crtc_clock * 1000 / adjusted_mode->crtc_htotal;
> +	actual_refresh_rate =
> +		pixel_clock_per_line * FIXED_POINT_PRECISION / 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) / FIXED_POINT_PRECISION;
I am still not clear about this part. isnt actual_pixel_rate simply 
adjusted_mode->crtc_clock?
> +
> +	multiplier_m = 1001;
> +	multiplier_n = 1000;
> +
> +	crtc_state->cmrr.cmrr_n =
> +		((desired_refresh_rate * adjusted_mode->crtc_htotal * multiplier_n) / multiplier_m);
> +	crtc_state->cmrr.cmrr_n *= multiplier_n;


This seems wrong: As per bspec:68925 this should be just: 
crtc_state->cmrr.cmrr_n = ((desired_refresh_rate * 
adjusted_mode->crtc_htotal * multiplier_n);


> +	vtotal = (actual_pixel_rate * multiplier_n) / crtc_state->cmrr.cmrr_n;
> +	adjusted_pixel_rate = actual_pixel_rate * multiplier_m;
> +	crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, crtc_state->cmrr.cmrr_n);
> +
> +	return vtotal;
> +}
> +
>   void
>   intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>   			 struct drm_connector_state *conn_state)
> @@ -116,6 +172,7 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>   	struct intel_connector *connector =
>   		to_intel_connector(conn_state->connector);
>   	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	bool is_edp = intel_dp_is_edp(intel_dp);
>   	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>   	const struct drm_display_info *info = &connector->base.display_info;
>   	int vmin, vmax;
> @@ -160,18 +217,10 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>   	crtc_state->vrr.flipline = crtc_state->vrr.vmin + 1;
>   
>   	/*
> -	 * For XE_LPD+, we use guardband and pipeline override
> -	 * is deprecated.
> +	 * When panel is VRR capable and userspace has
> +	 * not enabled adaptive sync mode then Fixed Average
> +	 * Vtotal mode should be enabled.
>   	 */
> -	if (DISPLAY_VER(i915) >= 13) {
> -		crtc_state->vrr.guardband =
> -			crtc_state->vrr.vmin + 1 - adjusted_mode->crtc_vblank_start;
> -	} else {
> -		crtc_state->vrr.pipeline_full =
> -			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;
> @@ -183,6 +232,25 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>   				(crtc_state->hw.adjusted_mode.crtc_vtotal -
>   					crtc_state->hw.adjusted_mode.vsync_end);
>   		}
> +	} else if (is_cmrr_frac_required(crtc_state, is_edp)) {
> +		crtc_state->vrr.enable = true;

I think it will be better to add crtc_state->cmrr.enable = true; here 
instead of a separate patch.


> +		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.
> +	 */
> +	if (DISPLAY_VER(i915) >= 13) {
> +		crtc_state->vrr.guardband =
> +			crtc_state->vrr.vmin + 1 - adjusted_mode->crtc_vblank_start;
> +	} else {
> +		crtc_state->vrr.pipeline_full =
> +			min(255, crtc_state->vrr.vmin - adjusted_mode->crtc_vblank_start -
> +			    crtc_state->framestart_delay - 1);
>   	}
>   }
>   
> @@ -323,7 +391,14 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>   	trans_vrr_ctl = intel_de_read(dev_priv,
>   				      TRANS_VRR_CTL(dev_priv, cpu_transcoder));
>   
> -	crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE;
> +	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);


Since vrr.enable and cmrr.enable are not mutually exclusive, this simple 
can be:

crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE;

if (HAS_CMRR(dev_priv)

      crtc_state->cmrr.enable = trans_vrr_ctl & VRR_CTL_CMRR_ENABLE;


Regards,

Ankit

> +	} else {
> +		crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE;
> +	}
>   
>   	if (crtc_state->cmrr.enable) {
>   		crtc_state->cmrr.cmrr_n =