| Message ID | 1424268118-20909-1-git-send-email-nicholas.hoath@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 5790
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV -7 277/277 270/277
ILK 313/313 313/313
SNB 309/309 309/309
IVB 382/382 382/382
BYT 296/296 296/296
HSW 425/425 425/425
BDW -1 318/318 317/318
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*PNV igt_gem_fence_thrash_bo-write-verify-none NRUN(1)PASS(4) FAIL(1)PASS(1)
*PNV igt_gem_fence_thrash_bo-write-verify-x PASS(5) FAIL(1)PASS(1)
*PNV igt_gem_fence_thrash_bo-write-verify-y NO_RESULT(1)PASS(4) FAIL(1)PASS(1)
PNV igt_gem_userptr_blits_coherency-sync NO_RESULT(1)CRASH(2)PASS(3) CRASH(2)
*PNV igt_gem_userptr_blits_coherency-unsync CRASH(2)PASS(3) NRUN(1)PASS(1)
*PNV igt_gem_userptr_blits_create-destroy-sync PASS(2) NRUN(1)PASS(1)
*PNV igt_gem_userptr_blits_dmabuf-unsync PASS(2) NRUN(1)PASS(1)
*BDW igt_gem_gtt_hog PASS(9) DMESG_WARN(1)PASS(1)
Note: You need to pay more attention to line start with '*'
On 18/02/15 14:01, Nick Hoath wrote: > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88652 > > When converting from implicitly tracked execlist queue items to ref counted > requests, not all frees of requests were replaced with unrefs, and extraneous > refs/unrefs of contexts were added. > Correct the unbalanced refcount & replace the frees. > Remove a noisy warning when hitting the request creation path. > > drm_i915_gem_request and intel_context are both kref reference counted > structures. Upon allocation, drm_i915_gem_request's ref count should be > bumped using kref_init. When a context is assigned to the request, > the context's reference count should be bumped using i915_gem_context_reference. > i915_gem_request_reference will reduce the context reference count when s/i915_gem_request_reference/i915_gem_request_free/ ? > the request is freed. > > Problem introduced in: > commit 6d3d8274bc45de4babb62d64562d92af984dd238 > Author: Nick Hoath <nicholas.hoath@intel.com> > AuthorDate: Thu Jan 15 13:10:39 2015 +0000 > > drm/i915: Subsume intel_ctx_submit_request in to drm_i915_gem_request > > v2: Added comments explaining how the ctx pointer and the request object should > be ref-counted. Removed noisy warning. > > Signed-off-by: Nick Hoath <nicholas.hoath@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 11 ++++++++++- > drivers/gpu/drm/i915/i915_gem.c | 3 +-- > drivers/gpu/drm/i915/intel_lrc.c | 8 ++++---- > 4 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 2dedd43..956fe26 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2121,6 +2121,9 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old, > * number comparisons on buffer last_read|write_seqno. It also allows an > * emission time to be associated with the request for tracking how far ahead > * of the GPU the submission is. > + * > + * The requests are reference counted, so upon creation they should have an > + * initial reference taken using kref_init > */ > struct drm_i915_gem_request { > struct kref ref; > @@ -2144,7 +2147,13 @@ struct drm_i915_gem_request { > /** Position in the ringbuffer of the end of the whole request */ > u32 tail; > > - /** Context related to this request */ > + /** > + * Context related to this request > + * intel_context is reference counted, so on attachment to this > + * request, the context should be i915_gem_context_reference'd. > + * When this request has its reference count cleared, the cleanup > + * code in i915_gem_request_free will deference the context. s/deference/unreference/ Or maybe replace the comment with this version, if you think it's nicer: Context related to this request Contexts are refcounted, so when this request is associated with a context, we must increment the context's refcount, to guarantee that it persists while any request is linked to it. Requests themselves are also refcounted, so the request will only be freed when the last reference to it is dismissed, and the code in i915_gem_request_free() will then decrement the refcount on the context. .Dave. > + */ > struct intel_context *ctx; > > /** Batch buffer related to this request if any */ > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 61134ab..996f60f 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2664,8 +2664,7 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, > if (submit_req->ctx != ring->default_context) > intel_lr_context_unpin(ring, submit_req->ctx); > > - i915_gem_context_unreference(submit_req->ctx); > - kfree(submit_req); > + i915_gem_request_unreference(submit_req); > } > > /* > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index aafcef3..62a2b2a 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -512,18 +512,19 @@ static int execlists_context_queue(struct intel_engine_cs *ring, > * If there isn't a request associated with this submission, > * create one as a temporary holder. > */ > - WARN(1, "execlist context submission without request"); > request = kzalloc(sizeof(*request), GFP_KERNEL); > if (request == NULL) > return -ENOMEM; > request->ring = ring; > request->ctx = to; > + kref_init(&request->ref); > + request->uniq = dev_priv->request_uniq++; > + i915_gem_context_reference(request->ctx); > } else { > + i915_gem_request_reference(request); > WARN_ON(to != request->ctx); > } > request->tail = tail; > - i915_gem_request_reference(request); > - i915_gem_context_reference(request->ctx); > > intel_runtime_pm_get(dev_priv); > > @@ -740,7 +741,6 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring) > if (ctx_obj && (ctx != ring->default_context)) > intel_lr_context_unpin(ring, ctx); > intel_runtime_pm_put(dev_priv); > - i915_gem_context_unreference(ctx); > list_del(&req->execlist_link); > i915_gem_request_unreference(req); > } >
On 19/02/2015 11:23, Gordon, David S wrote: > On 18/02/15 14:01, Nick Hoath wrote: >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88652 >> >> When converting from implicitly tracked execlist queue items to ref counted >> requests, not all frees of requests were replaced with unrefs, and extraneous >> refs/unrefs of contexts were added. >> Correct the unbalanced refcount & replace the frees. >> Remove a noisy warning when hitting the request creation path. >> >> drm_i915_gem_request and intel_context are both kref reference counted >> structures. Upon allocation, drm_i915_gem_request's ref count should be >> bumped using kref_init. When a context is assigned to the request, >> the context's reference count should be bumped using i915_gem_context_reference. >> i915_gem_request_reference will reduce the context reference count when > > s/i915_gem_request_reference/i915_gem_request_free/ ? I meant i915_gem_request_unreference, but should probably mention that i915_gem_request_free is called when the request reference count reaches zero, and it is that function which drops the context reference count. > >> the request is freed. >> >> Problem introduced in: >> commit 6d3d8274bc45de4babb62d64562d92af984dd238 >> Author: Nick Hoath <nicholas.hoath@intel.com> >> AuthorDate: Thu Jan 15 13:10:39 2015 +0000 >> >> drm/i915: Subsume intel_ctx_submit_request in to drm_i915_gem_request >> >> v2: Added comments explaining how the ctx pointer and the request object should >> be ref-counted. Removed noisy warning. >> >> Signed-off-by: Nick Hoath <nicholas.hoath@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 11 ++++++++++- >> drivers/gpu/drm/i915/i915_gem.c | 3 +-- >> drivers/gpu/drm/i915/intel_lrc.c | 8 ++++---- >> 4 files changed, 16 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 2dedd43..956fe26 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -2121,6 +2121,9 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old, >> * number comparisons on buffer last_read|write_seqno. It also allows an >> * emission time to be associated with the request for tracking how far ahead >> * of the GPU the submission is. >> + * >> + * The requests are reference counted, so upon creation they should have an >> + * initial reference taken using kref_init >> */ >> struct drm_i915_gem_request { >> struct kref ref; >> @@ -2144,7 +2147,13 @@ struct drm_i915_gem_request { >> /** Position in the ringbuffer of the end of the whole request */ >> u32 tail; >> >> - /** Context related to this request */ >> + /** >> + * Context related to this request >> + * intel_context is reference counted, so on attachment to this >> + * request, the context should be i915_gem_context_reference'd. >> + * When this request has its reference count cleared, the cleanup >> + * code in i915_gem_request_free will deference the context. > > s/deference/unreference/ > > Or maybe replace the comment with this version, if you think it's nicer: > > Context related to this request > Contexts are refcounted, so when this request is associated with a > context, we must increment the context's refcount, to guarantee that it > persists while any request is linked to it. Requests themselves are also > refcounted, so the request will only be freed when the last reference to > it is dismissed, and the code in i915_gem_request_free() will then > decrement the refcount on the context. That's a lot more understandable than my version :) > > .Dave. > >> + */ >> struct intel_context *ctx; >> >> /** Batch buffer related to this request if any */ >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index 61134ab..996f60f 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -2664,8 +2664,7 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, >> if (submit_req->ctx != ring->default_context) >> intel_lr_context_unpin(ring, submit_req->ctx); >> >> - i915_gem_context_unreference(submit_req->ctx); >> - kfree(submit_req); >> + i915_gem_request_unreference(submit_req); >> } >> >> /* >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c >> index aafcef3..62a2b2a 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -512,18 +512,19 @@ static int execlists_context_queue(struct intel_engine_cs *ring, >> * If there isn't a request associated with this submission, >> * create one as a temporary holder. >> */ >> - WARN(1, "execlist context submission without request"); >> request = kzalloc(sizeof(*request), GFP_KERNEL); >> if (request == NULL) >> return -ENOMEM; >> request->ring = ring; >> request->ctx = to; >> + kref_init(&request->ref); >> + request->uniq = dev_priv->request_uniq++; >> + i915_gem_context_reference(request->ctx); >> } else { >> + i915_gem_request_reference(request); >> WARN_ON(to != request->ctx); >> } >> request->tail = tail; >> - i915_gem_request_reference(request); >> - i915_gem_context_reference(request->ctx); >> >> intel_runtime_pm_get(dev_priv); >> >> @@ -740,7 +741,6 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring) >> if (ctx_obj && (ctx != ring->default_context)) >> intel_lr_context_unpin(ring, ctx); >> intel_runtime_pm_put(dev_priv); >> - i915_gem_context_unreference(ctx); >> list_del(&req->execlist_link); >> i915_gem_request_unreference(req); >> } >> >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 2dedd43..956fe26 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2121,6 +2121,9 @@ void i915_gem_track_fb(struct drm_i915_gem_object *old, * number comparisons on buffer last_read|write_seqno. It also allows an * emission time to be associated with the request for tracking how far ahead * of the GPU the submission is. + * + * The requests are reference counted, so upon creation they should have an + * initial reference taken using kref_init */ struct drm_i915_gem_request { struct kref ref; @@ -2144,7 +2147,13 @@ struct drm_i915_gem_request { /** Position in the ringbuffer of the end of the whole request */ u32 tail; - /** Context related to this request */ + /** + * Context related to this request + * intel_context is reference counted, so on attachment to this + * request, the context should be i915_gem_context_reference'd. + * When this request has its reference count cleared, the cleanup + * code in i915_gem_request_free will deference the context. + */ struct intel_context *ctx; /** Batch buffer related to this request if any */ diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 61134ab..996f60f 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2664,8 +2664,7 @@ static void i915_gem_reset_ring_cleanup(struct drm_i915_private *dev_priv, if (submit_req->ctx != ring->default_context) intel_lr_context_unpin(ring, submit_req->ctx); - i915_gem_context_unreference(submit_req->ctx); - kfree(submit_req); + i915_gem_request_unreference(submit_req); } /* diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index aafcef3..62a2b2a 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -512,18 +512,19 @@ static int execlists_context_queue(struct intel_engine_cs *ring, * If there isn't a request associated with this submission, * create one as a temporary holder. */ - WARN(1, "execlist context submission without request"); request = kzalloc(sizeof(*request), GFP_KERNEL); if (request == NULL) return -ENOMEM; request->ring = ring; request->ctx = to; + kref_init(&request->ref); + request->uniq = dev_priv->request_uniq++; + i915_gem_context_reference(request->ctx); } else { + i915_gem_request_reference(request); WARN_ON(to != request->ctx); } request->tail = tail; - i915_gem_request_reference(request); - i915_gem_context_reference(request->ctx); intel_runtime_pm_get(dev_priv); @@ -740,7 +741,6 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring) if (ctx_obj && (ctx != ring->default_context)) intel_lr_context_unpin(ring, ctx); intel_runtime_pm_put(dev_priv); - i915_gem_context_unreference(ctx); list_del(&req->execlist_link); i915_gem_request_unreference(req); }
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88652 When converting from implicitly tracked execlist queue items to ref counted requests, not all frees of requests were replaced with unrefs, and extraneous refs/unrefs of contexts were added. Correct the unbalanced refcount & replace the frees. Remove a noisy warning when hitting the request creation path. drm_i915_gem_request and intel_context are both kref reference counted structures. Upon allocation, drm_i915_gem_request's ref count should be bumped using kref_init. When a context is assigned to the request, the context's reference count should be bumped using i915_gem_context_reference. i915_gem_request_reference will reduce the context reference count when the request is freed. Problem introduced in: commit 6d3d8274bc45de4babb62d64562d92af984dd238 Author: Nick Hoath <nicholas.hoath@intel.com> AuthorDate: Thu Jan 15 13:10:39 2015 +0000 drm/i915: Subsume intel_ctx_submit_request in to drm_i915_gem_request v2: Added comments explaining how the ctx pointer and the request object should be ref-counted. Removed noisy warning. Signed-off-by: Nick Hoath <nicholas.hoath@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 11 ++++++++++- drivers/gpu/drm/i915/i915_gem.c | 3 +-- drivers/gpu/drm/i915/intel_lrc.c | 8 ++++---- 4 files changed, 16 insertions(+), 8 deletions(-)