Re: [CI 2/2] drm/i915/perf: reuse intel_lrc ctx regs macro

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@intel.com>
>>>>>>
>>>>>> Abstract the context image access a bit.
>>>>>>
>>>>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>>>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@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.

Regards,

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