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

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, Nov 25, 2013 at 06:44:09PM +0000, Chris Wilson wrote:
> On Mon, Nov 25, 2013 at 10:28:26AM -0800, Ben Widawsky wrote:
> > On Mon, Nov 25, 2013 at 06:18:52PM +0000, Chris Wilson wrote:
> > > 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>
> > > > ---
> > > >  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;
> > > >  
> > > 
> > > i915_reset_in_progress(&dev_priv->gpu_error));
> > > doesn't actually mean that we are in the middle of a reset. Or does
> > > it?  Anyway intel_ring_begin() returns EIO/EAGAIN so we do not need
> > > to pass down the parameter. But the issue with mixing and matching LRI
> > > vs mmio is that if this was not a reset call, then we just upset the
> > > GPU even further.
> > > -Chris
> > 
> > /me sighs
> > 
> > The synchronous argument comes from the future. There are
> > three places where one could conceivably use it:
> > 1. before rings are up
> > 2. during hang
> > 3. after rings are shut down
> > 
> > With the current code, only #2 is actually possible. I don't like
> > checking EIO/EAGAIN as there are cases where we expect failure, and
> > cases where we do not. Being able to strain out which one is which, is
> > helpful, and [in the future] the caller is the one that can take
> > appropriate action. Also I found also using the argument a bit nicer
> > since every gen would have to implement the same logic to determine if
> > the rings were usage.
> > 
> > You'll have to trust me that I won't use MMIO when I shouldn't be using
> > it.
> > 
> > I can understand your comment at this stage, but I hope my reasoning
> > makes sense. (Feel free to view my ppgtt branch if you'd like)
> 
> Sure having a parameter will be useful to do other things, I am just not
> convinced that what you want to do in this patch is sane. Currently, we
> only call ->enable in i915_gem_init_hw(), so what you do here could
> work. But presumably, with full-ppgtt it will then be possible to call
> between resets making us vulnerable to weird races?
> -Chris
> 

In the future case, as long as the MMIO always follows the GPU reset
(before we're using the rings), it should be race free.

FWIW, in the future case, MMIO is still always relegated to case #2 (for
exactly one patch, it is both #1, and #2.) The other case is
aliasing PPGTT only ofc.

-- 
Ben Widawsky, Intel Open Source Technology Center