| Message ID | 1408548564-3787-1-git-send-email-thomas.daniel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 20, 2014 at 04:29:24PM +0100, Thomas Daniel wrote: > These two functions make no sense in an Logical Ring Context & Execlists > world. > > v2: We got rid of lrc_enabled and centralized everything in the sanitized > i915.enable_execlists instead. > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > v3: Rebased. Corrected a typo in comment for i915_switch_context and > added a comment that it should not be called in execlist mode. Added > WARN_ON if i915_switch_context is called in execlist mode. Moved check > for execlist mode out of i915_switch_context and into callers. Added > comment in context_reset explaining why nothing is done in execlist > mode. No, this is not the way. The requirement is to reduce the number of special cases not increase them. These should be evaluated to be no-ops when execlists is used. -Chris
On Wed, Aug 20, 2014 at 04:36:05PM +0100, Chris Wilson wrote: > On Wed, Aug 20, 2014 at 04:29:24PM +0100, Thomas Daniel wrote: > > These two functions make no sense in an Logical Ring Context & Execlists > > world. > > > > v2: We got rid of lrc_enabled and centralized everything in the sanitized > > i915.enable_execlists instead. > > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > > > v3: Rebased. Corrected a typo in comment for i915_switch_context and > > added a comment that it should not be called in execlist mode. Added > > WARN_ON if i915_switch_context is called in execlist mode. Moved check > > for execlist mode out of i915_switch_context and into callers. Added > > comment in context_reset explaining why nothing is done in execlist > > mode. > > No, this is not the way. The requirement is to reduce the number of > special cases not increase them. These should be evaluated to be no-ops > when execlists is used. I think it's ok-ish for now. Maybe we need to reconsider when we wire up lrc reclaim - which is the real user of the switch_context in gpu_idle. The problem I have though is that I can't parse the subject of the patch, someone please translate that to simplified English for me. I can do the replacement while applying. -Daniel
On Mon, 25 Aug 2014, Daniel Vetter wrote: > On Wed, Aug 20, 2014 at 04:36:05PM +0100, Chris Wilson wrote: >> On Wed, Aug 20, 2014 at 04:29:24PM +0100, Thomas Daniel wrote: >>> These two functions make no sense in an Logical Ring Context & Execlists >>> world. >>> >>> v2: We got rid of lrc_enabled and centralized everything in the sanitized >>> i915.enable_execlists instead. >>> >>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> >>> >>> v3: Rebased. Corrected a typo in comment for i915_switch_context and >>> added a comment that it should not be called in execlist mode. Added >>> WARN_ON if i915_switch_context is called in execlist mode. Moved check >>> for execlist mode out of i915_switch_context and into callers. Added >>> comment in context_reset explaining why nothing is done in execlist >>> mode. >> >> No, this is not the way. The requirement is to reduce the number of >> special cases not increase them. These should be evaluated to be no-ops >> when execlists is used. > > I think it's ok-ish for now. Maybe we need to reconsider when we wire up > lrc reclaim - which is the real user of the switch_context in gpu_idle. > The problem I have though is that I can't parse the subject of the patch, > someone please translate that to simplified English for me. I can do the > replacement while applying. > -Daniel "Render moot" usually means something like "make obsolete", not sure about the rest.
On Mon, Aug 25, 2014 at 10:39:39PM +0200, Daniel Vetter wrote: > On Wed, Aug 20, 2014 at 04:36:05PM +0100, Chris Wilson wrote: > > On Wed, Aug 20, 2014 at 04:29:24PM +0100, Thomas Daniel wrote: > > > These two functions make no sense in an Logical Ring Context & Execlists > > > world. > > > > > > v2: We got rid of lrc_enabled and centralized everything in the sanitized > > > i915.enable_execlists instead. > > > > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > > > > > v3: Rebased. Corrected a typo in comment for i915_switch_context and > > > added a comment that it should not be called in execlist mode. Added > > > WARN_ON if i915_switch_context is called in execlist mode. Moved check > > > for execlist mode out of i915_switch_context and into callers. Added > > > comment in context_reset explaining why nothing is done in execlist > > > mode. > > > > No, this is not the way. The requirement is to reduce the number of > > special cases not increase them. These should be evaluated to be no-ops > > when execlists is used. > > I think it's ok-ish for now. Maybe we need to reconsider when we wire up > lrc reclaim - which is the real user of the switch_context in gpu_idle. > The problem I have though is that I can't parse the subject of the patch, > someone please translate that to simplified English for me. I can do the > replacement while applying. No, it is not. execlists is badly designed and this is a further symptom of that. -Chris
On 26/08/2014 06:59, Chris Wilson wrote: > On Mon, Aug 25, 2014 at 10:39:39PM +0200, Daniel Vetter wrote: >> On Wed, Aug 20, 2014 at 04:36:05PM +0100, Chris Wilson wrote: >>> On Wed, Aug 20, 2014 at 04:29:24PM +0100, Thomas Daniel wrote: >>>> These two functions make no sense in an Logical Ring Context & Execlists >>>> world. >>>> >>>> v2: We got rid of lrc_enabled and centralized everything in the sanitized >>>> i915.enable_execlists instead. >>>> >>>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> >>>> >>>> v3: Rebased. Corrected a typo in comment for i915_switch_context and >>>> added a comment that it should not be called in execlist mode. Added >>>> WARN_ON if i915_switch_context is called in execlist mode. Moved check >>>> for execlist mode out of i915_switch_context and into callers. Added >>>> comment in context_reset explaining why nothing is done in execlist >>>> mode. >>> >>> No, this is not the way. The requirement is to reduce the number of >>> special cases not increase them. These should be evaluated to be no-ops >>> when execlists is used. >> >> I think it's ok-ish for now. Maybe we need to reconsider when we wire up >> lrc reclaim - which is the real user of the switch_context in gpu_idle. >> The problem I have though is that I can't parse the subject of the patch, >> someone please translate that to simplified English for me. I can do the >> replacement while applying. > > No, it is not. execlists is badly designed and this is a further symptom > of that. > -Chris > Thomas is not available and I am replying on his behalf. Is the following subject is good for this patch? "Don't execute context reset and switch when using Execlists" regards Arun
On Tue, Aug 26, 2014 at 02:54:39PM +0100, Siluvery, Arun wrote: > On 26/08/2014 06:59, Chris Wilson wrote: > >On Mon, Aug 25, 2014 at 10:39:39PM +0200, Daniel Vetter wrote: > >>On Wed, Aug 20, 2014 at 04:36:05PM +0100, Chris Wilson wrote: > >>>On Wed, Aug 20, 2014 at 04:29:24PM +0100, Thomas Daniel wrote: > >>>>These two functions make no sense in an Logical Ring Context & Execlists > >>>>world. > >>>> > >>>>v2: We got rid of lrc_enabled and centralized everything in the sanitized > >>>>i915.enable_execlists instead. > >>>> > >>>>Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > >>>> > >>>>v3: Rebased. Corrected a typo in comment for i915_switch_context and > >>>>added a comment that it should not be called in execlist mode. Added > >>>>WARN_ON if i915_switch_context is called in execlist mode. Moved check > >>>>for execlist mode out of i915_switch_context and into callers. Added > >>>>comment in context_reset explaining why nothing is done in execlist > >>>>mode. > >>> > >>>No, this is not the way. The requirement is to reduce the number of > >>>special cases not increase them. These should be evaluated to be no-ops > >>>when execlists is used. > >> > >>I think it's ok-ish for now. Maybe we need to reconsider when we wire up > >>lrc reclaim - which is the real user of the switch_context in gpu_idle. > >>The problem I have though is that I can't parse the subject of the patch, > >>someone please translate that to simplified English for me. I can do the > >>replacement while applying. > > > >No, it is not. execlists is badly designed and this is a further symptom > >of that. > >-Chris > > > Thomas is not available and I am replying on his behalf. > Is the following subject is good for this patch? > > "Don't execute context reset and switch when using Execlists" Yeah, I think that's more parseable by mere mortals^W^W non-native speakers. Pulled in. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index cb9310b..954a5f9 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2981,9 +2981,11 @@ int i915_gpu_idle(struct drm_device *dev) /* Flush everything onto the inactive list. */ for_each_ring(ring, dev_priv, i) { - ret = i915_switch_context(ring, ring->default_context); - if (ret) - return ret; + if (!i915.enable_execlists) { + ret = i915_switch_context(ring, ring->default_context); + if (ret) + return ret; + } ret = intel_ring_idle(ring); if (ret) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 0fdb357..3face51 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -289,6 +289,12 @@ void i915_gem_context_reset(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; int i; + /* In execlists mode we will unreference the context when the execlist + * queue is cleared and the requests destroyed. + */ + if (i915.enable_execlists) + return; + for (i = 0; i < I915_NUM_RINGS; i++) { struct intel_engine_cs *ring = &dev_priv->ring[i]; struct intel_context *lctx = ring->last_context; @@ -397,6 +403,9 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv) BUG_ON(!dev_priv->ring[RCS].default_context); + if (i915.enable_execlists) + return 0; + for_each_ring(ring, dev_priv, i) { ret = i915_switch_context(ring, ring->default_context); if (ret) @@ -637,14 +646,19 @@ unpin_out: * * The context life cycle is simple. The context refcount is incremented and * decremented by 1 and create and destroy. If the context is in use by the GPU, - * it will have a refoucnt > 1. This allows us to destroy the context abstract + * it will have a refcount > 1. This allows us to destroy the context abstract * object while letting the normal object tracking destroy the backing BO. + * + * This function should not be used in execlists mode. Instead the context is + * switched by writing to the ELSP and requests keep a reference to their + * context. */ int i915_switch_context(struct intel_engine_cs *ring, struct intel_context *to) { struct drm_i915_private *dev_priv = ring->dev->dev_private; + WARN_ON(i915.enable_execlists); WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex)); if (to->legacy_hw_ctx.rcs_state == NULL) { /* We have the fake context */