Re: [PATCH v9 3/8] drm/i915: Compute CMRR and calculate vtotal

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

On 5/28/2024 3:04 PM, Nautiyal, Ankit K wrote:
>
> On 5/24/2024 3:54 PM, Mitul Golani wrote:
>> Compute Fixed Average Vtotal/CMRR with resepect to
>> userspace VRR enablement. Also calculate required
>> parameters in case of CMRR is  enabled. During
>> intel_vrr_compute_config, CMRR is getting enabled
>> based on userspace has enabled Variable refresh mode
>> with VRR timing generator or not. Make CMRR as small subset of
>> FAVT mode, when Panel is running on Fixed refresh rate
>> and on VRR framework then only enable CMRR to match with
>> actual refresh rate.
>>
>> --v2:
>> - Update is_cmrr_frac_required function return as bool, not int. [Jani]
>> - Use signed int math instead of unsigned in cmrr_get_vtotal2. [Jani]
>> - Fix typo and usage of camel case in cmrr_get_vtotal. [Jani]
>> - Use do_div in cmrr_get_vtotalwhile calculating cmrr_m. [ Jani]
>> - Simplify cmrr and vrr compute config in intel_vrr_compute_config. 
>> [Jani]
>> - Correct valiable name usage in is_cmrr_frac_required. [Ville]
>>
>> --v3:
>> - Removing RFC tag.
>>
>> --v4:
>> - Added edp check to address edp usecase for now. (ville)
>> - Updated is_cmrr_fraction_required to more simplified calculation.
>> - on longterm goal to be worked upon uapi as suggestion from ville.
>>
>> --v5:
>> - Correct vtotal paramas accuracy and add 2 digit precision.
>> - Avoid using DIV_ROUND_UP and improve scanline precision.
>>
>> --v6:
>> - Make CMRR a small subset of FAVT mode.
>>
>> --v7:
>> - Update commit message to avoid confusion with Legacy VRR (Ankit).
>> - Add cmrr.enable in last, so remove from this patch.
>>
>> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c  |  1 +
>>   .../drm/i915/display/intel_display_device.h   |  1 +
>>   drivers/gpu/drm/i915/display/intel_vrr.c      | 99 ++++++++++++++++---
>>   3 files changed, 89 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c 
>> b/drivers/gpu/drm/i915/display/intel_display.c
>> index 803360fcb0cc..f7e82d1e2bf9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -5437,6 +5437,7 @@ intel_pipe_config_compare(const struct 
>> intel_crtc_state *current_config,
>>           PIPE_CONF_CHECK_I(vrr.vsync_end);
>>           PIPE_CONF_CHECK_LLI(cmrr.cmrr_m);
>>           PIPE_CONF_CHECK_LLI(cmrr.cmrr_n);
>> +        PIPE_CONF_CHECK_BOOL(cmrr.enable);
>>       }
>>     #undef PIPE_CONF_CHECK_X
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_device.h 
>> b/drivers/gpu/drm/i915/display/intel_display_device.h
>> index 17ddf82f0b6e..b372b1acc19b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_device.h
>> +++ b/drivers/gpu/drm/i915/display/intel_display_device.h
>> @@ -71,6 +71,7 @@ struct drm_printer;
>>                         BIT(trans)) != 0)
>>   #define HAS_VRR(i915)            (DISPLAY_VER(i915) >= 11)
>>   #define HAS_AS_SDP(i915)        (DISPLAY_VER(i915) >= 13)
>> +#define HAS_CMRR(i915)            (DISPLAY_VER(i915) >= 20)
>>   #define INTEL_NUM_PIPES(i915) 
>> (hweight8(DISPLAY_RUNTIME_INFO(i915)->pipe_mask))
>>   #define I915_HAS_HOTPLUG(i915) (DISPLAY_INFO(i915)->has_hotplug)
>>   #define OVERLAY_NEEDS_PHYSICAL(i915) 
>> (DISPLAY_INFO(i915)->overlay_needs_physical)
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c 
>> b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 3b250e92af98..3fbedd7366bb 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -12,6 +12,9 @@
>>   #include "intel_vrr_reg.h"
>>   #include "intel_dp.h"
>>   +#define FIXED_POINT_PRECISION        100
>> +#define CMRR_PRECISION_TOLERANCE    10
>> +
>>   bool intel_vrr_is_capable(struct intel_connector *connector)
>>   {
>>       const struct drm_display_info *info = 
>> &connector->base.display_info;
>> @@ -107,6 +110,59 @@ int intel_vrr_vmax_vblank_start(const struct 
>> intel_crtc_state *crtc_state)
>>       return crtc_state->vrr.vmax - 
>> intel_vrr_vblank_exit_length(crtc_state);
>>   }
>>   +static bool
>> +is_cmrr_frac_required(struct intel_crtc_state *crtc_state, bool is_edp)
>> +{
>> +    int calculated_refresh_k, actual_refresh_k, pixel_clock_per_line;
>> +    struct drm_display_mode *adjusted_mode = 
>> &crtc_state->hw.adjusted_mode;
>> +    struct drm_i915_private *i915 = 
>> to_i915(crtc_state->uapi.crtc->dev);
>> +
>> +    if (!(HAS_CMRR(i915) && is_edp))
>> +        return false;
>> +
>> +    actual_refresh_k =
>> +        drm_mode_vrefresh(adjusted_mode) * FIXED_POINT_PRECISION;
>> +    pixel_clock_per_line =
>> +        adjusted_mode->crtc_clock * 1000 / adjusted_mode->crtc_htotal;
>> +    calculated_refresh_k =
>> +        pixel_clock_per_line * FIXED_POINT_PRECISION / 
>> adjusted_mode->crtc_vtotal;
>> +
>> +    if ((actual_refresh_k - calculated_refresh_k) < 
>> CMRR_PRECISION_TOLERANCE)
>> +        return false;
>> +
>> +    return true;
>> +}
>> +
>> +static unsigned int
>> +cmrr_get_vtotal(struct intel_crtc_state *crtc_state)
>> +{
>> +    int multiplier_m = 1, multiplier_n = 1, vtotal;
>> +    int actual_refresh_rate, desired_refresh_rate;
>> +    long long actual_pixel_rate, adjusted_pixel_rate, 
>> pixel_clock_per_line;
>> +    struct drm_display_mode *adjusted_mode = 
>> &crtc_state->hw.adjusted_mode;
>> +
>> +    pixel_clock_per_line =
>> +        adjusted_mode->crtc_clock * 1000 / adjusted_mode->crtc_htotal;
>> +    actual_refresh_rate =
>> +        pixel_clock_per_line * FIXED_POINT_PRECISION / 
>> adjusted_mode->crtc_vtotal;
>> +    desired_refresh_rate = drm_mode_vrefresh(adjusted_mode);
>> +    actual_pixel_rate = actual_refresh_rate * 
>> adjusted_mode->crtc_vtotal;
>> +    actual_pixel_rate =
>> +        (actual_pixel_rate * adjusted_mode->crtc_htotal) / 
>> FIXED_POINT_PRECISION;
> I am still not clear about this part. isnt actual_pixel_rate simply 
> adjusted_mode->crtc_clock?
>> +
>> +    multiplier_m = 1001;
>> +    multiplier_n = 1000;

I think multiplier_m and multiplier should be 1 as initialized above.

AFAIU the multipliers 1001 and 1000 are used in videomodes with 59.94, 
71.928Hz etc.


Regards,

Ankit

>> +
>> +    crtc_state->cmrr.cmrr_n =
>> +        ((desired_refresh_rate * adjusted_mode->crtc_htotal * 
>> multiplier_n) / multiplier_m);
>> +    crtc_state->cmrr.cmrr_n *= multiplier_n;
>
>
> This seems wrong: As per bspec:68925 this should be just: 
> crtc_state->cmrr.cmrr_n = ((desired_refresh_rate * 
> adjusted_mode->crtc_htotal * multiplier_n);
>
>
>> +    vtotal = (actual_pixel_rate * multiplier_n) / 
>> crtc_state->cmrr.cmrr_n;
>> +    adjusted_pixel_rate = actual_pixel_rate * multiplier_m;
>> +    crtc_state->cmrr.cmrr_m = do_div(adjusted_pixel_rate, 
>> crtc_state->cmrr.cmrr_n);
>> +
>> +    return vtotal;
>> +}
>> +
>>   void
>>   intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>>                struct drm_connector_state *conn_state)
>> @@ -116,6 +172,7 @@ intel_vrr_compute_config(struct intel_crtc_state 
>> *crtc_state,
>>       struct intel_connector *connector =
>>           to_intel_connector(conn_state->connector);
>>       struct intel_dp *intel_dp = intel_attached_dp(connector);
>> +    bool is_edp = intel_dp_is_edp(intel_dp);
>>       struct drm_display_mode *adjusted_mode = 
>> &crtc_state->hw.adjusted_mode;
>>       const struct drm_display_info *info = 
>> &connector->base.display_info;
>>       int vmin, vmax;
>> @@ -160,18 +217,10 @@ intel_vrr_compute_config(struct 
>> intel_crtc_state *crtc_state,
>>       crtc_state->vrr.flipline = crtc_state->vrr.vmin + 1;
>>         /*
>> -     * For XE_LPD+, we use guardband and pipeline override
>> -     * is deprecated.
>> +     * When panel is VRR capable and userspace has
>> +     * not enabled adaptive sync mode then Fixed Average
>> +     * Vtotal mode should be enabled.
>>        */
>> -    if (DISPLAY_VER(i915) >= 13) {
>> -        crtc_state->vrr.guardband =
>> -            crtc_state->vrr.vmin + 1 - 
>> adjusted_mode->crtc_vblank_start;
>> -    } else {
>> -        crtc_state->vrr.pipeline_full =
>> -            min(255, crtc_state->vrr.vmin - 
>> adjusted_mode->crtc_vblank_start -
>> -                crtc_state->framestart_delay - 1);
>> -    }
>> -
>>       if (crtc_state->uapi.vrr_enabled) {
>>           crtc_state->vrr.enable = true;
>>           crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
>> @@ -183,6 +232,25 @@ intel_vrr_compute_config(struct intel_crtc_state 
>> *crtc_state,
>>                   (crtc_state->hw.adjusted_mode.crtc_vtotal -
>> crtc_state->hw.adjusted_mode.vsync_end);
>>           }
>> +    } else if (is_cmrr_frac_required(crtc_state, is_edp)) {
>> +        crtc_state->vrr.enable = true;
>
> I think it will be better to add crtc_state->cmrr.enable = true; here 
> instead of a separate patch.
>
>
>> +        crtc_state->vrr.vmax = cmrr_get_vtotal(crtc_state);
>> +        crtc_state->vrr.vmin = crtc_state->vrr.vmax;
>> +        crtc_state->vrr.flipline = crtc_state->vrr.vmin;
>> +        crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
>> +    }
>> +
>> +    /*
>> +     * For XE_LPD+, we use guardband and pipeline override
>> +     * is deprecated.
>> +     */
>> +    if (DISPLAY_VER(i915) >= 13) {
>> +        crtc_state->vrr.guardband =
>> +            crtc_state->vrr.vmin + 1 - 
>> adjusted_mode->crtc_vblank_start;
>> +    } else {
>> +        crtc_state->vrr.pipeline_full =
>> +            min(255, crtc_state->vrr.vmin - 
>> adjusted_mode->crtc_vblank_start -
>> +                crtc_state->framestart_delay - 1);
>>       }
>>   }
>>   @@ -323,7 +391,14 @@ void intel_vrr_get_config(struct 
>> intel_crtc_state *crtc_state)
>>       trans_vrr_ctl = intel_de_read(dev_priv,
>>                         TRANS_VRR_CTL(dev_priv, cpu_transcoder));
>>   -    crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE;
>> +    if (HAS_CMRR(dev_priv)) {
>> +        crtc_state->cmrr.enable = (trans_vrr_ctl & 
>> VRR_CTL_CMRR_ENABLE) &&
>> +                      (trans_vrr_ctl & VRR_CTL_VRR_ENABLE);
>> +        crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE &&
>> +                     !(trans_vrr_ctl & VRR_CTL_CMRR_ENABLE);
>
>
> Since vrr.enable and cmrr.enable are not mutually exclusive, this 
> simple can be:
>
> crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE;
>
> if (HAS_CMRR(dev_priv)
>
>      crtc_state->cmrr.enable = trans_vrr_ctl & VRR_CTL_CMRR_ENABLE;
>
>
> Regards,
>
> Ankit
>
>> +    } else {
>> +        crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE;
>> +    }
>>         if (crtc_state->cmrr.enable) {
>>           crtc_state->cmrr.cmrr_n =