From patchwork Wed Jan 22 20:43:29 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?VmlsbGUgU3lyasOkbMOk?= X-Patchwork-Id: 11346369 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 5B7756C1 for ; Wed, 22 Jan 2020 20:43:35 +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 435E424655 for ; Wed, 22 Jan 2020 20:43:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 435E424655 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.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 B06D46F8D1; Wed, 22 Jan 2020 20:43:34 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3D2A66F8D1 for ; Wed, 22 Jan 2020 20:43:33 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Jan 2020 12:43:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,350,1574150400"; d="scan'208";a="216032771" Received: from stinkbox.fi.intel.com (HELO stinkbox) ([10.237.72.174]) by orsmga007.jf.intel.com with SMTP; 22 Jan 2020 12:43:30 -0800 Received: by stinkbox (sSMTP sendmail emulation); Wed, 22 Jan 2020 22:43:29 +0200 From: Ville Syrjala To: intel-gfx@lists.freedesktop.org Date: Wed, 22 Jan 2020 22:43:29 +0200 Message-Id: <20200122204329.2477-1-ville.syrjala@linux.intel.com> X-Mailer: git-send-email 2.24.1 MIME-Version: 1.0 Subject: [Intel-gfx] [PATCH] drm/i915: Fix modeset locks in sanitize_watermarks() 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: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" From: Ville Syrjälä We've added more internal things that use modeset locks and thus we need to be prepared for intel_atomic_check() grabbing more locks than what our initial drm_modeset_lock_all_ctx() took. So we're missing the backoff handling here. Also drm_atomic_helper_duplicate_state() works against us by clearing state->acquire_ctx in anticipation of drm_atomic_helper_commit_duplicated_state() being used to commit the state. We could probably just reset acquire_ctx back, but instead let's just rewrite the whole thing without using either of those "helpers". There's also no need to add any connectors to the state here since we just want the new watermarks which don't depend on connectors. Cc: Chris Wilson Signed-off-by: Ville Syrjälä Reviewed-by: Chris Wilson --- drivers/gpu/drm/i915/display/intel_display.c | 100 ++++++++++++------- 1 file changed, 62 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 878d331b9e8c..cfc0a067ad48 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -17297,6 +17297,30 @@ void intel_modeset_init_hw(struct drm_i915_private *i915) i915->cdclk.logical = i915->cdclk.actual = i915->cdclk.hw; } +static int sanitize_watermarks_add_affected(struct drm_atomic_state *state) +{ + struct drm_plane *plane; + struct drm_crtc *crtc; + + drm_for_each_crtc(crtc, state->dev) { + struct drm_crtc_state *crtc_state; + + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + } + + drm_for_each_plane(plane, state->dev) { + struct drm_plane_state *plane_state; + + plane_state = drm_atomic_get_plane_state(state, plane); + if (IS_ERR(plane_state)) + return PTR_ERR(plane_state); + } + + return 0; +} + /* * Calculate what we think the watermarks should be for the state we've read * out of the hardware and then immediately program those watermarks so that @@ -17307,9 +17331,8 @@ void intel_modeset_init_hw(struct drm_i915_private *i915) * through the atomic check code to calculate new watermark values in the * state object. */ -static void sanitize_watermarks(struct drm_device *dev) +static void sanitize_watermarks(struct drm_i915_private *dev_priv) { - struct drm_i915_private *dev_priv = to_i915(dev); struct drm_atomic_state *state; struct intel_atomic_state *intel_state; struct intel_crtc *crtc; @@ -17322,26 +17345,17 @@ static void sanitize_watermarks(struct drm_device *dev) if (!dev_priv->display.optimize_watermarks) return; - /* - * We need to hold connection_mutex before calling duplicate_state so - * that the connector loop is protected. - */ - drm_modeset_acquire_init(&ctx, 0); -retry: - ret = drm_modeset_lock_all_ctx(dev, &ctx); - if (ret == -EDEADLK) { - drm_modeset_backoff(&ctx); - goto retry; - } else if (WARN_ON(ret)) { - goto fail; - } - - state = drm_atomic_helper_duplicate_state(dev, &ctx); - if (WARN_ON(IS_ERR(state))) - goto fail; + state = drm_atomic_state_alloc(&dev_priv->drm); + if (WARN_ON(!state)) + return; intel_state = to_intel_atomic_state(state); + drm_modeset_acquire_init(&ctx, 0); + +retry: + state->acquire_ctx = &ctx; + /* * Hardware readout is the only time we don't want to calculate * intermediate watermarks (since we don't trust the current @@ -17350,22 +17364,13 @@ static void sanitize_watermarks(struct drm_device *dev) if (!HAS_GMCH(dev_priv)) intel_state->skip_intermediate_wm = true; - ret = intel_atomic_check(dev, state); - if (ret) { - /* - * If we fail here, it means that the hardware appears to be - * programmed in a way that shouldn't be possible, given our - * understanding of watermark requirements. This might mean a - * mistake in the hardware readout code or a mistake in the - * watermark calculations for a given platform. Raise a WARN - * so that this is noticeable. - * - * If this actually happens, we'll have to just leave the - * BIOS-programmed watermarks untouched and hope for the best. - */ - WARN(true, "Could not determine valid watermarks for inherited state\n"); - goto put_state; - } + ret = sanitize_watermarks_add_affected(state); + if (ret) + goto fail; + + ret = intel_atomic_check(&dev_priv->drm, state); + if (ret) + goto fail; /* Write calculated watermark values back */ for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) { @@ -17375,9 +17380,28 @@ static void sanitize_watermarks(struct drm_device *dev) to_intel_crtc_state(crtc->base.state)->wm = crtc_state->wm; } -put_state: - drm_atomic_state_put(state); fail: + if (ret == -EDEADLK) { + drm_atomic_state_clear(state); + drm_modeset_backoff(&ctx); + goto retry; + } + + /* + * If we fail here, it means that the hardware appears to be + * programmed in a way that shouldn't be possible, given our + * understanding of watermark requirements. This might mean a + * mistake in the hardware readout code or a mistake in the + * watermark calculations for a given platform. Raise a WARN + * so that this is noticeable. + * + * If this actually happens, we'll have to just leave the + * BIOS-programmed watermarks untouched and hope for the best. + */ + WARN(ret, "Could not determine valid watermarks for inherited state\n"); + + drm_atomic_state_put(state); + drm_modeset_drop_locks(&ctx); drm_modeset_acquire_fini(&ctx); } @@ -17593,7 +17617,7 @@ int intel_modeset_init(struct drm_i915_private *i915) * since the watermark calculation done here will use pstate->fb. */ if (!HAS_GMCH(i915)) - sanitize_watermarks(dev); + sanitize_watermarks(i915); /* * Force all active planes to recompute their states. So that on