| Message ID | 1477413323-1880-1-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 25, 2016 at 10:05:23PM +0530, akash.goel@intel.com wrote: > From: Akash Goel <akash.goel@intel.com> > > Driver accesses the ringbuffer pages, via GMADR BAR, if the pages are > pinned in mappable aperture portion of GGTT and for ringbuffer pages > allocated from Stolen memory, access can only be done through GMADR BAR. > In case of GuC based submission, updates done in ringbuffer via GMADR > may not get committed to memory by the time the Command streamer starts > reading them, resulting in fetching of stale data. > > For Host based submission, such problem is not there as the write to Ring > Tail or ELSP register happens from the Host side prior to submission. > Access to any GFX register from CPU side goes to GTTMMADR BAR and Hw already > enforces the ordering between outstanding GMADR writes & new GTTMADR access. > MMIO writes from GuC side do not go to GTTMMADR BAR as GuC communication to > registers within GT is contained within GT, so ordering is not enforced > resulting in a race, which can manifest in form of a hang. > > To ensure the flush of in-flight GMADR writes, a POSTING READ is done to > GuC register prior to doorbell ring. > There is already a similar WA in i915_gem_object_flush_gtt_write_domain(), > which takes care of GMADR writes from User space to GEM buffers, but not the > ringbuffer writes from KMD. > This WA is needed on all recent HW. > > v2: > - Use POSTING_READ_FW instead of POSTING_READ as GuC register do not lie > in any forcewake domain range and so the overhead of spinlock & search > in the forcewake table is avoidable. (Chris) > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Signed-off-by: Akash Goel <akash.goel@intel.com> Thanks for respinning, reviewed and pushed. -Chris
On 25/10/2016 21:03, Chris Wilson wrote: > On Tue, Oct 25, 2016 at 10:05:23PM +0530, akash.goel@intel.com wrote: >> From: Akash Goel <akash.goel@intel.com> >> >> Driver accesses the ringbuffer pages, via GMADR BAR, if the pages are >> pinned in mappable aperture portion of GGTT and for ringbuffer pages >> allocated from Stolen memory, access can only be done through GMADR BAR. >> In case of GuC based submission, updates done in ringbuffer via GMADR >> may not get committed to memory by the time the Command streamer starts >> reading them, resulting in fetching of stale data. >> >> For Host based submission, such problem is not there as the write to Ring >> Tail or ELSP register happens from the Host side prior to submission. >> Access to any GFX register from CPU side goes to GTTMMADR BAR and Hw already >> enforces the ordering between outstanding GMADR writes & new GTTMADR access. >> MMIO writes from GuC side do not go to GTTMMADR BAR as GuC communication to >> registers within GT is contained within GT, so ordering is not enforced >> resulting in a race, which can manifest in form of a hang. >> >> To ensure the flush of in-flight GMADR writes, a POSTING READ is done to >> GuC register prior to doorbell ring. >> There is already a similar WA in i915_gem_object_flush_gtt_write_domain(), >> which takes care of GMADR writes from User space to GEM buffers, but not the >> ringbuffer writes from KMD. >> This WA is needed on all recent HW. >> >> v2: >> - Use POSTING_READ_FW instead of POSTING_READ as GuC register do not lie >> in any forcewake domain range and so the overhead of spinlock & search >> in the forcewake table is avoidable. (Chris) >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Signed-off-by: Akash Goel <akash.goel@intel.com> > > Thanks for respinning, reviewed and pushed. No CI run with GuC enabled, or even any CI run? Regards, Tvrtko
On Wed, Oct 26, 2016 at 08:20:35AM +0100, Tvrtko Ursulin wrote: > > On 25/10/2016 21:03, Chris Wilson wrote: > >On Tue, Oct 25, 2016 at 10:05:23PM +0530, akash.goel@intel.com wrote: > >>From: Akash Goel <akash.goel@intel.com> > >> > >>Driver accesses the ringbuffer pages, via GMADR BAR, if the pages are > >>pinned in mappable aperture portion of GGTT and for ringbuffer pages > >>allocated from Stolen memory, access can only be done through GMADR BAR. > >>In case of GuC based submission, updates done in ringbuffer via GMADR > >>may not get committed to memory by the time the Command streamer starts > >>reading them, resulting in fetching of stale data. > >> > >>For Host based submission, such problem is not there as the write to Ring > >>Tail or ELSP register happens from the Host side prior to submission. > >>Access to any GFX register from CPU side goes to GTTMMADR BAR and Hw already > >>enforces the ordering between outstanding GMADR writes & new GTTMADR access. > >>MMIO writes from GuC side do not go to GTTMMADR BAR as GuC communication to > >>registers within GT is contained within GT, so ordering is not enforced > >>resulting in a race, which can manifest in form of a hang. > >> > >>To ensure the flush of in-flight GMADR writes, a POSTING READ is done to > >>GuC register prior to doorbell ring. > >>There is already a similar WA in i915_gem_object_flush_gtt_write_domain(), > >>which takes care of GMADR writes from User space to GEM buffers, but not the > >>ringbuffer writes from KMD. > >>This WA is needed on all recent HW. > >> > >>v2: > >>- Use POSTING_READ_FW instead of POSTING_READ as GuC register do not lie > >> in any forcewake domain range and so the overhead of spinlock & search > >> in the forcewake table is avoidable. (Chris) > >> > >>Cc: Chris Wilson <chris@chris-wilson.co.uk> > >>Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > >>Signed-off-by: Akash Goel <akash.goel@intel.com> > > > >Thanks for respinning, reviewed and pushed. > > No CI run with GuC enabled, or even any CI run? There were on previous versions. But yes, I am that confident that this is not broken and matches the failure we have seen in BAT for other GTT vs physical access (on different machines on different paths). The only qualm I had was the use of is_mappable() to decide if we are writing through the GTT. ring->vma->obj->stolen might be better, or a ring->ggtt_writes flag. I wanted to fix the bug first, repaint later. -Chris
On 26/10/2016 09:14, Chris Wilson wrote: > On Wed, Oct 26, 2016 at 08:20:35AM +0100, Tvrtko Ursulin wrote: >> >> On 25/10/2016 21:03, Chris Wilson wrote: >>> On Tue, Oct 25, 2016 at 10:05:23PM +0530, akash.goel@intel.com wrote: >>>> From: Akash Goel <akash.goel@intel.com> >>>> >>>> Driver accesses the ringbuffer pages, via GMADR BAR, if the pages are >>>> pinned in mappable aperture portion of GGTT and for ringbuffer pages >>>> allocated from Stolen memory, access can only be done through GMADR BAR. >>>> In case of GuC based submission, updates done in ringbuffer via GMADR >>>> may not get committed to memory by the time the Command streamer starts >>>> reading them, resulting in fetching of stale data. >>>> >>>> For Host based submission, such problem is not there as the write to Ring >>>> Tail or ELSP register happens from the Host side prior to submission. >>>> Access to any GFX register from CPU side goes to GTTMMADR BAR and Hw already >>>> enforces the ordering between outstanding GMADR writes & new GTTMADR access. >>>> MMIO writes from GuC side do not go to GTTMMADR BAR as GuC communication to >>>> registers within GT is contained within GT, so ordering is not enforced >>>> resulting in a race, which can manifest in form of a hang. >>>> >>>> To ensure the flush of in-flight GMADR writes, a POSTING READ is done to >>>> GuC register prior to doorbell ring. >>>> There is already a similar WA in i915_gem_object_flush_gtt_write_domain(), >>>> which takes care of GMADR writes from User space to GEM buffers, but not the >>>> ringbuffer writes from KMD. >>>> This WA is needed on all recent HW. >>>> >>>> v2: >>>> - Use POSTING_READ_FW instead of POSTING_READ as GuC register do not lie >>>> in any forcewake domain range and so the overhead of spinlock & search >>>> in the forcewake table is avoidable. (Chris) >>>> >>>> Cc: Chris Wilson <chris@chris-wilson.co.uk> >>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >>>> Signed-off-by: Akash Goel <akash.goel@intel.com> >>> >>> Thanks for respinning, reviewed and pushed. >> >> No CI run with GuC enabled, or even any CI run? > > There were on previous versions. But yes, I am that confident that No with the added code exercised afaict. > this is not broken and matches the failure we have seen in BAT for other > GTT vs physical access (on different machines on different paths). > > The only qualm I had was the use of is_mappable() to decide if we are > writing through the GTT. ring->vma->obj->stolen might be better, or a > ring->ggtt_writes flag. I wanted to fix the bug first, repaint later. It looks fine to me as well, and I know Akash tested it extensively. However process and all that. Regards, Tvrtko
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index bf65ffa..74235ea 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -634,6 +634,7 @@ static int guc_ring_doorbell(struct i915_guc_client *gc) */ static void i915_guc_submit(struct drm_i915_gem_request *rq) { + struct drm_i915_private *dev_priv = rq->i915; unsigned int engine_id = rq->engine->id; struct intel_guc *guc = &rq->i915->guc; struct i915_guc_client *client = guc->execbuf_client; @@ -641,6 +642,11 @@ static void i915_guc_submit(struct drm_i915_gem_request *rq) spin_lock(&client->wq_lock); guc_wq_item_append(client, 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); + b_ret = guc_ring_doorbell(client); client->submissions[engine_id] += 1;