| Message ID | 20170322205930.12762-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/03/2017 20:59, Chris Wilson wrote: > Commit e8a9c58fcd9a ("drm/i915: Unify active context tracking between > legacy/execlists/guc") converted the legacy intel_ringbuffer submission > to the same context pinning mechanism as execlists - that is to pin the > context until the subsequent request is retired. Previously it used the > vma retirement of the context object to keep itself pinned until the > next request (after i915_vma_move_to_active()). In the conversion, I > missed that the vma retirement was also responsible for marking the > object as dirty. Mark the context object as dirty when pinning > (equivalent to execlists) which ensures that if the context is swapped > out due to mempressure or suspend/hibernation, when it is loaded back in > it does so with the previous state (and not all zero). > > Fixes: e8a9c58fcd9a ("drm/i915: Unify active context tracking between legacy/execlists/guc") > Reported-by: Dennis Gilmore <dennis@ausil.us> > Reported-by: Mathieu Marquer <mathieu.marquer@gmail.com> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99993 > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100181 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.11-rc1 > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 0ca5ea7a9407..62756eb2bd4a 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1440,6 +1440,8 @@ static int intel_ring_context_pin(struct intel_engine_cs *engine, > ret = context_pin(ctx); > if (ret) > goto error; > + > + ce->state->obj->mm.dirty = true; > } > > /* The kernel context is only used as a placeholder for flushing the > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Could it be related to 99671, it also mentions suspend? Regards, Tvrtko
On Thu, Mar 23, 2017 at 09:41:30AM +0000, Tvrtko Ursulin wrote: > > On 22/03/2017 20:59, Chris Wilson wrote: > >Commit e8a9c58fcd9a ("drm/i915: Unify active context tracking between > >legacy/execlists/guc") converted the legacy intel_ringbuffer submission > >to the same context pinning mechanism as execlists - that is to pin the > >context until the subsequent request is retired. Previously it used the > >vma retirement of the context object to keep itself pinned until the > >next request (after i915_vma_move_to_active()). In the conversion, I > >missed that the vma retirement was also responsible for marking the > >object as dirty. Mark the context object as dirty when pinning > >(equivalent to execlists) which ensures that if the context is swapped > >out due to mempressure or suspend/hibernation, when it is loaded back in > >it does so with the previous state (and not all zero). > > > >Fixes: e8a9c58fcd9a ("drm/i915: Unify active context tracking between legacy/execlists/guc") > >Reported-by: Dennis Gilmore <dennis@ausil.us> > >Reported-by: Mathieu Marquer <mathieu.marquer@gmail.com> > >Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99993 > >Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100181 > >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > >Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > >Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.11-rc1 > >--- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > >diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > >index 0ca5ea7a9407..62756eb2bd4a 100644 > >--- a/drivers/gpu/drm/i915/intel_ringbuffer.c > >+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > >@@ -1440,6 +1440,8 @@ static int intel_ring_context_pin(struct intel_engine_cs *engine, > > ret = context_pin(ctx); > > if (ret) > > goto error; > >+ > >+ ce->state->obj->mm.dirty = true; > > } > > > > /* The kernel context is only used as a placeholder for flushing the > > > > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Could it be related to 99671, it also mentions suspend? No, I do not believe so. The symptoms do not match (but that may just be snb handling the failure differently) but more tellingly it started (or at least based on the bug reports became more prevalent) before the breakage was introduced. -Chris
On Thu, Mar 23, 2017 at 09:03:30AM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Restore marking context objects as dirty on pinning > URL : https://patchwork.freedesktop.org/series/21708/ > State : failure > > == Summary == > > Series 21708v1 drm/i915: Restore marking context objects as dirty on pinning > https://patchwork.freedesktop.org/api/1.0/series/21708/revisions/1/mbox/ > > Test kms_pipe_crc_basic: > Subgroup nonblocking-crc-pipe-b-frame-sequence: > pass -> INCOMPLETE (fi-byt-j1900) > Subgroup suspend-read-crc-pipe-a: > fail -> PASS (fi-skl-6700k) Lalalala, pushed anyway. Thanks for the review. -Chris
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 0ca5ea7a9407..62756eb2bd4a 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1440,6 +1440,8 @@ static int intel_ring_context_pin(struct intel_engine_cs *engine, ret = context_pin(ctx); if (ret) goto error; + + ce->state->obj->mm.dirty = true; } /* The kernel context is only used as a placeholder for flushing the
Commit e8a9c58fcd9a ("drm/i915: Unify active context tracking between legacy/execlists/guc") converted the legacy intel_ringbuffer submission to the same context pinning mechanism as execlists - that is to pin the context until the subsequent request is retired. Previously it used the vma retirement of the context object to keep itself pinned until the next request (after i915_vma_move_to_active()). In the conversion, I missed that the vma retirement was also responsible for marking the object as dirty. Mark the context object as dirty when pinning (equivalent to execlists) which ensures that if the context is swapped out due to mempressure or suspend/hibernation, when it is loaded back in it does so with the previous state (and not all zero). Fixes: e8a9c58fcd9a ("drm/i915: Unify active context tracking between legacy/execlists/guc") Reported-by: Dennis Gilmore <dennis@ausil.us> Reported-by: Mathieu Marquer <mathieu.marquer@gmail.com> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99993 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100181 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: <drm-intel-fixes@lists.freedesktop.org> # v4.11-rc1 --- drivers/gpu/drm/i915/intel_ringbuffer.c | 2 ++ 1 file changed, 2 insertions(+)