Re: [PATCH 3/3] drm/i915/bdw: Use the GEN8 SRM when qeueing a flip

From: Daniel Vetter <daniel@ffwll.ch>
To: Ben Widawsky <benjamin.widawsky@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 3/3] drm/i915/bdw: Use the GEN8 SRM when qeueing a flip
Date: Wed, 9 Apr 2014 14:59:51 +0200	[thread overview]
Message-ID: <20140409125951.GM9262@phenom.ffwll.local> (raw)
In-Reply-To: <20140407205917.GD18726@intel.com>

On Mon, Apr 07, 2014 at 01:59:17PM -0700, Ben Widawsky wrote:
> On Mon, Apr 07, 2014 at 08:24:34PM +0100, Damien Lespiau wrote:
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 22 +++++++++++++++++++---
> >  1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3697433..a646ed4 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8821,8 +8821,16 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
> >  	}
> >  
> >  	len = 4;
> > -	if (ring->id == RCS)
> > +	if (ring->id == RCS) {
> >  		len += 6;
> > +		/*
> > +		 * On Gen 8, SRM is now taking an extra dword to accommodate
> > +		 * 48bits addresses, and we need a NOOP for the batch size to
> > +		 * stay even.
> > +		 */
> > +		if (IS_GEN8(dev))
> > +			len += 2;
> > +	}
> >  
> >  	/*
> >  	 * BSpec MI_DISPLAY_FLIP for IVB:
> > @@ -8857,10 +8865,18 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
> >  		intel_ring_emit(ring, ~(DERRMR_PIPEA_PRI_FLIP_DONE |
> >  					DERRMR_PIPEB_PRI_FLIP_DONE |
> >  					DERRMR_PIPEC_PRI_FLIP_DONE));
> > -		intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) |
> > -				MI_SRM_LRM_GLOBAL_GTT);
> > +		if (IS_GEN8(dev))
> > +			intel_ring_emit(ring, MI_STORE_REGISTER_MEM_GEN8(1) |
> > +					      MI_SRM_LRM_GLOBAL_GTT);
> > +		else
> > +			intel_ring_emit(ring, MI_STORE_REGISTER_MEM(1) |
> > +					      MI_SRM_LRM_GLOBAL_GTT);
> >  		intel_ring_emit(ring, DERRMR);
> >  		intel_ring_emit(ring, ring->scratch.gtt_offset + 256);
> > +		if (IS_GEN8(dev)) {
> > +			intel_ring_emit(ring, 0);
> > +			intel_ring_emit(ring, MI_NOOP);
> > +		}
> >  	}
> >  
> >  	intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
> 
> Cool. This explains the bad DERRMR values I was seeing in in error
> states. I'm honestly didn't check if we actually need an SRM for BDW
> still, but I'll assume you did check.
> 
> I also think it's worth to make a intel_gen8_queue_flip, but since I
> don't touch this code much, I'll leave it to you to decide.

Yeah, gen8_queue_flip would kinda look neat. Anyway pull in the entire
series, thanks.
-Daniel

> 
> The series is:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> 
> minus the above mentioned fact of whether it's needed.
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> 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