On Mon, Jul 13, 2015 at 04:30:17PM +0200, Maarten Lankhorst wrote:
> Instead of doing ad-hoc checks we already have a way of checking
> if the state is compatible or not. Use this to force a modeset.
>
> Only during modesets, or with PIPE_CONFIG_QUIRK_INHERITED_MODE
> we should check if a full modeset is really needed.
>
> Fastboot will allow the adjust parameter to ignore some stuff
> too, and it will fix up differences in state that are ignored
> by the compare function.
>
> Changes since v1:
> - Increase the value of the lowest m/n to prevent truncation.
> - Dump pipe config when fastboot's used, without a modeset.
Needs to mention that the dp M/N comparison helper grew the "exact"
parameter.
-Daniel
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 218 +++++++++++++++++++++++++----------
> 1 file changed, 157 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6eed925f3300..c3a3d1087777 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12290,19 +12290,6 @@ encoder_retry:
> DRM_DEBUG_KMS("plane bpp: %i, pipe bpp: %i, dithering: %i\n",
> base_bpp, pipe_config->pipe_bpp, pipe_config->dither);
>
> - /* Check if we need to force a modeset */
> - if (pipe_config->has_audio !=
> - to_intel_crtc_state(crtc->state)->has_audio) {
> - pipe_config->base.mode_changed = true;
> - ret = drm_atomic_add_affected_planes(state, crtc);
> - }
> -
> - /*
> - * Note we have an issue here with infoframes: current code
> - * only updates them on the full mode set path per hw
> - * requirements. So here we should be checking for any
> - * required changes and forcing a mode set.
> - */
> fail:
> return ret;
> }
> @@ -12406,27 +12393,133 @@ static bool intel_fuzzy_clock_check(int clock1, int clock2)
> base.head) \
> if (mask & (1 <<(intel_crtc)->pipe))
>
> +
> +static bool
> +intel_compare_m_n(unsigned int m, unsigned int n,
> + unsigned int m2, unsigned int n2,
> + bool exact)
> +{
> + if (m == m2 && n == n2)
> + return true;
> +
> + if (exact || !m || !n || !m2 || !n2)
> + return false;
> +
> + BUILD_BUG_ON(DATA_LINK_M_N_MASK > INT_MAX);
> +
> + if (m > m2) {
> + while (m > m2) {
> + m2 <<= 1;
> + n2 <<= 1;
> + }
> + } else if (m < m2) {
> + while (m < m2) {
> + m <<= 1;
> + n <<= 1;
> + }
> + }
> +
> + return m == m2 && n == n2;
> +}
> +
> +static bool
> +intel_compare_link_m_n(const struct intel_link_m_n *m_n,
> + struct intel_link_m_n *m2_n2,
> + bool adjust)
> +{
> + if (m_n->tu == m2_n2->tu &&
> + intel_compare_m_n(m_n->gmch_m, m_n->gmch_n,
> + m2_n2->gmch_m, m2_n2->gmch_n, !adjust) &&
> + intel_compare_m_n(m_n->link_m, m_n->link_n,
> + m2_n2->link_m, m2_n2->link_n, !adjust)) {
> + if (adjust)
> + *m2_n2 = *m_n;
> +
> + return true;
> + }
> +
> + return false;
> +}
> +
> static bool
> intel_pipe_config_compare(struct drm_device *dev,
> struct intel_crtc_state *current_config,
> - struct intel_crtc_state *pipe_config)
> + struct intel_crtc_state *pipe_config,
> + bool adjust)
> {
> + bool ret = true;
> +
> +#define INTEL_ERR_OR_DBG_KMS(fmt, ...) \
> + do { \
> + if (!adjust) \
> + DRM_ERROR(fmt, ##__VA_ARGS__); \
> + else \
> + DRM_DEBUG_KMS(fmt, ##__VA_ARGS__); \
> + } while (0)
> +
> #define PIPE_CONF_CHECK_X(name) \
> if (current_config->name != pipe_config->name) { \
> - DRM_ERROR("mismatch in " #name " " \
> + INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
> "(expected 0x%08x, found 0x%08x)\n", \
> current_config->name, \
> pipe_config->name); \
> - return false; \
> + ret = false; \
> }
>
> #define PIPE_CONF_CHECK_I(name) \
> if (current_config->name != pipe_config->name) { \
> - DRM_ERROR("mismatch in " #name " " \
> + INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
> "(expected %i, found %i)\n", \
> current_config->name, \
> pipe_config->name); \
> - return false; \
> + ret = false; \
> + }
> +
> +#define PIPE_CONF_CHECK_M_N(name) \
> + if (!intel_compare_link_m_n(¤t_config->name, \
> + &pipe_config->name,\
> + adjust)) { \
> + INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
> + "(expected tu %i gmch %i/%i link %i/%i, " \
> + "found tu %i, gmch %i/%i link %i/%i)\n", \
> + current_config->name.tu, \
> + current_config->name.gmch_m, \
> + current_config->name.gmch_n, \
> + current_config->name.link_m, \
> + current_config->name.link_n, \
> + pipe_config->name.tu, \
> + pipe_config->name.gmch_m, \
> + pipe_config->name.gmch_n, \
> + pipe_config->name.link_m, \
> + pipe_config->name.link_n); \
> + ret = false; \
> + }
> +
> +#define PIPE_CONF_CHECK_M_N_ALT(name, alt_name) \
> + if (!intel_compare_link_m_n(¤t_config->name, \
> + &pipe_config->name, adjust) && \
> + !intel_compare_link_m_n(¤t_config->alt_name, \
> + &pipe_config->name, adjust)) { \
> + INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
> + "(expected tu %i gmch %i/%i link %i/%i, " \
> + "or tu %i gmch %i/%i link %i/%i, " \
> + "found tu %i, gmch %i/%i link %i/%i)\n", \
> + current_config->name.tu, \
> + current_config->name.gmch_m, \
> + current_config->name.gmch_n, \
> + current_config->name.link_m, \
> + current_config->name.link_n, \
> + current_config->alt_name.tu, \
> + current_config->alt_name.gmch_m, \
> + current_config->alt_name.gmch_n, \
> + current_config->alt_name.link_m, \
> + current_config->alt_name.link_n, \
> + pipe_config->name.tu, \
> + pipe_config->name.gmch_m, \
> + pipe_config->name.gmch_n, \
> + pipe_config->name.link_m, \
> + pipe_config->name.link_n); \
> + ret = false; \
> }
>
> /* This is required for BDW+ where there is only one set of registers for
> @@ -12437,30 +12530,30 @@ intel_pipe_config_compare(struct drm_device *dev,
> #define PIPE_CONF_CHECK_I_ALT(name, alt_name) \
> if ((current_config->name != pipe_config->name) && \
> (current_config->alt_name != pipe_config->name)) { \
> - DRM_ERROR("mismatch in " #name " " \
> + INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
> "(expected %i or %i, found %i)\n", \
> current_config->name, \
> current_config->alt_name, \
> pipe_config->name); \
> - return false; \
> + ret = false; \
> }
>
> #define PIPE_CONF_CHECK_FLAGS(name, mask) \
> if ((current_config->name ^ pipe_config->name) & (mask)) { \
> - DRM_ERROR("mismatch in " #name "(" #mask ") " \
> + INTEL_ERR_OR_DBG_KMS("mismatch in " #name "(" #mask ") " \
> "(expected %i, found %i)\n", \
> current_config->name & (mask), \
> pipe_config->name & (mask)); \
> - return false; \
> + ret = false; \
> }
>
> #define PIPE_CONF_CHECK_CLOCK_FUZZY(name) \
> if (!intel_fuzzy_clock_check(current_config->name, pipe_config->name)) { \
> - DRM_ERROR("mismatch in " #name " " \
> + INTEL_ERR_OR_DBG_KMS("mismatch in " #name " " \
> "(expected %i, found %i)\n", \
> current_config->name, \
> pipe_config->name); \
> - return false; \
> + ret = false; \
> }
>
> #define PIPE_CONF_QUIRK(quirk) \
> @@ -12470,35 +12563,18 @@ intel_pipe_config_compare(struct drm_device *dev,
>
> PIPE_CONF_CHECK_I(has_pch_encoder);
> PIPE_CONF_CHECK_I(fdi_lanes);
> - PIPE_CONF_CHECK_I(fdi_m_n.gmch_m);
> - PIPE_CONF_CHECK_I(fdi_m_n.gmch_n);
> - PIPE_CONF_CHECK_I(fdi_m_n.link_m);
> - PIPE_CONF_CHECK_I(fdi_m_n.link_n);
> - PIPE_CONF_CHECK_I(fdi_m_n.tu);
> + PIPE_CONF_CHECK_M_N(fdi_m_n);
>
> PIPE_CONF_CHECK_I(has_dp_encoder);
>
> if (INTEL_INFO(dev)->gen < 8) {
> - PIPE_CONF_CHECK_I(dp_m_n.gmch_m);
> - PIPE_CONF_CHECK_I(dp_m_n.gmch_n);
> - PIPE_CONF_CHECK_I(dp_m_n.link_m);
> - PIPE_CONF_CHECK_I(dp_m_n.link_n);
> - PIPE_CONF_CHECK_I(dp_m_n.tu);
> -
> - if (current_config->has_drrs) {
> - PIPE_CONF_CHECK_I(dp_m2_n2.gmch_m);
> - PIPE_CONF_CHECK_I(dp_m2_n2.gmch_n);
> - PIPE_CONF_CHECK_I(dp_m2_n2.link_m);
> - PIPE_CONF_CHECK_I(dp_m2_n2.link_n);
> - PIPE_CONF_CHECK_I(dp_m2_n2.tu);
> - }
> - } else {
> - PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_m, dp_m2_n2.gmch_m);
> - PIPE_CONF_CHECK_I_ALT(dp_m_n.gmch_n, dp_m2_n2.gmch_n);
> - PIPE_CONF_CHECK_I_ALT(dp_m_n.link_m, dp_m2_n2.link_m);
> - PIPE_CONF_CHECK_I_ALT(dp_m_n.link_n, dp_m2_n2.link_n);
> - PIPE_CONF_CHECK_I_ALT(dp_m_n.tu, dp_m2_n2.tu);
> - }
> + PIPE_CONF_CHECK_M_N(dp_m_n);
> +
> + PIPE_CONF_CHECK_I(has_drrs);
> + if (current_config->has_drrs)
> + PIPE_CONF_CHECK_M_N(dp_m2_n2);
> + } else
> + PIPE_CONF_CHECK_M_N_ALT(dp_m_n, dp_m2_n2);
>
> PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_hdisplay);
> PIPE_CONF_CHECK_I(base.adjusted_mode.crtc_htotal);
> @@ -12594,8 +12670,9 @@ intel_pipe_config_compare(struct drm_device *dev,
> #undef PIPE_CONF_CHECK_FLAGS
> #undef PIPE_CONF_CHECK_CLOCK_FUZZY
> #undef PIPE_CONF_QUIRK
> +#undef INTEL_ERR_OR_DBG_KMS
>
> - return true;
> + return ret;
> }
>
> static void check_wm_state(struct drm_device *dev)
> @@ -12787,8 +12864,11 @@ check_crtc_state(struct drm_device *dev)
> "transitional active state does not match atomic hw state "
> "(expected %i, found %i)\n", crtc->base.state->active, crtc->active);
>
> - if (active &&
> - !intel_pipe_config_compare(dev, crtc->config, &pipe_config)) {
> + if (!active)
> + continue;
> +
> + if (!intel_pipe_config_compare(dev, crtc->config,
> + &pipe_config, false)) {
> I915_STATE_WARN(1, "pipe state doesn't match!\n");
> intel_dump_pipe_config(crtc, &pipe_config,
> "[hw state]");
> @@ -13089,14 +13169,17 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
> return ret;
>
> for_each_crtc_in_state(state, crtc, crtc_state, i) {
> + struct intel_crtc_state *pipe_config =
> + to_intel_crtc_state(crtc_state);
> + bool modeset, recalc;
> +
> if (!crtc_state->enable) {
> if (needs_modeset(crtc_state))
> any_ms = true;
> continue;
> }
>
> - if (to_intel_crtc_state(crtc_state)->quirks &
> - PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
> + if (pipe_config->quirks & PIPE_CONFIG_QUIRK_INITIAL_PLANES) {
> ret = drm_atomic_add_affected_planes(state, crtc);
> if (ret)
> return ret;
> @@ -13109,23 +13192,36 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
> */
> }
>
> - if (!needs_modeset(crtc_state)) {
> + modeset = needs_modeset(crtc_state);
> + recalc = pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE;
> +
> + if (!modeset && !recalc)
> + continue;
> +
> + if (recalc) {
> ret = drm_atomic_add_affected_connectors(state, crtc);
> if (ret)
> return ret;
> }
>
> - ret = intel_modeset_pipe_config(crtc,
> - to_intel_crtc_state(crtc_state));
> + ret = intel_modeset_pipe_config(crtc, pipe_config);
> if (ret)
> return ret;
>
> - if (needs_modeset(crtc_state))
> - any_ms = true;
> + if (recalc && !intel_pipe_config_compare(state->dev,
> + to_intel_crtc_state(crtc->state),
> + pipe_config, true)) {
> + modeset = crtc_state->mode_changed = true;
> +
> + ret = drm_atomic_add_affected_planes(state, crtc);
> + if (ret)
> + return ret;
> + }
>
> + any_ms = modeset;
> intel_dump_pipe_config(to_intel_crtc(crtc),
> - to_intel_crtc_state(crtc_state),
> - "[modeset]");
> + pipe_config,
> + modeset ? "[modeset]" : "[fastboot]");
> }
>
> if (any_ms) {
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
