Re: [PATCH v2] drm/i915: Insert a command barrier on BLT/BSD cache flushes

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 09 Feb 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Feb 09, 2015 at 06:00:25PM +0200, Jani Nikula wrote:
>> On Thu, 22 Jan 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > This looked like an odd regression from
>> >
>> > commit ec5cc0f9b019af95e4571a9fa162d94294c8d90b
>> > Author: Chris Wilson <chris@chris-wilson.co.uk>
>> > Date:   Thu Jun 12 10:28:55 2014 +0100
>> >
>> >     drm/i915: Restrict GPU boost to the RCS engine
>> >
>> > but in reality it undercovered a much older coherency bug. The issue that
>> > boosting the GPU frequency on the BCS ring was masking was that we could
>> > wake the CPU up after completion of a BCS batch and inspect memory prior
>> > to the write cache being fully evicted. In order to serialise the
>> > breadcrumb interrupt (and so ensure that the CPU's view of memory is
>> > coherent) we need to perform a post-sync operation in the MI_FLUSH_DW.
>> >
>> > v2: Fix all the MI_FLUSH_DW (bsd plus the duplication in execlists).
>> >
>> > Testcase: gpuX-rcs-gpu-read-after-write
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: stable@vger.kernel.org
>> > Acked-by: Daniel Vetter <daniel@ffwll.ch>
>> > ---
>> >  drivers/gpu/drm/i915/intel_lrc.c        | 20 +++++++++++---------
>> >  drivers/gpu/drm/i915/intel_ringbuffer.c | 23 +++++++++++++++++++----
>> >  2 files changed, 30 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> > index e405b61cdac5..8e71d8851c9a 100644
>> > --- a/drivers/gpu/drm/i915/intel_lrc.c
>> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> > @@ -1237,15 +1237,17 @@ static int gen8_emit_flush(struct intel_ringbuffer *ringbuf,
>> >  
>> >  	cmd = MI_FLUSH_DW + 1;
>> >  
>> > -	if (ring == &dev_priv->ring[VCS]) {
>> > -		if (invalidate_domains & I915_GEM_GPU_DOMAINS)
>> > -			cmd |= MI_INVALIDATE_TLB | MI_INVALIDATE_BSD |
>> > -				MI_FLUSH_DW_STORE_INDEX |
>> > -				MI_FLUSH_DW_OP_STOREDW;
>> > -	} else {
>> > -		if (invalidate_domains & I915_GEM_DOMAIN_RENDER)
>> > -			cmd |= MI_INVALIDATE_TLB | MI_FLUSH_DW_STORE_INDEX |
>> > -				MI_FLUSH_DW_OP_STOREDW;
>> > +	/* We always require a command barrier so that subsequent
>> > +	 * commands, such as breadcrumb interrupts, are strictly ordered
>> > +	 * wrt the contents of the write cache being flushed to memory
>> > +	 * (and thus being coherent from the CPU).
>> > +	 */
>> > +	cmd |= MI_FLUSH_DW_STORE_INDEX | MI_FLUSH_DW_OP_STOREDW;
>> > +
>> > +	if (invalidate_domains & I915_GEM_GPU_DOMAINS) {
>> 
>> Why do you change the mask from I915_GEM_DOMAIN_RENDER to
>> I915_GEM_GPU_DOMAINS for ring != VCS?
>
> My bad, I didn't notice that execlists was originally broken. The patch
> is correct.

I'll take your and Daniel's word for it. I hope I won't have to regret
not asking you to split this into two patches...

Pushed to drm-intel-next-fixes, thanks for the patch and ack.

BR,
Jani.



> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx