[Intel-gfx] [PATCH v2 06/20] drm/i915: Allow fuzzy matching in pipe_config_compare.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Wed Jul 8 01:38:33 PDT 2015
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 at 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(¤t_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(¤t_config->name, \
>> + &pipe_config->name) && \
>> + !intel_compare_link_m_n(¤t_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
More information about the Intel-gfx
mailing list