| Message ID | 1406217891-8912-12-git-send-email-thomas.daniel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 24, 2014 at 05:04:19PM +0100, Thomas Daniel wrote: > From: Oscar Mateo <oscar.mateo@intel.com> > > 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.enbale_execlists instead. > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index fbe7278..288f5de 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -380,6 +380,9 @@ void i915_gem_context_reset(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > int i; > > + if (i915.enable_execlists) > + return; This will conflict badly with Alistair's patch at a functional level. I'm pretty sure that we want _some_ form of reset for the context state, since the hw didn't just magically load the previously running context. So NACK on this hunk. > + > /* Prevent the hardware from restoring the last context (which hung) on > * the next switch */ > for (i = 0; i < I915_NUM_RINGS; i++) { > @@ -514,6 +517,9 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv) > ppgtt->enable(ppgtt); > } > > + if (i915.enable_execlists) > + return 0; Again this conflicts with Alistair's patch. Furthermore it looks redudant since you no-op out i915_switch_context separately. > + > /* FIXME: We should make this work, even in reset */ > if (i915_reset_in_progress(&dev_priv->gpu_error)) > return 0; > @@ -769,6 +775,9 @@ int i915_switch_context(struct intel_engine_cs *ring, > { > struct drm_i915_private *dev_priv = ring->dev->dev_private; > > + if (i915.enable_execlists) > + return 0; I've hoped we don't need this with the legacy ringbuffer cmdsubmission fuly split out. If there are other paths (resume, gpu reset) where this comes into play then I guess we need to look at where the best place is to make this call. So until this comes with a bit a better justification I'll punt on this for now. -Daniel > + > WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex)); > > if (to->legacy_hw_ctx.rcs_state == NULL) { /* We have the fake context */ > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Monday, August 11, 2014 3:30 PM > To: Daniel, Thomas > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 11/43] drm/i915/bdw: Render moot context > reset and switch with Execlists > > On Thu, Jul 24, 2014 at 05:04:19PM +0100, Thomas Daniel wrote: > > From: Oscar Mateo <oscar.mateo@intel.com> > > > > 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.enbale_execlists instead. > > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem_context.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > > b/drivers/gpu/drm/i915/i915_gem_context.c > > index fbe7278..288f5de 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -380,6 +380,9 @@ void i915_gem_context_reset(struct drm_device > *dev) > > struct drm_i915_private *dev_priv = dev->dev_private; > > int i; > > > > + if (i915.enable_execlists) > > + return; > > This will conflict badly with Alistair's patch at a functional level. I'm pretty sure > that we want _some_ form of reset for the context state, since the hw didn't > just magically load the previously running context. So NACK on this hunk. OK I'll wait to see the final version of Alistair's patch and decide what to do about this hunk. > > > + > > /* Prevent the hardware from restoring the last context (which > hung) on > > * the next switch */ > > for (i = 0; i < I915_NUM_RINGS; i++) { @@ -514,6 +517,9 @@ int > > i915_gem_context_enable(struct drm_i915_private *dev_priv) > > ppgtt->enable(ppgtt); > > } > > > > + if (i915.enable_execlists) > > + return 0; > > Again this conflicts with Alistair's patch. Furthermore it looks redudant since > you no-op out i915_switch_context separately. I don't think this is a conflict. Doesn't Alistair's change here just involve writing PDPs for full PPGTT? We don't want to do that in lrc mode. > > > + > > /* FIXME: We should make this work, even in reset */ > > if (i915_reset_in_progress(&dev_priv->gpu_error)) > > return 0; > > @@ -769,6 +775,9 @@ int i915_switch_context(struct intel_engine_cs > > *ring, { > > struct drm_i915_private *dev_priv = ring->dev->dev_private; > > > > + if (i915.enable_execlists) > > + return 0; > > I've hoped we don't need this with the legacy ringbuffer cmdsubmission fuly > split out. If there are other paths (resume, gpu reset) where this comes into > play then I guess we need to look at where the best place is to make this call. > So until this comes with a bit a better justification I'll punt on this for now. > -Daniel Yes, the command submission lrc path doesn't call this but other codepaths do. If we keep the check in context_enable() the only remaining call I see is in i915_gpu_idle(). I don't mind if the check is done there but perhaps a WARN_ON should then be added into switch_context() because we don't want to be putting illegal MI_SET_CONTEXT commands into the ring. Thomas. > > > + > > WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex)); > > > > if (to->legacy_hw_ctx.rcs_state == NULL) { /* We have the fake > > context */ > > -- > > 1.7.9.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Fri, Aug 15, 2014 at 10:22:01AM +0000, Daniel, Thomas wrote: > > > > -----Original Message----- > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel > > Vetter > > Sent: Monday, August 11, 2014 3:30 PM > > To: Daniel, Thomas > > Cc: intel-gfx@lists.freedesktop.org > > Subject: Re: [Intel-gfx] [PATCH 11/43] drm/i915/bdw: Render moot context > > reset and switch with Execlists > > > > On Thu, Jul 24, 2014 at 05:04:19PM +0100, Thomas Daniel wrote: > > > From: Oscar Mateo <oscar.mateo@intel.com> > > > > > > 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.enbale_execlists instead. > > > > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_gem_context.c | 9 +++++++++ > > > 1 file changed, 9 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > > > b/drivers/gpu/drm/i915/i915_gem_context.c > > > index fbe7278..288f5de 100644 > > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > > @@ -380,6 +380,9 @@ void i915_gem_context_reset(struct drm_device > > *dev) > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > int i; > > > > > > + if (i915.enable_execlists) > > > + return; > > > > This will conflict badly with Alistair's patch at a functional level. I'm pretty sure > > that we want _some_ form of reset for the context state, since the hw didn't > > just magically load the previously running context. So NACK on this hunk. > OK I'll wait to see the final version of Alistair's patch and decide what to do > about this hunk. > > > > > > + > > > /* Prevent the hardware from restoring the last context (which > > hung) on > > > * the next switch */ > > > for (i = 0; i < I915_NUM_RINGS; i++) { @@ -514,6 +517,9 @@ int > > > i915_gem_context_enable(struct drm_i915_private *dev_priv) > > > ppgtt->enable(ppgtt); > > > } > > > > > > + if (i915.enable_execlists) > > > + return 0; > > > > Again this conflicts with Alistair's patch. Furthermore it looks redudant since > > you no-op out i915_switch_context separately. > I don't think this is a conflict. Doesn't Alistair's change here just involve > writing PDPs for full PPGTT? We don't want to do that in lrc mode. Oh, just context conflict since Alistair's patch removes the next line ;-) Also I shuffled some of the ppgtt code right above this around. > > > + > > > /* FIXME: We should make this work, even in reset */ > > > if (i915_reset_in_progress(&dev_priv->gpu_error)) > > > return 0; > > > @@ -769,6 +775,9 @@ int i915_switch_context(struct intel_engine_cs > > > *ring, { > > > struct drm_i915_private *dev_priv = ring->dev->dev_private; > > > > > > + if (i915.enable_execlists) > > > + return 0; > > > > I've hoped we don't need this with the legacy ringbuffer cmdsubmission fuly > > split out. If there are other paths (resume, gpu reset) where this comes into > > play then I guess we need to look at where the best place is to make this call. > > So until this comes with a bit a better justification I'll punt on this for now. > > -Daniel > Yes, the command submission lrc path doesn't call this but other codepaths > do. If we keep the check in context_enable() the only remaining call I see is > in i915_gpu_idle(). I don't mind if the check is done there but perhaps a > WARN_ON should then be added into switch_context() because we don't > want to be putting illegal MI_SET_CONTEXT commands into the ring. Yeah, that sounds like a plan. There's still a bit of confusion around the ctx reset in Alistairs patch but unrelated to the code bits we've discussed here. So when you resend this revised patch can you please merge Alistair's patch into your baseline to avoid conflicts. Thanks, Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index fbe7278..288f5de 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -380,6 +380,9 @@ void i915_gem_context_reset(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; int i; + if (i915.enable_execlists) + return; + /* Prevent the hardware from restoring the last context (which hung) on * the next switch */ for (i = 0; i < I915_NUM_RINGS; i++) { @@ -514,6 +517,9 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv) ppgtt->enable(ppgtt); } + if (i915.enable_execlists) + return 0; + /* FIXME: We should make this work, even in reset */ if (i915_reset_in_progress(&dev_priv->gpu_error)) return 0; @@ -769,6 +775,9 @@ int i915_switch_context(struct intel_engine_cs *ring, { struct drm_i915_private *dev_priv = ring->dev->dev_private; + if (i915.enable_execlists) + return 0; + WARN_ON(!mutex_is_locked(&dev_priv->dev->struct_mutex)); if (to->legacy_hw_ctx.rcs_state == NULL) { /* We have the fake context */