* [PATCH] drm/i915/guc: Set init value for cached work queue head
@ 2016-02-10 0:05 yu.dai
2016-02-10 8:49 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-02-10 17:30 ` [PATCH] " Tvrtko Ursulin
0 siblings, 2 replies; 5+ messages in thread
From: yu.dai @ 2016-02-10 0:05 UTC (permalink / raw)
To: intel-gfx; +Cc: daniel.vetter
From: Alex Dai <yu.dai@intel.com>
The cached work queue header pointer is set to last byte of work
queue buffer. It will make sure the whole work queue buffer is
available after coming back from reset or init.
Do not hold kmap_atomic mapping before going to sleep when work
queue is full.
Signed-off-by: Alex Dai <yu.dai@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index d7543ef..41f4a96 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -486,11 +486,11 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size)
return 0;
- base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
- desc = base + gc->proc_desc_offset;
-
while (timeout_counter-- > 0) {
+ base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
+ desc = base + gc->proc_desc_offset;
gc->wq_head = desc->head;
+ kunmap_atomic(base);
if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size) {
ret = 0;
@@ -501,8 +501,6 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
usleep_range(1000, 2000);
};
- kunmap_atomic(base);
-
return ret;
}
@@ -730,6 +728,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
client->client_obj = obj;
client->wq_offset = GUC_DB_SIZE;
client->wq_size = GUC_WQ_SIZE;
+ client->wq_head = GUC_WQ_SIZE - 1;
+ client->wq_tail = 0;
client->doorbell_offset = select_doorbell_cacheline(guc);
--
2.5.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread* ✗ Fi.CI.BAT: warning for drm/i915/guc: Set init value for cached work queue head
2016-02-10 0:05 [PATCH] drm/i915/guc: Set init value for cached work queue head yu.dai
@ 2016-02-10 8:49 ` Patchwork
2016-02-10 17:30 ` [PATCH] " Tvrtko Ursulin
1 sibling, 0 replies; 5+ messages in thread
From: Patchwork @ 2016-02-10 8:49 UTC (permalink / raw)
To: yu.dai; +Cc: intel-gfx
== Summary ==
Series 3212v1 drm/i915/guc: Set init value for cached work queue head
http://patchwork.freedesktop.org/api/1.0/series/3212/revisions/1/mbox/
Test core_auth:
Subgroup basic-auth:
pass -> SKIP (bdw-nuci7)
Test drv_hangman:
Subgroup error-state-basic:
skip -> PASS (bdw-nuci7)
Test drv_module_reload_basic:
pass -> DMESG-WARN (ilk-hp8440p)
Test gem_basic:
Subgroup create-close:
skip -> PASS (bdw-nuci7)
Subgroup create-fd-close:
pass -> SKIP (bdw-nuci7)
Test gem_cpu_reloc:
Subgroup basic:
pass -> SKIP (bdw-nuci7)
Test gem_ctx_basic:
pass -> SKIP (bdw-nuci7)
Test gem_ctx_exec:
Subgroup basic:
skip -> PASS (bdw-nuci7)
Test gem_ctx_param_basic:
Subgroup invalid-param-set:
skip -> PASS (bdw-nuci7)
Subgroup non-root-set-no-zeromap:
pass -> SKIP (bdw-nuci7)
Test gem_exec_basic:
Subgroup basic-blt:
pass -> SKIP (bdw-nuci7)
Subgroup basic-bsd1:
skip -> PASS (bdw-nuci7)
Test gem_flink_basic:
Subgroup flink-lifetime:
pass -> SKIP (bdw-nuci7)
Test gem_mmap_gtt:
Subgroup basic-read:
pass -> SKIP (bdw-nuci7)
Subgroup basic-read-write-distinct:
pass -> SKIP (bdw-nuci7)
Subgroup basic-short:
skip -> PASS (bdw-nuci7)
Subgroup basic-small-bo:
skip -> PASS (bdw-nuci7)
Subgroup basic-small-bo-tiledx:
skip -> PASS (bdw-nuci7)
Subgroup basic-small-bo-tiledy:
pass -> DMESG-WARN (ilk-hp8440p)
Subgroup basic-small-copy:
pass -> SKIP (bdw-nuci7)
Subgroup basic-small-copy-xy:
skip -> PASS (bdw-nuci7)
Subgroup basic-write-cpu-read-gtt:
skip -> PASS (bdw-nuci7)
Subgroup basic-write-no-prefault:
pass -> SKIP (bdw-nuci7)
Subgroup basic-write-read:
pass -> SKIP (bdw-nuci7)
Test gem_pwrite:
Subgroup basic:
skip -> PASS (bdw-nuci7)
Test gem_ringfill:
Subgroup basic-default:
pass -> SKIP (bdw-nuci7)
Subgroup basic-default-bomb:
pass -> SKIP (bdw-nuci7)
Test gem_storedw_loop:
Subgroup basic-blt:
pass -> SKIP (bdw-nuci7)
Subgroup basic-bsd1:
pass -> SKIP (bdw-nuci7)
Subgroup basic-bsd2:
pass -> SKIP (bdw-nuci7)
Test gem_sync:
Subgroup basic-blt:
pass -> SKIP (bdw-nuci7)
Subgroup basic-bsd:
skip -> PASS (bdw-nuci7)
Test gem_tiled_blits:
Subgroup basic:
pass -> SKIP (bdw-nuci7)
Test gem_tiled_fence_blits:
Subgroup basic:
pass -> SKIP (bdw-nuci7)
Test gem_tiled_pread_basic:
skip -> PASS (bdw-nuci7)
Test kms_addfb_basic:
Subgroup addfb25-framebuffer-vs-set-tiling:
skip -> PASS (bdw-nuci7)
Subgroup addfb25-modifier-no-flag:
skip -> PASS (bdw-nuci7)
Subgroup addfb25-y-tiled:
pass -> SKIP (bdw-nuci7)
Subgroup bad-pitch-1024:
pass -> SKIP (bdw-nuci7)
Subgroup bad-pitch-128:
pass -> SKIP (bdw-nuci7)
Subgroup bad-pitch-63:
pass -> SKIP (bdw-nuci7)
Subgroup bad-pitch-65536:
pass -> SKIP (bdw-nuci7)
Subgroup basic:
skip -> PASS (bdw-nuci7)
Subgroup basic-x-tiled:
skip -> PASS (bdw-nuci7)
Subgroup bo-too-small-due-to-tiling:
skip -> PASS (bdw-nuci7)
Subgroup no-handle:
pass -> SKIP (bdw-nuci7)
Subgroup size-max:
skip -> PASS (bdw-nuci7)
Subgroup small-bo:
pass -> SKIP (bdw-nuci7)
Subgroup unused-modifier:
pass -> SKIP (bdw-nuci7)
Subgroup unused-pitches:
pass -> SKIP (bdw-nuci7)
Test kms_flip:
Subgroup basic-flip-vs-wf_vblank:
skip -> PASS (bdw-nuci7)
pass -> INCOMPLETE (ilk-hp8440p) UNSTABLE
Test kms_pipe_crc_basic:
Subgroup bad-nb-words-1:
skip -> PASS (bdw-nuci7)
Subgroup bad-source:
pass -> SKIP (bdw-nuci7)
Subgroup hang-read-crc-pipe-c:
skip -> PASS (bdw-nuci7)
Subgroup nonblocking-crc-pipe-a:
pass -> SKIP (bdw-nuci7)
Subgroup nonblocking-crc-pipe-a-frame-sequence:
pass -> SKIP (bdw-nuci7)
Subgroup nonblocking-crc-pipe-b-frame-sequence:
skip -> PASS (bdw-nuci7)
Subgroup nonblocking-crc-pipe-c:
pass -> SKIP (bdw-nuci7)
Subgroup nonblocking-crc-pipe-c-frame-sequence:
skip -> PASS (bdw-nuci7)
Subgroup read-crc-pipe-a-frame-sequence:
pass -> SKIP (bdw-nuci7)
Subgroup read-crc-pipe-b-frame-sequence:
skip -> PASS (bdw-nuci7)
Test pm_rpm:
Subgroup basic-pci-d3-state:
skip -> PASS (bdw-nuci7)
Test prime_self_import:
Subgroup basic-llseek-bad:
skip -> PASS (bdw-nuci7)
Subgroup basic-with_one_bo:
pass -> SKIP (bdw-nuci7)
bdw-nuci7 total:161 pass:33 dwarn:0 dfail:1 fail:0 skip:127
byt-nuc total:164 pass:141 dwarn:0 dfail:0 fail:0 skip:23
hsw-brixbox total:164 pass:151 dwarn:0 dfail:0 fail:0 skip:13
hsw-gt2 total:164 pass:154 dwarn:0 dfail:0 fail:0 skip:10
hsw-xps12 total:161 pass:151 dwarn:0 dfail:0 fail:0 skip:10
ilk-hp8440p total:93 pass:57 dwarn:2 dfail:0 fail:0 skip:33
ivb-t430s total:164 pass:150 dwarn:0 dfail:0 fail:0 skip:14
snb-dellxps total:164 pass:142 dwarn:0 dfail:0 fail:0 skip:22
snb-x220t total:164 pass:142 dwarn:0 dfail:0 fail:1 skip:21
Results at /archive/results/CI_IGT_test/Patchwork_1386/
ae853e8a2d6fc784288c198a3cf8c7aa9a451cf8 drm-intel-nightly: 2016y-02m-10d-07h-54m-38s UTC integration manifest
e71f28f558450601492665953216bf3ba08746d2 drm/i915/guc: Set init value for cached work queue head
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915/guc: Set init value for cached work queue head
2016-02-10 0:05 [PATCH] drm/i915/guc: Set init value for cached work queue head yu.dai
2016-02-10 8:49 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2016-02-10 17:30 ` Tvrtko Ursulin
2016-02-10 20:31 ` Yu Dai
1 sibling, 1 reply; 5+ messages in thread
From: Tvrtko Ursulin @ 2016-02-10 17:30 UTC (permalink / raw)
To: yu.dai, intel-gfx; +Cc: daniel.vetter
Hi,
On 10/02/16 00:05, yu.dai@intel.com wrote:
> From: Alex Dai <yu.dai@intel.com>
>
> The cached work queue header pointer is set to last byte of work
> queue buffer. It will make sure the whole work queue buffer is
> available after coming back from reset or init.
>
> Do not hold kmap_atomic mapping before going to sleep when work
> queue is full.
Could you please split this into two patches? They are two completely
separate issues and it is customary to do so.
For the kmap_atomic issue you can also reference
https://bugs.freedesktop.org/show_bug.cgi?id=93847 in the commit message.
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> ---
> drivers/gpu/drm/i915/i915_guc_submission.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index d7543ef..41f4a96 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -486,11 +486,11 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
> if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size)
> return 0;
>
> - base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> - desc = base + gc->proc_desc_offset;
> -
> while (timeout_counter-- > 0) {
> + base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> + desc = base + gc->proc_desc_offset;
> gc->wq_head = desc->head;
> + kunmap_atomic(base);
>
> if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size) {
> ret = 0;
> @@ -501,8 +501,6 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
> usleep_range(1000, 2000);
> };
>
> - kunmap_atomic(base);
> -
> return ret;
> }
This part is OK to extinguish this fire. But in general you could also
consider caching the kmap in the client since it looks to me that object
is persistently pinned for its lifetime. So kmap_atomic just complicates
things.
> @@ -730,6 +728,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
> client->client_obj = obj;
> client->wq_offset = GUC_DB_SIZE;
> client->wq_size = GUC_WQ_SIZE;
> + client->wq_head = GUC_WQ_SIZE - 1;
> + client->wq_tail = 0;
>
> client->doorbell_offset = select_doorbell_cacheline(guc);
>
>
This one I can't really figure out without I suppose knowing more about
the code design. How come it was OK when it was zero (apart after reset)?
The value is otherwise only updated from the GuC shared page and a
driver does not appear to modify it. Perhaps just a better commit
message to explain things?
Perhaps
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915/guc: Set init value for cached work queue head
2016-02-10 17:30 ` [PATCH] " Tvrtko Ursulin
@ 2016-02-10 20:31 ` Yu Dai
2016-02-15 15:01 ` Dave Gordon
0 siblings, 1 reply; 5+ messages in thread
From: Yu Dai @ 2016-02-10 20:31 UTC (permalink / raw)
To: Tvrtko Ursulin, intel-gfx; +Cc: daniel.vetter
On 02/10/2016 09:30 AM, Tvrtko Ursulin wrote:
> Hi,
>
> On 10/02/16 00:05, yu.dai@intel.com wrote:
> > From: Alex Dai <yu.dai@intel.com>
> >
> > The cached work queue header pointer is set to last byte of work
> > queue buffer. It will make sure the whole work queue buffer is
> > available after coming back from reset or init.
> >
> > Do not hold kmap_atomic mapping before going to sleep when work
> > queue is full.
>
> Could you please split this into two patches? They are two completely
> separate issues and it is customary to do so.
>
> For the kmap_atomic issue you can also reference
> https://bugs.freedesktop.org/show_bug.cgi?id=93847 in the commit message.
Yes, will do.
> > Signed-off-by: Alex Dai <yu.dai@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_guc_submission.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index d7543ef..41f4a96 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -486,11 +486,11 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
> > if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size)
> > return 0;
> >
> > - base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> > - desc = base + gc->proc_desc_offset;
> > -
> > while (timeout_counter-- > 0) {
> > + base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0));
> > + desc = base + gc->proc_desc_offset;
> > gc->wq_head = desc->head;
> > + kunmap_atomic(base);
> >
> > if (CIRC_SPACE(gc->wq_tail, gc->wq_head, gc->wq_size) >= size) {
> > ret = 0;
> > @@ -501,8 +501,6 @@ int i915_guc_wq_check_space(struct i915_guc_client *gc)
> > usleep_range(1000, 2000);
> > };
> >
> > - kunmap_atomic(base);
> > -
> > return ret;
> > }
>
> This part is OK to extinguish this fire. But in general you could also
> consider caching the kmap in the client since it looks to me that object
> is persistently pinned for its lifetime. So kmap_atomic just complicates
> things.
Yes this object must be pinned for its lifetime as it is used by GuC
internally too. I will think about a way to cache it.
> > @@ -730,6 +728,8 @@ static struct i915_guc_client *guc_client_alloc(struct drm_device *dev,
> > client->client_obj = obj;
> > client->wq_offset = GUC_DB_SIZE;
> > client->wq_size = GUC_WQ_SIZE;
> > + client->wq_head = GUC_WQ_SIZE - 1;
> > + client->wq_tail = 0;
> >
> > client->doorbell_offset = select_doorbell_cacheline(guc);
> >
> >
>
> This one I can't really figure out without I suppose knowing more about
> the code design. How come it was OK when it was zero (apart after reset)?
>
> The value is otherwise only updated from the GuC shared page and a
> driver does not appear to modify it. Perhaps just a better commit
> message to explain things?
The way this kernel CIRC_xx works is it leaves one byte free and treat
head == tail case as empty. So, there won't be a problem if this head
happens to be 0. If it comes with some random number between [1,
sizeof(WQ item)], there will be a SW dead looping in driver.
And, I will split this patch into two ones.
Thanks,
Alex
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] drm/i915/guc: Set init value for cached work queue head
2016-02-10 20:31 ` Yu Dai
@ 2016-02-15 15:01 ` Dave Gordon
0 siblings, 0 replies; 5+ messages in thread
From: Dave Gordon @ 2016-02-15 15:01 UTC (permalink / raw)
To: Yu Dai, Tvrtko Ursulin, intel-gfx; +Cc: daniel.vetter
On 10/02/16 20:31, Yu Dai wrote:
>
>
> On 02/10/2016 09:30 AM, Tvrtko Ursulin wrote:
>> Hi,
>>
>> On 10/02/16 00:05, yu.dai@intel.com wrote:
>> > From: Alex Dai <yu.dai@intel.com>
>> >
>> > The cached work queue header pointer is set to last byte of work
>> > queue buffer. It will make sure the whole work queue buffer is
>> > available after coming back from reset or init.
[snip]
>> > @@ -730,6 +728,8 @@ static struct i915_guc_client
>> *guc_client_alloc(struct drm_device *dev,
>> > client->client_obj = obj;
>> > client->wq_offset = GUC_DB_SIZE;
>> > client->wq_size = GUC_WQ_SIZE;
>> > + client->wq_head = GUC_WQ_SIZE - 1;
>> > + client->wq_tail = 0;
>> >
>> > client->doorbell_offset = select_doorbell_cacheline(guc);
>>
>> This one I can't really figure out without I suppose knowing more about
>> the code design. How come it was OK when it was zero (apart after reset)?
>>
>> The value is otherwise only updated from the GuC shared page and a
>> driver does not appear to modify it. Perhaps just a better commit
>> message to explain things?
>
> The way this kernel CIRC_xx works is it leaves one byte free and treat
> head == tail case as empty. So, there won't be a problem if this head
> happens to be 0. If it comes with some random number between [1,
> sizeof(WQ item)], there will be a SW dead looping in driver.
Actually, I think it works like this:
if both wq_tail and wq_head (which is a CACHE of the GuC's WQ tail) are
initialised to 0, the driver thinks the WQ is empty, which is correct at
startup but not necessarily after a GPU reset.
By initialising them as above, the driver will at first think the WQ is
FULL, and will then refresh the actual value from the GuC's process
descriptor. This should ensure that it doesn't assume more WQ space than
the GuC thinks there is.
It's very much the same sort of issue as (re)initialising the CSB
pointers after a reset, because the CS h/w (or in this case the GuC
firmware) has internally set them to a new value and the driver needs to
get in sync.
Anyway all of this is superseded by Alex's later patch that leaves the
client permanently kmapped, 'cos that gets rid of wq_head entirely :)
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-02-15 15:01 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-02-10 0:05 [PATCH] drm/i915/guc: Set init value for cached work queue head yu.dai
2016-02-10 8:49 ` ✗ Fi.CI.BAT: warning for " Patchwork
2016-02-10 17:30 ` [PATCH] " Tvrtko Ursulin
2016-02-10 20:31 ` Yu Dai
2016-02-15 15:01 ` Dave Gordon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).