Re: [Intel-gfx] [PATCH] drm/i915: Only mark the ctx as initialised after a SET_CONTEXT operation
On Mon, 23 Jun 2014, Ben Widawsky b...@bwidawsk.net wrote: On Mon, Jun 23, 2014 at 12:55:47PM +0300, Jani Nikula wrote: On Fri, 30 May 2014, Chris Wilson ch...@chris-wilson.co.uk wrote: Fallout from commit 46470fc932ac8a0e8317a220b3f4ea4ed903338e Author: Mika Kuoppala mika.kuopp...@linux.intel.com Date: Wed May 21 19:01:06 2014 +0300 drm/i915: Add null state batch to active list undid the earlier fix of only marking the ctx as initialised after it is saved by the hardware during a SET_CONTEXT operation. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com Cc: Damien Lespiau damien.lesp...@intel.com Cc: Mika Kuoppala mika.kuopp...@intel.com Cc: Ben Widawsky b...@bwidawsk.net Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79108 Daniel put his IRC r-b on this on IIRC. Pushed to -fixes, thanks for the patch and review. BR, Jani. [snip] -- Ben Widawsky, Intel Open Source Technology Center -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only mark the ctx as initialised after a SET_CONTEXT operation
On Fri, 30 May 2014, Chris Wilson ch...@chris-wilson.co.uk wrote:
Fallout from
commit 46470fc932ac8a0e8317a220b3f4ea4ed903338e
Author: Mika Kuoppala mika.kuopp...@linux.intel.com
Date: Wed May 21 19:01:06 2014 +0300
drm/i915: Add null state batch to active list
undid the earlier fix of only marking the ctx as initialised after it is
saved by the hardware during a SET_CONTEXT operation.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Damien Lespiau damien.lesp...@intel.com
Cc: Mika Kuoppala mika.kuopp...@intel.com
Cc: Ben Widawsky b...@bwidawsk.net
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79108
---
drivers/gpu/drm/i915/i915_gem_context.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 5a71ef1975b3..34a0b49e6add 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -641,6 +641,7 @@ static int do_switch(struct intel_engine_cs *ring,
struct intel_context *from = ring-last_context;
struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
u32 hw_flags = 0;
+ bool uninitialized = false;
int ret, i;
if (from != NULL ring == dev_priv-ring[RCS]) {
@@ -739,18 +740,19 @@ static int do_switch(struct intel_engine_cs *ring,
i915_gem_context_unreference(from);
}
+ uninitialized = !to-is_initialized from == NULL;
+ to-is_initialized = true;
+
done:
i915_gem_context_reference(to);
ring-last_context = to;
- if (ring-id == RCS !to-is_initialized from == NULL) {
+ if (uninitialized) {
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:
--
2.0.0.rc4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only mark the ctx as initialised after a SET_CONTEXT operation
On Mon, Jun 23, 2014 at 12:55:47PM +0300, Jani Nikula wrote: On Fri, 30 May 2014, Chris Wilson ch...@chris-wilson.co.uk wrote: Fallout from commit 46470fc932ac8a0e8317a220b3f4ea4ed903338e Author: Mika Kuoppala mika.kuopp...@linux.intel.com Date: Wed May 21 19:01:06 2014 +0300 drm/i915: Add null state batch to active list undid the earlier fix of only marking the ctx as initialised after it is saved by the hardware during a SET_CONTEXT operation. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com Cc: Damien Lespiau damien.lesp...@intel.com Cc: Mika Kuoppala mika.kuopp...@intel.com Cc: Ben Widawsky b...@bwidawsk.net Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79108 Daniel put his IRC r-b on this on IIRC. [snip] -- Ben Widawsky, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only mark the ctx as initialised after a SET_CONTEXT operation
On Fri, May 30, 2014 at 02:16:30PM +0100, Chris Wilson wrote: Fallout from commit 46470fc932ac8a0e8317a220b3f4ea4ed903338e Author: Mika Kuoppala mika.kuopp...@linux.intel.com Date: Wed May 21 19:01:06 2014 +0300 drm/i915: Add null state batch to active list undid the earlier fix of only marking the ctx as initialised after it is saved by the hardware during a SET_CONTEXT operation. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com Cc: Damien Lespiau damien.lesp...@intel.com Cc: Mika Kuoppala mika.kuopp...@intel.com Cc: Ben Widawsky b...@bwidawsk.net Ping. Ben's comments do not impact upon the urgent need for this fix. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only mark the ctx as initialised after a SET_CONTEXT operation
On Tue, Jun 10, 2014 at 11:34:34AM +0100, Chris Wilson wrote: On Fri, May 30, 2014 at 02:16:30PM +0100, Chris Wilson wrote: Fallout from commit 46470fc932ac8a0e8317a220b3f4ea4ed903338e Author: Mika Kuoppala mika.kuopp...@linux.intel.com Date: Wed May 21 19:01:06 2014 +0300 drm/i915: Add null state batch to active list undid the earlier fix of only marking the ctx as initialised after it is saved by the hardware during a SET_CONTEXT operation. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com Cc: Damien Lespiau damien.lesp...@intel.com Cc: Mika Kuoppala mika.kuopp...@intel.com Cc: Ben Widawsky b...@bwidawsk.net Ping. Ben's comments do not impact upon the urgent need for this fix. Ok, this code is extremely convoluted. ctx-last_ring is terrible, and we should make a clear disdinction of which ctx-foo fields are for the logical context and which for the per-engine stuff. Will be much more important for execlists so that we properly fan out the state. I'm ranting here a bit since it took me a while to realize that -is_initialized is _only_ for the RCS stuff. I guess we should have ctx-obj[ring] and ctx-is_initialized[ring] or even better, a substruct i915_hw_context with an array. /rant With that out of the way, this is Reviewed-by: Daniel Vetter daniel.vet...@ffwll.ch 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
[Intel-gfx] [PATCH] drm/i915: Only mark the ctx as initialised after a SET_CONTEXT operation
Fallout from
commit 46470fc932ac8a0e8317a220b3f4ea4ed903338e
Author: Mika Kuoppala mika.kuopp...@linux.intel.com
Date: Wed May 21 19:01:06 2014 +0300
drm/i915: Add null state batch to active list
undid the earlier fix of only marking the ctx as initialised after it is
saved by the hardware during a SET_CONTEXT operation.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Damien Lespiau damien.lesp...@intel.com
Cc: Mika Kuoppala mika.kuopp...@intel.com
Cc: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_context.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 5a71ef1975b3..34a0b49e6add 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -641,6 +641,7 @@ static int do_switch(struct intel_engine_cs *ring,
struct intel_context *from = ring-last_context;
struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
u32 hw_flags = 0;
+ bool uninitialized = false;
int ret, i;
if (from != NULL ring == dev_priv-ring[RCS]) {
@@ -739,18 +740,19 @@ static int do_switch(struct intel_engine_cs *ring,
i915_gem_context_unreference(from);
}
+ uninitialized = !to-is_initialized from == NULL;
+ to-is_initialized = true;
+
done:
i915_gem_context_reference(to);
ring-last_context = to;
- if (ring-id == RCS !to-is_initialized from == NULL) {
+ if (uninitialized) {
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:
--
2.0.0.rc4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only mark the ctx as initialised after a SET_CONTEXT operation
On Fri, May 30, 2014 at 02:16:30PM +0100, Chris Wilson wrote:
Fallout from
commit 46470fc932ac8a0e8317a220b3f4ea4ed903338e
Author: Mika Kuoppala mika.kuopp...@linux.intel.com
Date: Wed May 21 19:01:06 2014 +0300
drm/i915: Add null state batch to active list
undid the earlier fix of only marking the ctx as initialised after it is
saved by the hardware during a SET_CONTEXT operation.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Damien Lespiau damien.lesp...@intel.com
Cc: Mika Kuoppala mika.kuopp...@intel.com
Cc: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_context.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 5a71ef1975b3..34a0b49e6add 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -641,6 +641,7 @@ static int do_switch(struct intel_engine_cs *ring,
struct intel_context *from = ring-last_context;
struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
u32 hw_flags = 0;
+ bool uninitialized = false;
int ret, i;
if (from != NULL ring == dev_priv-ring[RCS]) {
@@ -739,18 +740,19 @@ static int do_switch(struct intel_engine_cs *ring,
i915_gem_context_unreference(from);
}
+ uninitialized = !to-is_initialized from == NULL;
+ to-is_initialized = true;
Aren't you missing some error paths if you do this? I think we need to
set the state after we've successfully emitted the MI_SET_CONTEXT
command (though really, until we move the ringbuffer tail, it's all
lies, on that note, I'm scared to look at if reset dtrt).
+
done:
i915_gem_context_reference(to);
ring-last_context = to;
- if (ring-id == RCS !to-is_initialized from == NULL) {
+ if (uninitialized) {
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:
I just realized the from == NULL check is something which seems fragile
to me given how much churn we might see with init/reset paths. It'd
probably be good to eventually switch to some global state in dev_priv
which we can track across init/reset/fini
Otherwise it looks correct to me.
--
Ben Widawsky, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Only mark the ctx as initialised after a SET_CONTEXT operation
On Fri, May 30, 2014 at 10:44:53AM -0700, Ben Widawsky wrote:
On Fri, May 30, 2014 at 02:16:30PM +0100, Chris Wilson wrote:
Fallout from
commit 46470fc932ac8a0e8317a220b3f4ea4ed903338e
Author: Mika Kuoppala mika.kuopp...@linux.intel.com
Date: Wed May 21 19:01:06 2014 +0300
drm/i915: Add null state batch to active list
undid the earlier fix of only marking the ctx as initialised after it is
saved by the hardware during a SET_CONTEXT operation.
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
Cc: Damien Lespiau damien.lesp...@intel.com
Cc: Mika Kuoppala mika.kuopp...@intel.com
Cc: Ben Widawsky b...@bwidawsk.net
---
drivers/gpu/drm/i915/i915_gem_context.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
b/drivers/gpu/drm/i915/i915_gem_context.c
index 5a71ef1975b3..34a0b49e6add 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -641,6 +641,7 @@ static int do_switch(struct intel_engine_cs *ring,
struct intel_context *from = ring-last_context;
struct i915_hw_ppgtt *ppgtt = ctx_to_ppgtt(to);
u32 hw_flags = 0;
+ bool uninitialized = false;
int ret, i;
if (from != NULL ring == dev_priv-ring[RCS]) {
@@ -739,18 +740,19 @@ static int do_switch(struct intel_engine_cs *ring,
i915_gem_context_unreference(from);
}
+ uninitialized = !to-is_initialized from == NULL;
+ to-is_initialized = true;
Aren't you missing some error paths if you do this? I think we need to
set the state after we've successfully emitted the MI_SET_CONTEXT
command (though really, until we move the ringbuffer tail, it's all
lies, on that note, I'm scared to look at if reset dtrt).
I also think that until we successfully emit the MI_SET_CONTEXT and
allow the execbuffer to proceed, we can continue to treat the context as
garbage and force-inhibit on the next attempt to run the execbuffer.
+
done:
i915_gem_context_reference(to);
ring-last_context = to;
- if (ring-id == RCS !to-is_initialized from == NULL) {
+ if (uninitialized) {
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:
I just realized the from == NULL check is something which seems fragile
to me given how much churn we might see with init/reset paths. It'd
probably be good to eventually switch to some global state in dev_priv
which we can track across init/reset/fini
It's as fragile as the bug we are papering over. Presumably we do need
to apply fresh paper after a GPU reset, but it is hard to tell without
knowing the bug.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
