[Intel-gfx] [PATCH] drm/i915/gt: Drop the timeline->mutex as we wait for retirement

[Intel-gfx] [PATCH] drm/i915/gt: Drop the timeline->mutex as we wait for retirement

[Chris Wilson]

As we have pinned the timeline (using tl->active_count), we can safely
drop the tl->mutex as we wait for what we believe to be the final
request on that timeline. This is useful for ensuring that we do not
block the engine heartbeat by hogging the kernel_context's timeline on a
dead GPU.

References: https://gitlab.freedesktop.org/drm/intel/issues/1364
Fixes: 058179e72e09 ("drm/i915/gt: Replace hangcheck by heartbeats")
Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_requests.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index 8a5054f21bf8..24c99d0838af 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -147,24 +147,32 @@ long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout)
 
 			fence = i915_active_fence_get(&tl->last_request);
 			if (fence) {
+				mutex_unlock(&tl->mutex);
+
 				timeout = dma_fence_wait_timeout(fence,
 								 interruptible,
 								 timeout);
 				dma_fence_put(fence);
+
+				/* Retirement is best effort */
+				if (!mutex_trylock(&tl->mutex)) {
+					active_count++;
+					goto out_active;
+				}
 			}
 		}
 
 		if (!retire_requests(tl) || flush_submission(gt))
 			active_count++;
+		mutex_unlock(&tl->mutex);
 
-		spin_lock(&timelines->lock);
+out_active:	spin_lock(&timelines->lock);
 
-		/* Resume iteration after dropping lock */
+		/* Resume list iteration after reacquiring spinlock */
 		list_safe_reset_next(tl, tn, link);
 		if (atomic_dec_and_test(&tl->active_count))
 			list_del(&tl->link);
 
-		mutex_unlock(&tl->mutex);
 
 		/* Defer the final release to after the spinlock */
 		if (refcount_dec_and_test(&tl->kref.refcount)) {
-- 
2.25.1

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

Re: [Intel-gfx] [PATCH] drm/i915/gt: Drop the timeline->mutex as we wait for retirement

[Mika Kuoppala]

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

> As we have pinned the timeline (using tl->active_count), we can safely
> drop the tl->mutex as we wait for what we believe to be the final
> request on that timeline. This is useful for ensuring that we do not
> block the engine heartbeat by hogging the kernel_context's timeline on a
> dead GPU.
>
> References: https://gitlab.freedesktop.org/drm/intel/issues/1364
> Fixes: 058179e72e09 ("drm/i915/gt: Replace hangcheck by heartbeats")
> Fixes: f33a8a51602c ("drm/i915: Merge wait_for_timelines with retire_request")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_requests.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index 8a5054f21bf8..24c99d0838af 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -147,24 +147,32 @@ long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout)
>  
>  			fence = i915_active_fence_get(&tl->last_request);
>  			if (fence) {
> +				mutex_unlock(&tl->mutex);
> +
>  				timeout = dma_fence_wait_timeout(fence,
>  								 interruptible,
>  								 timeout);
>  				dma_fence_put(fence);
> +
> +				/* Retirement is best effort */
> +				if (!mutex_trylock(&tl->mutex)) {
> +					active_count++;
> +					goto out_active;
> +				}
>  			}
>  		}
>  
>  		if (!retire_requests(tl) || flush_submission(gt))
>  			active_count++;
> +		mutex_unlock(&tl->mutex);
>  
> -		spin_lock(&timelines->lock);
> +out_active:	spin_lock(&timelines->lock);
>  
> -		/* Resume iteration after dropping lock */
> +		/* Resume list iteration after reacquiring spinlock */

Now it is in mint.

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

>  		list_safe_reset_next(tl, tn, link);
>  		if (atomic_dec_and_test(&tl->active_count))
>  			list_del(&tl->link);
>  
> -		mutex_unlock(&tl->mutex);
>  
>  		/* Defer the final release to after the spinlock */
>  		if (refcount_dec_and_test(&tl->kref.refcount)) {
> -- 
> 2.25.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.DOCS: warning for drm/i915/gt: Drop the timeline->mutex as we wait for retirement (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/gt: Drop the timeline->mutex as we wait for retirement (rev2)
URL   : https://patchwork.freedesktop.org/series/74204/
State : warning

== Summary ==

$ make htmldocs 2>&1 > /dev/null | grep i915
./drivers/gpu/drm/i915/display/intel_dpll_mgr.h:285: warning: Function parameter or member 'get_freq' not described in 'intel_shared_dpll_funcs'

_______________________________________________
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/gt: Drop the timeline->mutex as we wait for retirement (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/gt: Drop the timeline->mutex as we wait for retirement (rev2)
URL   : https://patchwork.freedesktop.org/series/74204/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8055 -> Patchwork_16798
====================================================

Summary
-------

  **FAILURE**

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

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_chamelium@dp-edid-read:
    - fi-icl-u2:          [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8055/fi-icl-u2/igt@kms_chamelium@dp-edid-read.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16798/fi-icl-u2/igt@kms_chamelium@dp-edid-read.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-tgl-y:           [PASS][3] -> [FAIL][4] ([CI#94])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8055/fi-tgl-y/igt@gem_exec_suspend@basic-s4-devices.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16798/fi-tgl-y/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@i915_module_load@reload:
    - fi-ilk-650:         [PASS][5] -> [DMESG-WARN][6] ([i915#116])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8055/fi-ilk-650/igt@i915_module_load@reload.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16798/fi-ilk-650/igt@i915_module_load@reload.html

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       [PASS][7] -> [FAIL][8] ([fdo#111096] / [i915#323])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8055/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16798/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html

  * igt@vgem_basic@dmabuf-fence-before:
    - fi-tgl-y:           [PASS][9] -> [DMESG-WARN][10] ([CI#94] / [i915#402]) +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8055/fi-tgl-y/igt@vgem_basic@dmabuf-fence-before.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16798/fi-tgl-y/igt@vgem_basic@dmabuf-fence-before.html

  
#### Possible fixes ####

  * igt@kms_addfb_basic@addfb25-framebuffer-vs-set-tiling:
    - fi-tgl-y:           [DMESG-WARN][11] ([CI#94] / [i915#402]) -> [PASS][12] +2 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8055/fi-tgl-y/igt@kms_addfb_basic@addfb25-framebuffer-vs-set-tiling.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16798/fi-tgl-y/igt@kms_addfb_basic@addfb25-framebuffer-vs-set-tiling.html

  
  [CI#94]: https://gitlab.freedesktop.org/gfx-ci/i915-infra/issues/94
  [fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
  [i915#116]: https://gitlab.freedesktop.org/drm/intel/issues/116
  [i915#323]: https://gitlab.freedesktop.org/drm/intel/issues/323
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402


Participating hosts (46 -> 47)
------------------------------

  Additional (5): fi-ehl-1 fi-cfl-8109u fi-elk-e7500 fi-bsw-kefka fi-kbl-7560u 
  Missing    (4): fi-ctg-p8600 fi-byt-clapper fi-bdw-samus fi-hsw-4200u 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8055 -> Patchwork_16798

  CI-20190529: 20190529
  CI_DRM_8055: df5c59f81df63a983b7490968e16c3255adc196b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5484: 91b36b61e76901a2bd09fe93ac7bf7b8a60f258c @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16798: 20b19f9f99f11d91bf02b00ac7af718f4753d406 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

20b19f9f99f1 drm/i915/gt: Drop the timeline->mutex as we wait for retirement

== Logs ==

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

[Intel-gfx] [PATCH] drm/i915/gt: Drop the timeline->mutex as we wait for retirement

[Chris Wilson]

As we have pinned the timeline (using tl->active_count), we can safely
drop the tl->mutex as we wait for what we believe to be the final
request on that timeline. This is useful for ensuring that we do not
block the engine heartbeat by hogging the kernel_context's timeline on a
dead GPU.

References: https://gitlab.freedesktop.org/drm/intel/issues/1364
Fixes: 058179e72e09 ("drm/i915/gt: Replace hangcheck by heartbeats")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/gt/intel_gt_requests.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
index 8a5054f21bf8..436412d07689 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
@@ -147,24 +147,31 @@ long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout)
 
 			fence = i915_active_fence_get(&tl->last_request);
 			if (fence) {
+				mutex_unlock(&tl->mutex);
+
 				timeout = dma_fence_wait_timeout(fence,
 								 interruptible,
 								 timeout);
 				dma_fence_put(fence);
+
+				if (!mutex_trylock(&tl->mutex)) {
+					active_count++;
+					goto out_active;
+				}
 			}
 		}
 
 		if (!retire_requests(tl) || flush_submission(gt))
 			active_count++;
+		mutex_unlock(&tl->mutex);
 
-		spin_lock(&timelines->lock);
+out_active:	spin_lock(&timelines->lock);
 
 		/* Resume iteration after dropping lock */
 		list_safe_reset_next(tl, tn, link);
 		if (atomic_dec_and_test(&tl->active_count))
 			list_del(&tl->link);
 
-		mutex_unlock(&tl->mutex);
 
 		/* Defer the final release to after the spinlock */
 		if (refcount_dec_and_test(&tl->kref.refcount)) {
-- 
2.25.1

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

Re: [Intel-gfx] [PATCH] drm/i915/gt: Drop the timeline->mutex as we wait for retirement

[Mika Kuoppala]

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

> As we have pinned the timeline (using tl->active_count), we can safely
> drop the tl->mutex as we wait for what we believe to be the final
> request on that timeline. This is useful for ensuring that we do not
> block the engine heartbeat by hogging the kernel_context's timeline on a
> dead GPU.
>
> References: https://gitlab.freedesktop.org/drm/intel/issues/1364
> Fixes: 058179e72e09 ("drm/i915/gt: Replace hangcheck by heartbeats")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_requests.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> index 8a5054f21bf8..436412d07689 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
> @@ -147,24 +147,31 @@ long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout)
>  
>  			fence = i915_active_fence_get(&tl->last_request);
>  			if (fence) {
> +				mutex_unlock(&tl->mutex);
> +
>  				timeout = dma_fence_wait_timeout(fence,
>  								 interruptible,
>  								 timeout);
>  				dma_fence_put(fence);
> +
> +				if (!mutex_trylock(&tl->mutex)) {

If you can't take it, it must be active and for the retirement
advancement we can bail out early.

Or is there something else with a sampled try?

> +					active_count++;
> +					goto out_active;
> +				}
>  			}
>  		}
>  
>  		if (!retire_requests(tl) || flush_submission(gt))
>  			active_count++;
> +		mutex_unlock(&tl->mutex);
>  
> -		spin_lock(&timelines->lock);
> +out_active:	spin_lock(&timelines->lock);
>  
>  		/* Resume iteration after dropping lock */

You either fixed this comment with this patch.
Or that the comment remains a highly confusing.

>  		list_safe_reset_next(tl, tn, link);
>  		if (atomic_dec_and_test(&tl->active_count))
>  			list_del(&tl->link);

We have the timelines lock and the above seems safe
wtithout the actual mutex.

But the comment is still hauting me.
-Mika

>  
> -		mutex_unlock(&tl->mutex);
>  
>  		/* Defer the final release to after the spinlock */
>  		if (refcount_dec_and_test(&tl->kref.refcount)) {
> -- 
> 2.25.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/gt: Drop the timeline->mutex as we wait for retirement

[Mika Kuoppala]

Mika Kuoppala <mika.kuoppala@linux.intel.com> writes:

> Chris Wilson <chris@chris-wilson.co.uk> writes:
>
>> As we have pinned the timeline (using tl->active_count), we can safely
>> drop the tl->mutex as we wait for what we believe to be the final
>> request on that timeline. This is useful for ensuring that we do not
>> block the engine heartbeat by hogging the kernel_context's timeline on a
>> dead GPU.
>>
>> References: https://gitlab.freedesktop.org/drm/intel/issues/1364
>> Fixes: 058179e72e09 ("drm/i915/gt: Replace hangcheck by heartbeats")
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/gt/intel_gt_requests.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_requests.c b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>> index 8a5054f21bf8..436412d07689 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_gt_requests.c
>> @@ -147,24 +147,31 @@ long intel_gt_retire_requests_timeout(struct intel_gt *gt, long timeout)
>>  
>>  			fence = i915_active_fence_get(&tl->last_request);
>>  			if (fence) {
>> +				mutex_unlock(&tl->mutex);
>> +
>>  				timeout = dma_fence_wait_timeout(fence,
>>  								 interruptible,
>>  								 timeout);
>>  				dma_fence_put(fence);
>> +
>> +				if (!mutex_trylock(&tl->mutex)) {
>
> If you can't take it, it must be active and for the retirement
> advancement we can bail out early.
>
> Or is there something else with a sampled try?

Got some answers in irc: this is best effort so yes,
if it is active it is used and we can bail out and
retire later.

>
>> +					active_count++;
>> +					goto out_active;
>> +				}
>>  			}
>>  		}
>>  
>>  		if (!retire_requests(tl) || flush_submission(gt))
>>  			active_count++;
>> +		mutex_unlock(&tl->mutex);
>>  
>> -		spin_lock(&timelines->lock);
>> +out_active:	spin_lock(&timelines->lock);
>>  
>>  		/* Resume iteration after dropping lock */
>
> You either fixed this comment with this patch.
> Or that the comment remains a highly confusing.
>

This is for the timeline spinlock.
Still, it is very confusing as we only drop it for the wait,
rather deep in iteration.

After discussing with Chris on irc, we agreed that
massaging it to a proper form is warranted.

With the comment bended to match the lock acquisition,

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

>>  		list_safe_reset_next(tl, tn, link);
>>  		if (atomic_dec_and_test(&tl->active_count))
>>  			list_del(&tl->link);
>
> We have the timelines lock and the above seems safe
> wtithout the actual mutex.
>
> But the comment is still hauting me.
> -Mika
>
>>  
>> -		mutex_unlock(&tl->mutex);
>>  
>>  		/* Defer the final release to after the spinlock */
>>  		if (refcount_dec_and_test(&tl->kref.refcount)) {
>> -- 
>> 2.25.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