On Thu, 20 Oct 2022, Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> On BDW+ we have just the one set of DP M/N registers. The
> values we write into said registers depends on whether we
> want DRRS to be in high or low gear. This causes issues
> for the state checker which currently has to assume either
> set of M/N (high or low refresh rate) values may appear there.
> That sort of works for M/N itself, but all other values
> derived from the M/N (dotclock, pixel rate) are not handled
> correctly, leading to potential for state checker mismatches.
>
> Let's avoid all those problems by simply keeping DRRS in
> high gear until the state checker has done its hardware
> state readout.
>
> Note that hitting this issue presumable became very hard
> after commit 1b333c679a0f ("drm/i915: Do DRRS disable/enable
> during pre/post_plane_update()") since the state check would
> have to laze about for one full second (delay used by
> intel_drrs_schedule_work()) to see the low refresh rate.
> But it is still theoretically possible.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
This makes a whole lot of sense.
Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_display.c | 43 ++++----------------
> 1 file changed, 7 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 606f9140d024..906a5ad2bbfa 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -1261,8 +1261,6 @@ static void intel_post_plane_update(struct intel_atomic_state *state,
> if (needs_cursorclk_wa(old_crtc_state) &&
> !needs_cursorclk_wa(new_crtc_state))
> icl_wa_cursorclkgating(dev_priv, pipe, false);
> -
> - intel_drrs_activate(new_crtc_state);
> }
>
> static void intel_crtc_enable_flip_done(struct intel_atomic_state *state,
> @@ -5646,39 +5644,6 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
> PIPE_CONF_CHECK_I(name.y2); \
> } while (0)
>
> -/* This is required for BDW+ where there is only one set of registers for
> - * switching between high and low RR.
> - * This macro can be used whenever a comparison has to be made between one
> - * hw state and multiple sw state variables.
> - */
> -#define PIPE_CONF_CHECK_M_N_ALT(name, alt_name) do { \
> - if (!intel_compare_link_m_n(¤t_config->name, \
> - &pipe_config->name) && \
> - !intel_compare_link_m_n(¤t_config->alt_name, \
> - &pipe_config->name)) { \
> - pipe_config_mismatch(fastset, crtc, __stringify(name), \
> - "(expected tu %i data %i/%i link %i/%i, " \
> - "or tu %i data %i/%i link %i/%i, " \
> - "found tu %i, data %i/%i link %i/%i)", \
> - current_config->name.tu, \
> - current_config->name.data_m, \
> - current_config->name.data_n, \
> - current_config->name.link_m, \
> - current_config->name.link_n, \
> - current_config->alt_name.tu, \
> - current_config->alt_name.data_m, \
> - current_config->alt_name.data_n, \
> - current_config->alt_name.link_m, \
> - current_config->alt_name.link_n, \
> - pipe_config->name.tu, \
> - pipe_config->name.data_m, \
> - pipe_config->name.data_n, \
> - pipe_config->name.link_m, \
> - pipe_config->name.link_n); \
> - ret = false; \
> - } \
> -} while (0)
> -
> #define PIPE_CONF_CHECK_FLAGS(name, mask) do { \
> if ((current_config->name ^ pipe_config->name) & (mask)) { \
> pipe_config_mismatch(fastset, crtc, __stringify(name), \
> @@ -5747,7 +5712,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>
> if (HAS_DOUBLE_BUFFERED_M_N(dev_priv)) {
> if (!fastset || !pipe_config->seamless_m_n)
> - PIPE_CONF_CHECK_M_N_ALT(dp_m_n, dp_m2_n2);
> + PIPE_CONF_CHECK_M_N(dp_m_n);
> } else {
> PIPE_CONF_CHECK_M_N(dp_m_n);
> PIPE_CONF_CHECK_M_N(dp_m2_n2);
> @@ -7615,6 +7580,12 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>
> intel_modeset_verify_crtc(crtc, state, old_crtc_state, new_crtc_state);
>
> + /*
> + * Activate DRRS after state readout to avoid
> + * dp_m_n vs. dp_m2_n2 confusion on BDW+.
> + */
> + intel_drrs_activate(new_crtc_state);
> +
> /*
> * DSB cleanup is done in cleanup_work aligning with framebuffer
> * cleanup. So copy and reset the dsb structure to sync with
--
Jani Nikula, Intel Open Source Graphics Center
