[Intel-gfx] [PATCH] drm/i915/gem: Perform all asynchronous waits prior to marking payload start

[Intel-gfx] [PATCH] drm/i915/gem: Perform all asynchronous waits prior to marking payload start

[Chris Wilson]

The initial breadcrumb marks the transition from context wait and setup
into the request payload. We use the marker to determine if the request
is merely waiting to begin, or is inside the payload and hung.
Forgetting to include a breadcrumb before the user payload would mean we
do not reset the guilty user request, and conversely if the initial
breadcrumb is too early we blame the user for a problem elsewhere.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gem/i915_gem_client_blt.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
index 272cf3ea68d5..44821d94544f 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
@@ -202,12 +202,6 @@ static void clear_pages_worker(struct work_struct *work)
 	if (unlikely(err))
 		goto out_request;
 
-	if (w->ce->engine->emit_init_breadcrumb) {
-		err = w->ce->engine->emit_init_breadcrumb(rq);
-		if (unlikely(err))
-			goto out_request;
-	}
-
 	/*
 	 * w->dma is already exported via (vma|obj)->resv we need only
 	 * keep track of the GPU activity within this vma/request, and
@@ -217,9 +211,15 @@ static void clear_pages_worker(struct work_struct *work)
 	if (err)
 		goto out_request;
 
-	err = w->ce->engine->emit_bb_start(rq,
-					   batch->node.start, batch->node.size,
-					   0);
+	if (rq->engine->emit_init_breadcrumb) {
+		err = rq->engine->emit_init_breadcrumb(rq);
+		if (unlikely(err))
+			goto out_request;
+	}
+
+	err = rq->engine->emit_bb_start(rq,
+					batch->node.start, batch->node.size,
+					0);
 out_request:
 	if (unlikely(err)) {
 		i915_request_set_error_once(rq, err);
-- 
2.20.1

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

Re: [Intel-gfx] [PATCH] drm/i915/gem: Perform all asynchronous waits prior to marking payload start

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> The initial breadcrumb marks the transition from context wait and setup
> into the request payload. We use the marker to determine if the request
> is merely waiting to begin, or is inside the payload and hung.
> Forgetting to include a breadcrumb before the user payload would mean we
> do not reset the guilty user request, and conversely if the initial
> breadcrumb is too early we blame the user for a problem elsewhere.

Agreed.

>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_client_blt.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> index 272cf3ea68d5..44821d94544f 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> @@ -202,12 +202,6 @@ static void clear_pages_worker(struct work_struct *work)
>  	if (unlikely(err))
>  		goto out_request;
>  
> -	if (w->ce->engine->emit_init_breadcrumb) {
> -		err = w->ce->engine->emit_init_breadcrumb(rq);
> -		if (unlikely(err))
> -			goto out_request;
> -	}
> -
>  	/*
>  	 * w->dma is already exported via (vma|obj)->resv we need only
>  	 * keep track of the GPU activity within this vma/request, and
> @@ -217,9 +211,15 @@ static void clear_pages_worker(struct work_struct *work)
>  	if (err)
>  		goto out_request;
>  
> -	err = w->ce->engine->emit_bb_start(rq,
> -					   batch->node.start, batch->node.size,
> -					   0);

We don't have to do any extra steps to counter

__i915_vma_move_to_active(vma, rq);

in error path?

-Mika

> +	if (rq->engine->emit_init_breadcrumb) {
> +		err = rq->engine->emit_init_breadcrumb(rq);
> +		if (unlikely(err))
> +			goto out_request;
> +	}
> +
> +	err = rq->engine->emit_bb_start(rq,
> +					batch->node.start, batch->node.size,
> +					0);
>  out_request:
>  	if (unlikely(err)) {
>  		i915_request_set_error_once(rq, err);
> -- 
> 2.20.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/gem: Perform all asynchronous waits prior to marking payload start

[Chris Wilson]

Quoting Mika Kuoppala (2020-10-07 10:40:59)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > The initial breadcrumb marks the transition from context wait and setup
> > into the request payload. We use the marker to determine if the request
> > is merely waiting to begin, or is inside the payload and hung.
> > Forgetting to include a breadcrumb before the user payload would mean we
> > do not reset the guilty user request, and conversely if the initial
> > breadcrumb is too early we blame the user for a problem elsewhere.
> 
> Agreed.
> 
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/gem/i915_gem_client_blt.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> > index 272cf3ea68d5..44821d94544f 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
> > @@ -202,12 +202,6 @@ static void clear_pages_worker(struct work_struct *work)
> >       if (unlikely(err))
> >               goto out_request;
> >  
> > -     if (w->ce->engine->emit_init_breadcrumb) {
> > -             err = w->ce->engine->emit_init_breadcrumb(rq);
> > -             if (unlikely(err))
> > -                     goto out_request;
> > -     }
> > -
> >       /*
> >        * w->dma is already exported via (vma|obj)->resv we need only
> >        * keep track of the GPU activity within this vma/request, and
> > @@ -217,9 +211,15 @@ static void clear_pages_worker(struct work_struct *work)
> >       if (err)
> >               goto out_request;
> >  
> > -     err = w->ce->engine->emit_bb_start(rq,
> > -                                        batch->node.start, batch->node.size,
> > -                                        0);
> 
> We don't have to do any extra steps to counter
> 
> __i915_vma_move_to_active(vma, rq);
> 
> in error path?

No. We always submit the request, and the request is always serialised
with the steps that have been setup before. So if we fail in adding
another serialisation step, we can safely abort and complete all the
steps prior to this point (by submitting the empty request), and
anything that subsequently wants to wait on those resources we have
already claimed will wait on this request (which in turn waits on the
previous resource holders and so forth, the lock chain is never broken).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/gem: Perform all asynchronous waits prior to marking payload start

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2020-10-07 10:40:59)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > The initial breadcrumb marks the transition from context wait and setup
>> > into the request payload. We use the marker to determine if the request
>> > is merely waiting to begin, or is inside the payload and hung.
>> > Forgetting to include a breadcrumb before the user payload would mean we
>> > do not reset the guilty user request, and conversely if the initial
>> > breadcrumb is too early we blame the user for a problem elsewhere.
>> 
>> Agreed.
>> 
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > ---
>> >  drivers/gpu/drm/i915/gem/i915_gem_client_blt.c | 18 +++++++++---------
>> >  1 file changed, 9 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
>> > index 272cf3ea68d5..44821d94544f 100644
>> > --- a/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
>> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_client_blt.c
>> > @@ -202,12 +202,6 @@ static void clear_pages_worker(struct work_struct *work)
>> >       if (unlikely(err))
>> >               goto out_request;
>> >  
>> > -     if (w->ce->engine->emit_init_breadcrumb) {
>> > -             err = w->ce->engine->emit_init_breadcrumb(rq);
>> > -             if (unlikely(err))
>> > -                     goto out_request;
>> > -     }
>> > -
>> >       /*
>> >        * w->dma is already exported via (vma|obj)->resv we need only
>> >        * keep track of the GPU activity within this vma/request, and
>> > @@ -217,9 +211,15 @@ static void clear_pages_worker(struct work_struct *work)
>> >       if (err)
>> >               goto out_request;
>> >  
>> > -     err = w->ce->engine->emit_bb_start(rq,
>> > -                                        batch->node.start, batch->node.size,
>> > -                                        0);
>> 
>> We don't have to do any extra steps to counter
>> 
>> __i915_vma_move_to_active(vma, rq);
>> 
>> in error path?
>
> No. We always submit the request, and the request is always serialised
> with the steps that have been setup before. So if we fail in adding
> another serialisation step, we can safely abort and complete all the
> steps prior to this point (by submitting the empty request), and
> anything that subsequently wants to wait on those resources we have
> already claimed will wait on this request (which in turn waits on the
> previous resource holders and so forth, the lock chain is never broken).

So decision of emitting anything is crossing the rubicon. No matter
if it part of the breadcrumbs. Well makes sense as rollback after
that decision is prolly much harder than just to emit empty.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> -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/gem: Perform all asynchronous waits prior to marking payload start

[Patchwork]

[-- Attachment #1.1: Type: text/plain, Size: 5862 bytes --]

== Series Details ==

Series: drm/i915/gem: Perform all asynchronous waits prior to marking payload start
URL   : https://patchwork.freedesktop.org/series/82434/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_9107 -> Patchwork_18644
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_18644 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_18644, 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_18644/index.html

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-tgl-u2:          [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9107/fi-tgl-u2/igt@i915_selftest@live@gt_heartbeat.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18644/fi-tgl-u2/igt@i915_selftest@live@gt_heartbeat.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload:
    - fi-byt-j1900:       [PASS][3] -> [DMESG-WARN][4] ([i915#1982])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9107/fi-byt-j1900/igt@i915_module_load@reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18644/fi-byt-j1900/igt@i915_module_load@reload.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-bsw-kefka:       [PASS][5] -> [DMESG-WARN][6] ([i915#1982])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9107/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18644/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  
#### Possible fixes ####

  * igt@i915_module_load@reload:
    - fi-tgl-u2:          [DMESG-WARN][7] ([i915#1982] / [k.org#205379]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9107/fi-tgl-u2/igt@i915_module_load@reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18644/fi-tgl-u2/igt@i915_module_load@reload.html

  * igt@kms_busy@basic@flip:
    - {fi-tgl-dsi}:       [DMESG-WARN][9] ([i915#1982]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9107/fi-tgl-dsi/igt@kms_busy@basic@flip.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18644/fi-tgl-dsi/igt@kms_busy@basic@flip.html

  * igt@vgem_basic@unload:
    - fi-skl-guc:         [DMESG-WARN][11] ([i915#2203]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9107/fi-skl-guc/igt@vgem_basic@unload.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18644/fi-skl-guc/igt@vgem_basic@unload.html

  
#### Warnings ####

  * igt@i915_pm_rpm@module-reload:
    - fi-kbl-x1275:       [DMESG-FAIL][13] ([i915#62]) -> [DMESG-FAIL][14] ([i915#62] / [i915#95])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9107/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18644/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-legacy:
    - fi-kbl-x1275:       [DMESG-WARN][15] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][16] ([i915#62] / [i915#92]) +2 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9107/fi-kbl-x1275/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18644/fi-kbl-x1275/igt@kms_cursor_legacy@basic-flip-before-cursor-legacy.html

  * igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size:
    - fi-kbl-x1275:       [DMESG-WARN][17] ([i915#62] / [i915#92]) -> [DMESG-WARN][18] ([i915#62] / [i915#92] / [i915#95]) +3 similar issues
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9107/fi-kbl-x1275/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18644/fi-kbl-x1275/igt@kms_cursor_legacy@basic-flip-before-cursor-varying-size.html

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

  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2203]: https://gitlab.freedesktop.org/drm/intel/issues/2203
  [i915#289]: https://gitlab.freedesktop.org/drm/intel/issues/289
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
  [k.org#205379]: https://bugzilla.kernel.org/show_bug.cgi?id=205379


Participating hosts (45 -> 39)
------------------------------

  Missing    (6): fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus 


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

  * Linux: CI_DRM_9107 -> Patchwork_18644

  CI-20190529: 20190529
  CI_DRM_9107: f0a9b069a83f0ab30eb2f4d632cc869555f98cde @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5803: 33f07391e5f6a8d9323e471c81db3f29f91ed4d7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18644: f236b6fcbe33fc2885295b0ecbfebc80200bf790 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

f236b6fcbe33 drm/i915/gem: Perform all asynchronous waits prior to marking payload start

== Logs ==

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

[-- Attachment #1.2: Type: text/html, Size: 7479 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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