On Fri, Mar 04, 2022 at 12:13:12PM +0200, Jani Nikula wrote:
> On Thu, 03 Mar 2022, Matt Roper <matthew.d.roper@xxxxxxxxx> wrote:
> > From: Akeem G Abodunrin <akeem.g.abodunrin@xxxxxxxxx>
> >
> > Starting with DG2, preemption can no longer be controlled using userspace
> > on a per-context basis. Instead, the hardware only allows us to enable or
> > disable preemption in a global, system-wide basis. Also, we lose the
> > ability to specify the preemption granularity (such as batch-level vs
> > command-level vs object-level).
> >
> > As a result of this - for debugging purposes, this patch adds debugfs
> > interface to configure (disable/enable) preemption globally.
> >
> > Jira: VLK-27831
>
> Please remove internal Jira references.
>
> > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > Cc: Prathap Kumar Valsan <prathap.kumar.valsan@xxxxxxxxx>
> > Cc: John Harrison <john.c.harrison@xxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > Signed-off-by: Akeem G Abodunrin <akeem.g.abodunrin@xxxxxxxxx>
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 3 ++
> > drivers/gpu/drm/i915/gt/intel_workarounds.c | 2 +-
> > drivers/gpu/drm/i915/i915_debugfs.c | 50 +++++++++++++++++++++
> > drivers/gpu/drm/i915/i915_drv.h | 3 ++
> > 4 files changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index 19cd34f24263..21ede1887b9f 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -468,6 +468,9 @@
> > #define VF_PREEMPTION _MMIO(0x83a4)
> > #define PREEMPTION_VERTEX_COUNT REG_GENMASK(15, 0)
> >
> > +#define GEN12_VFG_PREEMPTION_CHICKEN _MMIO(0x83b4)
> > +#define GEN12_VFG_PREEMPT_CHICKEN_DISABLE REG_BIT(8)
> > +
> > #define GEN8_RC6_CTX_INFO _MMIO(0x8504)
> >
> > #define GEN12_SQCM _MMIO(0x8724)
> > diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > index c014b40d2e9f..18dc82f29776 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> > @@ -2310,7 +2310,7 @@ rcs_engine_wa_init(struct intel_engine_cs *engine, struct i915_wa_list *wal)
> > FF_DOP_CLOCK_GATE_DISABLE);
> > }
> >
> > - if (IS_GRAPHICS_VER(i915, 9, 12)) {
> > + if (HAS_PERCTX_PREEMPT_CTRL(i915)) {
>
> Adding HAS_PERCTX_PREEMPT_CTRL(i915) and using it is a separate change
> from the debugfs. Please split it up.
>
> > /* FtrPerCtxtPreemptionGranularityControl:skl,bxt,kbl,cfl,cnl,icl,tgl */
> > wa_masked_en(wal,
> > GEN7_FF_SLICE_CS_CHICKEN1,
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 747fe9f41e1f..40e6e17e2950 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -571,6 +571,55 @@ static int i915_wa_registers(struct seq_file *m, void *unused)
> > return 0;
> > }
> >
> > +static void i915_global_preemption_config(struct drm_i915_private *i915,
> > + u32 val)
> > +{
> > + const u32 bit = GEN12_VFG_PREEMPT_CHICKEN_DISABLE;
>
> We rarely use const for locals, and usually only if the function is big.
>
> I'd probably use:
>
> u32 tmp = val ?
> _MASKED_BIT_DISABLE(GEN12_VFG_PREEMPT_CHICKEN_DISABLE) :
> _MASKED_BIT_ENABLE(GEN12_VFG_PREEMPT_CHICKEN_DISABLE);
>
> To have just one intel_uncore_write().
>
> > +
> > + if (val)
> > + intel_uncore_write(&i915->uncore, GEN12_VFG_PREEMPTION_CHICKEN,
> > + _MASKED_BIT_DISABLE(bit));
> > + else
> > + intel_uncore_write(&i915->uncore, GEN12_VFG_PREEMPTION_CHICKEN,
> > + _MASKED_BIT_ENABLE(bit));
>
> We really shouldn't be adding new direct low-level register access in
> i915_debugfs.c.
>
> Please define an interface for this and add the functionality to a
> suitable place, and then call the functions from here.
>
> > +}
> > +
> > +static int i915_global_preempt_support_get(void *data, u64 *val)
> > +{
> > + struct drm_i915_private *i915 = data;
> > + intel_wakeref_t wakeref;
> > + u32 curr_status = 0;
> > +
> > + if (HAS_PERCTX_PREEMPT_CTRL(i915) || GRAPHICS_VER(i915) < 11)
> > + return -EINVAL;
> > +
> > + with_intel_runtime_pm(&i915->runtime_pm, wakeref)
> > + curr_status = intel_uncore_read(&i915->uncore,
> > + GEN12_VFG_PREEMPTION_CHICKEN);
> > + *val = (curr_status & GEN12_VFG_PREEMPT_CHICKEN_DISABLE) ? 0 : 1;
> > +
> > + return 0;
> > +}
> > +
> > +static int i915_global_preempt_support_set(void *data, u64 val)
> > +{
> > + struct drm_i915_private *i915 = data;
> > + intel_wakeref_t wakeref;
> > +
> > + if (HAS_PERCTX_PREEMPT_CTRL(i915) || GRAPHICS_VER(i915) < 11)
> > + return -EINVAL;
> > +
> > + with_intel_runtime_pm(&i915->runtime_pm, wakeref)
> > + i915_global_preemption_config(i915, val);
> > +
> > + return 0;
> > +}
> > +
> > +DEFINE_SIMPLE_ATTRIBUTE(i915_global_preempt_support_fops,
> > + i915_global_preempt_support_get,
> > + i915_global_preempt_support_set,
> > + "%lld\n");
>
> DEFINE_DEBUGFS_ATTRIBUTE.
If I'm understanding the history correctly, I think
DEFINE_DEBUGFS_ATTRIBUTE is only supposed to be used if you're also
using debugfs_create_file_unsafe() for registration; if you're still
using debugfs_create_file(), then DEFINE_SIMPLE_ATTRIBUTE is preferred
to avoid the extra overhead of redundant protection.
Arguably we should shift over to debugfs_create_file_unsafe() +
DEFINE_DEBUGFS_ATTRIBUTE, but that's probably something we should do
driver-wide in a separate series since we're not doing that for any of
our debugfs today.
One other change we should make here is to move this into the GT debugfs
area, rather than having it at the i915 level. I'll make that change in
the next version.
Matt
>
> > +
> > static int i915_wedged_get(void *data, u64 *val)
> > {
> > struct drm_i915_private *i915 = data;
> > @@ -765,6 +814,7 @@ static const struct i915_debugfs_files {
> > const struct file_operations *fops;
> > } i915_debugfs_files[] = {
> > {"i915_perf_noa_delay", &i915_perf_noa_delay_fops},
> > + {"i915_global_preempt_support", &i915_global_preempt_support_fops},
> > {"i915_wedged", &i915_wedged_fops},
> > {"i915_gem_drop_caches", &i915_drop_caches_fops},
> > #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 457bc1993d19..8c3f69c87d36 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1407,6 +1407,9 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
> > #define HAS_GUC_DEPRIVILEGE(dev_priv) \
> > (INTEL_INFO(dev_priv)->has_guc_deprivilege)
> >
> > +#define HAS_PERCTX_PREEMPT_CTRL(i915) \
> > + ((GRAPHICS_VER(i915) >= 9) && GRAPHICS_VER_FULL(i915) < IP_VER(12, 55))
> > +
> > static inline bool run_as_guest(void)
> > {
> > return !hypervisor_is_type(X86_HYPER_NATIVE);
>
> --
> Jani Nikula, Intel Open Source Graphics Center
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
