On Mon, 09 Feb 2015, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Feb 09, 2015 at 06:00:25PM +0200, Jani Nikula wrote:
>> On Thu, 22 Jan 2015, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote:
>> > This looked like an odd regression from
>> >
>> > commit ec5cc0f9b019af95e4571a9fa162d94294c8d90b
>> > Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>> > 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@xxxxxxxxxxxxxxxxxx>
>> > Cc: stable@xxxxxxxxxxxxxxx
>> > Acked-by: Daniel Vetter <daniel@xxxxxxxx>
>> > ---
>> > 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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html