From patchwork Fri Dec 23 15:29:36 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: "Zanoni, Paulo R" X-Patchwork-Id: 9487533 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id DD34A601C0 for ; Fri, 23 Dec 2016 15:29:46 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CD9662621E for ; Fri, 23 Dec 2016 15:29:46 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id C016A2623D; Fri, 23 Dec 2016 15:29:46 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 07EEC2621E for ; Fri, 23 Dec 2016 15:29:46 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 569F06E084; Fri, 23 Dec 2016 15:29:45 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTPS id 416796E084 for ; Fri, 23 Dec 2016 15:29:44 +0000 (UTC) Received: from orsmga005.jf.intel.com ([10.7.209.41]) by orsmga102.jf.intel.com with ESMTP; 23 Dec 2016 07:29:43 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,393,1477983600"; d="scan'208";a="45847329" Received: from przanoni-mobl.amr.corp.intel.com ([10.254.176.247]) by orsmga005.jf.intel.com with ESMTP; 23 Dec 2016 07:29:41 -0800 From: Paulo Zanoni To: intel-gfx@lists.freedesktop.org Date: Fri, 23 Dec 2016 13:29:36 -0200 Message-Id: <1482506976-13830-1-git-send-email-paulo.r.zanoni@intel.com> X-Mailer: git-send-email 2.7.4 In-Reply-To: <20161223134023.GT31595@intel.com> References: <20161223134023.GT31595@intel.com> MIME-Version: 1.0 Cc: Paulo Zanoni Subject: [Intel-gfx] [PATCH 2/2] drm/i915: rewrite FBC's atomic CRTC-choosing code X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 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" X-Virus-Scanned: ClamAV using ClamSMTP Ville pointed out that intel_fbc_choose_crtc() is iterating over all planes instead of just the primary planes. There are no real consequences of this problem for HSW+, and for the other platforms it just means that in some obscure multi-screen cases we'll keep FBC disabled when we could have enabled it. Still, iterating over all planes doesn't seem to be the best thing to do. My initial idea was to just add a check for plane->type and be done, but then I realized that in commits not involving the primary plane we would reset crtc_state->enable_fbc back to false even when FBC is enabled. That also wouldn't result in a bug due to the way the enable_fbc variable is checked, but, still, our code can be better than this. So I went for the solution that involves tracking enable_fbc in the primary plane state instead of the CRTC state. This way, if a commit doesn't involve the primary plane for the CRTC we won't be resetting enable_fbc back to false, so the variable will always reflect the reality. And this change also makes more sense since FBC is actually tied to the single plane and not the full pipe. As a bonus, we only iterate over the CRTCs instead of iterating over all planes. v2: But Ville pointed that this only works properly if we have a single commit with multiple CRTCs. If we have multiple parallel commits, each with its own CRTC, we'll end with enable_fbc set to true in more than one plane. So the solution was to rename enable_fbc to can_enable_fbc. If we just did the rename as the patch was, we'd end up with a single plane with can_enable_fbc on commits involving multiple CRTCs: we'd choose the best one, but we'd still end up with a variable that doesn't 100% reflect reality. So in the end I adopted Ville's suggestion of the fbc_score variable. And then, instead of checking the score at intel_fbc_choose_crtc() it should be possible to check for the score at intel_fbc_enable(). This allows us to simplify intel_fbc_choose_crtc() to the point where it only needs to look at the given plane in order to assign its score instead of looking at every primary plane on the same commit. We still only set scores of 0 and 1 and we don't really do the score-checking loop. v3: Rebase. v4: Move intel_fbc_check_plane() call up so it will still zero the score if the plane has no FB. (Ville). Cc: Ville Syrjälä Reported-by: Ville Syrjälä Signed-off-by: Paulo Zanoni --- drivers/gpu/drm/i915/intel_display.c | 3 +- drivers/gpu/drm/i915/intel_drv.h | 8 ++-- drivers/gpu/drm/i915/intel_fbc.c | 85 ++++++++++++------------------------ 3 files changed, 35 insertions(+), 61 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d8effd4..56ae32c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14191,7 +14191,6 @@ static int intel_atomic_check(struct drm_device *dev, if (ret) return ret; - intel_fbc_choose_crtc(dev_priv, state); return calc_watermark_data(state); } @@ -14957,6 +14956,8 @@ intel_check_primary_plane(struct drm_plane *plane, if (ret) return ret; + intel_fbc_check_plane(state); + if (!state->base.fb) return 0; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 025e4c8..7430082 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -406,6 +406,9 @@ struct intel_plane_state { */ int scaler_id; + /* 0: not suitable for FBC, 1+: suitable for FBC, more is better. */ + unsigned int fbc_score; + struct drm_intel_sprite_colorkey ckey; }; @@ -652,8 +655,6 @@ struct intel_crtc_state { bool ips_enabled; - bool enable_fbc; - bool double_wide; int pbn; @@ -1535,8 +1536,7 @@ static inline void intel_fbdev_restore_mode(struct drm_device *dev) #endif /* intel_fbc.c */ -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv, - struct drm_atomic_state *state); +void intel_fbc_check_plane(struct intel_plane_state *plane_state); bool intel_fbc_is_active(struct drm_i915_private *dev_priv); void intel_fbc_pre_update(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state, diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 26a81a9..ceda6f4 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -1040,68 +1040,32 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv, } /** - * intel_fbc_choose_crtc - select a CRTC to enable FBC on - * @dev_priv: i915 device instance - * @state: the atomic state structure + * intel_fbc_check_plane - check plane for FBC suitability + * @plane_state: the plane state * - * This function looks at the proposed state for CRTCs and planes, then chooses - * which pipe is going to have FBC by setting intel_crtc_state->enable_fbc to - * true. + * This function should set fbc_score based on how suitable the plane is for + * FBC. For now the only scores used are 0 and 1. * - * Later, intel_fbc_enable is going to look for state->enable_fbc and then maybe - * enable FBC for the chosen CRTC. If it does, it will set dev_priv->fbc.crtc. + * We're not changing dev_priv->fbc, so there's no need for the FBC lock. */ -void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv, - struct drm_atomic_state *state) +void intel_fbc_check_plane(struct intel_plane_state *plane_state) { - struct intel_fbc *fbc = &dev_priv->fbc; - struct drm_plane *plane; - struct drm_plane_state *plane_state; - bool crtc_chosen = false; - int i; + struct drm_plane *plane = plane_state->base.plane; + struct drm_i915_private *dev_priv = to_i915(plane->dev); + struct intel_crtc *crtc = to_intel_crtc(plane_state->base.crtc); - mutex_lock(&fbc->lock); - - /* Does this atomic commit involve the CRTC currently tied to FBC? */ - if (fbc->crtc && - !drm_atomic_get_existing_crtc_state(state, &fbc->crtc->base)) - goto out; + plane_state->fbc_score = 0; if (!intel_fbc_can_enable(dev_priv)) - goto out; - - /* Simply choose the first CRTC that is compatible and has a visible - * plane. We could go for fancier schemes such as checking the plane - * size, but this would just affect the few platforms that don't tie FBC - * to pipe or plane A. */ - for_each_plane_in_state(state, plane, plane_state, i) { - struct intel_plane_state *intel_plane_state = - to_intel_plane_state(plane_state); - struct intel_crtc_state *intel_crtc_state; - struct intel_crtc *crtc = to_intel_crtc(plane_state->crtc); - - if (!intel_plane_state->base.visible) - continue; - - if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A) - continue; - - if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A) - continue; - - intel_crtc_state = to_intel_crtc_state( - drm_atomic_get_existing_crtc_state(state, &crtc->base)); - - intel_crtc_state->enable_fbc = true; - crtc_chosen = true; - break; - } - - if (!crtc_chosen) - fbc->no_fbc_reason = "no suitable CRTC for FBC"; + return; + if (fbc_on_pipe_a_only(dev_priv) && crtc->pipe != PIPE_A) + return; + if (fbc_on_plane_a_only(dev_priv) && crtc->plane != PLANE_A) + return; + if (!plane_state->base.visible) + return; -out: - mutex_unlock(&fbc->lock); + plane_state->fbc_score = 1; } /** @@ -1114,6 +1078,13 @@ void intel_fbc_choose_crtc(struct drm_i915_private *dev_priv, * possible. Notice that it doesn't activate FBC. It is valid to call * intel_fbc_enable multiple times for the same pipe without an * intel_fbc_disable in the middle, as long as it is deactivated. + * + * In the future this function could make better use of the fbc_score variable. + * We could, for example, loop through all the primary planes involved in the + * atomic commit and only enable FBC for the plane with the best fbc_score. We + * could also try to do some scheme where a plane with better score takes over + * FBC from another plane, but our driver currently can't handle the complexity + * of switching planes on the fly. This would only affect from Gen 4 up to IVB. */ void intel_fbc_enable(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state, @@ -1130,14 +1101,16 @@ void intel_fbc_enable(struct intel_crtc *crtc, if (fbc->enabled) { WARN_ON(fbc->crtc == NULL); if (fbc->crtc == crtc) { - WARN_ON(!crtc_state->enable_fbc); + WARN_ON(plane_state->fbc_score == 0); WARN_ON(fbc->active); } goto out; } - if (!crtc_state->enable_fbc) + if (plane_state->fbc_score == 0) { + fbc->no_fbc_reason = "no suitable CRTC for FBC"; goto out; + } WARN_ON(fbc->active); WARN_ON(fbc->crtc != NULL);