> -----Original Message----- > From: Nautiyal, Ankit K <[email protected]> > Sent: 28 November 2025 19:00 > To: Golani, Mitulkumar Ajitkumar <[email protected]>; > [email protected] > Cc: [email protected]; [email protected] > Subject: Re: [PATCH v9 07/17] drm/i915/vrr: Add compute config for DC > Balance params > > > On 11/28/2025 6:40 PM, Nautiyal, Ankit K wrote: > > > > 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; > > > I think we can introduce intel_vrr_dc_balance_possible() here itself, rather > than later. > > So: > > if (!intel_vrr_dc_balance_possible() || !crtc_state->vrr.enable) > > return; > > > Regards, > > Ankit > > > > > IMO, if (notA or not B) is more readable than: if not (A and B)
Sure, I will add in next revision. Thanks. > > > > > >> + > >> + 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 Thanks for the suggestion. I agree that clarifying the units helps readability. I’ve kept the macro name as DCB_CORRECTION_AGGRESSIVENESS and added a clear unit comment (/* ms × 100; 10 ms */) instead of renaming it to include _msecs_X100, this keeps the name consistent with surrounding defines while still making the unit explicit. > > > > > > 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
