| Message ID | 1400680976-19107-1-git-send-email-mika.kuoppala@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 21, 2014 at 05:02:56PM +0300, Mika Kuoppala wrote: > for proper refcounting to take place as we use > i915_add_request() for it. > > i915_add_request() also takes the context for the request > from ring->last_context so move the null state batch > submission after the ring context has been set. > > v2: we need to check for correct ring now (Ville Syrjälä) > v3: no need to expose i915_gem_move_object_to_active (Chris Wilson) > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Damien Lespiau <damien.lespiau@intel.com> > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> Merged with Ville's irc r-b, thanks for the quick fix. -Daniel > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_gem.c | 2 +- > drivers/gpu/drm/i915/i915_gem_context.c | 16 ++++++++-------- > drivers/gpu/drm/i915/i915_gem_render_state.c | 17 +++++++++++++++-- > 4 files changed, 25 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index b90ec69..0bf7bfb 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2206,6 +2206,7 @@ int i915_gem_object_sync(struct drm_i915_gem_object *obj, > struct intel_ring_buffer *to); > void i915_vma_move_to_active(struct i915_vma *vma, > struct intel_ring_buffer *ring); > +void i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj); > int i915_gem_dumb_create(struct drm_file *file_priv, > struct drm_device *dev, > struct drm_mode_create_dumb *args); > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 440979f..d9bf694 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2084,7 +2084,7 @@ void i915_vma_move_to_active(struct i915_vma *vma, > return i915_gem_object_move_to_active(vma->obj, ring); > } > > -static void > +void > i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj) > { > struct drm_i915_private *dev_priv = obj->base.dev->dev_private; > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index f220c94..6a2d847a 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -700,21 +700,21 @@ static int do_switch(struct intel_ring_buffer *ring, > /* obj is kept alive until the next request by its active ref */ > i915_gem_object_ggtt_unpin(from->obj); > i915_gem_context_unreference(from); > - } else { > - if (to->is_initialized == false) { > - ret = i915_gem_render_state_init(ring); > - if (ret) > - DRM_ERROR("init render state: %d\n", ret); > - } > } > > - to->is_initialized = true; > - > done: > i915_gem_context_reference(to); > ring->last_context = to; > to->last_ring = ring; > > + if (ring->id == RCS && !to->is_initialized && from == NULL) { > + ret = i915_gem_render_state_init(ring); > + if (ret) > + DRM_ERROR("init render state: %d\n", ret); > + } > + > + to->is_initialized = true; > + > return 0; > > unpin_out: > diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c > index 392aa7b..82abe1e 100644 > --- a/drivers/gpu/drm/i915/i915_gem_render_state.c > +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c > @@ -164,9 +164,12 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring) > const int gen = INTEL_INFO(ring->dev)->gen; > struct i915_render_state *so; > const struct intel_renderstate_rodata *rodata; > - u32 seqno; > + struct i915_vma *vma; > int ret; > > + if (WARN_ON(ring->id != RCS)) > + return -ENOENT; > + > rodata = render_state_get_rodata(ring->dev, gen); > if (rodata == NULL) > return 0; > @@ -186,7 +189,17 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring) > if (ret) > goto out; > > - ret = i915_add_request(ring, &seqno); > + vma = i915_gem_obj_to_ggtt(so->obj); > + if (vma == NULL) { > + ret = -ENOSPC; > + goto out; > + } > + > + i915_vma_move_to_active(vma, ring); > + > + ret = __i915_add_request(ring, NULL, so->obj, NULL); > + if (ret) > + i915_gem_object_move_to_inactive(so->obj); > > out: > render_state_free(so); > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, May 21, 2014 at 04:54:55PM +0200, Daniel Vetter wrote: > On Wed, May 21, 2014 at 05:02:56PM +0300, Mika Kuoppala wrote: > > for proper refcounting to take place as we use > > i915_add_request() for it. > > > > i915_add_request() also takes the context for the request > > from ring->last_context so move the null state batch > > submission after the ring context has been set. > > > > v2: we need to check for correct ring now (Ville Syrjälä) > > v3: no need to expose i915_gem_move_object_to_active (Chris Wilson) > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > Merged with Ville's irc r-b, thanks for the quick fix. Pity, it still contains some code that I'd rather not start cargo-culting. > > - ret = i915_add_request(ring, &seqno); > > + vma = i915_gem_obj_to_ggtt(so->obj); > > + if (vma == NULL) { > > + ret = -ENOSPC; > > + goto out; > > + } We use the GGTT vma much earlier, so this is suspect. > > + > > + i915_vma_move_to_active(vma, ring); > > + > > + ret = __i915_add_request(ring, NULL, so->obj, NULL); > > + if (ret) > > + i915_gem_object_move_to_inactive(so->obj); As is move-to-inactive here. The only way add-request can fail is via an EIO, and that will have triggered the move-to-inactive already - which should nicely catch a BUG or two. -Chris
On Wed, May 21, 2014 at 04:00:40PM +0100, Chris Wilson wrote: > On Wed, May 21, 2014 at 04:54:55PM +0200, Daniel Vetter wrote: > > On Wed, May 21, 2014 at 05:02:56PM +0300, Mika Kuoppala wrote: > > > for proper refcounting to take place as we use > > > i915_add_request() for it. > > > > > > i915_add_request() also takes the context for the request > > > from ring->last_context so move the null state batch > > > submission after the ring context has been set. > > > > > > v2: we need to check for correct ring now (Ville Syrjälä) > > > v3: no need to expose i915_gem_move_object_to_active (Chris Wilson) > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> > > > > Merged with Ville's irc r-b, thanks for the quick fix. > > Pity, it still contains some code that I'd rather not start > cargo-culting. Using vmas more as first-class objects I support. Mika can you please resend? I'll keep v3 for now to avoid blocking people. -Daniel > > > - ret = i915_add_request(ring, &seqno); > > > + vma = i915_gem_obj_to_ggtt(so->obj); > > > + if (vma == NULL) { > > > + ret = -ENOSPC; > > > + goto out; > > > + } > > We use the GGTT vma much earlier, so this is suspect. > > > > + > > > + i915_vma_move_to_active(vma, ring); > > > + > > > + ret = __i915_add_request(ring, NULL, so->obj, NULL); > > > + if (ret) > > > + i915_gem_object_move_to_inactive(so->obj); > > As is move-to-inactive here. The only way add-request can fail is via an > EIO, and that will have triggered the move-to-inactive already - which > should nicely catch a BUG or two. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Wed, May 21, 2014 at 07:02:56AM -0700, Mika Kuoppala wrote: > + if (ring->id == RCS && !to->is_initialized && from == NULL) { > + ret = i915_gem_render_state_init(ring); > + if (ret) > + DRM_ERROR("init render state: %d\n", ret); > + } Apologies if this has already been discussed, but why do we have the 'from == NULL' check? Shouldn't we initialize all uninitialized RCS contexts? Otherwise I thought we'll inherit whatever state 'from' left behind. The hw state should be valid in either case (and so I expect would fix the rc6 issue either way), it's just the difference between initializing every context to a specific valid state or initializing every context to _some_ valid state. The commit message on the first render state patch seemed to indicate the former while the implementation looks like the latter. Just want to understand which we intended. Thanks, Brad
"Volkin, Bradley D" <bradley.d.volkin@intel.com> writes: > On Wed, May 21, 2014 at 07:02:56AM -0700, Mika Kuoppala wrote: >> + if (ring->id == RCS && !to->is_initialized && from == NULL) { >> + ret = i915_gem_render_state_init(ring); >> + if (ret) >> + DRM_ERROR("init render state: %d\n", ret); >> + } > > Apologies if this has already been discussed, but why do we have the > 'from == NULL' check? Shouldn't we initialize all uninitialized RCS > contexts? Otherwise I thought we'll inherit whatever state 'from' left > behind. > > The hw state should be valid in either case (and so I expect would fix > the rc6 issue either way), it's just the difference between initializing > every context to a specific valid state or initializing every context to > _some_ valid state. The commit message on the first render state patch > seemed to indicate the former while the implementation looks like the > latter. Just want to understand which we intended. It seems that the intentions changed and I forgot to update the commit message. For now this is just to push some state and hoping that we can get in/out from rc6 without symptomps. The idea that we would restore/initialize to a specific (golden) state for each new context makes me think that we would get rid of some transient bugs we are seeing. As how I understand things are now, app might inherit some parts from previous ctx and then have lacking initialization by itself and then see a hang..sometimes. I guess the main opponent here is the performance implications. And I lack the experience from user/application side to estimate the impact. I hope that other devs with more experience on this topic will join the discussion. Thanks, -Mika
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b90ec69..0bf7bfb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2206,6 +2206,7 @@ int i915_gem_object_sync(struct drm_i915_gem_object *obj, struct intel_ring_buffer *to); void i915_vma_move_to_active(struct i915_vma *vma, struct intel_ring_buffer *ring); +void i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj); int i915_gem_dumb_create(struct drm_file *file_priv, struct drm_device *dev, struct drm_mode_create_dumb *args); diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 440979f..d9bf694 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2084,7 +2084,7 @@ void i915_vma_move_to_active(struct i915_vma *vma, return i915_gem_object_move_to_active(vma->obj, ring); } -static void +void i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj) { struct drm_i915_private *dev_priv = obj->base.dev->dev_private; diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index f220c94..6a2d847a 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -700,21 +700,21 @@ static int do_switch(struct intel_ring_buffer *ring, /* obj is kept alive until the next request by its active ref */ i915_gem_object_ggtt_unpin(from->obj); i915_gem_context_unreference(from); - } else { - if (to->is_initialized == false) { - ret = i915_gem_render_state_init(ring); - if (ret) - DRM_ERROR("init render state: %d\n", ret); - } } - to->is_initialized = true; - done: i915_gem_context_reference(to); ring->last_context = to; to->last_ring = ring; + if (ring->id == RCS && !to->is_initialized && from == NULL) { + ret = i915_gem_render_state_init(ring); + if (ret) + DRM_ERROR("init render state: %d\n", ret); + } + + to->is_initialized = true; + return 0; unpin_out: diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c index 392aa7b..82abe1e 100644 --- a/drivers/gpu/drm/i915/i915_gem_render_state.c +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c @@ -164,9 +164,12 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring) const int gen = INTEL_INFO(ring->dev)->gen; struct i915_render_state *so; const struct intel_renderstate_rodata *rodata; - u32 seqno; + struct i915_vma *vma; int ret; + if (WARN_ON(ring->id != RCS)) + return -ENOENT; + rodata = render_state_get_rodata(ring->dev, gen); if (rodata == NULL) return 0; @@ -186,7 +189,17 @@ int i915_gem_render_state_init(struct intel_ring_buffer *ring) if (ret) goto out; - ret = i915_add_request(ring, &seqno); + vma = i915_gem_obj_to_ggtt(so->obj); + if (vma == NULL) { + ret = -ENOSPC; + goto out; + } + + i915_vma_move_to_active(vma, ring); + + ret = __i915_add_request(ring, NULL, so->obj, NULL); + if (ret) + i915_gem_object_move_to_inactive(so->obj); out: render_state_free(so);
for proper refcounting to take place as we use i915_add_request() for it. i915_add_request() also takes the context for the request from ring->last_context so move the null state batch submission after the ring context has been set. v2: we need to check for correct ring now (Ville Syrjälä) v3: no need to expose i915_gem_move_object_to_active (Chris Wilson) Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Damien Lespiau <damien.lespiau@intel.com> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/i915_gem_context.c | 16 ++++++++-------- drivers/gpu/drm/i915/i915_gem_render_state.c | 17 +++++++++++++++-- 4 files changed, 25 insertions(+), 11 deletions(-)