Re: [PATCH v2 06/20] drm/i915: Allow fuzzy matching in pipe_config_compare.

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 07, 2015 at 09:08:17AM +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.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 198 ++++++++++++++++++++++++-----------
>  1 file changed, 139 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8cd3a7eb1e30..eb7c2e2819b7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12277,19 +12277,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;
>  }
> @@ -12392,27 +12379,124 @@ 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)
> +{
> +	if (m == m2 && n == n2)
> +		return true;
> +
> +	if (!m || !n || !m2 || !n2)
> +		return false;
> +
> +	if (m > m2) {
> +		while (m > m2) {

I think we need to make sure we don't reduce precision by eating low bits,
i.e.
		while (m > m2 && !(m & 1) && !(n & 1)) {

> +			m >>= 1;
> +			n >>= 1;
> +		}
> +	} else if (m < m2) {
> +		while (m < m2) {
> +			m2 >>= 1;
> +			n2 >>= 1;
> +		}
> +	}
> +
> +	return m == m2 && n == n2;
> +}
> +
> +static bool
> +intel_compare_link_m_n(const struct intel_link_m_n *m_n,
> +		       const struct intel_link_m_n *m2_n2)

I think these need to take adjust as an agurment and check for exact match
(not just matching ratio), like before.

> +{
> +	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) &&
> +	    intel_compare_m_n(m_n->link_m, m_n->link_n,
> +			      m2_n2->link_m, m2_n2->link_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(&current_config->name, \
> +				    &pipe_config->name)) { \
> +		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(&current_config->name, \
> +				    &pipe_config->name) && \
> +	    !intel_compare_link_m_n(&current_config->alt_name, \
> +				    &pipe_config->name)) { \
> +		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
> @@ -12423,30 +12507,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)	\
> @@ -12456,35 +12540,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);
> @@ -12580,8 +12647,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)
> @@ -12773,8 +12841,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]");
> @@ -13075,14 +13146,16 @@ 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);
> +
>  		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;
> @@ -13096,21 +13169,28 @@ intel_modeset_compute_config(struct drm_atomic_state *state)
>  		}
>  
>  		if (!needs_modeset(crtc_state)) {
> +			if (!(pipe_config->quirks & PIPE_CONFIG_QUIRK_INHERITED_MODE))
> +				continue;

double negative is harder to read ... what about:

		if (!needs_modeset && !INHERITED_MODE)
			continue;

		And then just unconditionally doing the
		add_affected_connectors? It'll be redundant almost always,
		but it also won't really matter since it'll be fast.

> +
>  			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 (!intel_pipe_config_compare(state->dev,
> +					       to_intel_crtc_state(crtc->state),
> +					       pipe_config, true))
> +			crtc_state->mode_changed = true;
> +		else if (!needs_modeset(crtc_state))
> +			continue;

Hm for bebugging I'd always dump the pipe config, even if we notice that
fastboot is posible. Perhaps changes the "[modeset]" below to a

	needs_modset() ? "[modeset: full]" : "[modeset: fast]"
>  
> +		any_ms = true;
>  		intel_dump_pipe_config(to_intel_crtc(crtc),
> -				       to_intel_crtc_state(crtc_state),
> +				       pipe_config,
>  				       "[modeset]");
>  	}
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx