[Intel-gfx] [PATCH 2/3] drm/i915: Add GT support for multiple types of multicast steering
Rodrigo Vivi
rodrigo.vivi at intel.com
Tue Jun 15 09:08:17 UTC 2021
On Mon, Jun 14, 2021 at 08:34:32PM -0700, Matt Roper wrote:
> Although most of our multicast registers are replicated per-subslice, we
> also have a small number of multicast registers that are replicated
> per-l3 bank instead. For both types of multicast registers we need to
> make sure we steer reads of these registers to a valid instance.
> Ideally we'd like to find a specific instance ID that would steer reads
> of either type of multicast register to a valid instance (i.e., not
> fused off and not powered down), but sometimes the combination of
> part-specific fusing and the additional restrictions imposed by Render
> Power Gating make it impossible to find any overlap between the set of
> valid subslices and valid l3 banks. This problem will become even more
> noticeable on our upcoming platforms since they will be adding
> additional types of multicast registers with new types of replication
> and rules for finding valid instances for reads.
>
> To handle this we'll continue to pick a suitable subslice instance at
> driver startup and program this as the default (sliceid,subsliceid)
> setting in the steering control register (0xFDC). In cases where we
> need to read another type of multicast GT register, but the default
> subslice steering would not correspond to a valid instance, we'll
> explicitly re-steer the single read to a valid value, perform the read,
> and then reset the steering to it's "subslice" default.
>
> This patch adds the general functionality to prepare for this explicit
> steering of other multicast register types. We'll plug L3 bank steering
> into this in the next patch, and then add additional types of multicast
> registers when the support for our next upcoming platform arrives.
>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> drivers/gpu/drm/i915/gt/intel_gt.c | 84 +++++++++++++++++++
> drivers/gpu/drm/i915/gt/intel_gt.h | 8 ++
> drivers/gpu/drm/i915/gt/intel_gt_types.h | 22 +++++
> drivers/gpu/drm/i915/gt/intel_workarounds.c | 28 ++++---
> .../gpu/drm/i915/gt/selftest_workarounds.c | 2 +-
> 5 files changed, 131 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 2161bf01ef8b..f2bea1c20d56 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -697,6 +697,90 @@ void intel_gt_driver_late_release(struct intel_gt *gt)
> intel_engines_free(gt);
> }
>
> +/**
> + * intel_gt_reg_needs_read_steering - determine whether a register read
> + * requires explicit steering
> + * @gt: GT structure
> + * @reg: the register to check steering requirements for
> + * @type: type of multicast steering to check
> + *
> + * Determines whether @reg needs explicit steering of a specific type for
> + * reads.
> + *
> + * Returns false if @reg does not belong to a register range of the given
> + * steering type, or if the default (subslice-based) steering IDs are suitable
> + * for @type steering too.
> + */
> +static bool intel_gt_reg_needs_read_steering(struct intel_gt *gt,
> + i915_reg_t reg,
> + enum intel_steering_type type)
> +{
> + const u32 offset = i915_mmio_reg_offset(reg);
> + const struct intel_mmio_range *entry;
> +
> + if (likely(!intel_gt_needs_read_steering(gt, type)))
> + return false;
> +
> + for (entry = gt->steering_table[type]; entry->start < 0xFFFFFF; entry++) {
I'm not comfortable with this stop condition...
we should know the right amount of entries that we have.
Even if that means having another intermediate struct.
> + if (offset >= entry->start && offset <= entry->end)
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/**
> + * intel_gt_get_valid_steering - determines valid IDs for a class of MCR steering
> + * @gt: GT structure
> + * @type: multicast register type
> + * @sliceid: Slice ID returned
> + * @subsliceid: Subslice ID returned
> + *
> + * Determines sliceid and subsliceid values that will steer reads
> + * of a specific multicast register class to a valid value.
> + */
> +static void intel_gt_get_valid_steering(struct intel_gt *gt,
> + enum intel_steering_type type,
> + u8 *sliceid, u8 *subsliceid)
> +{
> + switch (type) {
> + default:
> + MISSING_CASE(type);
I understand that we are preparing the infra for the upcoming cases,
but adding a missing_case warn by default doesn't look the right way...
did CI not complain?!
> + *sliceid = 0;
> + *subsliceid = 0;
> + }
> +}
> +
> +/**
> + * intel_gt_read_register_fw - reads a GT register with support for multicast
> + * @gt: GT structure
> + * @reg: register to read
> + *
> + * This function will read a GT register. If the register is a multicast
> + * register, the read will be steered to a valid instance (i.e., one that
> + * isn't fused off or powered down by power gating).
> + *
> + * Returns the value from a valid instance of @reg.
> + */
> +u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg)
> +{
> + int type;
> + u8 sliceid, subsliceid;
> +
> + for (type = 0; type < NUM_STEERING_TYPES; type++) {
> + if (intel_gt_reg_needs_read_steering(gt, reg, type)) {
> + intel_gt_get_valid_steering(gt, type, &sliceid,
> + &subsliceid);
> + return intel_uncore_read_with_mcr_steering_fw(gt->uncore,
> + reg,
> + sliceid,
> + subsliceid);
> + }
> + }
> +
> + return intel_uncore_read_fw(gt->uncore, reg);
> +}
> +
> void intel_gt_info_print(const struct intel_gt_info *info,
> struct drm_printer *p)
> {
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h b/drivers/gpu/drm/i915/gt/intel_gt.h
> index 7ec395cace69..e7aabe0cc5bf 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> @@ -75,6 +75,14 @@ static inline bool intel_gt_is_wedged(const struct intel_gt *gt)
> return unlikely(test_bit(I915_WEDGED, >->reset.flags));
> }
>
> +static inline bool intel_gt_needs_read_steering(struct intel_gt *gt,
> + enum intel_steering_type type)
> +{
> + return gt->steering_table[type];
then if we know the right amount we also don't need this function right?!
> +}
> +
> +u32 intel_gt_read_register_fw(struct intel_gt *gt, i915_reg_t reg);
> +
> void intel_gt_info_print(const struct intel_gt_info *info,
> struct drm_printer *p);
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_types.h b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> index fecfacf551d5..47957837c8c0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_types.h
> @@ -31,6 +31,26 @@ struct i915_ggtt;
> struct intel_engine_cs;
> struct intel_uncore;
>
> +struct intel_mmio_range {
> + u32 start;
> + u32 end;
> +};
> +
> +/*
> + * The hardware has multiple kinds of multicast register ranges that need
> + * special register steering (and future platforms are expected to add
> + * additional types).
> + *
> + * During driver startup, we initialize the steering control register to
> + * direct reads to a slice/subslice that are valid for the 'subslice' class
> + * of multicast registers. If another type of steering does not have any
> + * overlap in valid steering targets with 'subslice' style registers, we will
> + * need to explicitly re-steer reads of registers of the other type.
> + */
> +enum intel_steering_type {
> + NUM_STEERING_TYPES
> +};
> +
> enum intel_submission_method {
> INTEL_SUBMISSION_RING,
> INTEL_SUBMISSION_ELSP,
> @@ -145,6 +165,8 @@ struct intel_gt {
>
> struct i915_vma *scratch;
>
> + const struct intel_mmio_range *steering_table[NUM_STEERING_TYPES];
> +
> struct intel_gt_info {
> intel_engine_mask_t engine_mask;
> u8 num_engines;
> diff --git a/drivers/gpu/drm/i915/gt/intel_workarounds.c b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> index b62d1e31a645..689045d3752b 100644
> --- a/drivers/gpu/drm/i915/gt/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/intel_workarounds.c
> @@ -1247,8 +1247,9 @@ wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
> }
>
> static void
> -wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> +wa_list_apply(struct intel_gt *gt, const struct i915_wa_list *wal)
> {
> + struct intel_uncore *uncore = gt->uncore;
> enum forcewake_domains fw;
> unsigned long flags;
> struct i915_wa *wa;
> @@ -1263,13 +1264,16 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
> intel_uncore_forcewake_get__locked(uncore, fw);
>
> for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
> - if (wa->clr)
> - intel_uncore_rmw_fw(uncore, wa->reg, wa->clr, wa->set);
> - else
> - intel_uncore_write_fw(uncore, wa->reg, wa->set);
> + u32 val, old = 0;
> +
> + /* open-coded rmw due to steering */
> + old = wa->clr ? intel_gt_read_register_fw(gt, wa->reg) : 0;
> + val = (old & ~wa->clr) | wa->set;
> + if (val != old || !wa->clr)
> + intel_uncore_write_fw(uncore, wa->reg, val);
> +
> if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
> - wa_verify(wa,
> - intel_uncore_read_fw(uncore, wa->reg),
> + wa_verify(wa, intel_gt_read_register_fw(gt, wa->reg),
> wal->name, "application");
> }
>
> @@ -1279,10 +1283,10 @@ wa_list_apply(struct intel_uncore *uncore, const struct i915_wa_list *wal)
>
> void intel_gt_apply_workarounds(struct intel_gt *gt)
> {
> - wa_list_apply(gt->uncore, >->i915->gt_wa_list);
> + wa_list_apply(gt, >->i915->gt_wa_list);
> }
>
> -static bool wa_list_verify(struct intel_uncore *uncore,
> +static bool wa_list_verify(struct intel_gt *gt,
> const struct i915_wa_list *wal,
> const char *from)
> {
> @@ -1292,7 +1296,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
>
> for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
> ok &= wa_verify(wa,
> - intel_uncore_read(uncore, wa->reg),
> + intel_gt_read_register_fw(gt, wa->reg),
> wal->name, from);
>
> return ok;
> @@ -1300,7 +1304,7 @@ static bool wa_list_verify(struct intel_uncore *uncore,
>
> bool intel_gt_verify_workarounds(struct intel_gt *gt, const char *from)
> {
> - return wa_list_verify(gt->uncore, >->i915->gt_wa_list, from);
> + return wa_list_verify(gt, >->i915->gt_wa_list, from);
> }
>
> __maybe_unused
> @@ -2081,7 +2085,7 @@ void intel_engine_init_workarounds(struct intel_engine_cs *engine)
>
> void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
> {
> - wa_list_apply(engine->uncore, &engine->wa_list);
> + wa_list_apply(engine->gt, &engine->wa_list);
> }
>
> struct mcr_range {
> diff --git a/drivers/gpu/drm/i915/gt/selftest_workarounds.c b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> index c30754daf4b1..7ebc4edb8ecf 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_workarounds.c
> @@ -1147,7 +1147,7 @@ verify_wa_lists(struct intel_gt *gt, struct wa_lists *lists,
> enum intel_engine_id id;
> bool ok = true;
>
> - ok &= wa_list_verify(gt->uncore, &lists->gt_wa_list, str);
> + ok &= wa_list_verify(gt, &lists->gt_wa_list, str);
>
> for_each_engine(engine, gt, id) {
> struct intel_context *ce;
> --
> 2.25.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list