Re: [PATCH v9] drm/i915/icl: Check for fused-off VDBOX and VEBOX instances

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 2/21/2018 10:17 PM, Sagar Arun Kamble wrote:
Looks good to me. Few cosmetic changes suggested below. With those addressed:
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>

On 2/22/2018 5:05 AM, Oscar Mateo wrote:
In Gen11, the Video Decode engines (aka VDBOX, aka VCS, aka BSD) and the
Video Enhancement engines (aka VEBOX, aka VECS) could be fused off. Also, each VDBOX and VEBOX has its own power well, which only exist if the related
engine exists in the HW.

Unfortunately, we have a Catch-22 situation going on: we need the blitter forcewake to read the register with the fuse info, but we cannot initialize the forcewake domains without knowin about the engines present in the HW.
*knowing
We workaround this problem by pruning the forcewake domains after reading
the fuse information.
This line can be re-framed like:
"We workaround this problem by allowing initialization of all forcewake domains and then pruning the fused off forcewake domains
based on fuse info which can be read acquiring blitter forcewake"

Can do.


Bspec: 20680

v2: We were shifting incorrectly for vebox disable (Vinay)

v3: Assert mmio is ready and warn if we have attempted to initialize
     forcewake for fused-off engines (Paulo)

v4:
   - Use INTEL_GEN in new code (Tvrtko)
   - Shorter local variable (Tvrtko, Michal)
   - Keep "if (!...) continue" style (Tvrtko)
   - No unnecessary BUG_ON (Tvrtko)
   - WARN_ON and cleanup if wrong mask (Tvrtko, Michal)
   - Use I915_READ_FW (Michal)
   - Use I915_MAX_VCS/VECS macros (Michal)

v5: Rebased by Rodrigo fixing conflicts on top of:
     commit 33def1ff7b0 ("drm/i915: Simplify intel_engines_init")

v6: Fix v5. Remove info->num_rings. (by Oscar)

v7: Rebase (Rodrigo).

v8:
   - s/intel_device_info_fused_off_engines/intel_device_info_init_mmio (Chris)
   - Make vdbox_disable & vebox_disable local variables (Chris)

v9:
   - Move function declaration to intel_device_info.h (Michal)
   - Missing indent in bit fields definitions (Michal)
   - When RC6 is enabled by BIOS, the fuse register cannot be read until
     the blitter powerwell is awake. Shuffle where the fuse is read, prune
     the forcewake domains after the fact and change the commit message
     accordingly (Vinay, Sagar, Chris).

Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
Cc: Vinay Belgaumkar <vinay.belgaumkar@xxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
Signed-off-by: Oscar Mateo <oscar.mateo@xxxxxxxxx>
---
  drivers/gpu/drm/i915/i915_drv.c          |  4 +++
  drivers/gpu/drm/i915/i915_reg.h          |  5 +++
  drivers/gpu/drm/i915/intel_device_info.c | 47 +++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_device_info.h |  1 +
  drivers/gpu/drm/i915/intel_uncore.c      | 55 ++++++++++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_uncore.h      |  1 +
  6 files changed, 113 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index d09f8e6..2269b56 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1031,6 +1031,10 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv)
        intel_uncore_init(dev_priv);
  +    intel_device_info_init_mmio(dev_priv);
+
+    intel_uncore_prune(dev_priv);
+
      intel_uc_init_mmio(dev_priv);
        ret = intel_engines_init_mmio(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 784d79c..e6a0d84 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2854,6 +2854,11 @@ enum i915_power_well_id {
  #define GEN10_EU_DISABLE3        _MMIO(0x9140)
  #define   GEN10_EU_DIS_SS_MASK        0xff
  +#define GEN11_GT_VEBOX_VDBOX_DISABLE    _MMIO(0x9140)
+#define   GEN11_GT_VDBOX_DISABLE_MASK    0xff
+#define   GEN11_GT_VEBOX_DISABLE_SHIFT    16
+#define   GEN11_GT_VEBOX_DISABLE_MASK    (0xff << GEN11_GT_VEBOX_DISABLE_SHIFT)
+
  #define GEN6_BSD_SLEEP_PSMI_CONTROL    _MMIO(0x12050)
  #define   GEN6_BSD_SLEEP_MSG_DISABLE    (1 << 0)
  #define   GEN6_BSD_SLEEP_FLUSH_DISABLE    (1 << 2)
diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 9352f34..70ea654 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -595,3 +595,50 @@ void intel_driver_caps_print(const struct intel_driver_caps *caps,
  {
      drm_printf(p, "scheduler: %x\n", caps->scheduler);
  }
+
+/*
+ * Determine which engines are fused off in our particular hardware. Since the + * fuse register is in the blitter powerwell, we need forcewake to be ready at + * this point (but later we need to prune the forcewake domains for engines that
+ * are indeed fused off).
+ */
+void intel_device_info_init_mmio(struct drm_i915_private *dev_priv)
+{
+    struct intel_device_info *info = mkwrite_device_info(dev_priv);
+    u8 vdbox_disable, vebox_disable;
+    u32 media_fuse;
+    int i;
+
+    if (INTEL_GEN(dev_priv) < 11)
+        return;
+
+    media_fuse = I915_READ(GEN11_GT_VEBOX_VDBOX_DISABLE);
+
+    vdbox_disable = media_fuse & GEN11_GT_VDBOX_DISABLE_MASK;
+    vebox_disable = (media_fuse & GEN11_GT_VEBOX_DISABLE_MASK) >>
+            GEN11_GT_VEBOX_DISABLE_SHIFT;
+
+    DRM_DEBUG_DRIVER("vdbox disable: %04x\n", vdbox_disable);
+    for (i = 0; i < I915_MAX_VCS; i++) {
+        if (!HAS_ENGINE(dev_priv, _VCS(i)))
+            continue;
+
+        if (!(BIT(i) & vdbox_disable))
+            continue;
+
+        info->ring_mask &= ~ENGINE_MASK(_VCS(i));
+        DRM_DEBUG_DRIVER("vcs%u fused off\n", i);
+    }
+
+    DRM_DEBUG_DRIVER("vebox disable: %04x\n", vebox_disable);
+    for (i = 0; i < I915_MAX_VECS; i++) {
+        if (!HAS_ENGINE(dev_priv, _VECS(i)))
+            continue;
+
+        if (!(BIT(i) & vebox_disable))
+            continue;
+
+        info->ring_mask &= ~ENGINE_MASK(_VECS(i));
+        DRM_DEBUG_DRIVER("vecs%u fused off\n", i);
+    }
+}
diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
index 4c6f83b..2233a2f 100644
--- a/drivers/gpu/drm/i915/intel_device_info.h
+++ b/drivers/gpu/drm/i915/intel_device_info.h
@@ -187,6 +187,7 @@ void intel_device_info_dump_flags(const struct intel_device_info *info,
                    struct drm_printer *p);
  void intel_device_info_dump_runtime(const struct intel_device_info *info,
                      struct drm_printer *p);
May be we need to add extra line to differentiate from "device_info_*_runtime_*" function declarations.

Can do.

+void intel_device_info_init_mmio(struct drm_i915_private *dev_priv);
    void intel_driver_caps_print(const struct intel_driver_caps *caps,
                   struct drm_printer *p);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 5ae9a62..5de0d26 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -56,6 +56,10 @@
  fw_domain_reset(struct drm_i915_private *i915,
          const struct intel_uncore_forcewake_domain *d)
  {
+    /*
+     * We don't really know if the powerwell for the forcewake domain we are +     * trying to reset here does exist at this point, so no waiting for acks
+     */
We should also add that this applies to ICL.

Can do.

      __raw_i915_write32(i915, d->reg_set, i915->uncore.fw_reset);
  }
  @@ -1251,6 +1255,23 @@ static void fw_domain_init(struct drm_i915_private *dev_priv,
      fw_domain_reset(dev_priv, d);
  }
  +static void fw_domain_fini(struct drm_i915_private *dev_priv,
+               enum forcewake_domain_id domain_id)
+{
+    struct intel_uncore_forcewake_domain *d;
+
+    if (WARN_ON(domain_id >= FW_DOMAIN_ID_COUNT))
+        return;
+
+    d = &dev_priv->uncore.fw_domain[domain_id];
+
+    WARN_ON(d->wake_count);
+    WARN_ON(hrtimer_cancel(&d->timer));
+    memset(d, 0, sizeof(*d));
+
+    dev_priv->uncore.fw_domains &= ~BIT(domain_id);
+}
+
  static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
  {
      if (INTEL_GEN(dev_priv) <= 5 || intel_vgpu_active(dev_priv))
@@ -1432,6 +1453,40 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
          &dev_priv->uncore.pmic_bus_access_nb);
  }
  +/*
+ * We might have detected that some engines are fused off after we initialized + * the forcewake domains. Prune them, to make sure they only reference existing
+ * engines.
+ */
+void intel_uncore_prune(struct drm_i915_private *dev_priv)
+{
+    if (INTEL_GEN(dev_priv) >= 11) {
+        enum forcewake_domains fw_domains = dev_priv->uncore.fw_domains;
+        enum forcewake_domain_id domain_id;
+        int i;
+
+        for (i = 0; i < I915_MAX_VCS; i++) {
+            domain_id = FW_DOMAIN_ID_MEDIA_VDBOX0 + i;
+
+            if (HAS_ENGINE(dev_priv, _VCS(i)))
+                continue;
+
+            if (fw_domains & BIT(domain_id))
fw_domains check seems redundant as it is initialized based on HAS_ENGINE.
we can just have
if (!HAS_ENGINE(dev_priv, _VCS(i)))
    fw_domain_fini(dev_priv, domain_id);

I don't want to call fw_domain_fini on something we never initialized in the first place (e.g. VCS1/3 and VECS1/2/3 on an ICL-LP).

+                fw_domain_fini(dev_priv, domain_id);
+        }
+
+        for (i = 0; i < I915_MAX_VECS; i++) {
+            domain_id = FW_DOMAIN_ID_MEDIA_VEBOX0 + i;
+
+            if (HAS_ENGINE(dev_priv, _VECS(i)))
+                continue;
+
+            if (fw_domains & BIT(domain_id))
+                fw_domain_fini(dev_priv, domain_id);
+        }
+    }
+}
+
  void intel_uncore_fini(struct drm_i915_private *dev_priv)
  {
      /* Paranoia: make sure we have disabled everything before we exit. */ diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
index 53ef77d..28feabf 100644
--- a/drivers/gpu/drm/i915/intel_uncore.h
+++ b/drivers/gpu/drm/i915/intel_uncore.h
@@ -129,6 +129,7 @@ struct intel_uncore {
    void intel_uncore_sanitize(struct drm_i915_private *dev_priv);
  void intel_uncore_init(struct drm_i915_private *dev_priv);
+void intel_uncore_prune(struct drm_i915_private *dev_priv);
  bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
  bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv);
  void intel_uncore_fini(struct drm_i915_private *dev_priv);


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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux