Re: [PATCH 1/7] drm/i915: Record GT workarounds in a list

Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> · Mon, 3 Dec 2018 12:34:24 +0000

On 03/12/2018 11:54, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-12-03 11:46:11)
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

To enable later verification of GT workaround state at various stages of
driver lifetime, we record the list of applicable ones per platforms to a
list, from which they are also applied.

The added data structure is a simple array of register, mask and value
items, which is allocated on demand as workarounds are added to the list.

This is a temporary implementation which later in the series gets fused
with the existing per context workaround list handling. It is separated at
this stage since the following patch fixes a bug which needs to be as easy
to backport as possible.

Also, since in the following patch we will be adding a new class of
workarounds (per engine) which can be applied from interrupt context, we
straight away make the provision for safe read-modify-write cycle.

v2:
  * Change dev_priv to i915 along the init path. (Chris Wilson)
  * API rename. (Chris Wilson)

v3:
  * Remove explicit list size tracking in favour of growing the allocation
    in power of two chunks. (Chris Wilson)

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> # v2
---
  drivers/gpu/drm/i915/i915_drv.c          |   1 +
  drivers/gpu/drm/i915/i915_drv.h          |   2 +
  drivers/gpu/drm/i915/i915_gem.c          |   4 +-
  drivers/gpu/drm/i915/intel_workarounds.c | 491 +++++++++++++++--------
  drivers/gpu/drm/i915/intel_workarounds.h |  23 +-
  5 files changed, 353 insertions(+), 168 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e39016713464..ee5116b62cd2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1453,6 +1453,7 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
  
         intel_uncore_sanitize(dev_priv);
  
+       intel_gt_init_workarounds(dev_priv);
         i915_gem_load_init_fences(dev_priv);
  
         /* On the 945G/GM, the chipset reports the MSI capability on the
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 43ac6873a2bb..9ddbcc1f3554 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -69,6 +69,7 @@
  #include "intel_ringbuffer.h"
  #include "intel_uncore.h"
  #include "intel_wopcm.h"
+#include "intel_workarounds.h"
  #include "intel_uc.h"
  
  #include "i915_gem.h"
@@ -1652,6 +1653,7 @@ struct drm_i915_private {
         int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
  
         struct i915_workarounds workarounds;
+       struct i915_wa_list gt_wa_list;
  
         struct i915_frontbuffer_tracking fb_tracking;
  
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c55b1f75c980..6333a7d6af5a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5334,7 +5334,7 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
                 I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ?
                            LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED);
  
-       intel_gt_workarounds_apply(dev_priv);
+       intel_gt_apply_workarounds(dev_priv);
  
         i915_gem_init_swizzling(dev_priv);
  
@@ -5706,6 +5706,8 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
         i915_gem_contexts_fini(dev_priv);
         mutex_unlock(&dev_priv->drm.struct_mutex);
  
+       intel_wa_list_free(&dev_priv->gt_wa_list);
+
         intel_cleanup_gt_powersave(dev_priv);
  
         intel_uc_fini_misc(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
index e5cd6c6c66c3..f05f13547034 100644
--- a/drivers/gpu/drm/i915/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/intel_workarounds.c
@@ -48,6 +48,18 @@
   * - Public functions to init or apply the given workaround type.
   */
  
+static void wa_init_start(struct i915_wa_list *wal, const char *name)
+{
+       wal->name = name;
+}
+
+static void wa_init_finish(struct i915_wa_list *wal)
+{
+       if (wal->count)
+               DRM_DEBUG_DRIVER("Initialized %u %s workarounds\n",
+                                wal->count, wal->name);

Does this become if (!wal->count) return; later?

Done.

+}
+
  static void wa_add(struct drm_i915_private *i915,
                    i915_reg_t reg, const u32 mask, const u32 val)
  {
@@ -575,160 +587,240 @@ int intel_ctx_workarounds_emit(struct i915_request *rq)
         return 0;
  }
  
-static void bdw_gt_workarounds_apply(struct drm_i915_private *dev_priv)
+static void
+wal_add(struct i915_wa_list *wal, const struct i915_wa *wa)
+{
+       const unsigned int grow = 1 << 4;
+
+       GEM_BUG_ON(!is_power_of_2(grow));
+
+       if (IS_ALIGNED(wal->count, grow)) { /* Either uninitialized or full. */

Neat.

+               struct i915_wa *list;
+
+               list = kcalloc(ALIGN(wal->count + 1, grow), sizeof(*wa),
+                              GFP_KERNEL);

(Quietly comments on the calloc here ;)

Oh I don't want to complicate things with zeroing the tail. Or you 
wouldn't bother with zeroing at all since I always copy over used 
entries? So unused, who cares about them?


+               if (!list) {
+                       DRM_ERROR("No space for workaround init!\n");
+                       return;
+               }
+
+               if (wal->list)
+                       memcpy(list, wal->list, sizeof(*wa) * wal->count);
+
+               wal->list = list;
+       }
+
+       memcpy(&wal->list[wal->count], wa, sizeof(*wa));
+       wal->count++;

I'd push for
	wal->list[wal->count++] = wa;
Might as well use static type checking.

Ok.

+struct i915_wa_list {
+       const char      *name;
+       unsigned int    count;
+       struct i915_wa  *list;

Oh well, didn't save anything after all.

How nothing, one unsigned int per wa_list instance! :)

Regards,

Tvrtko

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







