From patchwork Sat Jan 18 23:21:59 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Souza, Jose" X-Patchwork-Id: 11340347 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 224D8138D for ; Sat, 18 Jan 2020 23:22:09 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 00D9E246AC for ; Sat, 18 Jan 2020 23:22:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 00D9E246AC Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 6CE696E174; Sat, 18 Jan 2020 23:22:06 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id 765076E172 for ; Sat, 18 Jan 2020 23:22:04 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jan 2020 15:22:03 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,335,1574150400"; d="scan'208";a="249608810" Received: from shenduya-mobl.amr.corp.intel.com (HELO josouza-MOBL.amr.corp.intel.com) ([10.255.228.238]) by fmsmga004.fm.intel.com with ESMTP; 18 Jan 2020 15:22:02 -0800 From: =?utf-8?q?Jos=C3=A9_Roberto_de_Souza?= To: intel-gfx@lists.freedesktop.org Date: Sat, 18 Jan 2020 15:21:59 -0800 Message-Id: <20200118232159.22338-1-jose.souza@intel.com> X-Mailer: git-send-email 2.25.0 MIME-Version: 1.0 Subject: [Intel-gfx] [PATCH] drm/i915: Nuke intel_atomic_lock/serialize_global_state() X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Jani Nikula Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" When computing GEN 9+ watermarks all the pipes were being added to the state whatever the state needs a modeset or the active pipes changed what is redundant. As when a pipe/CRTC is added to the state it means that the state also holds the CRTC lock, so we can remove a lot of code and complexity by adding all CRTCs to the state earlier with no drawbacks for GEN 9+ and little to nothing in older GENs. Also it brings other improvements like atomic state has now a device wide correctly active_pipes and it will fix the enabling of the right number of the dbufs as if the state was only enabling one pipe and only had one CRTC state it would only enable one slice. Bellow some justifications that might not be so easy to understand: - intel_atomic_lock_global_state() is not needed anymore in glk_force_audio_cdclk() because setting force_min_cdclk_changed will force a modeset - intel_cdclk.c also don't need intel_atomic_lock_global_state() or intel_atomic_serialize_global_state() as those functions are called by intel_modeset_checks() call chain after the CRCTs are added to the state - no need to call intel_atomic_lock_global_state() in intel_modeset_checks() as we just got the CRTCs locks. Cc: Stanislav Lisovskiy Cc: Ville Syrjälä Cc: Jani Nikula Signed-off-by: José Roberto de Souza --- drivers/gpu/drm/i915/display/intel_atomic.c | 38 ----------------- drivers/gpu/drm/i915/display/intel_atomic.h | 4 -- drivers/gpu/drm/i915/display/intel_audio.c | 6 +-- drivers/gpu/drm/i915/display/intel_cdclk.c | 33 +++------------ drivers/gpu/drm/i915/display/intel_display.c | 20 ++++----- .../drm/i915/display/intel_display_types.h | 18 -------- drivers/gpu/drm/i915/intel_pm.c | 42 +++---------------- 7 files changed, 18 insertions(+), 143 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c index c362eecdd414..03938ae2fb05 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic.c +++ b/drivers/gpu/drm/i915/display/intel_atomic.c @@ -499,7 +499,6 @@ void intel_atomic_state_clear(struct drm_atomic_state *s) struct intel_atomic_state *state = to_intel_atomic_state(s); drm_atomic_state_default_clear(&state->base); state->dpll_set = state->modeset = false; - state->global_state_changed = false; state->active_pipes = 0; memset(&state->min_cdclk, 0, sizeof(state->min_cdclk)); memset(&state->min_voltage_level, 0, sizeof(state->min_voltage_level)); @@ -519,40 +518,3 @@ intel_atomic_get_crtc_state(struct drm_atomic_state *state, return to_intel_crtc_state(crtc_state); } - -int intel_atomic_lock_global_state(struct intel_atomic_state *state) -{ - struct drm_i915_private *dev_priv = to_i915(state->base.dev); - struct intel_crtc *crtc; - - state->global_state_changed = true; - - for_each_intel_crtc(&dev_priv->drm, crtc) { - int ret; - - ret = drm_modeset_lock(&crtc->base.mutex, - state->base.acquire_ctx); - if (ret) - return ret; - } - - return 0; -} - -int intel_atomic_serialize_global_state(struct intel_atomic_state *state) -{ - struct drm_i915_private *dev_priv = to_i915(state->base.dev); - struct intel_crtc *crtc; - - state->global_state_changed = true; - - for_each_intel_crtc(&dev_priv->drm, crtc) { - struct intel_crtc_state *crtc_state; - - crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); - if (IS_ERR(crtc_state)) - return PTR_ERR(crtc_state); - } - - return 0; -} diff --git a/drivers/gpu/drm/i915/display/intel_atomic.h b/drivers/gpu/drm/i915/display/intel_atomic.h index 74c749dbfb4f..b324d12bdd97 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic.h +++ b/drivers/gpu/drm/i915/display/intel_atomic.h @@ -55,8 +55,4 @@ int intel_atomic_setup_scalers(struct drm_i915_private *dev_priv, struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state); -int intel_atomic_lock_global_state(struct intel_atomic_state *state); - -int intel_atomic_serialize_global_state(struct intel_atomic_state *state); - #endif /* __INTEL_ATOMIC_H__ */ diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c index b18040793d9e..0147e1f382b6 100644 --- a/drivers/gpu/drm/i915/display/intel_audio.c +++ b/drivers/gpu/drm/i915/display/intel_audio.c @@ -819,11 +819,7 @@ static void glk_force_audio_cdclk(struct drm_i915_private *dev_priv, to_intel_atomic_state(state)->cdclk.force_min_cdclk = enable ? 2 * 96000 : 0; - /* Protects dev_priv->cdclk.force_min_cdclk */ - ret = intel_atomic_lock_global_state(to_intel_atomic_state(state)); - if (!ret) - ret = drm_atomic_commit(state); - + ret = drm_atomic_commit(state); if (ret == -EDEADLK) { drm_atomic_state_clear(state); drm_modeset_backoff(&ctx); diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c index 0ce5926006ca..469617436815 100644 --- a/drivers/gpu/drm/i915/display/intel_cdclk.c +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c @@ -2037,8 +2037,6 @@ static int intel_compute_min_cdclk(struct intel_atomic_state *state) sizeof(state->min_cdclk)); for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { - int ret; - min_cdclk = intel_crtc_compute_min_cdclk(crtc_state); if (min_cdclk < 0) return min_cdclk; @@ -2047,10 +2045,6 @@ static int intel_compute_min_cdclk(struct intel_atomic_state *state) continue; state->min_cdclk[i] = min_cdclk; - - ret = intel_atomic_lock_global_state(state); - if (ret) - return ret; } min_cdclk = state->cdclk.force_min_cdclk; @@ -2086,8 +2080,6 @@ static int bxt_compute_min_voltage_level(struct intel_atomic_state *state) sizeof(state->min_voltage_level)); for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) { - int ret; - if (crtc_state->hw.enable) min_voltage_level = crtc_state->min_voltage_level; else @@ -2097,10 +2089,6 @@ static int bxt_compute_min_voltage_level(struct intel_atomic_state *state) continue; state->min_voltage_level[i] = min_voltage_level; - - ret = intel_atomic_lock_global_state(state); - if (ret) - return ret; } min_voltage_level = 0; @@ -2348,23 +2336,12 @@ int intel_modeset_calc_cdclk(struct intel_atomic_state *state) * by holding all the crtc mutexes even if we don't end up * touching the hardware */ - if (intel_cdclk_changed(&dev_priv->cdclk.actual, - &state->cdclk.actual)) { - /* - * Also serialize commits across all crtcs - * if the actual hw needs to be poked. - */ - ret = intel_atomic_serialize_global_state(state); - if (ret) - return ret; - } else if (intel_cdclk_changed(&dev_priv->cdclk.logical, - &state->cdclk.logical)) { - ret = intel_atomic_lock_global_state(state); - if (ret) - return ret; - } else { + ret = intel_cdclk_changed(&dev_priv->cdclk.actual, + &state->cdclk.actual); + ret |= intel_cdclk_changed(&dev_priv->cdclk.logical, + &state->cdclk.logical); + if (!ret) return 0; - } if (is_power_of_2(state->active_pipes) && intel_cdclk_needs_cd2x_update(dev_priv, diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index dd03987cc24f..b3a6e39e48c8 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -14284,25 +14284,19 @@ static int intel_modeset_checks(struct intel_atomic_state *state) state->cdclk.force_min_cdclk = dev_priv->cdclk.force_min_cdclk; state->modeset = true; - state->active_pipes = dev_priv->active_pipes; state->cdclk.logical = dev_priv->cdclk.logical; state->cdclk.actual = dev_priv->cdclk.actual; + for_each_intel_crtc(&dev_priv->drm, crtc) { + new_crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); + if (IS_ERR(new_crtc_state)) + return PTR_ERR(new_crtc_state); + } + for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { if (new_crtc_state->hw.active) state->active_pipes |= BIT(crtc->pipe); - else - state->active_pipes &= ~BIT(crtc->pipe); - - if (old_crtc_state->hw.active != new_crtc_state->hw.active) - state->active_pipe_changes |= BIT(crtc->pipe); - } - - if (state->active_pipe_changes) { - ret = intel_atomic_lock_global_state(state); - if (ret) - return ret; } ret = intel_modeset_calc_cdclk(state); @@ -15586,7 +15580,7 @@ static int intel_atomic_commit(struct drm_device *dev, intel_shared_dpll_swap_state(state); intel_atomic_track_fbs(state); - if (state->global_state_changed) { + if (state->modeset) { assert_global_state_locked(dev_priv); memcpy(dev_priv->min_cdclk, state->min_cdclk, diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index 155ce49ae764..60d812234654 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -483,16 +483,6 @@ struct intel_atomic_state { bool dpll_set, modeset; - /* - * Does this transaction change the pipes that are active? This mask - * tracks which CRTC's have changed their active state at the end of - * the transaction (not counting the temporary disable during modesets). - * This mask should only be non-zero when intel_state->modeset is true, - * but the converse is not necessarily true; simply changing a mode may - * not flip the final active status of any CRTC's - */ - u8 active_pipe_changes; - u8 active_pipes; /* minimum acceptable cdclk for each pipe */ int min_cdclk[I915_MAX_PIPES]; @@ -509,14 +499,6 @@ struct intel_atomic_state { bool rps_interactive; - /* - * active_pipes - * min_cdclk[] - * min_voltage_level[] - * cdclk.* - */ - bool global_state_changed; - /* Gen9+ only */ struct skl_ddb_values wm_results; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 81e5a3278fda..5135d887f328 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -3924,10 +3924,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv, return; } - if (intel_state->active_pipe_changes) - *num_active = hweight8(intel_state->active_pipes); - else - *num_active = hweight8(dev_priv->active_pipes); + *num_active = hweight8(intel_state->active_pipes); ddb_size = intel_get_ddb_size(dev_priv, crtc_state, total_data_rate, *num_active, ddb); @@ -3940,7 +3937,7 @@ skl_ddb_get_pipe_allocation_limits(struct drm_i915_private *dev_priv, * that changes the active CRTC list or do modeset would need to * grab _all_ crtc locks, including the one we currently hold. */ - if (!intel_state->active_pipe_changes && !intel_state->modeset) { + if (!intel_state->modeset) { /* * alloc may be cleared by clear_intel_crtc_state, * copy from old state to be sure @@ -5379,27 +5376,10 @@ skl_print_wm_changes(struct intel_atomic_state *state) } } -static int intel_add_all_pipes(struct intel_atomic_state *state) -{ - struct drm_i915_private *dev_priv = to_i915(state->base.dev); - struct intel_crtc *crtc; - - for_each_intel_crtc(&dev_priv->drm, crtc) { - struct intel_crtc_state *crtc_state; - - crtc_state = intel_atomic_get_crtc_state(&state->base, crtc); - if (IS_ERR(crtc_state)) - return PTR_ERR(crtc_state); - } - - return 0; -} - static int -skl_ddb_add_affected_pipes(struct intel_atomic_state *state) +skl_ddb_mark_dirty_affected_pipes(struct intel_atomic_state *state) { struct drm_i915_private *dev_priv = to_i915(state->base.dev); - int ret; /* * If this is our first atomic update following hardware readout, @@ -5408,13 +5388,6 @@ skl_ddb_add_affected_pipes(struct intel_atomic_state *state) * ensure a full DDB recompute. */ if (dev_priv->wm.distrust_bios_wm) { - ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex, - state->base.acquire_ctx); - if (ret) - return ret; - - state->active_pipe_changes = INTEL_INFO(dev_priv)->pipe_mask; - /* * We usually only initialize state->active_pipes if we * we're doing a modeset; make sure this field is always @@ -5438,14 +5411,9 @@ skl_ddb_add_affected_pipes(struct intel_atomic_state *state) * any other display updates race with this transaction, so we need * to grab the lock on *all* CRTC's. */ - if (state->active_pipe_changes || state->modeset) { + if (dev_priv->wm.distrust_bios_wm || state->modeset) state->wm_results.dirty_pipes = INTEL_INFO(dev_priv)->pipe_mask; - ret = intel_add_all_pipes(state); - if (ret) - return ret; - } - return 0; } @@ -5521,7 +5489,7 @@ skl_compute_wm(struct intel_atomic_state *state) /* Clear all dirty flags */ results->dirty_pipes = 0; - ret = skl_ddb_add_affected_pipes(state); + ret = skl_ddb_mark_dirty_affected_pipes(state); if (ret) return ret;