Re: [PATCH v9 07/17] drm/i915/vrr: Add compute config for DC Balance params

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

On 11/27/2025 2:46 PM, Mitul Golani wrote:
> Compute DC Balance parameters and tunable params based on
> experiments.
>
> --v2:
> - Document tunable params. (Ankit)
>
> --v3:
> - Add line spaces to compute config. (Ankit)
> - Remove redundancy checks.
>
> --v4:
> - Separate out conpute config to separate function.
> - As all the valuse are being computed in scanlines, and slope
> is still in usec, convert and store it to scanlines.
>
> --v5:
> - Update and add comments for slope calculation. (Ankit)
> - Update early return conditions for dc balance compute. (Ankit)
>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_vrr.c | 46 ++++++++++++++++++++++++
>   1 file changed, 46 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 650077eb280f..45e632e8a981 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -6,6 +6,7 @@
>   
>   #include <drm/drm_print.h>
>   
> +#include "intel_crtc.h"
>   #include "intel_de.h"
>   #include "intel_display_regs.h"
>   #include "intel_display_types.h"
> @@ -20,6 +21,14 @@
>   #define FIXED_POINT_PRECISION		100
>   #define CMRR_PRECISION_TOLERANCE	10
>   
> +/*
> + * Tunable parameters for DC Balance correction.
> + * These are captured based on experimentations.
> + */
> +#define DCB_CORRECTION_SENSITIVITY	30
> +#define DCB_CORRECTION_AGGRESSIVENESS	1000
> +#define DCB_BLANK_TARGET		50
> +
>   bool intel_vrr_is_capable(struct intel_connector *connector)
>   {
>   	struct intel_display *display = to_intel_display(connector);
> @@ -342,6 +351,41 @@ int intel_vrr_compute_vmax(struct intel_connector *connector,
>   	return vmax;
>   }
>   
> +static void
> +intel_vrr_dc_balance_compute_config(struct intel_crtc_state *crtc_state)
> +{
> +	int guardband_usec, adjustment_usec;
> +	struct intel_display *display = to_intel_display(crtc_state);
> +	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +
> +	if (!(HAS_VRR_DC_BALANCE(display) && crtc_state->vrr.enable))
> +		return;

Can simplify to:

if (!HAS_VRR_DC_BALANCE(display) || !crtc_state->vrr.enable)

     return;

IMO, if (notA or not B) is more readable than: if not (A and B)


> +
> +	crtc_state->vrr.dc_balance.vmax = crtc_state->vrr.vmax;
> +	crtc_state->vrr.dc_balance.vmin = crtc_state->vrr.vmin;
> +	crtc_state->vrr.dc_balance.max_increase =
> +		crtc_state->vrr.vmax - crtc_state->vrr.vmin;
> +	crtc_state->vrr.dc_balance.max_decrease =
> +		crtc_state->vrr.vmax - crtc_state->vrr.vmin;
> +	crtc_state->vrr.dc_balance.guardband =
> +		DIV_ROUND_UP(crtc_state->vrr.dc_balance.vmax *
> +			     DCB_CORRECTION_SENSITIVITY, 100);
> +	guardband_usec =
> +		intel_scanlines_to_usecs(adjusted_mode,
> +					 crtc_state->vrr.dc_balance.guardband);
> +	/*
> +	 *  The correction_aggressiveness/100 is the number of milliseconds to
> +	 *  adjust by when the balance is at twice the guardband.
> +	 *  guardband_slope = correction_aggressiveness / (guardband * 100)
> +	 */
> +	adjustment_usec = DCB_CORRECTION_AGGRESSIVENESS * 10;

The current value represents milliseconds x 100, so 10 msecs becomes 1000.
This scaling can be confusing compared to working directly with 
microseconds or milliseconds.
IMO ideally we could define the correction aggressiveness in either 
usecs or msecs for clarity, but that might make it harder to match 
values from the spec.
If this factor changes in the future (e.g., to 400 or 1400 based on 
experimentation), we might need to recalculate if we switch to pure 
msecs or usecs.

However, I think it would still be clearer if we rename the macro to:
#define DCB_CORRECTION_AGGRESSIVENESS_msecs_X100 1000

Then, when we use:
adjustment_usec = DCB_CORRECTION_AGGRESSIVENESS_msecs_X100 * 10;

it becomes more intuitive because we can see the conversion: msecs x 100 
x 10 -> usecs


Regards,

Ankit

> +	crtc_state->vrr.dc_balance.slope =
> +		DIV_ROUND_UP(adjustment_usec, guardband_usec);
> +	crtc_state->vrr.dc_balance.vblank_target =
> +		DIV_ROUND_UP((crtc_state->vrr.vmax - crtc_state->vrr.vmin) *
> +			     DCB_BLANK_TARGET, 100);
> +}
> +
>   void
>   intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>   			 struct drm_connector_state *conn_state)
> @@ -399,6 +443,8 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>   			(crtc_state->hw.adjusted_mode.crtc_vtotal -
>   			 crtc_state->hw.adjusted_mode.crtc_vsync_end);
>   	}
> +
> +	intel_vrr_dc_balance_compute_config(crtc_state);
>   }
>   
>   static int