Re: [Intel-gfx] [PATCH 10/19] drm/i915: Remove USES_GUC_SUBMISSION() pointer chasing from gen8_cs_irq_handler

2018-05-17 Thread Tvrtko Ursulin 


On 17/05/2018 12:24, Chris Wilson wrote:

Quoting Tvrtko Ursulin (2018-05-17 11:58:21)


On 17/05/2018 08:40, Chris Wilson wrote:

Store whether or not we need to kick the guc's execlists emulation on
the engine itself to avoid chasing the device info.


We do not chase device info but modparams in this case.


gen8_cs_irq_handler  512 428 -84


I guess my point from before, (unfortunately I forgot to reply), was how
much of the saving remains if GEM_BUG_ON is compiled out?


Remember the motto of killing off checking globals and only checking
derived state? :) (I'm definitely not in favour of sprinkling more global
checking CAPS over the code.)

Out of the debug build

gen8_cs_irq_handler  170 185 +15
gen8_cs_irq_handler  170 128 -42 (later)
gen8_cs_irq_handler  170 128 -42 (+ 
USES_GUC_SUBMISSION)


I don't get which is which.

[snip]


If nothing or almost nothing, I don't see a need to fiddle with this now.


Also please consider later patches which also need conditionals as
execlists is unfortunately gaining new tricks that are harder to pull
off with the current guc submission :|
(Might be time to stop mixing the two backends?)


I might say fine if the commit message contained truth. :)

Regards,

Tvrtko

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

Re: [Intel-gfx] [PATCH 10/19] drm/i915: Remove USES_GUC_SUBMISSION() pointer chasing from gen8_cs_irq_handler

2018-05-17 Thread Chris Wilson 
Quoting Tvrtko Ursulin (2018-05-17 11:58:21)
> 
> On 17/05/2018 08:40, Chris Wilson wrote:
> > Store whether or not we need to kick the guc's execlists emulation on
> > the engine itself to avoid chasing the device info.
> 
> We do not chase device info but modparams in this case.
> 
> > gen8_cs_irq_handler  512 428 -84
> 
> I guess my point from before, (unfortunately I forgot to reply), was how 
> much of the saving remains if GEM_BUG_ON is compiled out?

Remember the motto of killing off checking globals and only checking
derived state? :) (I'm definitely not in favour of sprinkling more global
checking CAPS over the code.)

Out of the debug build

gen8_cs_irq_handler  170 185 +15
gen8_cs_irq_handler  170 128 -42 (later)
gen8_cs_irq_handler  170 128 -42 (+ 
USES_GUC_SUBMISSION)

Hmm,

With USES_GUC_SUBMISSION:

   0xa780 <+0>: callq  0xa785 
   0xa785 <+5>: push   %r12
   0xa787 <+7>: mov%esi,%r12d
   0xa78a <+10>:and$0x1,%r12d
   0xa78e <+14>:and$0x100,%esi
   0xa794 <+20>:push   %rbp
   0xa795 <+21>:push   %rbx
   0xa796 <+22>:mov%rdi,%rbx
   0xa799 <+25>:je 0xa7c6 
   0xa79b <+27>:lea0x458(%rdi),%rdi
   0xa7a2 <+34>:callq  0xa7a7 
   0xa7a7 <+39>:mov0x458(%rbx),%eax
   0xa7ad <+45>:test   %eax,%eax
   0xa7af <+47>:je 0xa7c6 
   0xa7b1 <+49>:lock btsq $0x1,0x108(%rbx)
   0xa7bb <+59>:setae  %bpl
   0xa7bf <+63>:test   %r12d,%r12d
   0xa7c2 <+66>:je 0xa7fa 
   0xa7c4 <+68>:jmp0xa7cd 
   0xa7c6 <+70>:test   %r12d,%r12d
   0xa7c9 <+73>:je 0xa812 
   0xa7cb <+75>:xor%ebp,%ebp
   0xa7cd <+77>:lea0x240(%rbx),%rdi
   0xa7d4 <+84>:callq  0xa7d9 
   0xa7d9 <+89>:testb  $0x1,0x240(%rbx)
   0xa7e0 <+96>:jne0xa820 
   0xa7e2 <+98>:mov$0x0,%rdi
   0xa7e9 <+105>:   callq  0xa7ee 
   0xa7ee <+110>:   movzbl 0x0(%rip),%eax# 0xa7f5 

   0xa7f5 <+117>:   and$0x1,%eax
   0xa7f8 <+120>:   or %eax,%ebp
   0xa7fa <+122>:   test   %bpl,%bpl
   0xa7fd <+125>:   je 0xa812 
   0xa7ff <+127>:   lea0x3d8(%rbx),%rdi
   0xa806 <+134>:   lock btsq $0x0,0x3e0(%rbx)
   0xa810 <+144>:   jae0xa817 
   0xa812 <+146>:   pop%rbx
   0xa813 <+147>:   pop%rbp
   0xa814 <+148>:   pop%r12
   0xa816 <+150>:   retq   
   0xa817 <+151>:   pop%rbx
   0xa818 <+152>:   pop%rbp
   0xa819 <+153>:   pop%r12
   0xa81b <+155>:   jmpq   0xa820 
   0xa820 <+160>:   mov%rbx,%rdi
   0xa823 <+163>:   callq  0xa4a0 
   0xa828 <+168>:   jmp0xa7e2 

to

   0xa780 <+0>: callq  0xa785 
   0xa785 <+5>: push   %r12
   0xa787 <+7>: push   %rbp
   0xa788 <+8>: mov%esi,%ebp
   0xa78a <+10>:and$0x1,%ebp
   0xa78d <+13>:and$0x100,%esi
   0xa793 <+19>:push   %rbx
   0xa794 <+20>:mov%rdi,%rbx
   0xa797 <+23>:je 0xa7cb 
   0xa799 <+25>:lea0x458(%rdi),%rdi
   0xa7a0 <+32>:callq  0xa7a5 
   0xa7a5 <+37>:mov0x458(%rbx),%eax
   0xa7ab <+43>:test   %eax,%eax
   0xa7ad <+45>:je 0xa7cb 
   0xa7af <+47>:lock btsq $0x1,0x108(%rbx)
   0xa7b9 <+57>:setae  %r12b
   0xa7bd <+61>:test   %ebp,%ebp
   0xa7bf <+63>:jne0xa7d2 
   0xa7c1 <+65>:test   %r12b,%r12b
   0xa7c4 <+68>:jne0xa805 
   0xa7c6 <+70>:pop%rbx
   0xa7c7 <+71>:pop%rbp
   0xa7c8 <+72>:pop%r12
   0xa7ca <+74>:retq   
   0xa7cb <+75>:test   %ebp,%ebp
   0xa7cd <+77>:

Re: [Intel-gfx] [PATCH 10/19] drm/i915: Remove USES_GUC_SUBMISSION() pointer chasing from gen8_cs_irq_handler

2018-05-17 Thread Tvrtko Ursulin 


On 17/05/2018 08:40, Chris Wilson wrote:

Store whether or not we need to kick the guc's execlists emulation on
the engine itself to avoid chasing the device info.


We do not chase device info but modparams in this case.


gen8_cs_irq_handler  512 428 -84


I guess my point from before, (unfortunately I forgot to reply), was how 
much of the saving remains if GEM_BUG_ON is compiled out?


If nothing or almost nothing, I don't see a need to fiddle with this now.

Regards,

Tvrtko



Signed-off-by: Chris Wilson 
---
  drivers/gpu/drm/i915/i915_irq.c | 4 +++-
  drivers/gpu/drm/i915/intel_guc_submission.c | 1 +
  drivers/gpu/drm/i915/intel_lrc.c| 1 +
  drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +++
  4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f9bc3aaa90d0..460878572515 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1472,8 +1472,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 
iir)
}
  
  	if (iir & GT_RENDER_USER_INTERRUPT) {

+   if (intel_engine_uses_guc(engine))
+   tasklet = true;
+
notify_ring(engine);
-   tasklet |= USES_GUC_SUBMISSION(engine->i915);
}
  
  	if (tasklet)

diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c 
b/drivers/gpu/drm/i915/intel_guc_submission.c
index 133367a17863..d9fcd5db4ea4 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -1312,6 +1312,7 @@ int intel_guc_submission_enable(struct intel_guc *guc)
engine->unpark = guc_submission_unpark;
  
  		engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;

+   engine->flags |= I915_ENGINE_USES_GUC;
}
  
  	return 0;

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 857ab04452f0..4928e9ad7826 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2305,6 +2305,7 @@ static void execlists_set_default_submission(struct 
intel_engine_cs *engine)
engine->park = NULL;
engine->unpark = NULL;
  
+	engine->flags &= ~I915_ENGINE_USES_GUC;

engine->flags |= I915_ENGINE_SUPPORTS_STATS;
if (engine->i915->preempt_context)
engine->flags |= I915_ENGINE_HAS_PREEMPTION;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
b/drivers/gpu/drm/i915/intel_ringbuffer.h
index acef385c4c80..4ad9c5842575 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -574,6 +574,7 @@ struct intel_engine_cs {
  #define I915_ENGINE_NEEDS_CMD_PARSER BIT(0)
  #define I915_ENGINE_SUPPORTS_STATS   BIT(1)
  #define I915_ENGINE_HAS_PREEMPTION   BIT(2)
+#define I915_ENGINE_USES_GUC BIT(3)
unsigned int flags;
  
  	/*

@@ -651,6 +652,12 @@ intel_engine_has_preemption(const struct intel_engine_cs 
*engine)
return engine->flags & I915_ENGINE_HAS_PREEMPTION;
  }
  
+static inline bool

+intel_engine_uses_guc(const struct intel_engine_cs *engine)
+{
+   return engine->flags & I915_ENGINE_USES_GUC;
+}
+
  static inline bool __execlists_need_preempt(int prio, int last)
  {
return prio > max(0, last);


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

[Intel-gfx] [PATCH 10/19] drm/i915: Remove USES_GUC_SUBMISSION() pointer chasing from gen8_cs_irq_handler

2018-05-17 Thread Chris Wilson 
Store whether or not we need to kick the guc's execlists emulation on
the engine itself to avoid chasing the device info.

gen8_cs_irq_handler  512 428 -84

Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_irq.c | 4 +++-
 drivers/gpu/drm/i915/intel_guc_submission.c | 1 +
 drivers/gpu/drm/i915/intel_lrc.c| 1 +
 drivers/gpu/drm/i915/intel_ringbuffer.h | 7 +++
 4 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f9bc3aaa90d0..460878572515 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1472,8 +1472,10 @@ gen8_cs_irq_handler(struct intel_engine_cs *engine, u32 
iir)
}
 
if (iir & GT_RENDER_USER_INTERRUPT) {
+   if (intel_engine_uses_guc(engine))
+   tasklet = true;
+
notify_ring(engine);
-   tasklet |= USES_GUC_SUBMISSION(engine->i915);
}
 
if (tasklet)
diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c 
b/drivers/gpu/drm/i915/intel_guc_submission.c
index 133367a17863..d9fcd5db4ea4 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -1312,6 +1312,7 @@ int intel_guc_submission_enable(struct intel_guc *guc)
engine->unpark = guc_submission_unpark;
 
engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
+   engine->flags |= I915_ENGINE_USES_GUC;
}
 
return 0;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 857ab04452f0..4928e9ad7826 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2305,6 +2305,7 @@ static void execlists_set_default_submission(struct 
intel_engine_cs *engine)
engine->park = NULL;
engine->unpark = NULL;
 
+   engine->flags &= ~I915_ENGINE_USES_GUC;
engine->flags |= I915_ENGINE_SUPPORTS_STATS;
if (engine->i915->preempt_context)
engine->flags |= I915_ENGINE_HAS_PREEMPTION;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h 
b/drivers/gpu/drm/i915/intel_ringbuffer.h
index acef385c4c80..4ad9c5842575 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -574,6 +574,7 @@ struct intel_engine_cs {
 #define I915_ENGINE_NEEDS_CMD_PARSER BIT(0)
 #define I915_ENGINE_SUPPORTS_STATS   BIT(1)
 #define I915_ENGINE_HAS_PREEMPTION   BIT(2)
+#define I915_ENGINE_USES_GUC BIT(3)
unsigned int flags;
 
/*
@@ -651,6 +652,12 @@ intel_engine_has_preemption(const struct intel_engine_cs 
*engine)
return engine->flags & I915_ENGINE_HAS_PREEMPTION;
 }
 
+static inline bool
+intel_engine_uses_guc(const struct intel_engine_cs *engine)
+{
+   return engine->flags & I915_ENGINE_USES_GUC;
+}
+
 static inline bool __execlists_need_preempt(int prio, int last)
 {
return prio > max(0, last);
-- 
2.17.0

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