Re: [PATCH 02/12] drm/i915: Provide PDP updates via MMIO

From: Daniel Vetter <daniel@ffwll.ch>
To: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: Intel GFX <intel-gfx@lists.freedesktop.org>,
	Ben Widawsky <ben@bwidawsk.net>
Subject: Re: [PATCH 02/12] drm/i915: Provide PDP updates via MMIO
Date: Tue, 26 Nov 2013 10:05:32 +0100	[thread overview]
Message-ID: <20131126090532.GE27344@phenom.ffwll.local> (raw)
In-Reply-To: <1385402083-8719-3-git-send-email-benjamin.widawsky@intel.com>

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 <ben@bwidawsk.net>

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