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

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

On 11/26/2025 9:39 AM, Nautiyal, Ankit K wrote:
>
> On 11/17/2025 11:14 AM, 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.
>>
>> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_vrr.c | 38 ++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c 
>> b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 650077eb280f..7cb484dd96df 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
>
>
> As mentioned in comment in last version, we can just have this value 
> as 1000 * 10 instead of multiplying 10 where we are using this macro.
>
> This is a bit unclear to me. More about this below.
>
>
>> +#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,33 @@ 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 val;
>> +    struct drm_display_mode *adjusted_mode = 
>> &crtc_state->hw.adjusted_mode;
>> +
>> +    if (!crtc_state->vrr.dc_balance.enable)
>> +        return;
>
> Here check should be for crtc_state->vrr.enable, and 
> HAS_VRR_DC_BALANCE(display) and return early if these are not true.
> I think this vrr.dc_balance.enable should be set in this function, 
> perhaps not in this patch, but in the last patch.
> Currently its set in intel_vrr_compute_vrr_timings() in #patch17.
>
>
>
>> +
>> +    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);
>> +    val = DIV_ROUND_UP(DCB_CORRECTION_AGGRESSIVENESS * 10,
>> +               crtc_state->vrr.dc_balance.guardband);
>> +    crtc_state->vrr.dc_balance.slope =
>> +        intel_usecs_to_scanlines(adjusted_mode, val);
>
>
> This needs to be written with more clarity.
>
> Perhaps need some comments to explain what is happening.
>
> DCB_CORRECTION_AGGRESSIVENESS is the number of millisecs to adjust 
> when balance is twice the guardband, as per our settings we perhaps 
> want 10msec.
>
> Slope is the ratio between Agressiveness msecs : Guardband msecs.
> We can use ratio of agressiveness usecs : guardband usecs.
>
> Currently guardband is in lines, (30% of vmax lines to be precise)
>
> guardband_usecs = intel_usecs_to_scanlines(adjusted_mode, 
> crtc_state->vrr.dc_balance.guardband);


(face palm).. I meant:

guardband_usecs = intel_scanlines_to_usecs(adjusted_mode-> 
crtc_state->vrr.dc_balance.guardband)

Sorry for the confusion.


Regards,

Ankit


> agressiveness usecs = (10 msec) * 1000 = 10000 usecs;
>
> slope = DIV_ROUND_UP(agressiveness_usecs, guardband_usecs)
>
> So IMO name the macro     10 * 1000
> Use slope as ratio DCB_CORRECTION_AGGRESSIVENESS_USECS : guardband_usecs
>
> Regards,
> Ankit
>
>
>> + 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 +435,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