| Message ID | 1479374255-16870-1-git-send-email-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Nov 17, 2016 at 09:17:35AM +0000, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Commit ed4596ea992d ("drm/i915/guc: WA to address the Ringbuffer > coherency issue"), based on incorrect assumptions from a partialy > broken commit 0dd356bb6ff5 ("drm/i915: Eliminate Gen9 special > case") used POSTING_READ_FW instead of the POSTING_READ. With the > latter buggy commit fixed this call site needs fixing as well. The theory here is that we don't need the powerwell to force the write from CPU to be visible before another agent. I missed the report, so I am genuinely interested in knowing whether the theory about the write being posted without the powerwll. -Chris
On 17/11/2016 09:28, Chris Wilson wrote: > On Thu, Nov 17, 2016 at 09:17:35AM +0000, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Commit ed4596ea992d ("drm/i915/guc: WA to address the Ringbuffer >> coherency issue"), based on incorrect assumptions from a partialy >> broken commit 0dd356bb6ff5 ("drm/i915: Eliminate Gen9 special >> case") used POSTING_READ_FW instead of the POSTING_READ. With the >> latter buggy commit fixed this call site needs fixing as well. > > The theory here is that we don't need the powerwell to force the write > from CPU to be visible before another agent. > > I missed the report, so I am genuinely interested in knowing whether the > theory about the write being posted without the powerwll. Just that the commit message for the patch used "guc registers are not in any forcewake domain" reasoning, which was false - based on a partially broken patch. See "drm/i915: Fix gen9 forcewake range table". Regards, Tvrtko
On 11/17/2016 3:06 PM, Tvrtko Ursulin wrote: > > On 17/11/2016 09:28, Chris Wilson wrote: >> On Thu, Nov 17, 2016 at 09:17:35AM +0000, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> Commit ed4596ea992d ("drm/i915/guc: WA to address the Ringbuffer >>> coherency issue"), based on incorrect assumptions from a partialy >>> broken commit 0dd356bb6ff5 ("drm/i915: Eliminate Gen9 special >>> case") used POSTING_READ_FW instead of the POSTING_READ. With the >>> latter buggy commit fixed this call site needs fixing as well. >> >> The theory here is that we don't need the powerwell to force the write >> from CPU to be visible before another agent. >> >> I missed the report, so I am genuinely interested in knowing whether the >> theory about the write being posted without the powerwll. > > Just that the commit message for the patch used "guc registers are not > in any forcewake domain" reasoning, which was false - based on a > partially broken patch. See "drm/i915: Fix gen9 forcewake range table". > > Regards, > > Tvrtko > Verified this fix without forcewake, so this patch will not be needed. Have couple of queries. Chris, could you please clarify: 1. why POSTING_READ is done in flush_gtt_write_domain and not POSTING_READ_FW like this case? 2. how does read from forcewake mmio range work if well is down?
On Mon, Dec 05, 2016 at 03:47:24PM +0530, Kamble, Sagar A wrote: > > > On 11/17/2016 3:06 PM, Tvrtko Ursulin wrote: > > > >On 17/11/2016 09:28, Chris Wilson wrote: > >>On Thu, Nov 17, 2016 at 09:17:35AM +0000, Tvrtko Ursulin wrote: > >>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >>> > >>>Commit ed4596ea992d ("drm/i915/guc: WA to address the Ringbuffer > >>>coherency issue"), based on incorrect assumptions from a partialy > >>>broken commit 0dd356bb6ff5 ("drm/i915: Eliminate Gen9 special > >>>case") used POSTING_READ_FW instead of the POSTING_READ. With the > >>>latter buggy commit fixed this call site needs fixing as well. > >> > >>The theory here is that we don't need the powerwell to force the write > >>from CPU to be visible before another agent. > >> > >>I missed the report, so I am genuinely interested in knowing whether the > >>theory about the write being posted without the powerwll. > > > >Just that the commit message for the patch used "guc registers are > >not in any forcewake domain" reasoning, which was false - based on > >a partially broken patch. See "drm/i915: Fix gen9 forcewake range > >table". > > > >Regards, > > > >Tvrtko > > > Verified this fix without forcewake, so this patch will not be needed. > Have couple of queries. Chris, could you please clarify: > 1. why POSTING_READ is done in flush_gtt_write_domain and not > POSTING_READ_FW like this case? There is a risk that we read from the register at the same time as a write to another register in the same cacheline (gen7 issue). It should be safe, but having erred on the side of skipping the uncore.lock in a imilar posting read for gen6_seqno_barrier() this time I played safe. Along the guc submit path, we know we won't hit the same problematic hw. > 2. how does read from forcewake mmio range work if well is down? Returns garbage, usually zero. -Chris
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index 4462112725ef..c603837324a0 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -651,7 +651,7 @@ static void i915_guc_submit(struct drm_i915_gem_request *rq) /* WA to flush out the pending GMADR writes to ring buffer. */ if (i915_vma_is_map_and_fenceable(rq->ring->vma)) - POSTING_READ_FW(GUC_STATUS); + POSTING_READ(GUC_STATUS); b_ret = guc_ring_doorbell(client);