* Re: [PATCH] drm/i915/guc: WA to address the Ringbuffer coherency issue
2016-10-14 18:23 [PATCH] drm/i915/guc: WA to address the Ringbuffer coherency issue akash.goel
@ 2016-10-14 18:15 ` Chris Wilson
2016-10-14 18:32 ` Goel, Akash
2016-10-14 19:21 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-10-25 16:46 ` ✗ Fi.CI.BAT: warning for drm/i915/guc: WA to address the Ringbuffer coherency issue (rev2) Patchwork
2 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2016-10-14 18:15 UTC (permalink / raw)
To: akash.goel; +Cc: intel-gfx
On Fri, Oct 14, 2016 at 11:53:44PM +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 commited to memory by the time the Command streamer starts
> reading them, resulting in fetching of stale data.
Please leave a blank line between paragraphs, or try to not leave so
much whitespace at the end of a sentence.
> 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.
>
> 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>
> ---
> drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index a1f76c8..43c8a72 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -601,6 +601,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;
> @@ -608,6 +609,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(GUC_STATUS);
Did you test POSTING_READ_FW() ?
Otherwise it makes an unfortunate amount of sense, and I feel justified
in what I had to do in flush_gtt_write_domwin! :)
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm/i915/guc: WA to address the Ringbuffer coherency issue
@ 2016-10-14 18:23 akash.goel
2016-10-14 18:15 ` Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: akash.goel @ 2016-10-14 18:23 UTC (permalink / raw)
To: intel-gfx; +Cc: Akash Goel
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 commited 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.
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>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index a1f76c8..43c8a72 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -601,6 +601,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;
@@ -608,6 +609,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(GUC_STATUS);
+
b_ret = guc_ring_doorbell(client);
client->submissions[engine_id] += 1;
--
1.9.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/guc: WA to address the Ringbuffer coherency issue
2016-10-14 18:15 ` Chris Wilson
@ 2016-10-14 18:32 ` Goel, Akash
2016-10-25 16:35 ` [PATCH v2] " akash.goel
0 siblings, 1 reply; 10+ messages in thread
From: Goel, Akash @ 2016-10-14 18:32 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: akash.goel
On 10/14/2016 11:45 PM, Chris Wilson wrote:
> On Fri, Oct 14, 2016 at 11:53:44PM +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 commited to memory by the time the Command streamer starts
>> reading them, resulting in fetching of stale data.
>
> Please leave a blank line between paragraphs, or try to not leave so
> much whitespace at the end of a sentence.
>
I am sorry. Will be mindful of this from now.
>> 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.
>>
>> 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>
>> ---
>> drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
>> index a1f76c8..43c8a72 100644
>> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
>> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
>> @@ -601,6 +601,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;
>> @@ -608,6 +609,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(GUC_STATUS);
>
> Did you test POSTING_READ_FW() ?
Sorry though we haven't explicitly tried POSTING_READ_FW() but it should
work since, as per the __gen9_fw_ranges[] table, GuC registers
(C000-Cxxx) do not lie in any Forcewake domain range.
>
> Otherwise it makes an unfortunate amount of sense, and I feel justified
> in what I had to do in flush_gtt_write_domwin! :)
Yes your hunch, expectedly, was spot on :).
Best regards
Akash
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915/guc: WA to address the Ringbuffer coherency issue
2016-10-14 18:23 [PATCH] drm/i915/guc: WA to address the Ringbuffer coherency issue akash.goel
2016-10-14 18:15 ` Chris Wilson
@ 2016-10-14 19:21 ` Patchwork
2016-10-25 16:46 ` ✗ Fi.CI.BAT: warning for drm/i915/guc: WA to address the Ringbuffer coherency issue (rev2) Patchwork
2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2016-10-14 19:21 UTC (permalink / raw)
To: Akash Goel; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/guc: WA to address the Ringbuffer coherency issue
URL : https://patchwork.freedesktop.org/series/13807/
State : warning
== Summary ==
Series 13807v1 drm/i915/guc: WA to address the Ringbuffer coherency issue
https://patchwork.freedesktop.org/api/1.0/series/13807/revisions/1/mbox/
Test drv_module_reload_basic:
pass -> SKIP (fi-bdw-5557u)
Test kms_flip:
Subgroup basic-flip-vs-wf_vblank:
dmesg-warn -> PASS (fi-skl-6700k)
dmesg-warn -> PASS (fi-skl-6770hq)
Test kms_pipe_crc_basic:
Subgroup read-crc-pipe-a-frame-sequence:
dmesg-warn -> PASS (fi-ilk-650)
Subgroup read-crc-pipe-b:
dmesg-warn -> PASS (fi-ilk-650)
Subgroup suspend-read-crc-pipe-c:
incomplete -> PASS (fi-skl-6700k)
Test vgem_basic:
Subgroup unload:
skip -> PASS (fi-skl-6770hq)
pass -> SKIP (fi-hsw-4770)
fi-bdw-5557u total:246 pass:230 dwarn:0 dfail:0 fail:0 skip:16
fi-bsw-n3050 total:246 pass:204 dwarn:0 dfail:0 fail:0 skip:42
fi-bxt-t5700 total:246 pass:216 dwarn:0 dfail:0 fail:0 skip:30
fi-byt-n2820 total:246 pass:210 dwarn:0 dfail:0 fail:1 skip:35
fi-hsw-4770 total:246 pass:223 dwarn:0 dfail:0 fail:0 skip:23
fi-hsw-4770r total:246 pass:224 dwarn:0 dfail:0 fail:0 skip:22
fi-ilk-650 total:246 pass:184 dwarn:0 dfail:0 fail:2 skip:60
fi-ivb-3520m total:246 pass:221 dwarn:0 dfail:0 fail:0 skip:25
fi-ivb-3770 total:246 pass:221 dwarn:0 dfail:0 fail:0 skip:25
fi-kbl-7200u total:246 pass:222 dwarn:0 dfail:0 fail:0 skip:24
fi-skl-6260u total:246 pass:232 dwarn:0 dfail:0 fail:0 skip:14
fi-skl-6700hq total:246 pass:223 dwarn:0 dfail:0 fail:0 skip:23
fi-skl-6700k total:246 pass:221 dwarn:1 dfail:0 fail:0 skip:24
fi-skl-6770hq total:246 pass:230 dwarn:1 dfail:0 fail:1 skip:14
fi-snb-2520m total:246 pass:210 dwarn:0 dfail:0 fail:0 skip:36
fi-snb-2600 total:246 pass:209 dwarn:0 dfail:0 fail:0 skip:37
fi-byt-j1900 failed to connect after reboot
Results at /archive/results/CI_IGT_test/Patchwork_2728/
38ecbe9082e477953fe165265c76e6c6061aeaf6 drm-intel-nightly: 2016y-10m-14d-16h-23m-48s UTC integration manifest
86dd25f drm/i915/guc: WA to address the Ringbuffer coherency issue
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2] drm/i915/guc: WA to address the Ringbuffer coherency issue
2016-10-14 18:32 ` Goel, Akash
@ 2016-10-25 16:35 ` akash.goel
2016-10-25 20:03 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: akash.goel @ 2016-10-25 16:35 UTC (permalink / raw)
To: intel-gfx; +Cc: Akash Goel
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>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++++++
1 file changed, 6 insertions(+)
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;
--
1.9.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915/guc: WA to address the Ringbuffer coherency issue (rev2)
2016-10-14 18:23 [PATCH] drm/i915/guc: WA to address the Ringbuffer coherency issue akash.goel
2016-10-14 18:15 ` Chris Wilson
2016-10-14 19:21 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2016-10-25 16:46 ` Patchwork
2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2016-10-25 16:46 UTC (permalink / raw)
To: Akash Goel; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/guc: WA to address the Ringbuffer coherency issue (rev2)
URL : https://patchwork.freedesktop.org/series/13807/
State : warning
== Summary ==
Series 13807v2 drm/i915/guc: WA to address the Ringbuffer coherency issue
https://patchwork.freedesktop.org/api/1.0/series/13807/revisions/2/mbox/
Test drv_module_reload_basic:
pass -> DMESG-WARN (fi-skl-6700hq)
dmesg-warn -> PASS (fi-skl-6770hq)
Test gem_exec_suspend:
Subgroup basic-s3:
dmesg-warn -> PASS (fi-skl-6700hq)
dmesg-warn -> PASS (fi-skl-6770hq)
Test kms_frontbuffer_tracking:
Subgroup basic:
fail -> PASS (fi-skl-6770hq)
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-a:
dmesg-warn -> PASS (fi-skl-6700hq)
pass -> DMESG-WARN (fi-ilk-650)
Subgroup suspend-read-crc-pipe-b:
dmesg-warn -> PASS (fi-skl-6700hq)
Subgroup suspend-read-crc-pipe-c:
dmesg-warn -> PASS (fi-skl-6700hq)
fi-bdw-5557u total:246 pass:231 dwarn:0 dfail:0 fail:0 skip:15
fi-bsw-n3050 total:246 pass:204 dwarn:0 dfail:0 fail:0 skip:42
fi-bxt-t5700 total:246 pass:216 dwarn:0 dfail:0 fail:0 skip:30
fi-byt-j1900 total:246 pass:215 dwarn:0 dfail:0 fail:0 skip:31
fi-byt-n2820 total:246 pass:211 dwarn:0 dfail:0 fail:0 skip:35
fi-hsw-4770 total:246 pass:224 dwarn:0 dfail:0 fail:0 skip:22
fi-hsw-4770r total:246 pass:223 dwarn:0 dfail:0 fail:0 skip:23
fi-ilk-650 total:246 pass:184 dwarn:1 dfail:0 fail:0 skip:61
fi-ivb-3520m total:246 pass:220 dwarn:0 dfail:0 fail:0 skip:26
fi-ivb-3770 total:246 pass:220 dwarn:0 dfail:0 fail:0 skip:26
fi-kbl-7200u total:246 pass:222 dwarn:0 dfail:0 fail:0 skip:24
fi-skl-6260u total:246 pass:232 dwarn:0 dfail:0 fail:0 skip:14
fi-skl-6700hq total:246 pass:222 dwarn:1 dfail:0 fail:0 skip:23
fi-skl-6700k total:246 pass:222 dwarn:1 dfail:0 fail:0 skip:23
fi-skl-6770hq total:246 pass:232 dwarn:0 dfail:0 fail:0 skip:14
fi-snb-2520m total:246 pass:209 dwarn:0 dfail:0 fail:0 skip:37
fi-snb-2600 total:246 pass:208 dwarn:0 dfail:0 fail:0 skip:38
3ce99d02d8f2b7d1414d20d5972f8e277b33693e drm-intel-nightly: 2016y-10m-25d-13h-55m-46s UTC integration manifest
c503679 drm/i915/guc: WA to address the Ringbuffer coherency issue
Full results at https://intel-gfx-ci.01.org/CI/Patchwork_2814/
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2814/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915/guc: WA to address the Ringbuffer coherency issue
2016-10-25 16:35 ` [PATCH v2] " akash.goel
@ 2016-10-25 20:03 ` Chris Wilson
2016-10-26 7:20 ` Tvrtko Ursulin
0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2016-10-25 20:03 UTC (permalink / raw)
To: akash.goel; +Cc: intel-gfx
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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915/guc: WA to address the Ringbuffer coherency issue
2016-10-25 20:03 ` Chris Wilson
@ 2016-10-26 7:20 ` Tvrtko Ursulin
2016-10-26 8:14 ` Chris Wilson
0 siblings, 1 reply; 10+ messages in thread
From: Tvrtko Ursulin @ 2016-10-26 7:20 UTC (permalink / raw)
To: Chris Wilson, akash.goel, intel-gfx, Sagar Arun Kamble
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915/guc: WA to address the Ringbuffer coherency issue
2016-10-26 7:20 ` Tvrtko Ursulin
@ 2016-10-26 8:14 ` Chris Wilson
2016-10-26 8:24 ` Tvrtko Ursulin
0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2016-10-26 8:14 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: akash.goel, intel-gfx
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
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] drm/i915/guc: WA to address the Ringbuffer coherency issue
2016-10-26 8:14 ` Chris Wilson
@ 2016-10-26 8:24 ` Tvrtko Ursulin
0 siblings, 0 replies; 10+ messages in thread
From: Tvrtko Ursulin @ 2016-10-26 8:24 UTC (permalink / raw)
To: Chris Wilson, akash.goel, intel-gfx, Sagar Arun Kamble
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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-10-26 8:24 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-14 18:23 [PATCH] drm/i915/guc: WA to address the Ringbuffer coherency issue akash.goel
2016-10-14 18:15 ` Chris Wilson
2016-10-14 18:32 ` Goel, Akash
2016-10-25 16:35 ` [PATCH v2] " akash.goel
2016-10-25 20:03 ` Chris Wilson
2016-10-26 7:20 ` Tvrtko Ursulin
2016-10-26 8:14 ` Chris Wilson
2016-10-26 8:24 ` Tvrtko Ursulin
2016-10-14 19:21 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-10-25 16:46 ` ✗ Fi.CI.BAT: warning for drm/i915/guc: WA to address the Ringbuffer coherency issue (rev2) Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.