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 <[email protected]>
---
  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

Reply via email to