Skip to contentSkip to conversation

Restore workarounds after engine reset and unify their handling

PATCH Superseded Tvrtko Ursulin 3 participants 18 messages Dec 3, 2018

Subsystems (2)

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore
#0 lore raw

v3 cover Restore workarounds after engine reset and unify their handling

From: Tvrtko Ursulin <[email protected]>

First two patches in this series fix losing of workarounds after engine reset
(https://bugzilla.freedesktop.org/show_bug.cgi?id=107945) which started happening after 59b449d5c82a ("drm/i915: Split out functions for different kinds of workarounds").

But since it was discovered to be unsafe to simply re-apply all of them, against a possibly active GPU, and potentially from IRQ context, the approach taken was to split GT workarounds and per-engine workarounds. Latter so far contain the ones living in the 0x2xxx and 0xbxxx range, which were empirically shown to be lost after RCS reset.

This way only a smaller set of affected workarounds can be applied after engine resetm, which is done with irq safe read-modify-write cycle.

The series is structured like this so first two patches are as standalone as possible so it is easy (easier) to backport them. The rest of the series cleans up the whole workaround handling by moving all four classes of them to a common framework.

v2:
* One patch less due removing verification after engine reset.
* See patch change logs.

v3:
* Further review comments as per individual change logs.

Tvrtko Ursulin (7):
drm/i915: Record GT workarounds in a list
drm/i915: Introduce per-engine workarounds
drm/i915: Verify GT workaround state after GPU init drm/i915/selftests: Add tests for GT and engine workaround
verification
drm/i915: Move register white-listing to the common workaround
framework
drm/i915: Fuse per-context workaround handling with the common
framework
drm/i915: Trim unused workaround list entries

 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/i915_debugfs.c           |  12 +-
 drivers/gpu/drm/i915/i915_drv.c               |   2 +
 drivers/gpu/drm/i915/i915_drv.h               |  17 +-
 drivers/gpu/drm/i915/i915_gem.c               |   7 +-
 drivers/gpu/drm/i915/i915_gem_context.c       |   6 +-
 drivers/gpu/drm/i915/intel_engine_cs.c        |   4 +
 drivers/gpu/drm/i915/intel_lrc.c              |  11 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c       |   2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h       |   4 +
 drivers/gpu/drm/i915/intel_workarounds.c      | 987 +++++++++++-------
 drivers/gpu/drm/i915/intel_workarounds.h      |  36 +-
 drivers/gpu/drm/i915/selftests/igt_reset.c    |  44 +
 drivers/gpu/drm/i915/selftests/igt_reset.h    |  15 +
 .../gpu/drm/i915/selftests/intel_hangcheck.c  |  51 +-
 .../drm/i915/selftests/intel_workarounds.c    | 182 +++-
 16 files changed, 882 insertions(+), 499 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/selftests/igt_reset.c
 create mode 100644 drivers/gpu/drm/i915/selftests/igt_reset.h
Signature · 6 lines
2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#1 lore raw

PATCH v3 1/7 drm/i915: Record GT workarounds in a list

From: Tvrtko Ursulin <[email protected]>

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)

---
 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(-)
drivers/gpu/drm/i915/i915_drv.c +1 -0
index e39016713464..ee5116b62cd2 100644
@@ -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
drivers/gpu/drm/i915/i915_drv.h +2 -0
index 43ac6873a2bb..9ddbcc1f3554 100644
@@ -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;
 
drivers/gpu/drm/i915/i915_gem.c +3 -1
index c55b1f75c980..6333a7d6af5a 100644
@@ -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);
drivers/gpu/drm/i915/intel_workarounds.c +325 -166

655 lines not on this page · raw message

drivers/gpu/drm/i915/intel_workarounds.h +22 -1
index b11d0623e626..7c3adf794f52 100644
@@ -7,10 +7,31 @@
 #ifndef _I915_WORKAROUNDS_H_
 #define _I915_WORKAROUNDS_H_
 
+#include <linux/slab.h>
+
+struct i915_wa {
+	i915_reg_t	  reg;
+	u32		  mask;
+	u32		  val;
+};
+
+struct i915_wa_list {
+	const char	*name;
+	unsigned int 	count;
+	struct i915_wa	*list;
+};
+
+static inline void intel_wa_list_free(struct i915_wa_list *wal)
+{
+	kfree(wal->list);
+	memset(wal, 0, sizeof(*wal));
+}
+
 int intel_ctx_workarounds_init(struct drm_i915_private *dev_priv);
 int intel_ctx_workarounds_emit(struct i915_request *rq);
 
-void intel_gt_workarounds_apply(struct drm_i915_private *dev_priv);
+void intel_gt_init_workarounds(struct drm_i915_private *dev_priv);
+void intel_gt_apply_workarounds(struct drm_i915_private *dev_priv);
 
 void intel_whitelist_workarounds_apply(struct intel_engine_cs *engine);
 
Signature · 6 lines
2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#2 lore raw

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

Quoting Tvrtko Ursulin (2018-12-03 11:46:11)

Quoted 98 lines

From: Tvrtko Ursulin <[email protected]>

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)

---
 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(-)
drivers/gpu/drm/i915/i915_drv.c +1 -0
index e39016713464..ee5116b62cd2 100644
@@ -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
drivers/gpu/drm/i915/i915_drv.h +2 -0
index 43ac6873a2bb..9ddbcc1f3554 100644
@@ -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;
 
drivers/gpu/drm/i915/i915_gem.c +3 -1
index c55b1f75c980..6333a7d6af5a 100644
@@ -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);
drivers/gpu/drm/i915/intel_workarounds.c +0 -0
index e5cd6c6c66c3..f05f13547034 100644
@@ -48,6 +48,18 @@
  * - Public functions to init or apply the given workaround type.
  */
 

drivers/gpu/drm/i915/intel_workarounds.c line 60

Quoted 10 lines
drivers/gpu/drm/i915/intel_workarounds.c +10 -0
+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?

Quoted 8 lines
drivers/gpu/drm/i915/intel_workarounds.c +2 -0
+}
+
 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;
 }

drivers/gpu/drm/i915/intel_workarounds.c int intel_ctx_workarounds_emit(struct i915_request *rq) · line 597

Quoted 10 lines
drivers/gpu/drm/i915/intel_workarounds.c +8 -1
 
-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.

drivers/gpu/drm/i915/intel_workarounds.c int intel_ctx_workarounds_emit(struct i915_request *rq) · line 601

Quoted 4 lines
drivers/gpu/drm/i915/intel_workarounds.c +4 -0
+               struct i915_wa *list;
+
+               list = kcalloc(ALIGN(wal->count + 1, grow), sizeof(*wa),
+                              GFP_KERNEL);

(Quietly comments on the calloc here ;)

drivers/gpu/drm/i915/intel_workarounds.c int intel_ctx_workarounds_emit(struct i915_request *rq) · line 614

Quoted 13 lines
drivers/gpu/drm/i915/intel_workarounds.c +13 -0
+               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.

drivers/gpu/drm/i915/intel_workarounds.c int intel_ctx_workarounds_emit(struct i915_request *rq) · line 618

Quoted 4 lines
drivers/gpu/drm/i915/intel_workarounds.c +4 -0
+struct i915_wa_list {
+       const char      *name;
+       unsigned int    count;
+       struct i915_wa  *list;

Oh well, didn't save anything after all.
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#3 lore raw

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

Quoted 99 lines · Chris Wilson

Quoting Tvrtko Ursulin (2018-12-03 11:46:11)

Quoted 98 lines

From: Tvrtko Ursulin <[email protected]>

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)

---
  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(-)
drivers/gpu/drm/i915/i915_drv.c +1 -0
index e39016713464..ee5116b62cd2 100644
@@ -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
drivers/gpu/drm/i915/i915_drv.h +2 -0
index 43ac6873a2bb..9ddbcc1f3554 100644
@@ -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;
 
drivers/gpu/drm/i915/i915_gem.c +3 -1
index c55b1f75c980..6333a7d6af5a 100644
@@ -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);
drivers/gpu/drm/i915/intel_workarounds.c +0 -0
index e5cd6c6c66c3..f05f13547034 100644
@@ -48,6 +48,18 @@
   * - Public functions to init or apply the given workaround type.
   */
 
Quoted 12 lines

drivers/gpu/drm/i915/intel_workarounds.c line 60

Quoted 10 lines
drivers/gpu/drm/i915/intel_workarounds.c +10 -0
+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.

Quoted 21 lines
Quoted 8 lines
drivers/gpu/drm/i915/intel_workarounds.c +2 -0
+}
+
  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;
  }

drivers/gpu/drm/i915/intel_workarounds.c int intel_ctx_workarounds_emit(struct i915_request *rq) · line 597

Quoted 10 lines
drivers/gpu/drm/i915/intel_workarounds.c +8 -1
 
-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.

Quoted 6 lines

drivers/gpu/drm/i915/intel_workarounds.c int intel_ctx_workarounds_emit(struct i915_request *rq) · line 601

Quoted 4 lines
drivers/gpu/drm/i915/intel_workarounds.c +4 -0
+               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?

Quoted 16 lines
Quoted 4 lines
drivers/gpu/drm/i915/intel_workarounds.c +3 -0
+               if (!list) {
+                       DRM_ERROR("No space for workaround init!\n");
+                       return;

drivers/gpu/drm/i915/intel_workarounds.c int intel_ctx_workarounds_emit(struct i915_request *rq) · line 614

Quoted 10 lines
drivers/gpu/drm/i915/intel_workarounds.c +10 -0
+               }
+
+               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

Quoted 2 lines
	wal->list[wal->count++] = wa;

Might as well use static type checking.

Ok.

Quoted 6 lines

drivers/gpu/drm/i915/intel_workarounds.c int intel_ctx_workarounds_emit(struct i915_request *rq) · line 618

Quoted 4 lines
drivers/gpu/drm/i915/intel_workarounds.c +4 -0
+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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#4 lore raw

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

Quoting Tvrtko Ursulin (2018-12-03 12:34:24)

Quoted 25 lines · Chris Wilson
Quoted 23 lines

Quoting Tvrtko Ursulin (2018-12-03 11:46:11)

Quoted 4 lines

From: Tvrtko Ursulin <[email protected]>

drivers/gpu/drm/i915/intel_workarounds.c +0 -0
@@ -575,160 +587,240 @@ int intel_ctx_workarounds_emit(struct i915_request *rq)
         return 0;
  }

drivers/gpu/drm/i915/intel_workarounds.c int intel_ctx_workarounds_emit(struct i915_request *rq) · line 597

Quoted 10 lines
drivers/gpu/drm/i915/intel_workarounds.c +8 -1
 
-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.

Quoted 4 lines
drivers/gpu/drm/i915/intel_workarounds.c +4 -0
+               struct i915_wa *list;
+
+               list = kcalloc(ALIGN(wal->count + 1, grow), sizeof(*wa),
+                              GFP_KERNEL);
Quoted 5 lines
Quoted 1 line

(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?

Exactly, nothing after wal->count is accessed, so no need to zero it as it will always be initialised by wal_add().

Quoted 8 lines
Quoted 6 lines

drivers/gpu/drm/i915/intel_workarounds.h line 21

Quoted 4 lines
drivers/gpu/drm/i915/intel_workarounds.h +4 -0
+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! :)

struct is padded to 8 bytes for the pointer. There's a hole after count, even if we place it at the end. :|
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#5 lore raw

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

Quoted 26 lines · Chris Wilson

Quoting Tvrtko Ursulin (2018-12-03 12:34:24)

Quoted 25 lines · Chris Wilson
Quoted 23 lines

Quoting Tvrtko Ursulin (2018-12-03 11:46:11)

Quoted 4 lines

From: Tvrtko Ursulin <[email protected]>

drivers/gpu/drm/i915/intel_workarounds.c +0 -0
@@ -575,160 +587,240 @@ int intel_ctx_workarounds_emit(struct i915_request *rq)
          return 0;
   }

drivers/gpu/drm/i915/intel_workarounds.c int intel_ctx_workarounds_emit(struct i915_request *rq) · line 597

Quoted 10 lines
drivers/gpu/drm/i915/intel_workarounds.c +8 -1
 
-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.

Quoted 4 lines
drivers/gpu/drm/i915/intel_workarounds.c +4 -0
+               struct i915_wa *list;
+
+               list = kcalloc(ALIGN(wal->count + 1, grow), sizeof(*wa),
+                              GFP_KERNEL);
Quoted 8 lines
Quoted 5 lines
Quoted 1 line

(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?

Exactly, nothing after wal->count is accessed, so no need to zero it as it will always be initialised by wal_add().

Ok.

Quoted 11 lines
Quoted 8 lines
Quoted 6 lines

drivers/gpu/drm/i915/intel_workarounds.h line 21

Quoted 4 lines
drivers/gpu/drm/i915/intel_workarounds.h +4 -0
+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! :)

struct is padded to 8 bytes for the pointer. There's a hole after count, even if we place it at the end. :|

Another unsigned int gets in there later in the series - for the benefit of being able to log the exact, and not merged, number of workarounds. But it makes sense to move the count at the end anyway so I'll do that.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#6 lore raw

PATCH v3 2/7 drm/i915: Introduce per-engine workarounds

In reply to Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

We stopped re-applying the GT workarounds after engine reset since commit 59b449d5c82a ("drm/i915: Split out functions for different kinds of workarounds").

Issue with this is that some of the GT workarounds live in the MMIO space which gets lost during engine resets. So far the registers in 0x2xxx and 0xbxxx address range have been identified to be affected.

This losing of applied workarounds has obvious negative effects and can even lead to hard system hangs (see the linked Bugzilla).

Rather than just restoring this re-application, because we have also observed that it is not safe to just re-write all GT workarounds after engine resets (GPU might be live and weird hardware states can happen), we introduce a new class of per-engine workarounds and move only the affected GT workarounds over.

Using the framework introduced in the previous patch, we therefore after engine reset, re-apply only the workarounds living in the affected MMIO address ranges.

v2:
* Move Wa_1406609255:icl to engine workarounds as well.
* Rename API. (Chris Wilson)
* Drop redundant IS_KABYLAKE. (Chris Wilson)
* Re-order engine wa/ init so latest platforms are first. (Rodrigo Vivi)

Bugzilla: https://bugzilla.freedesktop.org/show_bug.cgi?id=107945

Fixes 59b449d5c82a ("drm/i915: Split out functions for different kinds of workarounds")
Cc Mika Kuoppala <[email protected]>
Cc Ville Syrjälä <[email protected]>
Cc Chris Wilson <[email protected]>
Cc Jani Nikula <[email protected]>
Cc Joonas Lahtinen <[email protected]>
Cc Rodrigo Vivi <[email protected]>
---
 drivers/gpu/drm/i915/intel_engine_cs.c   |   2 +
 drivers/gpu/drm/i915/intel_lrc.c         |   4 +
 drivers/gpu/drm/i915/intel_ringbuffer.h  |   2 +
 drivers/gpu/drm/i915/intel_workarounds.c | 261 ++++++++++++-----------
 drivers/gpu/drm/i915/intel_workarounds.h |   3 +
 5 files changed, 153 insertions(+), 119 deletions(-)
drivers/gpu/drm/i915/intel_engine_cs.c +2 -0
index 759c0fd58f8c..ef5d202e9d45 100644
@@ -723,6 +723,8 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 	__intel_context_unpin(i915->kernel_context, engine);
 
 	i915_timeline_fini(&engine->timeline);
+
+	intel_wa_list_free(&engine->wa_list);
 }
 
 u64 intel_engine_get_active_head(const struct intel_engine_cs *engine)
drivers/gpu/drm/i915/intel_lrc.c +4 -0
index 11f4e6148557..7a7787cbafee 100644
@@ -1617,6 +1617,8 @@ static bool unexpected_starting_state(struct intel_engine_cs *engine)
 
 static int gen8_init_common_ring(struct intel_engine_cs *engine)
 {
+	intel_engine_apply_workarounds(engine);
+
 	intel_mocs_init_engine(engine);
 
 	intel_engine_reset_breadcrumbs(engine);
@@ -2314,6 +2316,8 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
 			  ret);
 	}
 
+	intel_engine_init_workarounds(engine);
+
 	return 0;
 
 err_cleanup_common:
drivers/gpu/drm/i915/intel_ringbuffer.h +2 -0
index 8a2270b209b0..c5ff3d31cab7 100644
@@ -15,6 +15,7 @@
 #include "i915_selftest.h"
 #include "i915_timeline.h"
 #include "intel_gpu_commands.h"
+#include "intel_workarounds.h"
 
 struct drm_printer;
 struct i915_sched_attr;
@@ -451,6 +452,7 @@ struct intel_engine_cs {
 
 	struct intel_hw_status_page status_page;
 	struct i915_ctx_workarounds wa_ctx;
+	struct i915_wa_list wa_list;
 	struct i915_vma *scratch;
 
 	u32             irq_keep_mask; /* always keep these interrupts */
drivers/gpu/drm/i915/intel_workarounds.c +142 -119

344 lines not on this page · raw message

drivers/gpu/drm/i915/intel_workarounds.h +3 -0
index 7c3adf794f52..f960bd9ce5e9 100644
@@ -35,4 +35,7 @@ void intel_gt_apply_workarounds(struct drm_i915_private *dev_priv);
 
 void intel_whitelist_workarounds_apply(struct intel_engine_cs *engine);
 
+void intel_engine_init_workarounds(struct intel_engine_cs *engine);
+void intel_engine_apply_workarounds(struct intel_engine_cs *engine);
+
 #endif
Signature · 6 lines
2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#7 lore raw

PATCH v3 3/7 drm/i915: Verify GT workaround state after GPU init

In reply to Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

Since we now have all the GT workarounds in a table, by adding a simple shared helper function we can now verify that their values are still applied after some interesting events in the lifetime of the driver.

Initially we only do this after GPU initialization.

v2:
Chris Wilson:
* Simplify verification by realizing it's a simple xor and and.
* Remove verification from engine reset path.
* Return bool straight away from the verify API.

v3:
* API rename. (Chris Wilson)

---
 drivers/gpu/drm/i915/i915_drv.c          |  1 +
 drivers/gpu/drm/i915/i915_gem.c          |  3 +++
 drivers/gpu/drm/i915/intel_workarounds.c | 34 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_workarounds.h |  2 ++
 4 files changed, 40 insertions(+)
drivers/gpu/drm/i915/i915_drv.c +1 -0
index ee5116b62cd2..1ab6ba5771a9 100644
@@ -53,6 +53,7 @@
 #include "i915_vgpu.h"
 #include "intel_drv.h"
 #include "intel_uc.h"
+#include "intel_workarounds.h"
 
 static struct drm_driver driver;
 
drivers/gpu/drm/i915/i915_gem.c +3 -0
index 6333a7d6af5a..298f79c137eb 100644
@@ -5334,7 +5334,10 @@ 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);
 
+	/* Apply the GT workarounds... */
 	intel_gt_apply_workarounds(dev_priv);
+	/* ...and determine whether they are sticking. */
+	intel_gt_verify_workarounds(dev_priv, "init");
 
 	i915_gem_init_swizzling(dev_priv);
 
drivers/gpu/drm/i915/intel_workarounds.c +34 -0
index d5d5ca7f8f17..161ac61058a8 100644
@@ -976,6 +976,40 @@ void intel_gt_apply_workarounds(struct drm_i915_private *dev_priv)
 	wa_list_apply(dev_priv, &dev_priv->gt_wa_list);
 }
 
+static bool
+wa_verify(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
+{
+	if ((cur ^ wa->val) & wa->mask) {
+		DRM_ERROR("%s workaround lost on %s! (%x=%x/%x, expected %x, mask=%x)\n",
+			  name, from, i915_mmio_reg_offset(wa->reg), cur,
+			  cur & wa->mask, wa->val, wa->mask);
+
+		return false;
+	}
+
+	return true;
+}
+
+static bool wa_list_verify(struct drm_i915_private *dev_priv,
+			   const struct i915_wa_list *wal,
+			   const char *from)
+{
+	struct i915_wa *wa;
+	unsigned int i;
+	bool ok = true;
+
+	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
+		ok &= wa_verify(wa, I915_READ(wa->reg), wal->name, from);
+
+	return ok;
+}
+
+bool intel_gt_verify_workarounds(struct drm_i915_private *dev_priv,
+				 const char *from)
+{
+	return wa_list_verify(dev_priv, &dev_priv->gt_wa_list, from);
+}
+
 struct whitelist {
 	i915_reg_t reg[RING_MAX_NONPRIV_SLOTS];
 	unsigned int count;
drivers/gpu/drm/i915/intel_workarounds.h +2 -0
index f960bd9ce5e9..2506db0380f1 100644
@@ -32,6 +32,8 @@ int intel_ctx_workarounds_emit(struct i915_request *rq);
 
 void intel_gt_init_workarounds(struct drm_i915_private *dev_priv);
 void intel_gt_apply_workarounds(struct drm_i915_private *dev_priv);
+bool intel_gt_verify_workarounds(struct drm_i915_private *dev_priv,
+				 const char *from);
 
 void intel_whitelist_workarounds_apply(struct intel_engine_cs *engine);
 
Signature · 6 lines
2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#8 lore raw

PATCH v3 4/7 drm/i915/selftests: Add tests for GT and engine workaround verification

In reply to Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

Two simple selftests which test that both GT and engine workarounds are not lost after either a full GPU reset, or after the per-engine ones.

(Including checks that one engine reset is not affecting workarounds not belonging to itself.)

v2:
* Rebase for series refactoring.
* Add spinner for actual engine reset!
* Add idle reset test as well. (Chris Wilson)
* Share existing global_reset_lock. (Chris Wilson)

v3:
* intel_engine_verify_workarounds can be static.
* API rename. (Chris Wilson)
* Move global reset lock out of the loop. (Chris Wilson)

---
 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/intel_workarounds.c      |   6 +
 drivers/gpu/drm/i915/selftests/igt_reset.c    |  44 ++++++
 drivers/gpu/drm/i915/selftests/igt_reset.h    |  15 ++
 .../gpu/drm/i915/selftests/intel_hangcheck.c  |  51 ++-----
 .../drm/i915/selftests/intel_workarounds.c    | 142 +++++++++++++++++-
 6 files changed, 212 insertions(+), 47 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/selftests/igt_reset.c
 create mode 100644 drivers/gpu/drm/i915/selftests/igt_reset.h
drivers/gpu/drm/i915/Makefile +1 -0
index 50a8fa8fce64..19b5fe5016bf 100644
@@ -166,6 +166,7 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \
 	selftests/i915_random.o \
 	selftests/i915_selftest.o \
 	selftests/igt_flush_test.o \
+	selftests/igt_reset.o \
 	selftests/igt_spinner.o
 
 # virtual gpu code
drivers/gpu/drm/i915/intel_workarounds.c +6 -0
index 161ac61058a8..e7b0c6b98d94 100644
@@ -1302,5 +1302,11 @@ void intel_engine_apply_workarounds(struct intel_engine_cs *engine)
 }
 
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
+static bool intel_engine_verify_workarounds(struct intel_engine_cs *engine,
+					    const char *from)
+{
+	return wa_list_verify(engine->i915, &engine->wa_list, from);
+}
+
 #include "selftests/intel_workarounds.c"
 #endif
drivers/gpu/drm/i915/selftests/igt_reset.c new file +44 -0

44 lines not on this page · raw message

drivers/gpu/drm/i915/selftests/igt_reset.h new file +15 -0
new file mode 100644
index 000000000000..5f0234d045d5
@@ -0,0 +1,15 @@
+/*
+ * SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2018 Intel Corporation
+ */
+
+#ifndef __I915_SELFTESTS_IGT_RESET_H__
+#define __I915_SELFTESTS_IGT_RESET_H__
+
+#include "../i915_drv.h"
+
+void igt_global_reset_lock(struct drm_i915_private *i915);
+void igt_global_reset_unlock(struct drm_i915_private *i915);
+
+#endif
drivers/gpu/drm/i915/selftests/intel_hangcheck.c +9 -42

111 lines not on this page · raw message

drivers/gpu/drm/i915/selftests/intel_workarounds.c +137 -5

173 lines not on this page · raw message

Signature · 6 lines
2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#9 lore raw

Re: [PATCH 4/7] drm/i915/selftests: Add tests for GT and engine workaround verification

Quoting Tvrtko Ursulin (2018-12-03 11:46:14)

Quoted 3 lines
drivers/gpu/drm/i915/selftests/intel_workarounds.c +3 -0
+static int
+live_engine_reset_gt_engine_workarounds(void *arg)
+{

...

Quoted 6 lines
drivers/gpu/drm/i915/selftests/intel_workarounds.c +6 -0
+               ok = verify_gt_engine_wa(i915, "after idle reset");
+               if (!ok) {
+                       ret = -ESRCH;
+                       goto err;
+               }
+

drivers/gpu/drm/i915/selftests/intel_workarounds.c static int live_reset_whitelist(void *arg) · line 424

Quoted 10 lines
drivers/gpu/drm/i915/selftests/intel_workarounds.c +10 -0
+               ret = igt_spinner_init(&spin, i915);
+               if (ret)
+                       goto err;
+
+               intel_runtime_pm_get(i915);
+
+               rq = igt_spinner_create_request(&spin, ctx, engine, MI_NOOP);
+               if (IS_ERR(rq)) {
+                       ret = PTR_ERR(rq);
+                       igt_spinner_fini(&spin);

Missing rpm_put

drivers/gpu/drm/i915/selftests/intel_workarounds.c static int live_reset_whitelist(void *arg) · line 433

Quoted 9 lines
drivers/gpu/drm/i915/selftests/intel_workarounds.c +9 -0
+                       goto err;
+               }
+
+               i915_request_add(rq);
+
+               if (!igt_wait_for_spinner(&spin, rq)) {
+                       pr_err("Spinner failed to start\n");
+                       igt_spinner_fini(&spin);
+                       ret = -ETIMEDOUT;

Missing rpm_put

I need to convince people to take the wakeref tracking to make these leaks more obvious :)
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#10 lore raw

PATCH v3 5/7 drm/i915: Move register white-listing to the common workaround framework

In reply to Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

Instead of having a separate list of white-listed registers we can trivially move this to the common workarounds framework.

This brings us one step closer to the goal of driving all workaround classes using the same code.

v2:
* Use GEM_DEBUG_WARN_ON for the sanity check. (Chris Wilson)

v3:
* API rename. (Chris Wilson)

---
 drivers/gpu/drm/i915/intel_engine_cs.c        |  1 +
 drivers/gpu/drm/i915/intel_lrc.c              |  5 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h       |  1 +
 drivers/gpu/drm/i915/intel_workarounds.c      | 83 ++++++++-----------
 drivers/gpu/drm/i915/intel_workarounds.h      |  3 +-
 .../drm/i915/selftests/intel_workarounds.c    | 40 ++++-----
 6 files changed, 60 insertions(+), 73 deletions(-)
drivers/gpu/drm/i915/intel_engine_cs.c +1 -0
index ef5d202e9d45..496462d77ebc 100644
@@ -725,6 +725,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 	i915_timeline_fini(&engine->timeline);
 
 	intel_wa_list_free(&engine->wa_list);
+	intel_wa_list_free(&engine->whitelist);
 }
 
 u64 intel_engine_get_active_head(const struct intel_engine_cs *engine)
drivers/gpu/drm/i915/intel_lrc.c +3 -2
index 7a7787cbafee..af61a3cf9cb8 100644
@@ -1643,7 +1643,7 @@ static int gen8_init_render_ring(struct intel_engine_cs *engine)
 	if (ret)
 		return ret;
 
-	intel_whitelist_workarounds_apply(engine);
+	intel_engine_apply_whitelist(engine);
 
 	/* We need to disable the AsyncFlip performance optimisations in order
 	 * to use MI_WAIT_FOR_EVENT within the CS. It should already be
@@ -1666,7 +1666,7 @@ static int gen9_init_render_ring(struct intel_engine_cs *engine)
 	if (ret)
 		return ret;
 
-	intel_whitelist_workarounds_apply(engine);
+	intel_engine_apply_whitelist(engine);
 
 	return 0;
 }
@@ -2316,6 +2316,7 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
 			  ret);
 	}
 
+	intel_engine_init_whitelist(engine);
 	intel_engine_init_workarounds(engine);
 
 	return 0;
drivers/gpu/drm/i915/intel_ringbuffer.h +1 -0
index c5ff3d31cab7..91a750e90dc4 100644
@@ -453,6 +453,7 @@ struct intel_engine_cs {
 	struct intel_hw_status_page status_page;
 	struct i915_ctx_workarounds wa_ctx;
 	struct i915_wa_list wa_list;
+	struct i915_wa_list whitelist;
 	struct i915_vma *scratch;
 
 	u32             irq_keep_mask; /* always keep these interrupts */
drivers/gpu/drm/i915/intel_workarounds.c +33 -50

165 lines not on this page · raw message

drivers/gpu/drm/i915/intel_workarounds.h +2 -1
index 2506db0380f1..bc6aa503f6c6 100644
@@ -35,7 +35,8 @@ void intel_gt_apply_workarounds(struct drm_i915_private *dev_priv);
 bool intel_gt_verify_workarounds(struct drm_i915_private *dev_priv,
 				 const char *from);
 
-void intel_whitelist_workarounds_apply(struct intel_engine_cs *engine);
+void intel_engine_init_whitelist(struct intel_engine_cs *engine);
+void intel_engine_apply_whitelist(struct intel_engine_cs *engine);
 
 void intel_engine_init_workarounds(struct intel_engine_cs *engine);
 void intel_engine_apply_workarounds(struct intel_engine_cs *engine);
drivers/gpu/drm/i915/selftests/intel_workarounds.c +20 -20
index 0c118d18f825..7b37bbce2308 100644
@@ -94,17 +94,23 @@ read_nonprivs(struct i915_gem_context *ctx, struct intel_engine_cs *engine)
 	return ERR_PTR(err);
 }
 
-static u32 get_whitelist_reg(const struct whitelist *w, unsigned int i)
+static u32
+get_whitelist_reg(const struct intel_engine_cs *engine, unsigned int i)
 {
-	return i < w->count ? i915_mmio_reg_offset(w->reg[i]) : w->nopid;
+	i915_reg_t reg = i < engine->whitelist.count ?
+			 engine->whitelist.list[i].reg :
+			 RING_NOPID(engine->mmio_base);
+
+	return i915_mmio_reg_offset(reg);
 }
 
-static void print_results(const struct whitelist *w, const u32 *results)
+static void
+print_results(const struct intel_engine_cs *engine, const u32 *results)
 {
 	unsigned int i;
 
 	for (i = 0; i < RING_MAX_NONPRIV_SLOTS; i++) {
-		u32 expected = get_whitelist_reg(w, i);
+		u32 expected = get_whitelist_reg(engine, i);
 		u32 actual = results[i];
 
 		pr_info("RING_NONPRIV[%d]: expected 0x%08x, found 0x%08x\n",
@@ -112,8 +118,7 @@ static void print_results(const struct whitelist *w, const u32 *results)
 	}
 }
 
-static int check_whitelist(const struct whitelist *w,
-			   struct i915_gem_context *ctx,
+static int check_whitelist(struct i915_gem_context *ctx,
 			   struct intel_engine_cs *engine)
 {
 	struct drm_i915_gem_object *results;
@@ -141,11 +146,11 @@ static int check_whitelist(const struct whitelist *w,
 	}
 
 	for (i = 0; i < RING_MAX_NONPRIV_SLOTS; i++) {
-		u32 expected = get_whitelist_reg(w, i);
+		u32 expected = get_whitelist_reg(engine, i);
 		u32 actual = vaddr[i];
 
 		if (expected != actual) {
-			print_results(w, vaddr);
+			print_results(engine, vaddr);
 			pr_err("Invalid RING_NONPRIV[%d], expected 0x%08x, found 0x%08x\n",
 			       i, expected, actual);
 
@@ -217,7 +222,6 @@ switch_to_scratch_context(struct intel_engine_cs *engine,
 
 static int check_whitelist_across_reset(struct intel_engine_cs *engine,
 					int (*reset)(struct intel_engine_cs *),
-					const struct whitelist *w,
 					const char *name)
 {
 	struct drm_i915_private *i915 = engine->i915;
@@ -227,7 +231,7 @@ static int check_whitelist_across_reset(struct intel_engine_cs *engine,
 	int err;
 
 	pr_info("Checking %d whitelisted registers (RING_NONPRIV) [%s]\n",
-		w->count, name);
+		engine->whitelist.count, name);
 
 	if (want_spin) {
 		err = igt_spinner_init(&spin, i915);
@@ -239,7 +243,7 @@ static int check_whitelist_across_reset(struct intel_engine_cs *engine,
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 
-	err = check_whitelist(w, ctx, engine);
+	err = check_whitelist(ctx, engine);
 	if (err) {
 		pr_err("Invalid whitelist *before* %s reset!\n", name);
 		goto out;
@@ -263,7 +267,7 @@ static int check_whitelist_across_reset(struct intel_engine_cs *engine,
 		goto out;
 	}
 
-	err = check_whitelist(w, ctx, engine);
+	err = check_whitelist(ctx, engine);
 	if (err) {
 		pr_err("Whitelist not preserved in context across %s reset!\n",
 		       name);
@@ -276,7 +280,7 @@ static int check_whitelist_across_reset(struct intel_engine_cs *engine,
 	if (IS_ERR(ctx))
 		return PTR_ERR(ctx);
 
-	err = check_whitelist(w, ctx, engine);
+	err = check_whitelist(ctx, engine);
 	if (err) {
 		pr_err("Invalid whitelist *after* %s reset in fresh context!\n",
 		       name);
@@ -292,22 +296,18 @@ static int live_reset_whitelist(void *arg)
 {
 	struct drm_i915_private *i915 = arg;
 	struct intel_engine_cs *engine = i915->engine[RCS];
-	struct whitelist w;
 	int err = 0;
 
 	/* If we reset the gpu, we should not lose the RING_NONPRIV */
 
-	if (!engine)
-		return 0;
-
-	if (!whitelist_build(engine, &w))
+	if (!engine || engine->whitelist.count == 0)
 		return 0;
 
 	igt_global_reset_lock(i915);
 
 	if (intel_has_reset_engine(i915)) {
 		err = check_whitelist_across_reset(engine,
-						   do_engine_reset, &w,
+						   do_engine_reset,
 						   "engine");
 		if (err)
 			goto out;
@@ -315,7 +315,7 @@ static int live_reset_whitelist(void *arg)
 
 	if (intel_has_gpu_reset(i915)) {
 		err = check_whitelist_across_reset(engine,
-						   do_device_reset, &w,
+						   do_device_reset,
 						   "device");
 		if (err)
 			goto out;
Signature · 6 lines
2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#11 lore raw

PATCH v3 6/7 drm/i915: Fuse per-context workaround handling with the common framework

In reply to Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

Convert the per context workaround handling code to run against the newly introduced common workaround framework and fuse the two to use the existing smarter list add helper, the one which does the sorted insert and merges registers where possible.

This completes migration of all four classes of workarounds onto the common framework.

Existing macros are kept untouched for smaller code churn.

v2:
* Rename to list name ctx_wa_list and move from dev_priv to engine.

v3:
* API rename and parameters tweaking. (Chris Wilson)

---
 drivers/gpu/drm/i915/i915_debugfs.c      |  12 +-
 drivers/gpu/drm/i915/i915_drv.h          |  15 -
 drivers/gpu/drm/i915/i915_gem_context.c  |   6 +-
 drivers/gpu/drm/i915/intel_engine_cs.c   |   1 +
 drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c  |   2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h  |   1 +
 drivers/gpu/drm/i915/intel_workarounds.c | 334 +++++++++++------------
 drivers/gpu/drm/i915/intel_workarounds.h |   5 +-
 9 files changed, 168 insertions(+), 210 deletions(-)
drivers/gpu/drm/i915/i915_debugfs.c +7 -5
index 129b9a6f8309..38dcee1ca062 100644
@@ -3375,13 +3375,15 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
 
 static int i915_wa_registers(struct seq_file *m, void *unused)
 {
-	struct i915_workarounds *wa = &node_to_i915(m->private)->workarounds;
-	int i;
+	struct drm_i915_private *i915 = node_to_i915(m->private);
+	const struct i915_wa_list *wal = &i915->engine[RCS]->ctx_wa_list;
+	struct i915_wa *wa;
+	unsigned int i;
 
-	seq_printf(m, "Workarounds applied: %d\n", wa->count);
-	for (i = 0; i < wa->count; ++i)
+	seq_printf(m, "Workarounds applied: %u\n", wal->count);
+	for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
 		seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n",
-			   wa->reg[i].addr, wa->reg[i].value, wa->reg[i].mask);
+			   i915_mmio_reg_offset(wa->reg), wa->val, wa->mask);
 
 	return 0;
 }
drivers/gpu/drm/i915/i915_drv.h +0 -15
index 9ddbcc1f3554..443e08f4736a 100644
@@ -1189,20 +1189,6 @@ struct i915_frontbuffer_tracking {
 	unsigned flip_bits;
 };
 
-struct i915_wa_reg {
-	u32 addr;
-	u32 value;
-	/* bitmask representing WA bits */
-	u32 mask;
-};
-
-#define I915_MAX_WA_REGS 16
-
-struct i915_workarounds {
-	struct i915_wa_reg reg[I915_MAX_WA_REGS];
-	u32 count;
-};
-
 struct i915_virtual_gpu {
 	bool active;
 	u32 caps;
@@ -1652,7 +1638,6 @@ 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;
drivers/gpu/drm/i915/i915_gem_context.c +1 -5
index b97963db0287..371c07087095 100644
@@ -535,16 +535,12 @@ static bool needs_preempt_context(struct drm_i915_private *i915)
 int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
 {
 	struct i915_gem_context *ctx;
-	int ret;
 
 	/* Reassure ourselves we are only called once */
 	GEM_BUG_ON(dev_priv->kernel_context);
 	GEM_BUG_ON(dev_priv->preempt_context);
 
-	ret = intel_ctx_workarounds_init(dev_priv);
-	if (ret)
-		return ret;
-
+	intel_engine_init_ctx_wa(dev_priv->engine[RCS]);
 	init_contexts(dev_priv);
 
 	/* lowest priority; idle task */
drivers/gpu/drm/i915/intel_engine_cs.c +1 -0
index 496462d77ebc..6b427bc52f78 100644
@@ -724,6 +724,7 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 
 	i915_timeline_fini(&engine->timeline);
 
+	intel_wa_list_free(&engine->ctx_wa_list);
 	intel_wa_list_free(&engine->wa_list);
 	intel_wa_list_free(&engine->whitelist);
 }
drivers/gpu/drm/i915/intel_lrc.c +1 -1
index af61a3cf9cb8..7de7aa4715fc 100644
@@ -2078,7 +2078,7 @@ static int gen8_init_rcs_context(struct i915_request *rq)
 {
 	int ret;
 
-	ret = intel_ctx_workarounds_emit(rq);
+	ret = intel_engine_emit_ctx_wa(rq);
 	if (ret)
 		return ret;
 
drivers/gpu/drm/i915/intel_ringbuffer.c +1 -1
index 28ae1e436ea6..026bda2b6df9 100644
@@ -649,7 +649,7 @@ static int intel_rcs_ctx_init(struct i915_request *rq)
 {
 	int ret;
 
-	ret = intel_ctx_workarounds_emit(rq);
+	ret = intel_engine_emit_ctx_wa(rq);
 	if (ret != 0)
 		return ret;
 
drivers/gpu/drm/i915/intel_ringbuffer.h +1 -0
index 91a750e90dc4..8f985c35ec92 100644
@@ -452,6 +452,7 @@ struct intel_engine_cs {
 
 	struct intel_hw_status_page status_page;
 	struct i915_ctx_workarounds wa_ctx;
+	struct i915_wa_list ctx_wa_list;
 	struct i915_wa_list wa_list;
 	struct i915_wa_list whitelist;
 	struct i915_vma *scratch;
drivers/gpu/drm/i915/intel_workarounds.c +153 -181

571 lines not on this page · raw message

drivers/gpu/drm/i915/intel_workarounds.h +3 -2
index bc6aa503f6c6..65104a0bf7a4 100644
@@ -18,6 +18,7 @@ struct i915_wa {
 struct i915_wa_list {
 	const char	*name;
 	unsigned int 	count;
+	unsigned int	wa_count;
 	struct i915_wa	*list;
 };
 
@@ -27,8 +28,8 @@ static inline void intel_wa_list_free(struct i915_wa_list *wal)
 	memset(wal, 0, sizeof(*wal));
 }
 
-int intel_ctx_workarounds_init(struct drm_i915_private *dev_priv);
-int intel_ctx_workarounds_emit(struct i915_request *rq);
+void intel_engine_init_ctx_wa(struct intel_engine_cs *engine);
+int intel_engine_emit_ctx_wa(struct i915_request *rq);
 
 void intel_gt_init_workarounds(struct drm_i915_private *dev_priv);
 void intel_gt_apply_workarounds(struct drm_i915_private *dev_priv);
Signature · 6 lines
2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Reviewed-by on v3 6/7
#12 lore raw

Re: [PATCH 6/7] drm/i915: Fuse per-context workaround handling with the common framework

Quoting Tvrtko Ursulin (2018-12-03 11:46:16)

Quoted 36 lines

From: Tvrtko Ursulin <[email protected]>

Convert the per context workaround handling code to run against the newly introduced common workaround framework and fuse the two to use the existing smarter list add helper, the one which does the sorted insert and merges registers where possible.

This completes migration of all four classes of workarounds onto the common framework.

Existing macros are kept untouched for smaller code churn.

v2:
* Rename to list name ctx_wa_list and move from dev_priv to engine.

v3:
* API rename and parameters tweaking. (Chris Wilson)

---
 drivers/gpu/drm/i915/i915_debugfs.c      |  12 +-
 drivers/gpu/drm/i915/i915_drv.h          |  15 -
 drivers/gpu/drm/i915/i915_gem_context.c  |   6 +-
 drivers/gpu/drm/i915/intel_engine_cs.c   |   1 +
 drivers/gpu/drm/i915/intel_lrc.c         |   2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.c  |   2 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h  |   1 +
 drivers/gpu/drm/i915/intel_workarounds.c | 334 +++++++++++------------
 drivers/gpu/drm/i915/intel_workarounds.h |   5 +-
 9 files changed, 168 insertions(+), 210 deletions(-)
drivers/gpu/drm/i915/i915_debugfs.c +0 -0
index 129b9a6f8309..38dcee1ca062 100644

drivers/gpu/drm/i915/i915_debugfs.c static int i915_shared_dplls_info(struct seq_file *m, void *unused) · line 3381

Quoted 9 lines
drivers/gpu/drm/i915/i915_debugfs.c +4 -2
@@ -3375,13 +3375,15 @@ static int i915_shared_dplls_info(struct seq_file *m, void *unused)
 
 static int i915_wa_registers(struct seq_file *m, void *unused)
 {
-       struct i915_workarounds *wa = &node_to_i915(m->private)->workarounds;
-       int i;
+       struct drm_i915_private *i915 = node_to_i915(m->private);
+       const struct i915_wa_list *wal = &i915->engine[RCS]->ctx_wa_list;
+       struct i915_wa *wa;
+       unsigned int i;

You keep telling me to be defensive around the existence of RCS :)

drivers/gpu/drm/i915/intel_workarounds.c

Quoted 15 lines
drivers/gpu/drm/i915/i915_debugfs.c +3 -3
-       seq_printf(m, "Workarounds applied: %d\n", wa->count);
-       for (i = 0; i < wa->count; ++i)
+       seq_printf(m, "Workarounds applied: %u\n", wal->count);
+       for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
                seq_printf(m, "0x%X: 0x%08X, mask: 0x%08X\n",
-                          wa->reg[i].addr, wa->reg[i].value, wa->reg[i].mask);
+                          i915_mmio_reg_offset(wa->reg), wa->val, wa->mask);
 
        return 0;
 }
drivers/gpu/drm/i915/intel_workarounds.c +0 -0
index 0350513bf7f7..ee220a031741 100644

Lgtm,

-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#13 lore raw

PATCH v3 7/7 drm/i915: Trim unused workaround list entries

In reply to Tvrtko Ursulin

From: Tvrtko Ursulin <[email protected]>

The new workaround list allocator grows the list in chunks so will end up with some unused space. Trim it when the initialization phase is done to free up a tiny bit of slab.

v2:
* Simplify with kmemdup. (Chris Wilson)

v3:
* Refactor for __size removal.

---
 drivers/gpu/drm/i915/intel_workarounds.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_workarounds.c +15 -1
index ee220a031741..0c6da9e6d370 100644
@@ -53,8 +53,22 @@ static void wa_init_start(struct i915_wa_list *wal, const char *name)
 	wal->name = name;
 }
 
+#define WA_LIST_CHUNK (1 << 4)
+
 static void wa_init_finish(struct i915_wa_list *wal)
 {
+	/* Trim unused entries. */
+	if (!IS_ALIGNED(wal->count, WA_LIST_CHUNK)) {
+		struct i915_wa *list = kmemdup(wal->list,
+					       wal->count * sizeof(*list),
+					       GFP_KERNEL);
+
+		if (list) {
+			kfree(wal->list);
+			wal->list = list;
+		}
+	}
+
 	if (wal->count)
 		DRM_DEBUG_DRIVER("Initialized %u %s workarounds\n",
 				 wal->wa_count, wal->name);
@@ -64,7 +78,7 @@ static void _wa_add(struct i915_wa_list *wal, const struct i915_wa *wa)
 {
 	unsigned int addr = i915_mmio_reg_offset(wa->reg);
 	unsigned int start = 0, end = wal->count;
-	const unsigned int grow = 1 << 4;
+	const unsigned int grow = WA_LIST_CHUNK;
 	struct i915_wa *wa_;
 
 	GEM_BUG_ON(!is_power_of_2(grow));
Signature · 6 lines
2.19.1

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#14 lore raw

Re: [PATCH 7/7] drm/i915: Trim unused workaround list entries

Quoting Tvrtko Ursulin (2018-12-03 11:46:17)

Quoted 6 lines

From: Tvrtko Ursulin <[email protected]>

The new workaround list allocator grows the list in chunks so will end up with some unused space. Trim it when the initialization phase is done to free up a tiny bit of slab.

Quoted 8 lines

v2:
* Simplify with kmemdup. (Chris Wilson)

v3:
* Refactor for __size removal.

I'd jiggle the if (!wal->count) return around, r-b will remain. -Chris
_______________________________________________
Intel-gfx mailing list [email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#15 lore raw

✗ Fi.CI.CHECKPATCH: warning for Restore workarounds after engine reset and unify their handling (rev3)

In reply to Tvrtko Ursulin
== Series Details ==

== Series Details ==

Series: Restore workarounds after engine reset and unify their handling (rev3)
URL : https://patchwork.freedesktop.org/series/53313/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
ed91418aaeec drm/i915: Record GT workarounds in a list
-:492: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#492: FILE: drivers/gpu/drm/i915/intel_workarounds.c:914:
+	wa_masked_en(wal,
+		    _3D_CHICKEN3,

-:768: WARNING:SPACE_BEFORE_TAB: please, no space before tabs #768: FILE: drivers/gpu/drm/i915/intel_workarounds.h:20:
+^Iunsigned int ^Icount;$

total: 0 errors, 1 warnings, 1 checks, 724 lines checked
ccb6a2c1f539 drm/i915: Introduce per-engine workarounds
-:10: ERROR:GIT_COMMIT_ID: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 59b449d5c82a ("drm/i915: Split out functions for different kinds of workarounds")' #10:
59b449d5c82a ("drm/i915: Split out functions for different kinds of

total: 1 errors, 0 warnings, 0 checks, 389 lines checked
8d055232d8d8 drm/i915: Verify GT workaround state after GPU init
e779d6bfdfef drm/i915/selftests: Add tests for GT and engine workaround verification -:56: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating? #56:
new file mode 100644

-:61: WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1 #61: FILE: drivers/gpu/drm/i915/selftests/igt_reset.c:1:
+/*

-:111: WARNING:SPDX_LICENSE_TAG: Missing or malformed SPDX-License-Identifier tag in line 1 #111: FILE: drivers/gpu/drm/i915/selftests/igt_reset.h:1:
+/*

total: 0 errors, 3 warnings, 0 checks, 361 lines checked
d695d6e32eb8 drm/i915: Move register white-listing to the common workaround framework ea620326b3cb drm/i915: Fuse per-context workaround handling with the common framework dc515637f6ec drm/i915: Trim unused workaround list entries

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#16 lore raw

✗ Fi.CI.SPARSE: warning for Restore workarounds after engine reset and unify their handling (rev3)

In reply to Tvrtko Ursulin
== Series Details ==

== Series Details ==

Series: Restore workarounds after engine reset and unify their handling (rev3)
URL : https://patchwork.freedesktop.org/series/53313/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.5.2
Commit: drm/i915: Record GT workarounds in a list
-drivers/gpu/drm/i915/selftests/../i915_drv.h:3571:16: warning: expression using sizeof(void)
+drivers/gpu/drm/i915/selftests/../i915_drv.h:3573:16: warning: expression using sizeof(void)
+./include/linux/slab.h:665:13: error: undefined identifier '__builtin_mul_overflow'
+./include/linux/slab.h:665:13: warning: call with no type!

Commit: drm/i915: Introduce per-engine workarounds Okay!

Commit: drm/i915: Verify GT workaround state after GPU init Okay!

Commit: drm/i915/selftests: Add tests for GT and engine workaround verification +./include/uapi/linux/perf_event.h:147:56: warning: cast truncates bits from constant value (8000000000000000 becomes 0)

Commit: drm/i915: Move register white-listing to the common workaround framework Okay!

Commit: drm/i915: Fuse per-context workaround handling with the common framework -drivers/gpu/drm/i915/selftests/../i915_drv.h:3573:16: warning: expression using sizeof(void) +drivers/gpu/drm/i915/selftests/../i915_drv.h:3558:16: warning: expression using sizeof(void)

Commit: drm/i915: Trim unused workaround list entries Okay!

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#17 lore raw

✓ Fi.CI.BAT: success for Restore workarounds after engine reset and unify their handling (rev3)

In reply to Tvrtko Ursulin
== Series Details ==

== Series Details ==

Series: Restore workarounds after engine reset and unify their handling (rev3)
URL : https://patchwork.freedesktop.org/series/53313/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5239 -> Patchwork_10997
====================================================

Summary
-------

**SUCCESS**

No regressions found.

External URL: https://patchwork.freedesktop.org/api/1.0/series/53313/revisions/3/mbox/

Known issues
------------

Here are the changes found in Patchwork_10997 that come from known issues:

### IGT changes ###

#### Issues hit ####

* igt@gem_ctx_create@basic-files:
- fi-bsw-n3050: PASS -> FAIL [fdo#108656]

* igt@gem_exec_reloc@basic-gtt:
- fi-skl-6770hq: PASS -> DMESG-WARN [fdo#105541]

* {igt@runner@aborted}:
- {fi-icl-y}: NOTRUN -> FAIL [fdo#108070]

#### Possible fixes ####

* igt@i915_selftest@live_execlists:
- fi-apl-guc: INCOMPLETE [fdo#103927] / [fdo#108622] -> PASS

#### Warnings ####

* igt@i915_selftest@live_contexts:
- {fi-icl-u3}: INCOMPLETE [fdo#108315] -> DMESG-FAIL [fdo#108569]

{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).

[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#105541]: https://bugs.freedesktop.org/show_bug.cgi?id=105541
[fdo#108070]: https://bugs.freedesktop.org/show_bug.cgi?id=108070
[fdo#108315]: https://bugs.freedesktop.org/show_bug.cgi?id=108315
[fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
[fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
[fdo#108656]: https://bugs.freedesktop.org/show_bug.cgi?id=108656

Participating hosts (47 -> 42)
------------------------------

Additional (1): fi-icl-y
Missing (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-pnv-d510

Build changes
-------------

* Linux: CI_DRM_5239 -> Patchwork_10997

CI_DRM_5239: 6b82ae50cbf9b70fb3884937a221f69261c4c41c @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_4736: 285ebfb3b7adc56586031afa5150c4e5ad40c229 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_10997: dc515637f6ecad20b108197a91ecbdd6046b7e31 @ git://anongit.freedesktop.org/gfx-ci/linux

== Linux commits ==

dc515637f6ec drm/i915: Trim unused workaround list entries
ea620326b3cb drm/i915: Fuse per-context workaround handling with the common framework d695d6e32eb8 drm/i915: Move register white-listing to the common workaround framework e779d6bfdfef drm/i915/selftests: Add tests for GT and engine workaround verification 8d055232d8d8 drm/i915: Verify GT workaround state after GPU init ccb6a2c1f539 drm/i915: Introduce per-engine workarounds
ed91418aaeec drm/i915: Record GT workarounds in a list

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10997/
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx