From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 02/12] drm/i915: Provide PDP updates via MMIO Date: Tue, 26 Nov 2013 10:05:32 +0100 Message-ID: <20131126090532.GE27344@phenom.ffwll.local> References: <1385402083-8719-1-git-send-email-benjamin.widawsky@intel.com> <1385402083-8719-3-git-send-email-benjamin.widawsky@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f175.google.com (mail-ea0-f175.google.com [209.85.215.175]) by gabe.freedesktop.org (Postfix) with ESMTP id B8429105782 for ; Tue, 26 Nov 2013 01:04:52 -0800 (PST) Received: by mail-ea0-f175.google.com with SMTP id z10so3218771ead.6 for ; Tue, 26 Nov 2013 01:04:52 -0800 (PST) Content-Disposition: inline In-Reply-To: <1385402083-8719-3-git-send-email-benjamin.widawsky@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Ben Widawsky Cc: Intel GFX , Ben Widawsky List-Id: intel-gfx@lists.freedesktop.org On Mon, Nov 25, 2013 at 09:54:33AM -0800, Ben Widawsky wrote: > The initial implementation of this function used MMIO to write the PDPs. > Upon review it was determined (correctly) that the docs say to use LRI. > The issue is there are times where we want to do a synchronous write > (GPU reset). > > I've tested this, and it works. I've verified with as many people as > possible that it should work. > > This should fix the failing reset problems. > > Signed-off-by: Ben Widawsky This makes me nervous, for two reasons: - Historically we've _always_ ended up in hilarity, bloodshed and tears when having different setup/teardown code in the different parts of the driver. Always. - Looking at the reset code we should be initializing the rings before we try to re-enable ppgtt. So if I don't misread stuff horribly this needs at least one giant comment explaining what's exactly going on here - either the hw is busted or we have some other issue. Aside: Mika noticed that we're loosing a bunch of the w/a settings since a few of the per-ring workarounds have been errornously put into init_clock_gating. We need to move those to the correct per-ring init function. This is even more important once we have per-ring reset. Cheers, Daniel > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 1a5272c..96dbf3d 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -197,12 +197,19 @@ static gen6_gtt_pte_t iris_pte_encode(dma_addr_t addr, > > /* Broadwell Page Directory Pointer Descriptors */ > static int gen8_write_pdp(struct intel_ring_buffer *ring, unsigned entry, > - uint64_t val) > + uint64_t val, bool synchronous) > { > + struct drm_i915_private *dev_priv = ring->dev->dev_private; > int ret; > > BUG_ON(entry >= 4); > > + if (synchronous) { > + I915_WRITE(GEN8_RING_PDP_UDW(ring, entry), val >> 32); > + I915_WRITE(GEN8_RING_PDP_LDW(ring, entry), (u32)val); > + return 0; > + } > + > ret = intel_ring_begin(ring, 6); > if (ret) > return ret; > @@ -236,7 +243,8 @@ static int gen8_ppgtt_enable(struct drm_device *dev) > for (i = used_pd - 1; i >= 0; i--) { > dma_addr_t addr = ppgtt->pd_dma_addr[i]; > for_each_ring(ring, dev_priv, j) { > - ret = gen8_write_pdp(ring, i, addr); > + ret = gen8_write_pdp(ring, i, addr, > + i915_reset_in_progress(&dev_priv->gpu_error)); > if (ret) > goto err_out; > } > -- > 1.8.4.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch