From patchwork Mon Apr 24 07:17:39 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Lionel Landwerlin X-Patchwork-Id: 9695713 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 477E860113 for ; Mon, 24 Apr 2017 07:18:11 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 36DE022B26 for ; Mon, 24 Apr 2017 07:18:11 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2A880267EC; Mon, 24 Apr 2017 07:18:11 +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 B979422B26 for ; Mon, 24 Apr 2017 07:18:10 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 3A8E16E1A1; Mon, 24 Apr 2017 07:18:07 +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 913956E063 for ; Mon, 24 Apr 2017 07:18:03 +0000 (UTC) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP; 24 Apr 2017 00:18:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos; i="5.37,243,1488873600"; d="scan'208"; a="1160112630" Received: from thimaith-mobl.ccr.corp.intel.com (HELO delly.amr.corp.intel.com) ([10.252.134.160]) by fmsmga002.fm.intel.com with ESMTP; 24 Apr 2017 00:17:59 -0700 From: Lionel Landwerlin To: intel-gfx@lists.freedesktop.org Date: Mon, 24 Apr 2017 00:17:39 -0700 Message-Id: <20170424071751.2416-4-lionel.g.landwerlin@intel.com> X-Mailer: git-send-email 2.11.0 In-Reply-To: <20170424071751.2416-1-lionel.g.landwerlin@intel.com> References: <20170412155556.6602-1-robert@sixbynine.org> <20170424071751.2416-1-lionel.g.landwerlin@intel.com> Subject: [Intel-gfx] [PATCH v5 03/15] drm/i915/perf: avoid read back of head register 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: , MIME-Version: 1.0 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP From: Robert Bragg There's no need for the driver to keep reading back the head pointer from hardware since the hardware doesn't update it automatically. This way we can treat any invalid head pointer value as a software/driver bug instead of spurious hardware behaviour. This change is also a small stepping stone towards re-working how the head and tail state is managed as part of an improved workaround for the tail register race condition. Signed-off-by: Robert Bragg Reviewed-by: Matthew Auld --- drivers/gpu/drm/i915/i915_drv.h | 11 ++++++++++ drivers/gpu/drm/i915/i915_perf.c | 46 ++++++++++++++++++---------------------- 2 files changed, 32 insertions(+), 25 deletions(-) -- 2.11.0 diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 357b6c6c2f04..b618a8bc54f6 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2469,6 +2469,17 @@ struct drm_i915_private { u8 *vaddr; int format; int format_size; + + /** + * Although we can always read back the head + * pointer register, we prefer to avoid + * trusting the HW state, just to avoid any + * risk that some hardware condition could + * somehow bump the head pointer unpredictably + * and cause us to forward the wrong OA buffer + * data to userspace. + */ + u32 head; } oa_buffer; u32 gen7_latched_oastatus1; diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index f59f6dd20922..f47d1cc2144b 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -322,9 +322,8 @@ struct perf_open_properties { static bool gen7_oa_buffer_is_empty_fop_unlocked(struct drm_i915_private *dev_priv) { int report_size = dev_priv->perf.oa.oa_buffer.format_size; - u32 oastatus2 = I915_READ(GEN7_OASTATUS2); u32 oastatus1 = I915_READ(GEN7_OASTATUS1); - u32 head = oastatus2 & GEN7_OASTATUS2_HEAD_MASK; + u32 head = dev_priv->perf.oa.oa_buffer.head; u32 tail = oastatus1 & GEN7_OASTATUS1_TAIL_MASK; return OA_TAKEN(tail, head) < @@ -458,16 +457,24 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream, return -EIO; head = *head_ptr - gtt_offset; + + /* An out of bounds or misaligned head pointer implies a driver bug + * since we are in full control of head pointer which should only + * be incremented by multiples of the report size (notably also + * all a power of two). + */ + if (WARN_ONCE(head > OA_BUFFER_SIZE || head % report_size, + "Inconsistent OA buffer head pointer = %u\n", head)) + return -EIO; + tail -= gtt_offset; /* The OA unit is expected to wrap the tail pointer according to the OA - * buffer size and since we should never write a misaligned head - * pointer we don't expect to read one back either... + * buffer size */ - if (tail > OA_BUFFER_SIZE || head > OA_BUFFER_SIZE || - head % report_size) { - DRM_ERROR("Inconsistent OA buffer pointer (head = %u, tail = %u): force restart\n", - head, tail); + if (tail > OA_BUFFER_SIZE) { + DRM_ERROR("Inconsistent OA buffer tail pointer = %u: force restart\n", + tail); dev_priv->perf.oa.ops.oa_disable(dev_priv); dev_priv->perf.oa.ops.oa_enable(dev_priv); *head_ptr = I915_READ(GEN7_OASTATUS2) & @@ -562,8 +569,6 @@ static int gen7_oa_read(struct i915_perf_stream *stream, size_t *offset) { struct drm_i915_private *dev_priv = stream->dev_priv; - int report_size = dev_priv->perf.oa.oa_buffer.format_size; - u32 oastatus2; u32 oastatus1; u32 head; u32 tail; @@ -572,10 +577,9 @@ static int gen7_oa_read(struct i915_perf_stream *stream, if (WARN_ON(!dev_priv->perf.oa.oa_buffer.vaddr)) return -EIO; - oastatus2 = I915_READ(GEN7_OASTATUS2); oastatus1 = I915_READ(GEN7_OASTATUS1); - head = oastatus2 & GEN7_OASTATUS2_HEAD_MASK; + head = dev_priv->perf.oa.oa_buffer.head; tail = oastatus1 & GEN7_OASTATUS1_TAIL_MASK; /* XXX: On Haswell we don't have a safe way to clear oastatus1 @@ -616,10 +620,9 @@ static int gen7_oa_read(struct i915_perf_stream *stream, dev_priv->perf.oa.ops.oa_disable(dev_priv); dev_priv->perf.oa.ops.oa_enable(dev_priv); - oastatus2 = I915_READ(GEN7_OASTATUS2); oastatus1 = I915_READ(GEN7_OASTATUS1); - head = oastatus2 & GEN7_OASTATUS2_HEAD_MASK; + head = dev_priv->perf.oa.oa_buffer.head; tail = oastatus1 & GEN7_OASTATUS1_TAIL_MASK; } @@ -635,17 +638,6 @@ static int gen7_oa_read(struct i915_perf_stream *stream, ret = gen7_append_oa_reports(stream, buf, count, offset, &head, tail); - /* All the report sizes are a power of two and the - * head should always be incremented by some multiple - * of the report size. - * - * A warning here, but notably if we later read back a - * misaligned pointer we will treat that as a bug since - * it could lead to a buffer overrun. - */ - WARN_ONCE(head & (report_size - 1), - "i915: Writing misaligned OA head pointer"); - /* Note: we update the head pointer here even if an error * was returned since the error may represent a short read * where some some reports were successfully copied. @@ -653,6 +645,7 @@ static int gen7_oa_read(struct i915_perf_stream *stream, I915_WRITE(GEN7_OASTATUS2, ((head & GEN7_OASTATUS2_HEAD_MASK) | OA_MEM_SELECT_GGTT)); + dev_priv->perf.oa.oa_buffer.head = head; return ret; } @@ -834,7 +827,10 @@ static void gen7_init_oa_buffer(struct drm_i915_private *dev_priv) * before OASTATUS1, but after OASTATUS2 */ I915_WRITE(GEN7_OASTATUS2, gtt_offset | OA_MEM_SELECT_GGTT); /* head */ + dev_priv->perf.oa.oa_buffer.head = gtt_offset; + I915_WRITE(GEN7_OABUFFER, gtt_offset); + I915_WRITE(GEN7_OASTATUS1, gtt_offset | OABUFFER_SIZE_16M); /* tail */ /* On Haswell we have to track which OASTATUS1 flags we've