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/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 <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;


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)
>
>
>> +
>> +    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