[Intel-gfx] [PATCH 0/2] drm/i915: fix rb-tree/llist/list confusion

[Intel-gfx] [PATCH 0/2] drm/i915: fix rb-tree/llist/list confusion

[Mathias Krause]

Commit 1ec23ed7126e ("drm/i915: Use uabi engines for the default engine
map") introduced a bug regarding engine iteration in default_engines()
as the rb tree isn't set up yet that early during driver initialization.
This triggered a sanity check we have in our grsecurity kernels, fixed
by reverting the offending commit (patch 1) and giving the
type-multiplexed members some more visibility to avoid making a similar
mistake again in the future (patch 2).

Please apply!

Thanks,
Mathias

Mathias Krause (2):
  Revert "drm/i915: Use uabi engines for the default engine map"
  drm/i915: Clarify type evolution of uabi_node/uabi_engines

 drivers/gpu/drm/i915/gem/i915_gem_context.c  |  9 +++++----
 drivers/gpu/drm/i915/gt/intel_engine_types.h | 10 +++++++++-
 drivers/gpu/drm/i915/gt/intel_engine_user.c  | 17 +++++++----------
 drivers/gpu/drm/i915/i915_drv.h              | 17 ++++++++++++++++-
 4 files changed, 37 insertions(+), 16 deletions(-)

-- 
2.39.2

[Intel-gfx] [PATCH 1/2] Revert "drm/i915: Use uabi engines for the default engine map"

[Mathias Krause]

Commit 1ec23ed7126e ("drm/i915: Use uabi engines for the default engine
map") switched from using for_each_engine() to for_each_uabi_engine() to
iterate over the user engines. While this seems to be a sensible change,
it's only safe to do when the engines are actually chained using the
rb-tree structure which is not the case during early driver
initialization where it can be either a lock-less list or regular
double-linked list.

In fact, the modesetting initialization code may end up calling
default_engines() through the fb helper code while the engines list
is still llist_node-based:

  i915_driver_probe() ->
    intel_modeset_init() ->
      intel_fbdev_init() ->
        drm_fb_helper_init() ->
          drm_client_init() ->
            drm_client_open() ->
              drm_file_alloc() ->
                i915_driver_open() ->
                  i915_gem_open() ->
                    i915_gem_context_open() ->
                      i915_gem_create_context() ->
                        default_engines()

Using for_each_uabi_engine() in default_engines() is therefore wrong, as
it would try to interpret the llist as rb-tree, making it find no engine
at all, as the rb_left and rb_right members will still be NULL, as they
haven't been iniitalized yet.

Revert back to use for_each_engine() which will look at each individual
engine based on its id, i.e. purely array-index based, avoiding the type
confusion mess.

Reported-by: sanitiy checks in grsecurity
Fixes: 1ec23ed7126e ("drm/i915: Use uabi engines for the default engine map")
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
Cc: Jonathan Cavitt <jonathan.cavitt@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_context.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index 9a9ff84c90d7..e4f7ebbd2690 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -1100,15 +1100,16 @@ static struct i915_gem_engines *alloc_engines(unsigned int count)
 static struct i915_gem_engines *default_engines(struct i915_gem_context *ctx,
 						struct intel_sseu rcs_sseu)
 {
-	const unsigned int max = I915_NUM_ENGINES;
+	const struct intel_gt *gt = to_gt(ctx->i915);
 	struct intel_engine_cs *engine;
 	struct i915_gem_engines *e, *err;
+	enum intel_engine_id id;
 
-	e = alloc_engines(max);
+	e = alloc_engines(I915_NUM_ENGINES);
 	if (!e)
 		return ERR_PTR(-ENOMEM);
 
-	for_each_uabi_engine(engine, ctx->i915) {
+	for_each_engine(engine, gt, id) {
 		struct intel_context *ce;
 		struct intel_sseu sseu = {};
 		int ret;
@@ -1116,7 +1117,7 @@ static struct i915_gem_engines *default_engines(struct i915_gem_context *ctx,
 		if (engine->legacy_idx == INVALID_ENGINE)
 			continue;
 
-		GEM_BUG_ON(engine->legacy_idx >= max);
+		GEM_BUG_ON(engine->legacy_idx >= I915_NUM_ENGINES);
 		GEM_BUG_ON(e->engines[engine->legacy_idx]);
 
 		ce = intel_context_create(engine);
-- 
2.39.2

[Intel-gfx] [PATCH 2/2] drm/i915: Clarify type evolution of uabi_node/uabi_engines

[Mathias Krause]

Chaining user engines happens in multiple passes during driver
initialization, mutating its type along the way. It starts off with a
simple lock-less linked list (struct llist_node/head) populated by
intel_engine_add_user() which later gets sorted and converted to an
intermediate regular list (struct list_head) just to be converted once
more to its final rb-tree structure (struct rb_node/root) in
intel_engines_driver_register().

All of these types overlay the uabi_node/uabi_engines members which is
unfortunate but safe if one takes care about using the rb-tree based
structure only after the conversion has completed. However, mistakes
happen and commit 1ec23ed7126e ("drm/i915: Use uabi engines for the
default engine map") violated that assumption, as the multiple types
evolution was all to easy hidden behind casts papering over it.

Make the type evolution of uabi_node/uabi_engines more visible by
putting all members into an anonymous union and use the correctly typed
member in its various users. This allows us to drop quite some ugly
casts and, hopefully, make the evolution of the members better
recognisable to avoid future mistakes.

Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
 drivers/gpu/drm/i915/gt/intel_engine_types.h | 10 +++++++++-
 drivers/gpu/drm/i915/gt/intel_engine_user.c  | 17 +++++++----------
 drivers/gpu/drm/i915/i915_drv.h              | 17 ++++++++++++++++-
 3 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index e99a6fa03d45..f0e91efb2acc 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -401,7 +401,15 @@ struct intel_engine_cs {
 
 	unsigned long context_tag;
 
-	struct rb_node uabi_node;
+	/*
+	 * The type evolves during initialization, see related comment for
+	 * struct drm_i915_private's uabi_engines member.
+	 */
+	union {
+		struct llist_node uabi_llist;
+		struct list_head uabi_list;
+		struct rb_node uabi_node;
+	};
 
 	struct intel_sseu sseu;
 
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_user.c b/drivers/gpu/drm/i915/gt/intel_engine_user.c
index dcedff41a825..118164ddbb2e 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_user.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_user.c
@@ -38,8 +38,7 @@ intel_engine_lookup_user(struct drm_i915_private *i915, u8 class, u8 instance)
 
 void intel_engine_add_user(struct intel_engine_cs *engine)
 {
-	llist_add((struct llist_node *)&engine->uabi_node,
-		  (struct llist_head *)&engine->i915->uabi_engines);
+	llist_add(&engine->uabi_llist, &engine->i915->uabi_engines_llist);
 }
 
 static const u8 uabi_classes[] = {
@@ -54,9 +53,9 @@ static int engine_cmp(void *priv, const struct list_head *A,
 		      const struct list_head *B)
 {
 	const struct intel_engine_cs *a =
-		container_of((struct rb_node *)A, typeof(*a), uabi_node);
+		container_of(A, typeof(*a), uabi_list);
 	const struct intel_engine_cs *b =
-		container_of((struct rb_node *)B, typeof(*b), uabi_node);
+		container_of(B, typeof(*b), uabi_list);
 
 	if (uabi_classes[a->class] < uabi_classes[b->class])
 		return -1;
@@ -73,7 +72,7 @@ static int engine_cmp(void *priv, const struct list_head *A,
 
 static struct llist_node *get_engines(struct drm_i915_private *i915)
 {
-	return llist_del_all((struct llist_head *)&i915->uabi_engines);
+	return llist_del_all(&i915->uabi_engines_llist);
 }
 
 static void sort_engines(struct drm_i915_private *i915,
@@ -83,9 +82,8 @@ static void sort_engines(struct drm_i915_private *i915,
 
 	llist_for_each_safe(pos, next, get_engines(i915)) {
 		struct intel_engine_cs *engine =
-			container_of((struct rb_node *)pos, typeof(*engine),
-				     uabi_node);
-		list_add((struct list_head *)&engine->uabi_node, engines);
+			container_of(pos, typeof(*engine), uabi_llist);
+		list_add(&engine->uabi_list, engines);
 	}
 	list_sort(NULL, engines, engine_cmp);
 }
@@ -213,8 +211,7 @@ void intel_engines_driver_register(struct drm_i915_private *i915)
 	p = &i915->uabi_engines.rb_node;
 	list_for_each_safe(it, next, &engines) {
 		struct intel_engine_cs *engine =
-			container_of((struct rb_node *)it, typeof(*engine),
-				     uabi_node);
+			container_of(it, typeof(*engine), uabi_list);
 
 		if (intel_gt_has_unrecoverable_error(engine->gt))
 			continue; /* ignore incomplete engines */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b4cf6f0f636d..68fd64d6a880 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -223,7 +223,22 @@ struct drm_i915_private {
 		bool mchbar_need_disable;
 	} gmch;
 
-	struct rb_root uabi_engines;
+	/*
+	 * Chaining user engines happens in multiple stages, starting with a
+	 * simple lock-less linked list created by intel_engine_add_user(),
+	 * which later gets sorted and converted to an intermediate regular
+	 * list, just to be converted once again to its final rb tree structure
+	 * in intel_engines_driver_register().
+	 *
+	 * Make sure to use the right iterator helper, depending on if the code
+	 * in question runs before or after intel_engines_driver_register() --
+	 * for_each_uabi_engine() can only be used afterwards!
+	 */
+	union {
+		struct llist_head uabi_engines_llist;
+		struct list_head uabi_engines_list;
+		struct rb_root uabi_engines;
+	};
 	unsigned int engine_uabi_class_count[I915_LAST_UABI_ENGINE_CLASS + 1];
 
 	/* protects the irq masks */
-- 
2.39.2

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: fix rb-tree/llist/list confusion

[Patchwork]

== Series Details ==

Series: drm/i915: fix rb-tree/llist/list confusion
URL   : https://patchwork.freedesktop.org/series/123282/
State : warning

== Summary ==

Error: dim checkpatch failed
f1aa77045061 Revert "drm/i915: Use uabi engines for the default engine map"
-:41: ERROR:BAD_SIGN_OFF: Unrecognized email address: 'sanitiy checks in grsecurity'
#41: 
Reported-by: sanitiy checks in grsecurity

-:41: WARNING:BAD_REPORTED_BY_LINK: Reported-by: should be immediately followed by Closes: with a URL to the report
#41: 
Reported-by: sanitiy checks in grsecurity
Fixes: 1ec23ed7126e ("drm/i915: Use uabi engines for the default engine map")

-:76: WARNING:AVOID_BUG: Do not crash the kernel unless it is absolutely unavoidable--use WARN_ON_ONCE() plus recovery code (if feasible) instead of BUG() or variants
#76: FILE: drivers/gpu/drm/i915/gem/i915_gem_context.c:1120:
+		GEM_BUG_ON(engine->legacy_idx >= I915_NUM_ENGINES);

total: 1 errors, 2 warnings, 0 checks, 27 lines checked
ca8889ed26b0 drm/i915: Clarify type evolution of uabi_node/uabi_engines

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915: fix rb-tree/llist/list confusion

[Patchwork]

== Series Details ==

Series: drm/i915: fix rb-tree/llist/list confusion
URL   : https://patchwork.freedesktop.org/series/123282/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: fix rb-tree/llist/list confusion

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 6095 bytes --]

== Series Details ==

Series: drm/i915: fix rb-tree/llist/list confusion
URL   : https://patchwork.freedesktop.org/series/123282/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13599 -> Patchwork_123282v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_123282v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_123282v1, please notify your bug team (lgci.bug.filing@intel.com) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/index.html

Participating hosts (38 -> 36)
------------------------------

  Missing    (2): fi-bsw-nick fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_123282v1:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_huc_copy@huc-copy:
    - bat-mtlp-8:         [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13599/bat-mtlp-8/igt@gem_huc_copy@huc-copy.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/bat-mtlp-8/igt@gem_huc_copy@huc-copy.html

  * igt@i915_selftest@live@requests:
    - bat-mtlp-8:         [PASS][3] -> [ABORT][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13599/bat-mtlp-8/igt@i915_selftest@live@requests.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/bat-mtlp-8/igt@i915_selftest@live@requests.html

  * igt@kms_pipe_crc_basic@read-crc@pipe-d-dp-5:
    - bat-adlp-11:        [PASS][5] -> [ABORT][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13599/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc@pipe-d-dp-5.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/bat-adlp-11/igt@kms_pipe_crc_basic@read-crc@pipe-d-dp-5.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@workarounds:
    - bat-adlm-1:         [PASS][7] -> [INCOMPLETE][8] ([i915#4983] / [i915#7677])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13599/bat-adlm-1/igt@i915_selftest@live@workarounds.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/bat-adlm-1/igt@i915_selftest@live@workarounds.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         NOTRUN -> [SKIP][9] ([i915#1845]) +3 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1:
    - bat-rplp-1:         [PASS][10] -> [ABORT][11] ([i915#8442] / [i915#8668])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13599/bat-rplp-1/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/bat-rplp-1/igt@kms_pipe_crc_basic@read-crc-frame-sequence@pipe-d-edp-1.html

  
#### Possible fixes ####

  * igt@kms_chamelium_edid@hdmi-edid-read:
    - {bat-dg2-13}:       [DMESG-WARN][12] ([i915#7952]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13599/bat-dg2-13/igt@kms_chamelium_edid@hdmi-edid-read.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/bat-dg2-13/igt@kms_chamelium_edid@hdmi-edid-read.html

  * igt@kms_chamelium_frames@dp-crc-fast:
    - {bat-dg2-13}:       [DMESG-WARN][14] ([Intel XE#485]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13599/bat-dg2-13/igt@kms_chamelium_frames@dp-crc-fast.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/bat-dg2-13/igt@kms_chamelium_frames@dp-crc-fast.html

  * igt@kms_flip@basic-flip-vs-wf_vblank@a-dp6:
    - bat-adlp-11:        [FAIL][16] ([i915#6121]) -> [PASS][17] +4 other tests pass
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13599/bat-adlp-11/igt@kms_flip@basic-flip-vs-wf_vblank@a-dp6.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/bat-adlp-11/igt@kms_flip@basic-flip-vs-wf_vblank@a-dp6.html

  * igt@kms_flip@basic-flip-vs-wf_vblank@c-dp5:
    - bat-adlp-11:        [DMESG-WARN][18] ([i915#6868]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13599/bat-adlp-11/igt@kms_flip@basic-flip-vs-wf_vblank@c-dp5.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/bat-adlp-11/igt@kms_flip@basic-flip-vs-wf_vblank@c-dp5.html

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

  [Intel XE#485]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/485
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#6121]: https://gitlab.freedesktop.org/drm/intel/issues/6121
  [i915#6868]: https://gitlab.freedesktop.org/drm/intel/issues/6868
  [i915#7677]: https://gitlab.freedesktop.org/drm/intel/issues/7677
  [i915#7952]: https://gitlab.freedesktop.org/drm/intel/issues/7952
  [i915#8442]: https://gitlab.freedesktop.org/drm/intel/issues/8442
  [i915#8668]: https://gitlab.freedesktop.org/drm/intel/issues/8668


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

  * Linux: CI_DRM_13599 -> Patchwork_123282v1

  CI-20190529: 20190529
  CI_DRM_13599: 58fe10f34e80d0eeb5609128faa135260623a715 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7468: 7468
  Patchwork_123282v1: 58fe10f34e80d0eeb5609128faa135260623a715 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

c617dfc01fc9 drm/i915: Clarify type evolution of uabi_node/uabi_engines
c0699821c8c4 Revert "drm/i915: Use uabi engines for the default engine map"

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_123282v1/index.html

[-- Attachment #2: Type: text/html, Size: 6979 bytes --]

Re: [Intel-gfx] [PATCH 0/2] drm/i915: fix rb-tree/llist/list confusion

[Mathias Krause]

On 05.09.23 13:39, Mathias Krause wrote:
> Commit 1ec23ed7126e ("drm/i915: Use uabi engines for the default engine
> map") introduced a bug regarding engine iteration in default_engines()
> as the rb tree isn't set up yet that early during driver initialization.
> This triggered a sanity check we have in our grsecurity kernels, fixed
> by reverting the offending commit (patch 1) and giving the
> type-multiplexed members some more visibility to avoid making a similar
> mistake again in the future (patch 2).
> 
> Please apply!
> 
> Thanks,
> Mathias
> 
> Mathias Krause (2):
>   Revert "drm/i915: Use uabi engines for the default engine map"
>   drm/i915: Clarify type evolution of uabi_node/uabi_engines
> 
>  drivers/gpu/drm/i915/gem/i915_gem_context.c  |  9 +++++----
>  drivers/gpu/drm/i915/gt/intel_engine_types.h | 10 +++++++++-
>  drivers/gpu/drm/i915/gt/intel_engine_user.c  | 17 +++++++----------
>  drivers/gpu/drm/i915/i915_drv.h              | 17 ++++++++++++++++-
>  4 files changed, 37 insertions(+), 16 deletions(-)
> 

Ping. Any objections to this series?

- Mathias

Re: [Intel-gfx] [PATCH 0/2] drm/i915: fix rb-tree/llist/list confusion

[Tvrtko Ursulin]

Hi,

On 21/09/2023 07:24, Mathias Krause wrote:
> On 05.09.23 13:39, Mathias Krause wrote:
>> Commit 1ec23ed7126e ("drm/i915: Use uabi engines for the default engine
>> map") introduced a bug regarding engine iteration in default_engines()
>> as the rb tree isn't set up yet that early during driver initialization.
>> This triggered a sanity check we have in our grsecurity kernels, fixed
>> by reverting the offending commit (patch 1) and giving the
>> type-multiplexed members some more visibility to avoid making a similar
>> mistake again in the future (patch 2).
>>
>> Please apply!
>>
>> Thanks,
>> Mathias
>>
>> Mathias Krause (2):
>>    Revert "drm/i915: Use uabi engines for the default engine map"
>>    drm/i915: Clarify type evolution of uabi_node/uabi_engines
>>
>>   drivers/gpu/drm/i915/gem/i915_gem_context.c  |  9 +++++----
>>   drivers/gpu/drm/i915/gt/intel_engine_types.h | 10 +++++++++-
>>   drivers/gpu/drm/i915/gt/intel_engine_user.c  | 17 +++++++----------
>>   drivers/gpu/drm/i915/i915_drv.h              | 17 ++++++++++++++++-
>>   4 files changed, 37 insertions(+), 16 deletions(-)
>>
> 
> Ping. Any objections to this series?

Apologies for the delay in getting back to you, I was away from work for 
a bit.

Tricky thing you discovered and a great analysis in the commit text.

But we cannot simply revert 1ec23ed7126e since that would miss to 
include the media tile engines on Meteorlake.

What I think should work is to move the call to 
intel_engines_driver_register() from i915_gem_driver_register() to 
i915_gem_init(). I can double-check and send a patch, or you can, 
keeping parts of your great commit message in 1/2?

That would align the expectations of intel_display_driver_probe() which 
expects GEM to be fully initialized by the time it runs.

2/2 looks good and I would be happy to merge it in the interim. Going 
forward I would pencil in looking into removing the rbtree and 
multi-stage complications. Former I think isn't needed, code which needs 
fast random lookup via the tree never materialized, and latter perhaps 
could be sorted in place somehow, that is, without the need for two list 
types externally visible.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 0/2] drm/i915: fix rb-tree/llist/list confusion

[Mathias Krause]

On 28.09.23 13:15, Tvrtko Ursulin wrote:
> Hi,
> 
> On 21/09/2023 07:24, Mathias Krause wrote:
>> On 05.09.23 13:39, Mathias Krause wrote:
>>> Commit 1ec23ed7126e ("drm/i915: Use uabi engines for the default engine
>>> map") introduced a bug regarding engine iteration in default_engines()
>>> as the rb tree isn't set up yet that early during driver initialization.
>>> This triggered a sanity check we have in our grsecurity kernels, fixed
>>> by reverting the offending commit (patch 1) and giving the
>>> type-multiplexed members some more visibility to avoid making a similar
>>> mistake again in the future (patch 2).
>>>
>>> Please apply!
>>>
>>> Thanks,
>>> Mathias
>>>
>>> Mathias Krause (2):
>>>    Revert "drm/i915: Use uabi engines for the default engine map"
>>>    drm/i915: Clarify type evolution of uabi_node/uabi_engines
>>>
>>>   drivers/gpu/drm/i915/gem/i915_gem_context.c  |  9 +++++----
>>>   drivers/gpu/drm/i915/gt/intel_engine_types.h | 10 +++++++++-
>>>   drivers/gpu/drm/i915/gt/intel_engine_user.c  | 17 +++++++----------
>>>   drivers/gpu/drm/i915/i915_drv.h              | 17 ++++++++++++++++-
>>>   4 files changed, 37 insertions(+), 16 deletions(-)
>>>
>>
>> Ping. Any objections to this series?
> 
> Apologies for the delay in getting back to you, I was away from work for
> a bit.

Don't worry. vger had its hickups with gmail-originated Emails, adding
further delay. I think my "ping" Email only made it to the list three
days ago? -- 4 days late :D

> 
> Tricky thing you discovered and a great analysis in the commit text.

Thanks! Was a nasty bug to chase when the display simply stays black ;)

> 
> But we cannot simply revert 1ec23ed7126e since that would miss to
> include the media tile engines on Meteorlake.

Oh, missed that! But, honestly, I have no clue about i915's inner
workings. It's quite a beast---a lot of code to chase to grasp all
inter-dependencies.

> 
> What I think should work is to move the call to
> intel_engines_driver_register() from i915_gem_driver_register() to
> i915_gem_init(). I can double-check and send a patch, or you can,
> keeping parts of your great commit message in 1/2?

I can prepare a patch and test it on my systems (both ADL, a NUC12 and a
Lenovo X1). I've no Meteorlake, though. Should have something ready
tomorrow.

> 
> That would align the expectations of intel_display_driver_probe() which
> expects GEM to be fully initialized by the time it runs.

Sounds about right.

> 
> 2/2 looks good and I would be happy to merge it in the interim.

It helped me, at least, to clarify for each user what type it expects
the list to be. Then it's easy to follow a code chain and see when a
type transformation happens and when assumptions get invalidated / changed.

> Going
> forward I would pencil in looking into removing the rbtree and
> multi-stage complications. Former I think isn't needed, code which needs
> fast random lookup via the tree never materialized, and latter perhaps
> could be sorted in place somehow, that is, without the need for two list
> types externally visible.

I agree, simplifying or even completely removing the type overload makes
the code less fragile, so I'm all for it!

Thanks,
Mathias

> 
> Regards,
> 
> Tvrtko