Re: [Intel-gfx] [PATCH 11/43] drm/i915/bdw: Render moot context reset and switch with Execlists
-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@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.ma...@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.ma...@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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/43] drm/i915/bdw: Render moot context reset and switch with Execlists
On Fri, Aug 15, 2014 at 10:22:01AM +, Daniel, Thomas wrote:
-Original Message-
From: Daniel Vetter [mailto:daniel.vet...@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.ma...@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.ma...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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.ma...@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.ma...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/43] drm/i915/bdw: Render moot context reset and switch with Execlists
From: Oscar Mateo oscar.ma...@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.ma...@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;
+
/* 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
*/
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
