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.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to