[Intel-gfx] [PATCH] drm/i915: Fix preallocated barrier list append

[Intel-gfx] [PATCH] drm/i915: Fix preallocated barrier list append

[José Roberto de Souza]

Only the first and the last nodes were being added to
ref->preallocated_barriers.

I'm not familiar with this part of the code but if that is the
expected behavior it is leaking the nodes in between.

Renaming variables to make it more easy to read.

Fixes: 841350223816 ("drm/i915/gt: Drop mutex serialisation between context pin/unpin")
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_active.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 9d6830885d2e..3d2e7cf55e52 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -607,7 +607,7 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
 					    struct intel_engine_cs *engine)
 {
 	intel_engine_mask_t tmp, mask = engine->mask;
-	struct llist_node *pos = NULL, *next;
+	struct llist_node *first = NULL, *last = NULL;
 	struct intel_gt *gt = engine->gt;
 	int err;
 
@@ -626,6 +626,7 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
 	GEM_BUG_ON(!mask);
 	for_each_engine_masked(engine, gt, mask, tmp) {
 		u64 idx = engine->kernel_context->timeline->fence_context;
+		struct llist_node *prev = first;
 		struct active_node *node;
 
 		node = reuse_idle_barrier(ref, idx);
@@ -659,23 +660,23 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
 		GEM_BUG_ON(rcu_access_pointer(node->base.fence) != ERR_PTR(-EAGAIN));
 
 		GEM_BUG_ON(barrier_to_engine(node) != engine);
-		next = barrier_to_ll(node);
-		next->next = pos;
-		if (!pos)
-			pos = next;
+		first = barrier_to_ll(node);
+		first->next = prev;
+		if (!last)
+			last = first;
 		intel_engine_pm_get(engine);
 	}
 
 	GEM_BUG_ON(!llist_empty(&ref->preallocated_barriers));
-	llist_add_batch(next, pos, &ref->preallocated_barriers);
+	llist_add_batch(first, last, &ref->preallocated_barriers);
 
 	return 0;
 
 unwind:
-	while (pos) {
-		struct active_node *node = barrier_from_ll(pos);
+	while (first) {
+		struct active_node *node = barrier_from_ll(first);
 
-		pos = pos->next;
+		first = first->next;
 
 		atomic_dec(&ref->count);
 		intel_engine_pm_put(barrier_to_engine(node));
-- 
2.25.0

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

Re: [Intel-gfx] [PATCH] drm/i915: Fix preallocated barrier list append

[Chris Wilson]

Quoting José Roberto de Souza (2020-01-29 23:23:45)
> Only the first and the last nodes were being added to
> ref->preallocated_barriers.
> 
> I'm not familiar with this part of the code but if that is the
> expected behavior it is leaking the nodes in between.
> 
> Renaming variables to make it more easy to read.
> 
> Fixes: 841350223816 ("drm/i915/gt: Drop mutex serialisation between context pin/unpin")
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_active.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 9d6830885d2e..3d2e7cf55e52 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -607,7 +607,7 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
>                                             struct intel_engine_cs *engine)
>  {
>         intel_engine_mask_t tmp, mask = engine->mask;
> -       struct llist_node *pos = NULL, *next;
> +       struct llist_node *first = NULL, *last = NULL;

last cannot be NULL at the end.

>         struct intel_gt *gt = engine->gt;
>         int err;
>  
> @@ -626,6 +626,7 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
>         GEM_BUG_ON(!mask);
>         for_each_engine_masked(engine, gt, mask, tmp) {
>                 u64 idx = engine->kernel_context->timeline->fence_context;
> +               struct llist_node *prev = first;
>                 struct active_node *node;
>  
>                 node = reuse_idle_barrier(ref, idx);
> @@ -659,23 +660,23 @@ int i915_active_acquire_preallocate_barrier(struct i915_active *ref,
>                 GEM_BUG_ON(rcu_access_pointer(node->base.fence) != ERR_PTR(-EAGAIN));
>  
>                 GEM_BUG_ON(barrier_to_engine(node) != engine);
> -               next = barrier_to_ll(node);
> -               next->next = pos;
> -               if (!pos)
> -                       pos = next;

Yeah, that was a bit of nonsense.

Minus the spurious NULL,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix preallocated barrier list append

[Chris Wilson]

Quoting José Roberto de Souza (2020-01-29 23:23:45)
> Only the first and the last nodes were being added to
> ref->preallocated_barriers.
> 
> I'm not familiar with this part of the code but if that is the
> expected behavior it is leaking the nodes in between.

Fortunately only used with a maximum of a pair of engines.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix preallocated barrier list append

[Souza, Jose]

On Wed, 2020-01-29 at 23:33 +0000, Chris Wilson wrote:
> Quoting José Roberto de Souza (2020-01-29 23:23:45)
> > Only the first and the last nodes were being added to
> > ref->preallocated_barriers.
> > 
> > I'm not familiar with this part of the code but if that is the
> > expected behavior it is leaking the nodes in between.
> > 
> > Renaming variables to make it more easy to read.
> > 
> > Fixes: 841350223816 ("drm/i915/gt: Drop mutex serialisation between
> > context pin/unpin")
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_active.c | 19 ++++++++++---------
> >  1 file changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_active.c
> > b/drivers/gpu/drm/i915/i915_active.c
> > index 9d6830885d2e..3d2e7cf55e52 100644
> > --- a/drivers/gpu/drm/i915/i915_active.c
> > +++ b/drivers/gpu/drm/i915/i915_active.c
> > @@ -607,7 +607,7 @@ int
> > i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> >                                             struct intel_engine_cs
> > *engine)
> >  {
> >         intel_engine_mask_t tmp, mask = engine->mask;
> > -       struct llist_node *pos = NULL, *next;
> > +       struct llist_node *first = NULL, *last = NULL;
> 
> last cannot be NULL at the end.

last will be set in the first iteration and it will always interate at
least once because the mask will at least match with the engine in
i915_active_acquire_preallocate_barrier() parameter.

> 
> >         struct intel_gt *gt = engine->gt;
> >         int err;
> >  
> > @@ -626,6 +626,7 @@ int
> > i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> >         GEM_BUG_ON(!mask);
> >         for_each_engine_masked(engine, gt, mask, tmp) {
> >                 u64 idx = engine->kernel_context->timeline-
> > >fence_context;
> > +               struct llist_node *prev = first;
> >                 struct active_node *node;
> >  
> >                 node = reuse_idle_barrier(ref, idx);
> > @@ -659,23 +660,23 @@ int
> > i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> >                 GEM_BUG_ON(rcu_access_pointer(node->base.fence) !=
> > ERR_PTR(-EAGAIN));
> >  
> >                 GEM_BUG_ON(barrier_to_engine(node) != engine);
> > -               next = barrier_to_ll(node);
> > -               next->next = pos;
> > -               if (!pos)
> > -                       pos = next;
> 
> Yeah, that was a bit of nonsense.
> 
> Minus the spurious NULL,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix preallocated barrier list append

[Chris Wilson]

Quoting Souza, Jose (2020-01-29 23:53:21)
> On Wed, 2020-01-29 at 23:33 +0000, Chris Wilson wrote:
> > Quoting José Roberto de Souza (2020-01-29 23:23:45)
> > > Only the first and the last nodes were being added to
> > > ref->preallocated_barriers.
> > > 
> > > I'm not familiar with this part of the code but if that is the
> > > expected behavior it is leaking the nodes in between.
> > > 
> > > Renaming variables to make it more easy to read.
> > > 
> > > Fixes: 841350223816 ("drm/i915/gt: Drop mutex serialisation between
> > > context pin/unpin")
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_active.c | 19 ++++++++++---------
> > >  1 file changed, 10 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_active.c
> > > b/drivers/gpu/drm/i915/i915_active.c
> > > index 9d6830885d2e..3d2e7cf55e52 100644
> > > --- a/drivers/gpu/drm/i915/i915_active.c
> > > +++ b/drivers/gpu/drm/i915/i915_active.c
> > > @@ -607,7 +607,7 @@ int
> > > i915_active_acquire_preallocate_barrier(struct i915_active *ref,
> > >                                             struct intel_engine_cs
> > > *engine)
> > >  {
> > >         intel_engine_mask_t tmp, mask = engine->mask;
> > > -       struct llist_node *pos = NULL, *next;
> > > +       struct llist_node *first = NULL, *last = NULL;
> > 
> > last cannot be NULL at the end.
> 
> last will be set in the first iteration and it will always interate at
> least once because the mask will at least match with the engine in
> i915_active_acquire_preallocate_barrier() parameter.

I meant to say last cannot be unset at the end, so initialising it is
silly.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix preallocated barrier list append

[Souza, Jose]

On Wed, 2020-01-29 at 23:59 +0000, Chris Wilson wrote:
> Quoting Souza, Jose (2020-01-29 23:53:21)
> > On Wed, 2020-01-29 at 23:33 +0000, Chris Wilson wrote:
> > > Quoting José Roberto de Souza (2020-01-29 23:23:45)
> > > > Only the first and the last nodes were being added to
> > > > ref->preallocated_barriers.
> > > > 
> > > > I'm not familiar with this part of the code but if that is the
> > > > expected behavior it is leaking the nodes in between.
> > > > 
> > > > Renaming variables to make it more easy to read.
> > > > 
> > > > Fixes: 841350223816 ("drm/i915/gt: Drop mutex serialisation
> > > > between
> > > > context pin/unpin")
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_active.c | 19 ++++++++++---------
> > > >  1 file changed, 10 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_active.c
> > > > b/drivers/gpu/drm/i915/i915_active.c
> > > > index 9d6830885d2e..3d2e7cf55e52 100644
> > > > --- a/drivers/gpu/drm/i915/i915_active.c
> > > > +++ b/drivers/gpu/drm/i915/i915_active.c
> > > > @@ -607,7 +607,7 @@ int
> > > > i915_active_acquire_preallocate_barrier(struct i915_active
> > > > *ref,
> > > >                                             struct
> > > > intel_engine_cs
> > > > *engine)
> > > >  {
> > > >         intel_engine_mask_t tmp, mask = engine->mask;
> > > > -       struct llist_node *pos = NULL, *next;
> > > > +       struct llist_node *first = NULL, *last = NULL;
> > > 
> > > last cannot be NULL at the end.
> > 
> > last will be set in the first iteration and it will always interate
> > at
> > least once because the mask will at least match with the engine in
> > i915_active_acquire_preallocate_barrier() parameter.
> 
> I meant to say last cannot be unset at the end, so initialising it is
> silly.

It need to be initialized so it is set in the first iteration to point
to the last node.

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Fix preallocated barrier list append

[Patchwork]

== Series Details ==

Series: drm/i915: Fix preallocated barrier list append
URL   : https://patchwork.freedesktop.org/series/72750/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7839 -> Patchwork_16325
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_16325 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_16325, please notify your bug team 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_16325/index.html

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@runner@aborted:
    - fi-bsw-kefka:       NOTRUN -> [FAIL][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16325/fi-bsw-kefka/igt@runner@aborted.html

  
#### Warnings ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-cml-s:           [FAIL][2] ([fdo#103375]) -> [TIMEOUT][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7839/fi-cml-s/igt@gem_exec_suspend@basic-s3.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16325/fi-cml-s/igt@gem_exec_suspend@basic-s3.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_basic@create-close:
    - fi-icl-dsi:         [PASS][4] -> [DMESG-WARN][5] ([i915#109])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7839/fi-icl-dsi/igt@gem_basic@create-close.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16325/fi-icl-dsi/igt@gem_basic@create-close.html

  * igt@gem_close_race@basic-threads:
    - fi-byt-j1900:       [PASS][6] -> [TIMEOUT][7] ([fdo#112271] / [i915#816])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7839/fi-byt-j1900/igt@gem_close_race@basic-threads.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16325/fi-byt-j1900/igt@gem_close_race@basic-threads.html

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770:        [PASS][8] -> [DMESG-FAIL][9] ([i915#553] / [i915#725])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7839/fi-hsw-4770/igt@i915_selftest@live_blt.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16325/fi-hsw-4770/igt@i915_selftest@live_blt.html

  * igt@i915_selftest@live_gem_contexts:
    - fi-cfl-8700k:       [PASS][10] -> [INCOMPLETE][11] ([i915#424])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7839/fi-cfl-8700k/igt@i915_selftest@live_gem_contexts.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16325/fi-cfl-8700k/igt@i915_selftest@live_gem_contexts.html
    - fi-cfl-guc:         [PASS][12] -> [INCOMPLETE][13] ([fdo#106070] / [i915#424])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7839/fi-cfl-guc/igt@i915_selftest@live_gem_contexts.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16325/fi-cfl-guc/igt@i915_selftest@live_gem_contexts.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s0:
    - fi-cml-s:           [FAIL][14] ([fdo#103375]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7839/fi-cml-s/igt@gem_exec_suspend@basic-s0.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16325/fi-cml-s/igt@gem_exec_suspend@basic-s0.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [FAIL][16] ([fdo#111407]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7839/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16325/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#106070]: https://bugs.freedesktop.org/show_bug.cgi?id=106070
  [fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407
  [fdo#112271]: https://bugs.freedesktop.org/show_bug.cgi?id=112271
  [i915#109]: https://gitlab.freedesktop.org/drm/intel/issues/109
  [i915#424]: https://gitlab.freedesktop.org/drm/intel/issues/424
  [i915#553]: https://gitlab.freedesktop.org/drm/intel/issues/553
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#816]: https://gitlab.freedesktop.org/drm/intel/issues/816


Participating hosts (47 -> 40)
------------------------------

  Additional (3): fi-hsw-peppy fi-bsw-kefka fi-kbl-r 
  Missing    (10): fi-bdw-5557u fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-snb-2520m fi-gdg-551 fi-cfl-8109u fi-elk-e7500 fi-byt-n2820 fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7839 -> Patchwork_16325

  CI-20190529: 20190529
  CI_DRM_7839: 41a9319a45aaf77e220c8101d6ce76ec66036ffc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5406: 786c79af483a9f6e4688811f74116030c734ca1f @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16325: 1c213cdcd3aec5fbca65caaca1a58913f2d28e9d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

1c213cdcd3ae drm/i915: Fix preallocated barrier list append

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16325/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915: Fix preallocated barrier list append (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Fix preallocated barrier list append (rev2)
URL   : https://patchwork.freedesktop.org/series/72750/
State : failure

== Summary ==

Applying: drm/i915: Fix preallocated barrier list append
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_active.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.

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