[Intel-gfx] [CI 2/2] drm/i915/perf: reuse intel_lrc ctx regs macro

[Lionel Landwerlin]

On 15/08/18 11:56, Chris Wilson wrote:
> 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.landwerlin at intel.com>
>>>>>>>>>
>>>>>>>>> Abstract the context image access a bit.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>>>>>>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at 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
>
Then I guess it's all fine :)
Thanks,

-
Lionel