Re: [Intel-gfx] [PATCH] drm/i915: Split GAM and MSLICE steering
On Wed, Sep 21, 2022 at 12:26:17PM -0700, Matt Roper wrote: > On Wed, Sep 21, 2022 at 12:58:08PM -0400, Kumar Valsan, Prathap wrote: > > On Fri, Sep 16, 2022 at 07:53:40AM -0700, Matt Roper wrote: > > > On Fri, Sep 16, 2022 at 10:02:32AM +0100, Tvrtko Ursulin wrote: > > > > > > > > On 16/09/2022 02:43, Matt Roper wrote: > > > > > Although the bspec lists several MMIO ranges as "MSLICE," it turns out > > > > > that a subset of these are of a "GAM" subclass that has unique rules > > > > > and > > > > > doesn't followed regular mslice steering behavior. > > > > > > > > > > * Xe_HP SDV: GAM ranges must always be steered to 0,0. These > > > > > registers share the regular steering control register (0xFDC) with > > > > > other steering types > > > > > > > > > > * DG2: GAM ranges must always be steered to 1,0. GAM registers > > > > > have a > > > > > dedicated steering control register (0xFE0) so we can set the > > > > > value > > > > > once at startup and rely on implicit steering. Technically the > > > > > hardware default should already be set to 1,0 properly, but it > > > > > never > > > > > hurts to ensure that in the driver. > > > > > > > > Do you have any data on whether the "technically should" holds in > > > > practice? > > > > What would be the consequences of some platform/machine surprising us > > > > here? > > > > > > The bspec indicates the hardware default value is already the necessary > > > 1,0 value; I'm mostly paranoid about some kind of boot firmware wiping > > > it to 0,0 by accident. I don't have any evidence that has ever actually > > > happened, but explicitly re-programming it to 1,0 in the patch here is a > > > defensive measure just to be safe. > > > > > > If we didn't have this patch _and_ some firmware screwed up the GAM > > > steering target, then presumably we might read back garbage or 0 from > > > GAM registers in places where we should have received a real value. > > Will firmware ever touch the steering target registers. As i was going > > through the respective hsd. The software driver impact is marked as none > > so wondering if this change is really required ? > > The GAM only has a dedicated steering register on DG2; on XEHPSDV it > shares 0xFDC with all the other kinds of steering, so it is important to > handle this range independently of the MSLICE range and make sure we > properly re-steer GAM accesses to the primary instance (and not just any > random MSLICE) there. Ok. I missed that part. > > On DG2, if we assume firmware behaves properly, the dedicated steering > register is initialized properly and we don't need to explicitly > re-steer. However this patch will ensure that we don't needlessly > re-program 0xFDC according to MSLICE rules when accessing a GAM > register. > > There's also the worry that firmware may try to "sanitize" the registers > at startup by programming them to what it thinks are appropriate default > values. Given that DG2's primary GAM is unusual (instance 1, instead of > instance 0 as on other platforms), this feels like a place where > firmware bugs could creep in. They hopefully/probably won't, but > ensuring we forcefully initialize 0xFE0 to the proper value just ensures > that we don't even have to worry about it. Got it. > > Finally, splitting the GAM from MSLICE ensures we get more accurate > debug messages from the drm_printer in dmesg and debugfs. > Looks good to me. Reviewed-by: Prathap Kumar Valsan > > Matt > > > > > Thanks, > > Prathap > > > > > > > > > Matt > > > > > > > > > > > Regards, > > > > > > > > Tvrtko > > > > > > > > > > > > > > Bspec: 66534 > > > > > Signed-off-by: Matt Roper > > > > > --- > > > > > drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 24 > > > > > +++-- > > > > > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 + > > > > > drivers/gpu/drm/i915/gt/intel_gt_types.h| 1 + > > > > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 10 + > > > > > 4 files changed, 34 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/
Re: [Intel-gfx] [PATCH] drm/i915: Split GAM and MSLICE steering
On Fri, Sep 16, 2022 at 07:53:40AM -0700, Matt Roper wrote:
> On Fri, Sep 16, 2022 at 10:02:32AM +0100, Tvrtko Ursulin wrote:
> >
> > On 16/09/2022 02:43, Matt Roper wrote:
> > > Although the bspec lists several MMIO ranges as "MSLICE," it turns out
> > > that a subset of these are of a "GAM" subclass that has unique rules and
> > > doesn't followed regular mslice steering behavior.
> > >
> > > * Xe_HP SDV: GAM ranges must always be steered to 0,0. These
> > > registers share the regular steering control register (0xFDC) with
> > > other steering types
> > >
> > > * DG2: GAM ranges must always be steered to 1,0. GAM registers have a
> > > dedicated steering control register (0xFE0) so we can set the value
> > > once at startup and rely on implicit steering. Technically the
> > > hardware default should already be set to 1,0 properly, but it never
> > > hurts to ensure that in the driver.
> >
> > Do you have any data on whether the "technically should" holds in practice?
> > What would be the consequences of some platform/machine surprising us here?
>
> The bspec indicates the hardware default value is already the necessary
> 1,0 value; I'm mostly paranoid about some kind of boot firmware wiping
> it to 0,0 by accident. I don't have any evidence that has ever actually
> happened, but explicitly re-programming it to 1,0 in the patch here is a
> defensive measure just to be safe.
>
> If we didn't have this patch _and_ some firmware screwed up the GAM
> steering target, then presumably we might read back garbage or 0 from
> GAM registers in places where we should have received a real value.
Will firmware ever touch the steering target registers. As i was going
through the respective hsd. The software driver impact is marked as none
so wondering if this change is really required ?
Thanks,
Prathap
>
>
> Matt
>
> >
> > Regards,
> >
> > Tvrtko
> >
> > >
> > > Bspec: 66534
> > > Signed-off-by: Matt Roper
> > > ---
> > > drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 24 +++--
> > > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 +
> > > drivers/gpu/drm/i915/gt/intel_gt_types.h| 1 +
> > > drivers/gpu/drm/i915/gt/intel_workarounds.c | 10 +
> > > 4 files changed, 34 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > index e79405a45312..a2047a68ea7a 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> > > @@ -40,6 +40,7 @@ static const char * const intel_steering_types[] = {
> > > "L3BANK",
> > > "MSLICE",
> > > "LNCF",
> > > + "GAM",
> > > "INSTANCE 0",
> > > };
> > > @@ -48,14 +49,23 @@ static const struct intel_mmio_range
> > > icl_l3bank_steering_table[] = {
> > > {},
> > > };
> > > +/*
> > > + * Although the bspec lists more "MSLICE" ranges than shown here, some
> > > of those
> > > + * are of a "GAM" subclass that has special rules. Thus we use a
> > > separate
> > > + * GAM table farther down for those.
> > > + */
> > > static const struct intel_mmio_range xehpsdv_mslice_steering_table[] = {
> > > - { 0x004000, 0x004AFF },
> > > - { 0x00C800, 0x00CFFF },
> > > { 0x00DD00, 0x00DDFF },
> > > { 0x00E900, 0x00 }, /* 0xEA00 - OxEFFF is unused */
> > > {},
> > > };
> > > +static const struct intel_mmio_range xehpsdv_gam_steering_table[] = {
> > > + { 0x004000, 0x004AFF },
> > > + { 0x00C800, 0x00CFFF },
> > > + {},
> > > +};
> > > +
> > > static const struct intel_mmio_range xehpsdv_lncf_steering_table[] = {
> > > { 0x00B000, 0x00B0FF },
> > > { 0x00D800, 0x00D8FF },
> > > @@ -114,9 +124,15 @@ void intel_gt_mcr_init(struct intel_gt *gt)
> > > } else if (IS_DG2(i915)) {
> > > gt->steering_table[MSLICE] =
> > > xehpsdv_mslice_steering_table;
> > > gt->steering_table[LNCF] = dg2_lncf_steering_table;
> > > + /*
> > > + * No need to hook up the GAM table since it has a dedicated
> > > + * steering control register on DG2 and can use implicit
> > > + * steering.
> > > + */
> > > } else if (IS_XEHPSDV(i915)) {
> > > gt->steering_table[MSLICE] =
> > > xehpsdv_mslice_steering_table;
> > > gt->steering_table[LNCF] = xehpsdv_lncf_steering_table;
> > > + gt->steering_table[GAM] = xehpsdv_gam_steering_table;
> > > } else if (GRAPHICS_VER(i915) >= 11 &&
> > > GRAPHICS_VER_FULL(i915) < IP_VER(12, 50)) {
> > > gt->steering_table[L3BANK] = icl_l3bank_steering_table;
> > > @@ -351,6 +367,10 @@ static void get_nonterminated_steering(struct
> > > intel_gt *gt,
> > > *group = __ffs(gt->info.mslice_mask) << 1;
> > > *instance = 0; /* unused */
> > >
Re: [Intel-gfx] [PATCH 10/11] drm/i915/pvc: skip all copy engines from aux table invalidate
On Mon, May 02, 2022 at 09:34:16AM -0700, Matt Roper wrote:
> From: Lucas De Marchi
>
> As we have more copy engines now, mask all of them from aux table
> invalidate.
>
> Cc: Prathap Kumar Valsan
> Signed-off-by: Lucas De Marchi
> Signed-off-by: Matt Roper
> ---
> drivers/gpu/drm/i915/gt/gen8_engine_cs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> index 0de17b568b41..f262aed94ef3 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_engine_cs.c
> @@ -275,7 +275,7 @@ int gen12_emit_flush_xcs(struct i915_request *rq, u32
> mode)
> if (!HAS_FLAT_CCS(rq->engine->i915) &&
> (rq->engine->class == VIDEO_DECODE_CLASS ||
>rq->engine->class == VIDEO_ENHANCEMENT_CLASS)) {
> - aux_inv = rq->engine->mask & ~BIT(BCS0);
> + aux_inv = rq->engine->mask & ~GENMASK(BCS8, BCS0);
If we had defined I915_MAX_BCS earlier.
We use ~GENMASK(BCS0 + I915_MAX_BCS - 1, BCS0), so we don't need to
change this with the number of instances.
Otherwise looks good to me.
Reviewed-by: Prathap Kumar Valsan
> if (aux_inv)
> cmd += 4;
> }
> --
> 2.35.1
>
Re: [Intel-gfx] [PATCH v2 2/4] drm/i915/xehp: Add register for compute engine's MMIO-based TLB invalidation
On Wed, Apr 27, 2022 at 09:19:24PM -0700, Matt Roper wrote: > Compute engines have a separate register that the driver should use to > perform MMIO-based TLB invalidation. > > Note that the term "context" in this register's bspec description is > used to refer to the engine instance (in the same way "context" is used > on bspec 46167). > > Bspec: 43930 > Cc: Prathap Kumar Valsan > Cc: Tvrtko Ursulin > Signed-off-by: Matt Roper > --- > drivers/gpu/drm/i915/gt/intel_gt.c | 1 + > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c > b/drivers/gpu/drm/i915/gt/intel_gt.c > index 92394f13b42f..53307ca0eed0 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c > @@ -1175,6 +1175,7 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt) > [VIDEO_DECODE_CLASS]= GEN12_VD_TLB_INV_CR, > [VIDEO_ENHANCEMENT_CLASS] = GEN12_VE_TLB_INV_CR, > [COPY_ENGINE_CLASS] = GEN12_BLT_TLB_INV_CR, > + [COMPUTE_CLASS] = GEN12_COMPCTX_TLB_INV_CR, > }; > struct drm_i915_private *i915 = gt->i915; > struct intel_uncore *uncore = gt->uncore; > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > index a39718a40cc3..a0a49c16babd 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > @@ -1007,6 +1007,7 @@ > #define GEN12_VD_TLB_INV_CR _MMIO(0xcedc) > #define GEN12_VE_TLB_INV_CR _MMIO(0xcee0) > #define GEN12_BLT_TLB_INV_CR _MMIO(0xcee4) > +#define GEN12_COMPCTX_TLB_INV_CR _MMIO(0xcf04) > > #define GEN12_MERT_MOD_CTRL _MMIO(0xcf28) > #define RENDER_MOD_CTRL _MMIO(0xcf2c) Reviewed-by: Prathap Kumar Valsan > -- > 2.35.1 >
Re: [Intel-gfx] [PATCH 1/2] drm/i915/xehp: Add compute engine ABI
On Mon, Apr 25, 2022 at 11:41:36AM +0100, Tvrtko Ursulin wrote:
>
> On 22/04/2022 20:50, Matt Roper wrote:
> > We're now ready to start exposing compute engines to userspace.
> >
> > While we're at it, let's extend the kerneldoc description for the other
> > engine types as well.
> >
> > Cc: Daniele Ceraolo Spurio
> > Cc: Tvrtko Ursulin
> > Cc: Vinay Belgaumkar
> > Cc: Jordan Justen
> > Cc: Szymon Morek
> > UMD (mesa): https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/14395
> > UMD (compute): https://github.com/intel/compute-runtime/pull/451
>
> The compute one points to a commit named "Add compute engine class for xehp"
> but content of which seems more about engine query, including the yet
> non-existent distance query (and more)?! I certainly does not appear to be
> adding a definition of I915_ENGINE_CLASS_COMPUTE. This needs clarifying.
>
> > Signed-off-by: Matt Roper
> > ---
> > drivers/gpu/drm/i915/gt/intel_engine_user.c | 2 +-
> > drivers/gpu/drm/i915/gt/intel_gt.c | 1 +
> > drivers/gpu/drm/i915/i915_drm_client.c | 1 +
> > drivers/gpu/drm/i915/i915_drm_client.h | 2 +-
> > include/uapi/drm/i915_drm.h | 62 +++--
> > 5 files changed, 60 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c
> > b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> > index 0f6cd96b459f..46a174f8aa00 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
> > @@ -47,7 +47,7 @@ static const u8 uabi_classes[] = {
> > [COPY_ENGINE_CLASS] = I915_ENGINE_CLASS_COPY,
> > [VIDEO_DECODE_CLASS] = I915_ENGINE_CLASS_VIDEO,
> > [VIDEO_ENHANCEMENT_CLASS] = I915_ENGINE_CLASS_VIDEO_ENHANCE,
> > - /* TODO: Add COMPUTE_CLASS mapping once ABI is available */
> > + [COMPUTE_CLASS] = I915_ENGINE_CLASS_COMPUTE,
> > };
> > static int engine_cmp(void *priv, const struct list_head *A,
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> > b/drivers/gpu/drm/i915/gt/intel_gt.c
> > index 92394f13b42f..c96e123496a5 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> > @@ -1175,6 +1175,7 @@ void intel_gt_invalidate_tlbs(struct intel_gt *gt)
> > [VIDEO_DECODE_CLASS]= GEN12_VD_TLB_INV_CR,
> > [VIDEO_ENHANCEMENT_CLASS] = GEN12_VE_TLB_INV_CR,
> > [COPY_ENGINE_CLASS] = GEN12_BLT_TLB_INV_CR,
> > + [COMPUTE_CLASS] = GEN12_GFX_TLB_INV_CR,
>
> Do you know what 0xcf04 is?
The mmio 0xcf04 is the one we should use for compute class.
And the context bit in 0xcf04 represents engine instance.
GEN12_GFX_TLB_INV_CR is for render class.
Thanks,
Prathap
>
> Or if GEN12_GFX_TLB_INV_CR is correct then I think get_reg_and_bit() might
> need adjusting to always select bit 0 for any compute engine instance. Not
> sure how hardware would behave if value other than '1' would be written into
> 0xced8.
>
> Regards,
>
> Tvrtko
>
> > };
> > struct drm_i915_private *i915 = gt->i915;
> > struct intel_uncore *uncore = gt->uncore;
> > diff --git a/drivers/gpu/drm/i915/i915_drm_client.c
> > b/drivers/gpu/drm/i915/i915_drm_client.c
> > index 475a6f824cad..18d38cb59923 100644
> > --- a/drivers/gpu/drm/i915/i915_drm_client.c
> > +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> > @@ -81,6 +81,7 @@ static const char * const uabi_class_names[] = {
> > [I915_ENGINE_CLASS_COPY] = "copy",
> > [I915_ENGINE_CLASS_VIDEO] = "video",
> > [I915_ENGINE_CLASS_VIDEO_ENHANCE] = "video-enhance",
> > + [I915_ENGINE_CLASS_COMPUTE] = "compute",
> > };
> > static u64 busy_add(struct i915_gem_context *ctx, unsigned int class)
> > diff --git a/drivers/gpu/drm/i915/i915_drm_client.h
> > b/drivers/gpu/drm/i915/i915_drm_client.h
> > index 5f5b02b01ba0..f796c5e8e060 100644
> > --- a/drivers/gpu/drm/i915/i915_drm_client.h
> > +++ b/drivers/gpu/drm/i915/i915_drm_client.h
> > @@ -13,7 +13,7 @@
> > #include "gt/intel_engine_types.h"
> > -#define I915_LAST_UABI_ENGINE_CLASS I915_ENGINE_CLASS_VIDEO_ENHANCE
> > +#define I915_LAST_UABI_ENGINE_CLASS I915_ENGINE_CLASS_COMPUTE
> > struct drm_i915_private;
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index 35ca528803fd..a2def7b27009 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -154,21 +154,71 @@ enum i915_mocs_table_index {
> > I915_MOCS_CACHED,
> > };
> > -/*
> > +/**
> > + * enum drm_i915_gem_engine_class - uapi engine type enumeration
> > + *
> >* Different engines serve different roles, and there may be more than one
> > - * engine serving each role. enum drm_i915_gem_engine_class provides a
> > - * classification of the role of the engine, which may be used when
> > requesting
> > - * operations to be performed on a certain subset of engines, or for
> > providing
> > - * information about that group.
> >
Re: [Intel-gfx] [PATCH] drm/i915/selftests: Add coverage of mocs registers
On Thu, Oct 24, 2019 at 08:13:29AM +0100, Chris Wilson wrote: > Quoting Kumar Valsan, Prathap (2019-10-23 22:03:40) > > On Tue, Oct 22, 2019 at 12:57:05PM +0100, Chris Wilson wrote: > > > Probe the mocs registers for new contexts and across GPU resets. Similar > > > to intel_workarounds, we have tables of what register values we expect > > > to see, so verify that user contexts are affected by them. In the > > > future, we should add tests similar to intel_sseu to cover dynamic > > > reconfigurations. > > > > > > Signed-off-by: Chris Wilson > > > Cc: Prathap Kumar Valsan > > > Cc: Mika Kuoppala > > > > s/for_each_engine/for_each_uabi_engine ? > > No, we are inside the gt compartment, so we only operate within our > little enclosure. Think parallelism... Ok. Thanks. > -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [CI 4/4] drm/i915/gem: Cancel contexts when hangchecking is disabled
On Wed, Oct 23, 2019 at 01:21:51PM +0100, Chris Wilson wrote:
> Normally, we rely on our hangcheck to prevent persistent batches from
> hogging the GPU. However, if the user disables hangcheck, this mechanism
> breaks down. Despite our insistence that this is unsafe, the users are
> equally insistent that they want to use endless batches and will disable
> the hangcheck mechanism. We are looking at replacing hangcheck, in the
> next patch, with a softer mechanism, that sends a pulse down the engine
> to check if it is well. We can use the same preemptive pulse to flush an
> active context off the GPU upon context close, preventing resources
> being lost and unkillable requests remaining on the GPU after process
> termination.
>
> Testcase: igt/gem_ctx_exec/basic-nohangcheck
> Signed-off-by: Chris Wilson
> Cc: Joonas Lahtinen
> Cc: Michał Winiarski
> Cc: Jon Bloomfield
> Reviewed-by: Jon Bloomfield
> Reviewed-by: Tvrtko Ursulin
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 141
> 1 file changed, 141 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index 7b01f4605f21..b2f042d87be0 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -70,6 +70,7 @@
> #include
>
> #include "gt/intel_lrc_reg.h"
> +#include "gt/intel_engine_heartbeat.h"
> #include "gt/intel_engine_user.h"
>
> #include "i915_gem_context.h"
> @@ -276,6 +277,135 @@ void i915_gem_context_release(struct kref *ref)
> schedule_work(>free_work);
> }
>
> +static inline struct i915_gem_engines *
> +__context_engines_static(const struct i915_gem_context *ctx)
> +{
> + return rcu_dereference_protected(ctx->engines, true);
> +}
> +
> +static bool __reset_engine(struct intel_engine_cs *engine)
> +{
> + struct intel_gt *gt = engine->gt;
> + bool success = false;
> +
> + if (!intel_has_reset_engine(gt))
> + return false;
> +
> + if (!test_and_set_bit(I915_RESET_ENGINE + engine->id,
> + >reset.flags)) {
> + success = intel_engine_reset(engine, NULL) == 0;
> + clear_and_wake_up_bit(I915_RESET_ENGINE + engine->id,
> + >reset.flags);
> + }
> +
> + return success;
> +}
> +
> +static void __reset_context(struct i915_gem_context *ctx,
> + struct intel_engine_cs *engine)
> +{
> + intel_gt_handle_error(engine->gt, engine->mask, 0,
> + "context closure in %s", ctx->name);
> +}
> +
> +static bool __cancel_engine(struct intel_engine_cs *engine)
> +{
> + /*
> + * Send a "high priority pulse" down the engine to cause the
> + * current request to be momentarily preempted. (If it fails to
> + * be preempted, it will be reset). As we have marked our context
> + * as banned, any incomplete request, including any running, will
> + * be skipped following the preemption.
> + *
> + * If there is no hangchecking (one of the reasons why we try to
> + * cancel the context) and no forced preemption, there may be no
> + * means by which we reset the GPU and evict the persistent hog.
> + * Ergo if we are unable to inject a preemptive pulse that can
> + * kill the banned context, we fallback to doing a local reset
> + * instead.
> + */
> + if (CONFIG_DRM_I915_PREEMPT_TIMEOUT && !intel_engine_pulse(engine))
> + return true;
> +
> + /* If we are unable to send a pulse, try resetting this engine. */
> + return __reset_engine(engine);
> +}
> +
> +static struct intel_engine_cs *
> +active_engine(struct dma_fence *fence, struct intel_context *ce)
> +{
> + struct i915_request *rq = to_request(fence);
> + struct intel_engine_cs *engine, *locked;
> +
> + /*
> + * Serialise with __i915_request_submit() so that it sees
> + * is-banned?, or we know the request is already inflight.
> + */
> + locked = READ_ONCE(rq->engine);
> + spin_lock_irq(>active.lock);
> + while (unlikely(locked != (engine = READ_ONCE(rq->engine {
> + spin_unlock(>active.lock);
> + spin_lock(>active.lock);
> + locked = engine;
> + }
> +
> + engine = NULL;
> + if (i915_request_is_active(rq) && !rq->fence.error)
> + engine = rq->engine;
> +
> + spin_unlock_irq(>active.lock);
> +
> + return engine;
> +}
> +
> +static void kill_context(struct i915_gem_context *ctx)
> +{
> + struct i915_gem_engines_iter it;
> + struct intel_context *ce;
> +
> + /*
> + * If we are already banned, it was due to a guilty request causing
> + * a reset and the entire context being evicted from the GPU.
> + */
> + if (i915_gem_context_is_banned(ctx))
> + return;
> +
> + i915_gem_context_set_banned(ctx);
> +
> + /*
> + * Map the
Re: [Intel-gfx] [PATCH] drm/i915/selftests: Add coverage of mocs registers
On Tue, Oct 22, 2019 at 12:57:05PM +0100, Chris Wilson wrote:
> Probe the mocs registers for new contexts and across GPU resets. Similar
> to intel_workarounds, we have tables of what register values we expect
> to see, so verify that user contexts are affected by them. In the
> future, we should add tests similar to intel_sseu to cover dynamic
> reconfigurations.
>
> Signed-off-by: Chris Wilson
> Cc: Prathap Kumar Valsan
> Cc: Mika Kuoppala
s/for_each_engine/for_each_uabi_engine ?
Otherwise
Reviewed-by: Prathap Kumar Valsan
> ---
> drivers/gpu/drm/i915/gt/intel_mocs.c | 4 +
> drivers/gpu/drm/i915/gt/selftest_mocs.c | 393 ++
> .../drm/i915/selftests/i915_live_selftests.h | 1 +
> 3 files changed, 398 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/gt/selftest_mocs.c
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c
> b/drivers/gpu/drm/i915/gt/intel_mocs.c
> index 445ec025bda0..06dba7ff294e 100644
> --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> @@ -448,3 +448,7 @@ void intel_mocs_init(struct intel_gt *gt)
> if (HAS_GLOBAL_MOCS_REGISTERS(gt->i915))
> init_global_mocs(gt);
> }
> +
> +#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> +#include "selftest_mocs.c"
> +#endif
> diff --git a/drivers/gpu/drm/i915/gt/selftest_mocs.c
> b/drivers/gpu/drm/i915/gt/selftest_mocs.c
> new file mode 100644
> index ..ca9679c3ee68
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/selftest_mocs.c
> @@ -0,0 +1,393 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include "gt/intel_engine_pm.h"
> +#include "i915_selftest.h"
> +
> +#include "gem/selftests/mock_context.h"
> +#include "selftests/igt_reset.h"
> +#include "selftests/igt_spinner.h"
> +
> +struct live_mocs {
> + struct drm_i915_mocs_table table;
> + struct i915_vma *scratch;
> + void *vaddr;
> +};
> +
> +static int request_add_sync(struct i915_request *rq, int err)
> +{
> + i915_request_get(rq);
> + i915_request_add(rq);
> + if (i915_request_wait(rq, 0, HZ / 5) < 0)
> + err = -ETIME;
> + i915_request_put(rq);
> +
> + return err;
> +}
> +
> +static int request_add_spin(struct i915_request *rq, struct igt_spinner
> *spin)
> +{
> + int err = 0;
> +
> + i915_request_get(rq);
> + i915_request_add(rq);
> + if (spin && !igt_wait_for_spinner(spin, rq))
> + err = -ETIME;
> + i915_request_put(rq);
> +
> + return err;
> +}
> +
> +static struct i915_vma *create_scratch(struct intel_gt *gt)
> +{
> + struct drm_i915_gem_object *obj;
> + struct i915_vma *vma;
> + int err;
> +
> + obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
> + if (IS_ERR(obj))
> + return ERR_CAST(obj);
> +
> + i915_gem_object_set_cache_coherency(obj, I915_CACHING_CACHED);
> +
> + vma = i915_vma_instance(obj, >ggtt->vm, NULL);
> + if (IS_ERR(vma)) {
> + i915_gem_object_put(obj);
> + return vma;
> + }
> +
> + err = i915_vma_pin(vma, 0, 0, PIN_GLOBAL);
> + if (err) {
> + i915_gem_object_put(obj);
> + return ERR_PTR(err);
> + }
> +
> + return vma;
> +}
> +
> +static int live_mocs_init(struct live_mocs *arg, struct intel_gt *gt)
> +{
> + int err;
> +
> + if (!get_mocs_settings(gt->i915, >table))
> + return -EINVAL;
> +
> + arg->scratch = create_scratch(gt);
> + if (IS_ERR(arg->scratch))
> + return PTR_ERR(arg->scratch);
> +
> + arg->vaddr = i915_gem_object_pin_map(arg->scratch->obj, I915_MAP_WB);
> + if (IS_ERR(arg->vaddr)) {
> + err = PTR_ERR(arg->vaddr);
> + goto err_scratch;
> + }
> +
> + return 0;
> +
> +err_scratch:
> + i915_vma_unpin_and_release(>scratch, 0);
> + return err;
> +}
> +
> +static void live_mocs_fini(struct live_mocs *arg)
> +{
> + i915_vma_unpin_and_release(>scratch, I915_VMA_RELEASE_MAP);
> +}
> +
> +static int read_regs(struct i915_request *rq,
> + u32 addr, unsigned int count,
> + uint32_t *offset)
> +{
> + unsigned int i;
> + u32 *cs;
> +
> + GEM_BUG_ON(!IS_ALIGNED(*offset, sizeof(u32)));
> +
> + cs = intel_ring_begin(rq, 4 * count);
> + if (IS_ERR(cs))
> + return PTR_ERR(cs);
> +
> + for (i = 0; i < count; i++) {
> + *cs++ = MI_STORE_REGISTER_MEM_GEN8 | MI_USE_GGTT;
> + *cs++ = addr;
> + *cs++ = *offset;
> + *cs++ = 0;
> +
> + addr += sizeof(u32);
> + *offset += sizeof(u32);
> + }
> +
> + intel_ring_advance(rq, cs);
> +
> + return 0;
> +}
> +
> +static int read_mocs_table(struct i915_request *rq,
> +const struct drm_i915_mocs_table *table,
> +uint32_t *offset)
> +{
> + u32 addr;
> +
> + if
Re: [Intel-gfx] [PATCH 02/13] drm/i915/selftests: Add coverage of mocs registers
On Sat, Oct 19, 2019 at 12:20:18AM +0100, Chris Wilson wrote:
> Quoting Kumar Valsan, Prathap (2019-10-19 00:24:13)
> > On Fri, Oct 18, 2019 at 11:14:39PM +0100, Chris Wilson wrote:
> > > +static int check_l3cc_table(struct intel_engine_cs *engine,
> > > + const struct drm_i915_mocs_table *table,
> > > + const u32 *vaddr, int *offset)
> > > +{
> > > + u16 unused_value = table->table[I915_MOCS_PTE].l3cc_value;
> > > + unsigned int i;
> > > + u32 expect;
> > > +
> > > + if (1) { /* XXX skip MCR read back */
> > > + *offset += table->n_entries / 2;
> > > + return 0;
> > > + }
> >
> > I think l3cc reset test is valid only from Gen12+. Before that since its
> > loaded from the golden context, table stays the same between reset.
>
> That doesn't affect the validity of actually checking that the values
> don't change. The problem as I understand it is that they lie inside the
> magic 0xb00 region that is broadcast across the slices and not
> accessible from CS, see engine_wa_list_verify() and mcr_range. Sadly
> using the GPU is the cleanest way to emulate userspace interaction with
> the *GPU*.
> -Chris
Hmmm.. But from igt test we are submiting a secure BB to read the L3
MOCS. Not quite clear how that works then.
Thanks,
Prathap
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] drm/i915/selftests: Add coverage of mocs registers
On Fri, Oct 18, 2019 at 11:14:39PM +0100, Chris Wilson wrote:
> Probe the mocs registers for new contexts and across GPU resets. Similar
> to intel_workarounds, we have tables of what register values we expect
> to see, so verify that user contexts are affected by them. In the
> future, we should add tests similar to intel_sseu to cover dynamic
> reconfigurations.
>
> Signed-off-by: Chris Wilson
> Cc: Prathap Kumar Valsan
> Cc: Mika Kuoppala
> ---
> drivers/gpu/drm/i915/gt/intel_mocs.c | 4 +
> drivers/gpu/drm/i915/gt/selftest_mocs.c | 431 ++
> .../drm/i915/selftests/i915_live_selftests.h | 1 +
> 3 files changed, 436 insertions(+)
> create mode 100644 drivers/gpu/drm/i915/gt/selftest_mocs.c
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c
> b/drivers/gpu/drm/i915/gt/intel_mocs.c
> index 5bac3966906b..f5a239640553 100644
> --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> @@ -490,3 +490,7 @@ void intel_mocs_init(struct intel_gt *gt)
> if (HAS_GLOBAL_MOCS_REGISTERS(gt->i915))
> intel_mocs_init_global(gt);
> }
> +
> +#if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
> +#include "selftest_mocs.c"
> +#endif
> diff --git a/drivers/gpu/drm/i915/gt/selftest_mocs.c
> b/drivers/gpu/drm/i915/gt/selftest_mocs.c
> new file mode 100644
> index ..94c1c638621b
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/gt/selftest_mocs.c
> @@ -0,0 +1,431 @@
> +/*
> + * SPDX-License-Identifier: MIT
> + *
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#include "gt/intel_engine_pm.h"
> +#include "i915_selftest.h"
> +
> +#include "gem/selftests/mock_context.h"
> +#include "selftests/igt_reset.h"
> +#include "selftests/igt_spinner.h"
> +
> +struct live_mocs {
> + struct drm_i915_mocs_table table;
> + struct i915_gem_context *ctx;
> + struct i915_vma *scratch;
> + void *vaddr;
> +};
> +
> +static int request_add_sync(struct i915_request *rq, int err)
> +{
> + i915_request_get(rq);
> + i915_request_add(rq);
> + if (i915_request_wait(rq, 0, HZ / 5) < 0)
> + err = -ETIME;
> + i915_request_put(rq);
> +
> + return err;
> +}
> +
> +static int request_add_spin(struct i915_request *rq, struct igt_spinner
> *spin)
> +{
> + int err = 0;
> +
> + i915_request_get(rq);
> + i915_request_add(rq);
> + if (spin && !igt_wait_for_spinner(spin, rq))
> + err = -ETIME;
> + i915_request_put(rq);
> +
> + return err;
> +}
> +
> +static struct i915_vma *create_scratch(struct intel_gt *gt)
> +{
> + struct drm_i915_gem_object *obj;
> + struct i915_vma *vma;
> + int err;
> +
> + obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
> + if (IS_ERR(obj))
> + return ERR_CAST(obj);
> +
> + i915_gem_object_set_cache_coherency(obj, I915_CACHING_CACHED);
> +
> + vma = i915_vma_instance(obj, >ggtt->vm, NULL);
> + if (IS_ERR(vma)) {
> + i915_gem_object_put(obj);
> + return vma;
> + }
> +
> + err = i915_vma_pin(vma, 0, 0, PIN_GLOBAL);
> + if (err) {
> + i915_gem_object_put(obj);
> + return ERR_PTR(err);
> + }
> +
> + return vma;
> +}
> +
> +static int live_mocs_init(struct live_mocs *arg, struct intel_gt *gt)
> +{
> + int err;
> +
> + if (!get_mocs_settings(gt->i915, >table))
> + return -EINVAL;
> +
> + arg->ctx = kernel_context(gt->i915);
> + if (!arg->ctx)
> + return -ENOMEM;
> +
> + arg->scratch = create_scratch(gt);
> + if (IS_ERR(arg->scratch)) {
> + err = PTR_ERR(arg->scratch);
> + goto err_ctx;
> + }
> +
> + arg->vaddr = i915_gem_object_pin_map(arg->scratch->obj, I915_MAP_WB);
> + if (IS_ERR(arg->vaddr)) {
> + err = PTR_ERR(arg->vaddr);
> + goto err_scratch;
> + }
> +
> + return 0;
> +
> +err_scratch:
> + i915_vma_unpin_and_release(>scratch, 0);
> +err_ctx:
> + kernel_context_close(arg->ctx);
> + return err;
> +}
> +
> +static void live_mocs_fini(struct live_mocs *arg)
> +{
> + i915_vma_unpin_and_release(>scratch, I915_VMA_RELEASE_MAP);
> + kernel_context_close(arg->ctx);
> +}
> +
> +static int read_mocs_table(struct i915_request *rq,
> +const struct drm_i915_mocs_table *table,
> +struct i915_vma *vma, int *offset)
> +{
> + unsigned int i;
> + u32 *cs;
> +
> + cs = intel_ring_begin(rq, 4 * table->n_entries);
> + if (IS_ERR(cs))
> + return PTR_ERR(cs);
> +
> + for (i = 0; i < table->n_entries; i++) {
> + *cs++ = MI_STORE_REGISTER_MEM_GEN8 | MI_USE_GGTT;
> + *cs++ =
> i915_mmio_reg_offset(HAS_GLOBAL_MOCS_REGISTERS(rq->i915) ?
> + GEN12_GLOBAL_MOCS(i) :
> + mocs_register(rq->engine, i));
> + *cs++ = i915_ggtt_offset(vma) + i *
Re: [Intel-gfx] [PATCH] drm/i915/selftests: Add coverage of mocs registers
On Fri, Oct 18, 2019 at 01:06:39PM +0100, Chris Wilson wrote:
> Probe the mocs registers for new contexts and across GPU resets. Similar
> to intel_workarounds, we have tables of what register values we expect
> to see, so verify that user contexts are affected by them. In the
> future, we should add tests similar to intel_sseu to cover dynamic
> reconfigurations.
>
> Signed-off-by: Chris Wilson
> Cc: Prathap Kumar Valsan
> Cc: Mika Kuoppala
> ---
> +static int check_l3cc_table(struct intel_engine_cs *engine,
> + const struct drm_i915_mocs_table *table,
> + const u32 *vaddr, int *offset)
> +{
> + u16 unused_value = table->table[I915_MOCS_PTE].l3cc_value;
> + unsigned int i;
> + u32 expect;
> +
> + if (1) { /* XXX skip MCR read back */
> + *offset += table->n_entries / 2;
> + return 0;
> + }
Not checking l3cc table?
> +
> + for (i = 0; i < table->size / 2; i++) {
> + u16 low = get_entry_l3cc(table, 2 * i);
> + u16 high = get_entry_l3cc(table, 2 * i + 1);
> +
> + expect = l3cc_combine(table, low, high);
> + if (vaddr[*offset] != expect) {
> + pr_err("%s: Invalid L3CC[%d] entry, found %08x,
> expected %08x\n",
> +engine->name, i, vaddr[*offset], expect);
> + return -EINVAL;
> + }
> + ++*offset;
> + }
> +
> + /* Odd table size - 1 left over */
> + if (table->size & 1) {
> + u16 low = get_entry_l3cc(table, 2 * i);
> +
> + expect = l3cc_combine(table, low, unused_value);
> + if (vaddr[*offset] != expect) {
> + pr_err("%s: Invalid L3CC[%d] entry, found %08x,
> expected %08x\n",
> +engine->name, i, vaddr[*offset], expect);
> + return -EINVAL;
> + }
> + ++*offset;
> + i++;
> + }
> +
> + /* All remaining entries are also unused */
> + for (; i < table->n_entries / 2; i++) {
> + expect = l3cc_combine(table, unused_value, unused_value);
> + if (vaddr[*offset] != expect) {
> + pr_err("%s: Invalid L3CC[%d] entry, found %08x,
> expected %08x\n",
> +engine->name, i, vaddr[*offset], expect);
> + return -EINVAL;
> + }
> + ++*offset;
> + }
> +
> + return 0;
> +}
> +
> +static int check_mocs_engine(struct live_mocs *arg,
> + struct intel_context *ce)
> +{
> + struct i915_vma *vma = arg->scratch;
> + struct i915_request *rq;
> + int offset;
> + int err;
> +
> + memset32(arg->vaddr, STACK_MAGIC, PAGE_SIZE / sizeof(u32));
> +
> + rq = intel_context_create_request(ce);
> + if (IS_ERR(rq))
> + return PTR_ERR(rq);
> +
> + i915_vma_lock(vma);
> + err = i915_request_await_object(rq, vma->obj, true);
> + if (!err)
> + err = i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE);
> + i915_vma_unlock(vma);
> +
> + offset = 0;
> + if (!err)
> + err = read_mocs_table(rq, >table, vma, );
> + if (!err && ce->engine->class == RENDER_CLASS)
> + err = read_l3cc_table(rq, >table, vma, );
> +
> + err = request_add_sync(rq, err);
> + if (err)
> + return err;
> +
> + offset = 0;
> + if (!err)
> + err = check_mocs_table(ce->engine, >table,
> +arg->vaddr, );
> + if (!err && ce->engine->class == RENDER_CLASS)
> + err = check_l3cc_table(ce->engine, >table,
> +arg->vaddr, );
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +
> +static int live_mocs_clean(void *arg)
> +{
> + struct intel_gt *gt = arg;
> + struct intel_engine_cs *engine;
> + enum intel_engine_id id;
> + struct live_mocs mocs;
> + int err;
> +
> + err = live_mocs_init(, gt);
> + if (err)
> + return err;
> +
> + for_each_engine(engine, gt, id) {
> + struct intel_context *ce;
> +
> + ce = intel_context_create(engine->kernel_context->gem_context,
> + engine);
> + if (IS_ERR(ce)) {
> + err = PTR_ERR(ce);
> + break;
> + }
> +
> + err = check_mocs_engine(, ce);
> + intel_context_put(ce);
Need a _get() to pair with _put()?
> + if (err)
> + break;
> + }
> +
> + live_mocs_fini();
> +
> + return err;
> +}
> +
[snip]
> diff --git a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> b/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> index 6daf6599ec79..1a6abcffce81 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_live_selftests.h
> +++
Re: [Intel-gfx] [PATCH] drm/i915: Move L3 MOCS to engine reset domain.
On Wed, Oct 16, 2019 at 09:42:36AM +0100, Chris Wilson wrote: > Quoting Prathap Kumar Valsan (2019-10-16 05:05:58) > > Gen12 has L3 MOCS in engine reset domain, having us to re-initialize on > > an engine reset. > > Hmm, aiui we can do this by removing half of intel_mocs.c... Right. Tested "Do initial mocs configuration directly" on Gen12. Thanks, Prathap > -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Do initial mocs configuration directly
On Wed, Oct 16, 2019 at 10:07:49AM +0100, Chris Wilson wrote:
> Now that we record the default "goldenstate" context, we do not need to
> emit the mocs registers at the start of each context and can simply do
> mmio before the first context and capture the registers as part of its
> default image. As a consequence, this means that we repeat the mmio
> after each engine reset, fixing up any platform and registers that were
> zapped by the reset (for those platforms with global not context-saved
> settings).
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=111723
> References: https://bugs.freedesktop.org/show_bug.cgi?id=111645
> Signed-off-by: Chris Wilson
> Cc: Prathap Kumar Valsan
> ---
> drivers/gpu/drm/i915/gt/intel_mocs.c | 281 +++
> drivers/gpu/drm/i915/gt/intel_mocs.h | 3 -
> drivers/gpu/drm/i915/i915_gem.c | 9 -
> 3 files changed, 73 insertions(+), 220 deletions(-)
>
> +/**
> + * intel_mocs_init_engine() - emit the mocs control table
> + * @engine: The engine for whom to emit the registers.
> + *
> + * This function simply emits a MI_LOAD_REGISTER_IMM command for the
This function description needs a fix as we don't emit the mocs now.
Reviewed-by: Prathap Kumar Valsan
> + * given table starting at the given address.
> + */
> +void intel_mocs_init_engine(struct intel_engine_cs *engine)
> {
> --
> 2.23.0
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/1] drm/i915/ehl: Add sysfs interface to control class-of-service
On Mon, Oct 07, 2019 at 09:37:20PM +0100, Chris Wilson wrote:
> Quoting Prathap Kumar Valsan (2019-10-07 17:52:09)
> > To provide shared last-level-cache isolation to cpu workloads running
> > concurrently with gpu workloads, the gpu allocation of cache lines needs
> > to be restricted to certain ways. Currently GPU hardware supports four
> > class-of-service(CLOS) levels and there is an associated way-mask for
> > each CLOS. Each LLC MOCS register has a field to select the clos level.
> > So in-order to globally set the gpu to a clos level, driver needs
> > to program entire MOCS table.
> >
> > Hardware supports reading supported way-mask configuration for GPU using
> > a bios pcode interface. This interface has two files--llc_clos_modes and
> > llc_clos. The file llc_clos_modes is read only file and will list the
> > available way masks. The file llc_clos is read/write and will show the
> > currently active way mask and writing a new way mask will update the
> > active way mask of the gpu.
> >
> > Note of Caution: Restricting cache ways using this mechanism presents a
> > larger attack surface for side-channel attacks.
> >
> > Example usage:
> > > cat /sys/class/drm/card0/llc_clos_modes
> > 0xfff 0xfc0 0xc00 0x800
> >
> > >cat /sys/class/drm/card0/llc_clos
> > 0xfff
> >
> > Update to new clos
> > echo "0x800" > /sys/class/drm/card0/llc_clos
>
> So the first question is whether this is global on the device or local
> to the GT.
It is global to the device. I have sent the updated patch, which would
explicitly state this in commit message.
>
> > Signed-off-by: Prathap Kumar Valsan
> > ---
> > drivers/gpu/drm/i915/gt/intel_lrc.c | 7 +
> > drivers/gpu/drm/i915/gt/intel_lrc_reg.h | 1 +
> > drivers/gpu/drm/i915/gt/intel_mocs.c| 216 +++-
> > drivers/gpu/drm/i915/gt/intel_mocs.h| 6 +-
> > drivers/gpu/drm/i915/i915_drv.h | 8 +
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > drivers/gpu/drm/i915/i915_sysfs.c | 100 +++
> > 7 files changed, 337 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > index 468438fb47af..054051969d00 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> > @@ -2137,6 +2137,13 @@ __execlists_update_reg_state(const struct
> > intel_context *ce,
> > intel_sseu_make_rpcs(engine->i915, >sseu);
> >
> > i915_oa_init_reg_state(ce, engine);
> > + /*
> > +* Gen11+ wants to support update of LLC class-of-service
> > via
> > +* sysfs interface. CLOS is defined in MOCS registers and
> > for
> > +* Gen11, MOCS is part of context resgister state.
> > +*/
> > + if (IS_GEN(engine->i915, 11))
> > + intel_mocs_init_reg_state(ce);
>
> Do the filtering in intel_mocs_init_reg_state(). The intel_ prefix says
> it should be common to all.
>
> Not IS_ELKHARTLAKE(), that is implies by subject?
>
Gen11 PCode implementation has this support to read way mask. So updated
it to Gen11.
> > +static int
> > +mocs_store_clos(struct i915_request *rq,
> > + struct intel_context *ce)
> > +{
> > + struct drm_i915_mocs_table t;
> > + unsigned int count, active_clos, index;
> > + u32 offset;
> > + u32 value;
> > + u32 *cs;
> > +
> > + if (!get_mocs_settings(rq->engine->gt, ))
> > + return -ENODEV;
> > +
> > + count = t.n_entries;
> > + active_clos = rq->i915->clos.active_clos;
> > + cs = intel_ring_begin(rq, 4 * count);
> > + if (IS_ERR(cs))
> > + return PTR_ERR(cs);
> > +
> > + offset = i915_ggtt_offset(ce->state) + LRC_STATE_PN * PAGE_SIZE;
> > +
> > + for (index = 0; index < count; index++) {
> > + value = ce->lrc_reg_state[ctx_mocsN(index)];
>
> Their context image is volatile at this point. They are pinned and
> active. If you must do a rmw, do it on the GPU. But do we not know the
> full value (as I don't expect it to be nonpriv)? [If doing rmw on the
> GPU, we're probably have to use a secure batch here to avoid running out
> of ringspace.]
Right. Fixed.
>
> > + value &= ~LE_COS_MASK;
> > + value |= FIELD_PREP(LE_COS_MASK, active_clos);
> > +
> > + *cs++ = MI_STORE_DWORD_IMM_GEN4 | MI_USE_GGTT;
> > + *cs++ = offset + ctx_mocsN(index) * sizeof(uint32_t);
> > + *cs++ = 0;
> > + *cs++ = value;
> > + }
> > +
> > + intel_ring_advance(rq, cs);
> > +
> > + return 0;
> > +}
> > +
> > +static int modify_context_mocs(struct intel_context *ce)
> > +{
> > + struct i915_request *rq;
> > + int err;
> > +
> > + lockdep_assert_held(>pin_mutex);
> > +
> > + rq = i915_request_create(ce->engine->kernel_context);
> > +
Re: [Intel-gfx] [PATCH] drm/i915/tgl: Add sysfs interface to control class-of-service
On Mon, Sep 09, 2019 at 02:50:20PM +0300, Joonas Lahtinen wrote: > Quoting Prathap Kumar Valsan (2019-08-26 01:48:01) > > To provide shared last-level-cache isolation to cpu workloads running > > concurrently with gpu workloads, the gpu allocation of cache lines needs > > to be restricted to certain ways. Currently GPU hardware supports four > > class-of-service(CLOS) levels and there is an associated way-mask for > > each CLOS. > > > > Hardware supports reading supported way-mask configuration for GPU using > > a bios pcode interface. The supported way-masks and the one currently > > active is communicated to userspace via a sysfs file--closctrl. Admin user > > can then select a new mask by writing the mask value to the file. > > > > Note of Caution: Restricting cache ways using this mechanism presents a > > larger attack surface for side-channel attacks. > > I wonder if this is enough to justify some further protection before > enabling? Should there be a kernel warning message on enabling this or a commit message is enough? > > > Example usage: > > The active way-mask is highlighted within square brackets. > > > cat /sys/class/drm/card0/closctrl > > [0x] 0xff00 0xc000 0x8000 > > How about two files for easier scripting interface? > > /sys/class/drm/card0/llc_clos > /sys/class/drm/card0/llc_clos_modes > Agreed. A single file interface was suggested by Jon. Hope he is ok with having two files :) > Regards, Joonas Thanks, Prathap ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915/tgl: Add sysfs interface to control class-of-service
On Tue, Aug 27, 2019 at 03:35:14PM +0100, Chris Wilson wrote: > Quoting Kumar Valsan, Prathap (2019-08-27 15:17:51) > > We want to support this on Gen11 as well, where these registers > > are context saved and restored and we prime the register values of new > > contexts > > from recorded defaults. What could be the correct way to handle this, write > > to the > > default object or should ask GPU to re-record after modifying the > > registers. > > That depends on whether you want to apply to existing or only to new. > For OA / sseu, we modify the context images so that existing contexts > are updated to reflect the new defaults, and we update the defaults. > E.g. gen8_configure_all_contexts() Applying to the existing contexts as well should be the right thing to do. Thank you! I will look at the example. Prathap > -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915/tgl: Add sysfs interface to control class-of-service
On Mon, Aug 26, 2019 at 10:17:55AM +0100, Chris Wilson wrote: > Quoting Prathap Kumar Valsan (2019-08-26 00:35:27) > > To provide shared last-level-cache isolation to cpu workloads running > > concurrently with gpu workloads, the gpu allocation of cache lines needs > > to be restricted to certain ways. Currently GPU hardware supports four > > class-of-service(CLOS) levels and there is an associated way-mask for > > each CLOS. > > > > Hardware supports reading supported way-mask configuration for GPU using > > a bios pcode interface. The supported way-masks and the one currently > > active is communicated to userspace via a sysfs file--closctrl. Admin user > > can then select a new mask by writing the mask value to the file. > > What impact does this have on inflight work? Do you need to drain the > submission queue, change the global registers, force an invalidation and > then restart? Can it be done from inside the GPU so that it is > serialised with on-going submission? I believe this should not be impacting the inflight work. Because, way mask only influnece a new cache allocation on cache miss. Cache hits are not restricted by way-mask. So even the way-mask is changed, in-flight requests previously allocated cache lines from other ways will still be a cache hit until thrashed. We want to support this on Gen11 as well, where these registers are context saved and restored and we prime the register values of new contexts from recorded defaults. What could be the correct way to handle this, write to the default object or should ask GPU to re-record after modifying the registers. Thanks, Prathap > -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915/tgl: Add sysfs interface to control class-of-service
On Mon, Aug 26, 2019 at 09:39:48AM +0100, Chris Wilson wrote:
> Quoting Prathap Kumar Valsan (2019-08-26 00:35:27)
> > To provide shared last-level-cache isolation to cpu workloads running
> > concurrently with gpu workloads, the gpu allocation of cache lines needs
> > to be restricted to certain ways. Currently GPU hardware supports four
> > class-of-service(CLOS) levels and there is an associated way-mask for
> > each CLOS.
> >
> > Hardware supports reading supported way-mask configuration for GPU using
> > a bios pcode interface. The supported way-masks and the one currently
> > active is communicated to userspace via a sysfs file--closctrl. Admin user
> > can then select a new mask by writing the mask value to the file.
> >
> > Note of Caution: Restricting cache ways using this mechanism presents a
> > larger attack surface for side-channel attacks.
> >
> > Example usage:
> > The active way-mask is highlighted within square brackets.
> > > cat /sys/class/drm/card0/closctrl
> > [0x] 0xff00 0xc000 0x8000
> >
> > CLOS0 is currently active.
> >
> > > echo 0x8000 > /sys/class/drm/card0/closctrl
> > > cat /sys/class/drm/card0/closctrl
> > 0x 0xff00 0xc000 [0x8000]
> >
> > CLOS3 is currently active
> >
> > Signed-off-by: Prathap Kumar Valsan
> > ---
> > Changes in v2:
> > Declare closctrl_show and closctrl_store as static functions.
> > drivers/gpu/drm/i915/gt/intel_mocs.c | 57 ---
> > drivers/gpu/drm/i915/gt/intel_mocs.h | 1 +
> > drivers/gpu/drm/i915/i915_drv.h | 8
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > drivers/gpu/drm/i915/i915_sysfs.c| 67
> > 5 files changed, 129 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c
> > b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > index 728704bbbe18..dd13e61944fd 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> > @@ -26,6 +26,7 @@
> > #include "intel_gt.h"
> > #include "intel_mocs.h"
> > #include "intel_lrc.h"
> > +#include "intel_sideband.h"
> >
> > /* structures required */
> > struct drm_i915_mocs_entry {
> > @@ -51,6 +52,7 @@ struct drm_i915_mocs_table {
> > #define LE_SCF(value) ((value) << 14)
> > #define LE_COS(value) ((value) << 15)
> > #define LE_SSE(value) ((value) << 17)
> > +#define LE_COS_MASKGENMASK(16, 15)
> >
> > /* Defines for the tables (LNCFMOCS0 - LNCFMOCS31) - two entries per word
> > */
> > #define L3_ESC(value) ((value) << 0)
> > @@ -408,10 +410,13 @@ void intel_mocs_init_engine(struct intel_engine_cs
> > *engine)
> > unused_value);
> > }
> >
> > -static void intel_mocs_init_global(struct intel_gt *gt)
> > +void intel_mocs_init_global(struct intel_gt *gt)
> > {
> > + struct drm_i915_private *i915 = gt->i915;
> > struct intel_uncore *uncore = gt->uncore;
> > struct drm_i915_mocs_table table;
> > + unsigned int active_clos;
> > + u32 value, unused_value;
> > unsigned int index;
> >
> > GEM_BUG_ON(!HAS_GLOBAL_MOCS_REGISTERS(gt->i915));
> > @@ -422,20 +427,31 @@ static void intel_mocs_init_global(struct intel_gt
> > *gt)
> > if (GEM_DEBUG_WARN_ON(table.size > table.n_entries))
> > return;
> >
> > - for (index = 0; index < table.size; index++)
> > + active_clos = atomic_read(>clos.active_clos);
> > +
> > + for (index = 0; index < table.size; index++) {
> > + value = table.table[index].control_value;
> > + value &= ~LE_COS_MASK;
> > + value |= FIELD_PREP(LE_COS_MASK, active_clos);
> > +
> > intel_uncore_write(uncore,
> >GEN12_GLOBAL_MOCS(index),
> > - table.table[index].control_value);
> > + value);
> > + }
> >
> > /*
> > * Ok, now set the unused entries to the invalid entry (index 0).
> > These
> > * entries are officially undefined and no contract for the
> > contents and
> > * settings is given for these entries.
> > */
> > + unused_value = table.table[0].control_value;
> > + unused_value &= ~LE_COS_MASK;
> > + unused_value |= FIELD_PREP(LE_COS_MASK, active_clos);
> > +
> > for (; index < table.n_entries; index++)
> > intel_uncore_write(uncore,
> >GEN12_GLOBAL_MOCS(index),
> > - table.table[0].control_value);
> > + unused_value);
> > }
> >
> > static int emit_mocs_control_table(struct i915_request *rq,
> > @@ -625,10 +641,41 @@ int intel_mocs_emit(struct i915_request *rq)
> > return 0;
> > }
> >
> > +static void intel_read_clos_way_mask(struct intel_gt *gt)
> > +{
> > + struct drm_i915_private
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Return true by default in mocs settings
On Wed, Aug 07, 2019 at 01:55:56PM -0700, Stuart Summers wrote:
> Reduce code by defaulting to true in the MOCS settings
> initialization function. Set to false for unexpected
> platforms.
>
> Signed-off-by: Stuart Summers
Reviewed-by:Prathap Kumar Valsan
> ---
> drivers/gpu/drm/i915/gt/intel_mocs.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c
> b/drivers/gpu/drm/i915/gt/intel_mocs.c
> index fea8ef2fd2aa..ffd105c53ff4 100644
> --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> @@ -291,31 +291,28 @@ static bool get_mocs_settings(struct intel_gt *gt,
> struct drm_i915_mocs_table *table)
> {
> struct drm_i915_private *i915 = gt->i915;
> - bool result = false;
> + bool result = true;
>
> if (INTEL_GEN(i915) >= 12) {
> table->size = ARRAY_SIZE(tigerlake_mocs_table);
> table->table = tigerlake_mocs_table;
> table->n_entries = GEN11_NUM_MOCS_ENTRIES;
> - result = true;
> } else if (IS_GEN(i915, 11)) {
> table->size = ARRAY_SIZE(icelake_mocs_table);
> table->table = icelake_mocs_table;
> table->n_entries = GEN11_NUM_MOCS_ENTRIES;
> - result = true;
> } else if (IS_GEN9_BC(i915) || IS_CANNONLAKE(i915)) {
> table->size = ARRAY_SIZE(skylake_mocs_table);
> table->n_entries = GEN9_NUM_MOCS_ENTRIES;
> table->table = skylake_mocs_table;
> - result = true;
> } else if (IS_GEN9_LP(i915)) {
> table->size = ARRAY_SIZE(broxton_mocs_table);
> table->n_entries = GEN9_NUM_MOCS_ENTRIES;
> table->table = broxton_mocs_table;
> - result = true;
> } else {
> WARN_ONCE(INTEL_GEN(i915) >= 9,
> "Platform that should have a MOCS table does not.\n");
> + result = false;
> }
>
> /* WaDisableSkipCaching:skl,bxt,kbl,glk */
> --
> 2.22.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Add MOCS state dump to debugfs
On Wed, Aug 07, 2019 at 01:55:55PM -0700, Stuart Summers wrote:
> User applications might need to verify hardware configuration
> of the MOCS entries. To facilitate this debug, add a new debugfs
> entry to allow a dump of the MOCS state to verify expected values
> are set by i915.
>
> Signed-off-by: Stuart Summers
Acked-by: Prathap Kumar Valsan
> ---
> drivers/gpu/drm/i915/gt/intel_mocs.c | 50
> drivers/gpu/drm/i915/gt/intel_mocs.h | 3 ++
> drivers/gpu/drm/i915/i915_debugfs.c | 12 +++
> 3 files changed, 65 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.c
> b/drivers/gpu/drm/i915/gt/intel_mocs.c
> index 728704bbbe18..fea8ef2fd2aa 100644
> --- a/drivers/gpu/drm/i915/gt/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.c
> @@ -625,6 +625,56 @@ int intel_mocs_emit(struct i915_request *rq)
> return 0;
> }
>
> +static void
> +intel_mocs_dump_l3cc_table(struct intel_gt *gt, struct drm_printer *p)
> +{
> + struct intel_uncore *uncore = gt->uncore;
> + struct drm_i915_mocs_table table;
> + unsigned int i;
> +
> + if (!get_mocs_settings(gt, ))
> + return;
> +
> + drm_printf(p, "l3cc:\n");
> +
> + for (i = 0; i < table.n_entries / 2; i++) {
> + u32 reg = intel_uncore_read(uncore, GEN9_LNCFCMOCS(i));
> +
> + drm_printf(p, " MOCS[%d]: 0x%x\n", i * 2, reg & 0x);
> + drm_printf(p, " MOCS[%d]: 0x%x\n", i * 2 + 1, reg >> 16);
> + }
> +}
> +
> +static void
> +intel_mocs_dump_global(struct intel_gt *gt, struct drm_printer *p)
> +{
> + struct intel_uncore *uncore = gt->uncore;
> + struct drm_i915_mocs_table table;
> + unsigned int i;
> +
> + GEM_BUG_ON(!HAS_GLOBAL_MOCS_REGISTERS(gt->i915));
> +
> + if (!get_mocs_settings(gt, ))
> + return;
> +
> + if (GEM_DEBUG_WARN_ON(table.size > table.n_entries))
> + return;
> +
> + drm_printf(p, "global:\n");
> +
> + for (i = 0; i < table.n_entries; i++)
> + drm_printf(p, " MOCS[%d]: 0x%x\n",
> +i, intel_uncore_read(uncore, GEN12_GLOBAL_MOCS(i)));
> +}
> +
> +void intel_mocs_show_info(struct intel_gt *gt, struct drm_printer *p)
> +{
> + intel_mocs_dump_l3cc_table(gt, p);
> +
> + if (HAS_GLOBAL_MOCS_REGISTERS(gt->i915))
> + intel_mocs_dump_global(gt, p);
> +}
> +
> void intel_mocs_init(struct intel_gt *gt)
> {
> intel_mocs_init_l3cc_table(gt);
> diff --git a/drivers/gpu/drm/i915/gt/intel_mocs.h
> b/drivers/gpu/drm/i915/gt/intel_mocs.h
> index 2ae816b7ca19..0ef95ce818d3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_mocs.h
> +++ b/drivers/gpu/drm/i915/gt/intel_mocs.h
> @@ -24,6 +24,8 @@
> #ifndef INTEL_MOCS_H
> #define INTEL_MOCS_H
>
> +#include
> +
> /**
> * DOC: Memory Objects Control State (MOCS)
> *
> @@ -55,6 +57,7 @@ struct intel_gt;
>
> void intel_mocs_init(struct intel_gt *gt);
> void intel_mocs_init_engine(struct intel_engine_cs *engine);
> +void intel_mocs_show_info(struct intel_gt *gt, struct drm_printer *p);
>
> int intel_mocs_emit(struct i915_request *rq);
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 3b15266c54fd..1aa022eb2c3d 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -41,6 +41,7 @@
>
> #include "gem/i915_gem_context.h"
> #include "gt/intel_reset.h"
> +#include "gt/intel_mocs.h"
> #include "gt/uc/intel_guc_submission.h"
>
> #include "i915_debugfs.h"
> @@ -76,6 +77,16 @@ static int i915_capabilities(struct seq_file *m, void
> *data)
> return 0;
> }
>
> +static int show_mocs_info(struct seq_file *m, void *data)
> +{
> + struct drm_i915_private *i915 = node_to_i915(m->private);
> + struct drm_printer p = drm_seq_file_printer(m);
> +
> + intel_mocs_show_info(>gt, );
> +
> + return 0;
> +}
> +
> static char get_pin_flag(struct drm_i915_gem_object *obj)
> {
> return obj->pin_global ? 'p' : ' ';
> @@ -4352,6 +4363,7 @@ static const struct drm_info_list i915_debugfs_list[] =
> {
> {"i915_sseu_status", i915_sseu_status, 0},
> {"i915_drrs_status", i915_drrs_status, 0},
> {"i915_rps_boost_info", i915_rps_boost_info, 0},
> + {"i915_mocs_info", show_mocs_info, 0},
> };
> #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>
> --
> 2.22.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] drm/i915/uc: HuC firmware can't be supported without GuC
On Wed, Aug 07, 2019 at 05:00:29PM +, Michal Wajdeczko wrote: > There is no point in selecting HuC firmware if GuC is unsupported > or it was already disabled, as we need GuC to authenticate HuC. > We are calling intel_uc_init() irrespctive of intel_uc_fetch_firmwares() is successful. Is this ok? Thanks, Prathap > While around, make uc_fw_init_early work without direct access > to whole i915, pass only needed platform/rev info. > > Signed-off-by: Michal Wajdeczko > Cc: Daniele Ceraolo Spurio > Cc: Chris Wilson > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Separate pinning of pages from i915_vma_insert()
On Fri, Aug 02, 2019 at 09:30:43PM +0100, Chris Wilson wrote: > Quoting Prathap Kumar Valsan (2019-08-02 21:41:11) > > Currently i915_vma_insert() is responsible for allocating drm mm node > > and also allocating or gathering physical pages. Move the latter to a > > separate function for better readability. > > Close but if you look at the mutex patches, you'll see why it has to be > before. > -Chris Looked at the Mutex patches. With async get_pages and async bind, pinning and set pages are already separated out from i915_vma_insert(). So this patch may not be needed. Thanks, Prathap ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 32/39] drm/i915: Allow vma binding to occur asynchronously
On Fri, Jun 14, 2019 at 08:10:16AM +0100, Chris Wilson wrote:
> If we let pages be allocated asynchronously, we also then want to push
> the binding process into an asynchronous task. Make it so, utilising the
> recent improvements to fence error tracking and struct_mutex reduction.
>
> Signed-off-by: Chris Wilson
> ---
[snip]
> /**
> * i915_vma_bind - Sets up PTEs for an VMA in it's corresponding address
> space.
> * @vma: VMA to map
> @@ -300,17 +412,12 @@ int i915_vma_bind(struct i915_vma *vma, enum
> i915_cache_level cache_level,
> u32 vma_flags;
> int ret;
>
> + GEM_BUG_ON(!flags);
> GEM_BUG_ON(!drm_mm_node_allocated(>node));
> GEM_BUG_ON(vma->size > vma->node.size);
> -
> - if (GEM_DEBUG_WARN_ON(range_overflows(vma->node.start,
> - vma->node.size,
> - vma->vm->total)))
> - return -ENODEV;
> -
> - if (GEM_DEBUG_WARN_ON(!flags))
> - return -EINVAL;
> -
> + GEM_BUG_ON(range_overflows(vma->node.start,
> +vma->node.size,
> +vma->vm->total));
> bind_flags = 0;
> if (flags & PIN_GLOBAL)
> bind_flags |= I915_VMA_GLOBAL_BIND;
> @@ -325,16 +432,20 @@ int i915_vma_bind(struct i915_vma *vma, enum
> i915_cache_level cache_level,
> if (bind_flags == 0)
> return 0;
>
> - GEM_BUG_ON(!vma->pages);
> + if ((bind_flags & ~vma_flags) & I915_VMA_LOCAL_BIND)
> + bind_flags |= I915_VMA_ALLOC_BIND;
>
> trace_i915_vma_bind(vma, bind_flags);
> - ret = vma->ops->bind_vma(vma, cache_level, bind_flags);
> + if (bind_flags & I915_VMA_ALLOC_BIND)
> + ret = queue_async_bind(vma, cache_level, bind_flags);
> + else
> + ret = __vma_bind(vma, cache_level, bind_flags);
> if (ret)
> return ret;
i915_vma_remove() expects vma has pages set. This is no longer true with
async get pages. Shouldn't the clear_pages() called iff pages are set?
[snip]
> static inline void i915_vma_unlock(struct i915_vma *vma)
> {
> reservation_object_unlock(vma->resv);
> diff --git a/drivers/gpu/drm/i915/selftests/i915_vma.c
> b/drivers/gpu/drm/i915/selftests/i915_vma.c
> index 56c1cac368cc..30b831408b7b 100644
> --- a/drivers/gpu/drm/i915/selftests/i915_vma.c
> +++ b/drivers/gpu/drm/i915/selftests/i915_vma.c
> @@ -204,8 +204,10 @@ static int igt_vma_create(void *arg)
> mock_context_close(ctx);
> }
>
> - list_for_each_entry_safe(obj, on, , st_link)
> + list_for_each_entry_safe(obj, on, , st_link) {
> + i915_gem_object_wait(obj, I915_WAIT_ALL, MAX_SCHEDULE_TIMEOUT);
> i915_gem_object_put(obj);
> + }
> return err;
> }
>
> --
> 2.20.1
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Flush extra hard after writing relocations through the GTT
On Tue, Jul 30, 2019 at 12:21:51PM +0100, Chris Wilson wrote:
> Recently discovered in commit bdae33b8b82b ("drm/i915: Use maximum write
> flush for pwrite_gtt") was that we needed to our full write barrier
> before changing the GGTT PTE to ensure that our indirect writes through
> the GTT landed before the PTE changed (and the writes end up in a
> different page). That also applies to our GGTT relocation path.
>
> Signed-off-by: Chris Wilson
> Cc: sta...@vger.kernel.org
Reviewed-by: Prathap Kumar Valsan
> ---
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 8a2047c4e7c3..01901dad33f7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -1019,11 +1019,12 @@ static void reloc_cache_reset(struct reloc_cache
> *cache)
> kunmap_atomic(vaddr);
> i915_gem_object_finish_access((struct drm_i915_gem_object
> *)cache->node.mm);
> } else {
> - wmb();
> + struct i915_ggtt *ggtt = cache_to_ggtt(cache);
> +
> + intel_gt_flush_ggtt_writes(ggtt->vm.gt);
> io_mapping_unmap_atomic((void __iomem *)vaddr);
> - if (cache->node.allocated) {
> - struct i915_ggtt *ggtt = cache_to_ggtt(cache);
>
> + if (cache->node.allocated) {
> ggtt->vm.clear_range(>vm,
>cache->node.start,
>cache->node.size);
> @@ -1078,6 +1079,7 @@ static void *reloc_iomap(struct drm_i915_gem_object
> *obj,
> void *vaddr;
>
> if (cache->vaddr) {
> + intel_gt_flush_ggtt_writes(ggtt->vm.gt);
> io_mapping_unmap_atomic((void __force __iomem *)
> unmask_page(cache->vaddr));
> } else {
> struct i915_vma *vma;
> @@ -1119,7 +1121,6 @@ static void *reloc_iomap(struct drm_i915_gem_object
> *obj,
>
> offset = cache->node.start;
> if (cache->node.allocated) {
> - wmb();
> ggtt->vm.insert_page(>vm,
>i915_gem_object_get_dma_address(obj, page),
>offset, I915_CACHE_NONE, 0);
> --
> 2.22.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Flush extra hard after writing relocations through the GTT
On Tue, Jul 30, 2019 at 12:21:51PM +0100, Chris Wilson wrote:
> Recently discovered in commit bdae33b8b82b ("drm/i915: Use maximum write
> flush for pwrite_gtt") was that we needed to our full write barrier
> before changing the GGTT PTE to ensure that our indirect writes through
> the GTT landed before the PTE changed (and the writes end up in a
> different page). That also applies to our GGTT relocation path.
Chris,
As i understand, changing the GGTT PTE also an indirect write. If so, isn't a
wmb()
should be good enough.
Thanks,
Prathap
>
> Signed-off-by: Chris Wilson
> Cc: sta...@vger.kernel.org
> ---
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index 8a2047c4e7c3..01901dad33f7 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -1019,11 +1019,12 @@ static void reloc_cache_reset(struct reloc_cache
> *cache)
> kunmap_atomic(vaddr);
> i915_gem_object_finish_access((struct drm_i915_gem_object
> *)cache->node.mm);
> } else {
> - wmb();
> + struct i915_ggtt *ggtt = cache_to_ggtt(cache);
> +
> + intel_gt_flush_ggtt_writes(ggtt->vm.gt);
> io_mapping_unmap_atomic((void __iomem *)vaddr);
> - if (cache->node.allocated) {
> - struct i915_ggtt *ggtt = cache_to_ggtt(cache);
>
> + if (cache->node.allocated) {
> ggtt->vm.clear_range(>vm,
>cache->node.start,
>cache->node.size);
> @@ -1078,6 +1079,7 @@ static void *reloc_iomap(struct drm_i915_gem_object
> *obj,
> void *vaddr;
>
> if (cache->vaddr) {
> + intel_gt_flush_ggtt_writes(ggtt->vm.gt);
> io_mapping_unmap_atomic((void __force __iomem *)
> unmask_page(cache->vaddr));
> } else {
> struct i915_vma *vma;
> @@ -1119,7 +1121,6 @@ static void *reloc_iomap(struct drm_i915_gem_object
> *obj,
>
> offset = cache->node.start;
> if (cache->node.allocated) {
> - wmb();
> ggtt->vm.insert_page(>vm,
>i915_gem_object_get_dma_address(obj, page),
>offset, I915_CACHE_NONE, 0);
> --
> 2.22.0
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/17] drm/i915: Remove lrc default desc from GEM context
On Tue, Jul 30, 2019 at 02:30:26PM +0100, Chris Wilson wrote:
> We only compute the lrc_descriptor() on pinning the context, i.e.
> infrequently, so we do not benefit from storing the template as the
> addressing mode is also fixed for the lifetime of the intel_context.
>
> Signed-off-by: Chris Wilson
Reviewed-by: Prathap Kumar Valsan
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 28 ++-
> .../gpu/drm/i915/gem/i915_gem_context_types.h | 2 --
> drivers/gpu/drm/i915/gt/intel_lrc.c | 12 +---
> drivers/gpu/drm/i915/gvt/scheduler.c | 3 --
> 4 files changed, 10 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index b28c7ca681a8..1b3dc7258ef2 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -397,30 +397,6 @@ static void context_close(struct i915_gem_context *ctx)
> i915_gem_context_put(ctx);
> }
>
> -static u32 default_desc_template(const struct drm_i915_private *i915,
> - const struct i915_address_space *vm)
> -{
> - u32 address_mode;
> - u32 desc;
> -
> - desc = GEN8_CTX_VALID | GEN8_CTX_PRIVILEGE;
> -
> - address_mode = INTEL_LEGACY_32B_CONTEXT;
> - if (vm && i915_vm_is_4lvl(vm))
> - address_mode = INTEL_LEGACY_64B_CONTEXT;
> - desc |= address_mode << GEN8_CTX_ADDRESSING_MODE_SHIFT;
> -
> - if (IS_GEN(i915, 8))
> - desc |= GEN8_CTX_L3LLC_COHERENT;
> -
> - /* TODO: WaDisableLiteRestore when we start using semaphore
> - * signalling between Command Streamers
> - * ring->ctx_desc_template |= GEN8_CTX_FORCE_RESTORE;
> - */
> -
> - return desc;
> -}
> -
> static struct i915_gem_context *
> __create_context(struct drm_i915_private *i915)
> {
> @@ -459,7 +435,6 @@ __create_context(struct drm_i915_private *i915)
> i915_gem_context_set_recoverable(ctx);
>
> ctx->ring_size = 4 * PAGE_SIZE;
> - ctx->desc_template = default_desc_template(i915, NULL);
>
> for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
> ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
> @@ -478,8 +453,9 @@ __set_ppgtt(struct i915_gem_context *ctx, struct
> i915_address_space *vm)
> struct i915_gem_engines_iter it;
> struct intel_context *ce;
>
> + GEM_BUG_ON(old && i915_vm_is_4lvl(vm) != i915_vm_is_4lvl(old));
> +
> ctx->vm = i915_vm_get(vm);
> - ctx->desc_template = default_desc_template(ctx->i915, vm);
>
> for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
> i915_vm_put(ce->vm);
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> index 0ee61482ef94..a02d98494078 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context_types.h
> @@ -171,8 +171,6 @@ struct i915_gem_context {
>
> /** ring_size: size for allocating the per-engine ring buffer */
> u32 ring_size;
> - /** desc_template: invariant fields for the HW context descriptor */
> - u32 desc_template;
>
> /** guilty_count: How many times this context has caused a GPU hang. */
> atomic_t guilty_count;
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c
> b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 5181d29d272e..232f40fcb490 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -417,13 +417,17 @@ lrc_descriptor(struct intel_context *ce, struct
> intel_engine_cs *engine)
> BUILD_BUG_ON(MAX_CONTEXT_HW_ID > (BIT(GEN8_CTX_ID_WIDTH)));
> BUILD_BUG_ON(GEN11_MAX_CONTEXT_HW_ID > (BIT(GEN11_SW_CTX_ID_WIDTH)));
>
> - desc = ctx->desc_template; /* bits 0-11 */
> - GEM_BUG_ON(desc & GENMASK_ULL(63, 12));
> + desc = INTEL_LEGACY_32B_CONTEXT;
> + if (i915_vm_is_4lvl(ce->vm))
> + desc = INTEL_LEGACY_64B_CONTEXT;
> + desc <<= GEN8_CTX_ADDRESSING_MODE_SHIFT;
> +
> + desc |= GEN8_CTX_VALID | GEN8_CTX_PRIVILEGE;
> + if (IS_GEN(engine->i915, 8))
> + desc |= GEN8_CTX_L3LLC_COHERENT;
>
> desc |= i915_ggtt_offset(ce->state) + LRC_HEADER_PAGES * PAGE_SIZE;
> /* bits 12-31 */
> - GEM_BUG_ON(desc & GENMASK_ULL(63, 32));
> -
> /*
>* The following 32bits are copied into the OA reports (dword 2).
>* Consider updating oa_get_render_ctx_id in i915_perf.c when changing
> diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c
> b/drivers/gpu/drm/i915/gvt/scheduler.c
> index f40524b0e300..32ae6b5b7e16 100644
> --- a/drivers/gpu/drm/i915/gvt/scheduler.c
> +++ b/drivers/gpu/drm/i915/gvt/scheduler.c
> @@ -291,9 +291,6 @@ shadow_context_descriptor_update(struct intel_context *ce,
>*
Re: [Intel-gfx] [PATCH 21/24] drm/i915: Remove logical HW ID
On Mon, Jul 15, 2019 at 09:09:43AM +0100, Chris Wilson wrote:
> We only need to keep a unique tag for the active lifetime of the
> context, and for as long as we need to identify that context. The HW
> uses the tag to determine if it should use a lite-restore (why not the
> LRCA?) and passes the tag back for various status identifies. The only
> status we need to track is for OA, so when using perf, we assign the
> specific context a unique tag.
Device Page fault handler needs logical HW id. Logical HW id will be
used to identify the gem context to further lookup the address space.
Prathap
>
> Signed-off-by: Chris Wilson
> ---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 151 --
> drivers/gpu/drm/i915/gem/i915_gem_context.h | 15 --
> .../gpu/drm/i915/gem/i915_gem_context_types.h | 18 ---
> .../drm/i915/gem/selftests/i915_gem_context.c | 13 +-
> .../gpu/drm/i915/gem/selftests/mock_context.c | 8 -
> drivers/gpu/drm/i915/gt/intel_context_types.h | 1 +
> drivers/gpu/drm/i915/gt/intel_engine_types.h | 5 +-
> drivers/gpu/drm/i915/gt/intel_lrc.c | 28 ++--
> drivers/gpu/drm/i915/gvt/kvmgt.c | 17 --
> drivers/gpu/drm/i915/i915_debugfs.c | 3 -
> drivers/gpu/drm/i915/i915_gpu_error.c | 7 +-
> drivers/gpu/drm/i915/i915_gpu_error.h | 1 -
> drivers/gpu/drm/i915/i915_perf.c | 28 ++--
> drivers/gpu/drm/i915/i915_trace.h | 38 ++---
> .../gpu/drm/i915/selftests/i915_gem_evict.c | 4 +-
> drivers/gpu/drm/i915/selftests/i915_vma.c | 2 +-
> 16 files changed, 51 insertions(+), 288 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> index e9f5e19265b9..2669e038661e 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
> @@ -167,95 +167,6 @@ lookup_user_engine(struct i915_gem_context *ctx,
> return i915_gem_context_get_engine(ctx, idx);
> }
>
> -static inline int new_hw_id(struct drm_i915_private *i915, gfp_t gfp)
> -{
> - unsigned int max;
> -
> - lockdep_assert_held(>contexts.mutex);
> -
> - if (INTEL_GEN(i915) >= 11)
> - max = GEN11_MAX_CONTEXT_HW_ID;
> - else if (USES_GUC_SUBMISSION(i915))
> - /*
> - * When using GuC in proxy submission, GuC consumes the
> - * highest bit in the context id to indicate proxy submission.
> - */
> - max = MAX_GUC_CONTEXT_HW_ID;
> - else
> - max = MAX_CONTEXT_HW_ID;
> -
> - return ida_simple_get(>contexts.hw_ida, 0, max, gfp);
> -}
> -
> -static int steal_hw_id(struct drm_i915_private *i915)
> -{
> - struct i915_gem_context *ctx, *cn;
> - LIST_HEAD(pinned);
> - int id = -ENOSPC;
> -
> - lockdep_assert_held(>contexts.mutex);
> -
> - list_for_each_entry_safe(ctx, cn,
> - >contexts.hw_id_list, hw_id_link) {
> - if (atomic_read(>hw_id_pin_count)) {
> - list_move_tail(>hw_id_link, );
> - continue;
> - }
> -
> - GEM_BUG_ON(!ctx->hw_id); /* perma-pinned kernel context */
> - list_del_init(>hw_id_link);
> - id = ctx->hw_id;
> - break;
> - }
> -
> - /*
> - * Remember how far we got up on the last repossesion scan, so the
> - * list is kept in a "least recently scanned" order.
> - */
> - list_splice_tail(, >contexts.hw_id_list);
> - return id;
> -}
> -
> -static int assign_hw_id(struct drm_i915_private *i915, unsigned int *out)
> -{
> - int ret;
> -
> - lockdep_assert_held(>contexts.mutex);
> -
> - /*
> - * We prefer to steal/stall ourselves and our users over that of the
> - * entire system. That may be a little unfair to our users, and
> - * even hurt high priority clients. The choice is whether to oomkill
> - * something else, or steal a context id.
> - */
> - ret = new_hw_id(i915, GFP_KERNEL | __GFP_RETRY_MAYFAIL | __GFP_NOWARN);
> - if (unlikely(ret < 0)) {
> - ret = steal_hw_id(i915);
> - if (ret < 0) /* once again for the correct errno code */
> - ret = new_hw_id(i915, GFP_KERNEL);
> - if (ret < 0)
> - return ret;
> - }
> -
> - *out = ret;
> - return 0;
> -}
> -
> -static void release_hw_id(struct i915_gem_context *ctx)
> -{
> - struct drm_i915_private *i915 = ctx->i915;
> -
> - if (list_empty(>hw_id_link))
> - return;
> -
> - mutex_lock(>contexts.mutex);
> - if (!list_empty(>hw_id_link)) {
> - ida_simple_remove(>contexts.hw_ida, ctx->hw_id);
> - list_del_init(>hw_id_link);
> - }
> - mutex_unlock(>contexts.mutex);
> -}
> -
> static void __free_engines(struct i915_gem_engines *e, unsigned int count)
> {
> while (count--) {
>
