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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 07-07-15 om 12:11 schreef Daniel Vetter:
> 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)) {
Could be useful, lets hope everyone rounds in the same way. :)
>> +			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.
I just implemented it, then remembered why I didn't.

It will cause a failure in intel_modeset_check_state.

>> +{
>> +	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.
Ok.

>> +
>>  			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]"
Ok.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx