Re: [Intel-gfx] [PATCH v2] drm/i915/execlists: Inhibit context save/restore for the fake preempt context

2018-01-24 Thread Michel Thierry 

On 1/24/2018 12:46 PM, Chris Wilson wrote:

Quoting Chris Wilson (2018-01-24 09:44:45)

Quoting Michel Thierry (2018-01-24 01:24:25)

On 1/23/2018 1:04 PM, Chris Wilson wrote:

We only use the preempt context to inject an idle point into execlists.
We never need to reference its logical state, so tell the GPU never to
load it or save it.

v2: BIT(2) for save-inhibit.

N.B. Daniele mentioned this bit mbz for ICL, and has been moved into the
submission process rather than the context image.

Suggested-by: Daniele Ceraolo Spurio 
Signed-off-by: Chris Wilson 
Cc: Michal Winiarski 
Cc: Michel Thierry 
Cc: Michal Wajdeczko 
Cc: Tvrtko Ursulin 
Cc: Mika Kuoppala 
Cc: Daniele Ceraolo Spurio 
---
   drivers/gpu/drm/i915/intel_lrc.c | 4 
   drivers/gpu/drm/i915/intel_lrc.h | 1 +
   2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 22d471a4228d..c28f267a8417 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2285,6 +2285,10 @@ populate_lr_context(struct i915_gem_context *ctx,
   if (!engine->default_state)
   regs[CTX_CONTEXT_CONTROL + 1] |=
   _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
+ if (ctx->hw_id == PREEMPT_ID)
+ regs[CTX_CONTEXT_CONTROL + 1] |=
+ _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
+CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT);



This shouldn't break anything and ICL is not merged yet (plus things may
still change) so if you want to merge this,


I think it's the right thing conceptually to do.


Uhoh, seeing GPU hangs on Broxton when thrashing preemption.
Is the bit still valid?


It should be valid, but nobody used cxt save inhibit until now (it 
doesn't surprise bxt is doing something different).


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915/execlists: Inhibit context save/restore for the fake preempt context

2018-01-24 Thread Chris Wilson 
Quoting Chris Wilson (2018-01-24 09:44:45)
> Quoting Michel Thierry (2018-01-24 01:24:25)
> > On 1/23/2018 1:04 PM, Chris Wilson wrote:
> > > We only use the preempt context to inject an idle point into execlists.
> > > We never need to reference its logical state, so tell the GPU never to
> > > load it or save it.
> > > 
> > > v2: BIT(2) for save-inhibit.
> > > 
> > > N.B. Daniele mentioned this bit mbz for ICL, and has been moved into the
> > > submission process rather than the context image.
> > > 
> > > Suggested-by: Daniele Ceraolo Spurio 
> > > Signed-off-by: Chris Wilson 
> > > Cc: Michal Winiarski 
> > > Cc: Michel Thierry 
> > > Cc: Michal Wajdeczko 
> > > Cc: Tvrtko Ursulin 
> > > Cc: Mika Kuoppala 
> > > Cc: Daniele Ceraolo Spurio 
> > > ---
> > >   drivers/gpu/drm/i915/intel_lrc.c | 4 
> > >   drivers/gpu/drm/i915/intel_lrc.h | 1 +
> > >   2 files changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> > > b/drivers/gpu/drm/i915/intel_lrc.c
> > > index 22d471a4228d..c28f267a8417 100644
> > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > @@ -2285,6 +2285,10 @@ populate_lr_context(struct i915_gem_context *ctx,
> > >   if (!engine->default_state)
> > >   regs[CTX_CONTEXT_CONTROL + 1] |=
> > >   
> > > _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
> > > + if (ctx->hw_id == PREEMPT_ID)
> > > + regs[CTX_CONTEXT_CONTROL + 1] |=
> > > + 
> > > _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
> > > +
> > > CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT);
> > >
> > 
> > This shouldn't break anything and ICL is not merged yet (plus things may 
> > still change) so if you want to merge this,
> 
> I think it's the right thing conceptually to do.

Uhoh, seeing GPU hangs on Broxton when thrashing preemption.
Is the bit still valid?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915/execlists: Inhibit context save/restore for the fake preempt context

2018-01-24 Thread Chris Wilson 
Quoting Michel Thierry (2018-01-24 01:24:25)
> On 1/23/2018 1:04 PM, Chris Wilson wrote:
> > We only use the preempt context to inject an idle point into execlists.
> > We never need to reference its logical state, so tell the GPU never to
> > load it or save it.
> > 
> > v2: BIT(2) for save-inhibit.
> > 
> > N.B. Daniele mentioned this bit mbz for ICL, and has been moved into the
> > submission process rather than the context image.
> > 
> > Suggested-by: Daniele Ceraolo Spurio 
> > Signed-off-by: Chris Wilson 
> > Cc: Michal Winiarski 
> > Cc: Michel Thierry 
> > Cc: Michal Wajdeczko 
> > Cc: Tvrtko Ursulin 
> > Cc: Mika Kuoppala 
> > Cc: Daniele Ceraolo Spurio 
> > ---
> >   drivers/gpu/drm/i915/intel_lrc.c | 4 
> >   drivers/gpu/drm/i915/intel_lrc.h | 1 +
> >   2 files changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.c 
> > b/drivers/gpu/drm/i915/intel_lrc.c
> > index 22d471a4228d..c28f267a8417 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > @@ -2285,6 +2285,10 @@ populate_lr_context(struct i915_gem_context *ctx,
> >   if (!engine->default_state)
> >   regs[CTX_CONTEXT_CONTROL + 1] |=
> >   
> > _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
> > + if (ctx->hw_id == PREEMPT_ID)
> > + regs[CTX_CONTEXT_CONTROL + 1] |=
> > + 
> > _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
> > +CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT);
> >
> 
> This shouldn't break anything and ICL is not merged yet (plus things may 
> still change) so if you want to merge this,

I think it's the right thing conceptually to do.
 
> Reviewed-by: Michel Thierry 

Thanks.
 
> >   i915_gem_object_unpin_map(ctx_obj);
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_lrc.h 
> > b/drivers/gpu/drm/i915/intel_lrc.h
> > index 6d4f9b995a11..636ced41225d 100644
> > --- a/drivers/gpu/drm/i915/intel_lrc.h
> > +++ b/drivers/gpu/drm/i915/intel_lrc.h
> > @@ -37,6 +37,7 @@
> >   #define   CTX_CTRL_INHIBIT_SYN_CTX_SWITCH   (1 << 3)
> >   #define   CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT   (1 << 0)
> >   #define   CTX_CTRL_RS_CTX_ENABLE(1 << 1)
> > +#defineCTX_CTRL_ENGINE_CTX_SAVE_INHIBIT  (1 << 2)
> 
> CTX_CTRL_INHIBIT_SYN_CTX_SWITCH should be here, but that's for another 
> patch ;)

Also, now we have intel_lrc_reg.h. Hint, hint ;)

Should we also start using include/linux/bitfield.h for our register
descriptions? It looks a bit ungainly, but it's main advantage is static
checking of all the constants to catch trivial mistakes.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/i915/execlists: Inhibit context save/restore for the fake preempt context

2018-01-23 Thread Michel Thierry 

On 1/23/2018 1:04 PM, Chris Wilson wrote:

We only use the preempt context to inject an idle point into execlists.
We never need to reference its logical state, so tell the GPU never to
load it or save it.

v2: BIT(2) for save-inhibit.

N.B. Daniele mentioned this bit mbz for ICL, and has been moved into the
submission process rather than the context image.

Suggested-by: Daniele Ceraolo Spurio 
Signed-off-by: Chris Wilson 
Cc: Michal Winiarski 
Cc: Michel Thierry 
Cc: Michal Wajdeczko 
Cc: Tvrtko Ursulin 
Cc: Mika Kuoppala 
Cc: Daniele Ceraolo Spurio 
---
  drivers/gpu/drm/i915/intel_lrc.c | 4 
  drivers/gpu/drm/i915/intel_lrc.h | 1 +
  2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 22d471a4228d..c28f267a8417 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2285,6 +2285,10 @@ populate_lr_context(struct i915_gem_context *ctx,
if (!engine->default_state)
regs[CTX_CONTEXT_CONTROL + 1] |=
_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
+   if (ctx->hw_id == PREEMPT_ID)
+   regs[CTX_CONTEXT_CONTROL + 1] |=
+   _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
+  CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT);



This shouldn't break anything and ICL is not merged yet (plus things may 
still change) so if you want to merge this,


Reviewed-by: Michel Thierry 


i915_gem_object_unpin_map(ctx_obj);
  
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h

index 6d4f9b995a11..636ced41225d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -37,6 +37,7 @@
  #define CTX_CTRL_INHIBIT_SYN_CTX_SWITCH   (1 << 3)
  #define CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT   (1 << 0)
  #define   CTX_CTRL_RS_CTX_ENABLE(1 << 1)
+#define  CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT  (1 << 2)


CTX_CTRL_INHIBIT_SYN_CTX_SWITCH should be here, but that's for another 
patch ;)



  #define RING_CONTEXT_STATUS_BUF_BASE(engine)  _MMIO((engine)->mmio_base + 
0x370)
  #define RING_CONTEXT_STATUS_BUF_LO(engine, i) _MMIO((engine)->mmio_base + 
0x370 + (i) * 8)
  #define RING_CONTEXT_STATUS_BUF_HI(engine, i) _MMIO((engine)->mmio_base + 
0x370 + (i) * 8 + 4)


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915/execlists: Inhibit context save/restore for the fake preempt context

2018-01-23 Thread Chris Wilson 
We only use the preempt context to inject an idle point into execlists.
We never need to reference its logical state, so tell the GPU never to
load it or save it.

v2: BIT(2) for save-inhibit.

N.B. Daniele mentioned this bit mbz for ICL, and has been moved into the
submission process rather than the context image.

Suggested-by: Daniele Ceraolo Spurio 
Signed-off-by: Chris Wilson 
Cc: Michal Winiarski 
Cc: Michel Thierry 
Cc: Michal Wajdeczko 
Cc: Tvrtko Ursulin 
Cc: Mika Kuoppala 
Cc: Daniele Ceraolo Spurio 
---
 drivers/gpu/drm/i915/intel_lrc.c | 4 
 drivers/gpu/drm/i915/intel_lrc.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 22d471a4228d..c28f267a8417 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2285,6 +2285,10 @@ populate_lr_context(struct i915_gem_context *ctx,
if (!engine->default_state)
regs[CTX_CONTEXT_CONTROL + 1] |=
_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
+   if (ctx->hw_id == PREEMPT_ID)
+   regs[CTX_CONTEXT_CONTROL + 1] |=
+   _MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
+  CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT);
 
i915_gem_object_unpin_map(ctx_obj);
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 6d4f9b995a11..636ced41225d 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -37,6 +37,7 @@
 #define  CTX_CTRL_INHIBIT_SYN_CTX_SWITCH   (1 << 3)
 #define  CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT   (1 << 0)
 #define   CTX_CTRL_RS_CTX_ENABLE(1 << 1)
+#define  CTX_CTRL_ENGINE_CTX_SAVE_INHIBIT  (1 << 2)
 #define RING_CONTEXT_STATUS_BUF_BASE(engine)   _MMIO((engine)->mmio_base + 
0x370)
 #define RING_CONTEXT_STATUS_BUF_LO(engine, i)  _MMIO((engine)->mmio_base + 
0x370 + (i) * 8)
 #define RING_CONTEXT_STATUS_BUF_HI(engine, i)  _MMIO((engine)->mmio_base + 
0x370 + (i) * 8 + 4)
-- 
2.15.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx