Quoting Lionel Landwerlin (2018-08-15 11:50:55) > On 15/08/18 09:49, Tvrtko Ursulin wrote: > > > > On 14/08/2018 19:57, Chris Wilson wrote: > >> Quoting Tvrtko Ursulin (2018-08-14 19:49:46) > >>> > >>> On 13/08/2018 10:16, Chris Wilson wrote: > >>>> Quoting Tvrtko Ursulin (2018-08-13 10:11:44) > >>>>> > >>>>> On 13/08/2018 09:16, Chris Wilson wrote: > >>>>>> Quoting Tvrtko Ursulin (2018-08-13 09:02:18) > >>>>>>> From: Lionel Landwerlin <lionel.g.landwer...@intel.com> > >>>>>>> > >>>>>>> Abstract the context image access a bit. > >>>>>>> > >>>>>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwer...@intel.com> > >>>>>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursu...@intel.com> > >>>>>>> --- > >>>>>>> drivers/gpu/drm/i915/i915_perf.c | 34 > >>>>>>> +++++++++++++++----------------- > >>>>>>> 1 file changed, 16 insertions(+), 18 deletions(-) > >>>>>>> > >>>>>>> diff --git a/drivers/gpu/drm/i915/i915_perf.c > >>>>>>> b/drivers/gpu/drm/i915/i915_perf.c > >>>>>>> index 49597cf31707..ccb20230df2c 100644 > >>>>>>> --- a/drivers/gpu/drm/i915/i915_perf.c > >>>>>>> +++ b/drivers/gpu/drm/i915/i915_perf.c > >>>>>>> @@ -210,6 +210,7 @@ > >>>>>>> #include "i915_oa_cflgt3.h" > >>>>>>> #include "i915_oa_cnl.h" > >>>>>>> #include "i915_oa_icl.h" > >>>>>>> +#include "intel_lrc_reg.h" > >>>>>>> /* HW requires this to be a power of two, between 128k > >>>>>>> and 16M, though driver > >>>>>>> * is currently generally designed assuming the largest 16M > >>>>>>> size is used such > >>>>>>> @@ -1636,27 +1637,25 @@ static void > >>>>>>> gen8_update_reg_state_unlocked(struct i915_gem_context *ctx, > >>>>>>> u32 ctx_oactxctrl = > >>>>>>> dev_priv->perf.oa.ctx_oactxctrl_offset; > >>>>>>> u32 ctx_flexeu0 = dev_priv->perf.oa.ctx_flexeu0_offset; > >>>>>>> /* The MMIO offsets for Flex EU registers aren't > >>>>>>> contiguous */ > >>>>>>> - u32 flex_mmio[] = { > >>>>>>> - i915_mmio_reg_offset(EU_PERF_CNTL0), > >>>>>>> - i915_mmio_reg_offset(EU_PERF_CNTL1), > >>>>>>> - i915_mmio_reg_offset(EU_PERF_CNTL2), > >>>>>>> - i915_mmio_reg_offset(EU_PERF_CNTL3), > >>>>>>> - i915_mmio_reg_offset(EU_PERF_CNTL4), > >>>>>>> - i915_mmio_reg_offset(EU_PERF_CNTL5), > >>>>>>> - i915_mmio_reg_offset(EU_PERF_CNTL6), > >>>>>>> + i915_reg_t flex_regs[] = { > >>>>>>> + EU_PERF_CNTL0, > >>>>>>> + EU_PERF_CNTL1, > >>>>>>> + EU_PERF_CNTL2, > >>>>>>> + EU_PERF_CNTL3, > >>>>>>> + EU_PERF_CNTL4, > >>>>>>> + EU_PERF_CNTL5, > >>>>>>> + EU_PERF_CNTL6, > >>>>>>> }; > >>>>>>> int i; > >>>>>>> - reg_state[ctx_oactxctrl] = > >>>>>>> i915_mmio_reg_offset(GEN8_OACTXCONTROL); > >>>>>>> - reg_state[ctx_oactxctrl+1] = > >>>>>>> (dev_priv->perf.oa.period_exponent << > >>>>>>> - GEN8_OA_TIMER_PERIOD_SHIFT) | > >>>>>>> - (dev_priv->perf.oa.periodic ? > >>>>>>> - GEN8_OA_TIMER_ENABLE : 0) | > >>>>>>> - GEN8_OA_COUNTER_RESUME; > >>>>>>> + CTX_REG(reg_state, ctx_oactxctrl, GEN8_OACTXCONTROL, > >>>>>>> + (dev_priv->perf.oa.period_exponent << > >>>>>>> GEN8_OA_TIMER_PERIOD_SHIFT) | > >>>>>>> + (dev_priv->perf.oa.periodic ? > >>>>>>> GEN8_OA_TIMER_ENABLE : 0) | > >>>>>>> + GEN8_OA_COUNTER_RESUME); > >>>>>> > >>>>>> I'll be honest but, I don't think it's CTX_REG() that helps > >>>>>> improve the > >>>>>> readability here. > >>>>>> > >>>>>> The really odd part is that this sticks itself into a bare part > >>>>>> of the > >>>>>> register state not surrounded by any LRI and after a BB_END. This > >>>>>> routine can only work for established contexts, it should not > >>>>>> work for > >>>>>> execlists_init_reg_state. > >>>>> > >>>>> Unless I am missing something change is completely mechanical, so any > >>>>> question marks you have are already there, right? What do you > >>>>> suggest is > >>>>> the action here? > >>>> > >>>> Sure, the only thing I question of this patch itself is whether > >>>> CTX_REG() is simply too much horrible obfuscating magic. > >>> > >>> Turn a blind eye if the perceived badness factor is below some > >>> threshold? Following patch depends on this one, so if I have to drop > >>> this one, then I have to rework the next one etc.. well, it's not the > >>> worst problem, so yeah, whatever. Make a call and let me know. > >> > >> The patch was fine, just worrying about the surrounding code. > > > > I misunderstood. So only about ctx_oactxctrl_offset and > > ctx_flexeu0_offset from i915_perf.c? Maybe that is some OA magic, I > > have not idea. CC-ing Lionel in case he can shed some light on it. > > Those are the offsets at which the hardware will store the > OA_CTXCTRL/FLEX_EU registers values as documented. > I can see that's it's a bit odd not to have the MI_LRI written before we > do the first restore. > > I'm 99% sure I've verified in practice that application started after > i915/perf is opened have the right values programmed. > Not completely sure that the IGT tests cover that case though. > So maybe there is a problem with the first restore... > > What's the value set into most register that aren't explicitly > programmed in intel_lrc.c ?
That would be whatever was previously there In practice, since we now initialise all real contexts from the default state, the LRI will be there. execlists_init_reg_state() is wonderfully, perhaps dangerously, misleading atm. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx