[Intel-gfx] [PATCH] drm/i915: Unlock the shared hwsp_gtt object after pinning

[Intel-gfx] [PATCH] drm/i915: Unlock the shared hwsp_gtt object after pinning

[Mika Kuoppala]

From: Thomas Hellström <thomas.hellstrom@intel.com>

The hwsp_gtt object is used for sub-allocation and could therefore
be shared by many contexts causing unnecessary contention during
concurrent context pinning.
However since we're currently locking it only for pinning, it remains
resident until we unpin it, and therefore it's safe to drop the
lock early, allowing for concurrent thread access.

Signed-off-by: Thomas Hellström <thomas.hellstrom@intel.com>
---
 drivers/gpu/drm/i915/gt/intel_context.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
index 61b05cd4c47a..65e956ba19e1 100644
--- a/drivers/gpu/drm/i915/gt/intel_context.c
+++ b/drivers/gpu/drm/i915/gt/intel_context.c
@@ -271,6 +271,13 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
 	i915_active_release(&ce->active);
 err_ctx_unpin:
 	intel_context_post_unpin(ce);
+
+	/*
+	 * Unlock the hwsp_ggtt object since it's shared. This is fine for now
+	 * since the lock has been used for pinning only, not fencing.
+	 */
+	i915_gem_ww_unlock_single(ce->timeline->hwsp_ggtt->obj);
+
 	return err;
 }
 
-- 
2.17.1

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

Re: [Intel-gfx] [PATCH] drm/i915: Unlock the shared hwsp_gtt object after pinning

[Chris Wilson]

Quoting Mika Kuoppala (2020-09-03 14:31:44)
> From: Thomas Hellström <thomas.hellstrom@intel.com>
> 
> The hwsp_gtt object is used for sub-allocation and could therefore
> be shared by many contexts causing unnecessary contention during
> concurrent context pinning.
> However since we're currently locking it only for pinning, it remains
> resident until we unpin it, and therefore it's safe to drop the
> lock early, allowing for concurrent thread access.
> 
> Signed-off-by: Thomas Hellström <thomas.hellstrom@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_context.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
> index 61b05cd4c47a..65e956ba19e1 100644
> --- a/drivers/gpu/drm/i915/gt/intel_context.c
> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
> @@ -271,6 +271,13 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
>         i915_active_release(&ce->active);
>  err_ctx_unpin:
>         intel_context_post_unpin(ce);
> +
> +       /*
> +        * Unlock the hwsp_ggtt object since it's shared. This is fine for now
> +        * since the lock has been used for pinning only, not fencing.
> +        */
> +       i915_gem_ww_unlock_single(ce->timeline->hwsp_ggtt->obj);

The timeline is not special here, the same rules for locking/unlock can
equally be applied to all the global state. You may even go as far as
putting a local acquire context here, which would then have avoided the
cross driver pollution.

Anyway, if it works for the timeline, there is no reason to keep the
other globals locked. They are pinned and on completely different usage
cycles to the user objects. [They can't be evicted without interacting
with the HW to ensure the context has been flushed, so far there has
been no way to do so without stalling for a freshly submitted request,
hence we let them retire gracefully and kick the kernel timelines to
flush completed contexts.]
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Unlock the shared hwsp_gtt object after pinning

[Thomas Hellström (Intel)]

On 9/3/20 4:01 PM, Chris Wilson wrote:
> Quoting Mika Kuoppala (2020-09-03 14:31:44)
>> From: Thomas Hellström <thomas.hellstrom@intel.com>
>>
>> The hwsp_gtt object is used for sub-allocation and could therefore
>> be shared by many contexts causing unnecessary contention during
>> concurrent context pinning.
>> However since we're currently locking it only for pinning, it remains
>> resident until we unpin it, and therefore it's safe to drop the
>> lock early, allowing for concurrent thread access.
>>
>> Signed-off-by: Thomas Hellström <thomas.hellstrom@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_context.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c
>> index 61b05cd4c47a..65e956ba19e1 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_context.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_context.c
>> @@ -271,6 +271,13 @@ int __intel_context_do_pin_ww(struct intel_context *ce,
>>          i915_active_release(&ce->active);
>>   err_ctx_unpin:
>>          intel_context_post_unpin(ce);
>> +
>> +       /*
>> +        * Unlock the hwsp_ggtt object since it's shared. This is fine for now
>> +        * since the lock has been used for pinning only, not fencing.
>> +        */
>> +       i915_gem_ww_unlock_single(ce->timeline->hwsp_ggtt->obj);
> The timeline is not special here, the same rules for locking/unlock can
> equally be applied to all the global state. You may even go as far as
> putting a local acquire context here, which would then have avoided the
> cross driver pollution.
>
> Anyway, if it works for the timeline, there is no reason to keep the
> other globals locked. They are pinned and on completely different usage
> cycles to the user objects.

Agreed.  In principle everything we want to perma-pin we can unlock 
early, Keeping it under the same acquire context makes it possible to do 
this as part of a bigger ww transaction. Although initially this 
solution was discarded in favour of removing the suballocation to be 
able to ditch the perma-pinning in the future.

But is this acceptable to address the pi-isolated issue until we have 
something more thought-through in place? Or is there somethin specific 
you want me to change for a R-B?

Thanks,

Thomas


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

[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Unlock the shared hwsp_gtt object after pinning

[Patchwork]

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

== Series Details ==

Series: drm/i915: Unlock the shared hwsp_gtt object after pinning
URL   : https://patchwork.freedesktop.org/series/81290/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8959 -> Patchwork_18439
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-icl-u2:          [PASS][1] -> [DMESG-WARN][2] ([i915#1982])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8959/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18439/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gem_contexts:
    - fi-tgl-u2:          [INCOMPLETE][3] ([i915#2045]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8959/fi-tgl-u2/igt@i915_selftest@live@gem_contexts.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18439/fi-tgl-u2/igt@i915_selftest@live@gem_contexts.html

  * igt@i915_selftest@live@requests:
    - fi-icl-y:           [INCOMPLETE][5] ([i915#1958]) -> [PASS][6]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8959/fi-icl-y/igt@i915_selftest@live@requests.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18439/fi-icl-y/igt@i915_selftest@live@requests.html

  * igt@kms_chamelium@hdmi-crc-fast:
    - fi-kbl-7500u:       [DMESG-FAIL][7] ([i915#165]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8959/fi-kbl-7500u/igt@kms_chamelium@hdmi-crc-fast.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18439/fi-kbl-7500u/igt@kms_chamelium@hdmi-crc-fast.html

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

  [i915#165]: https://gitlab.freedesktop.org/drm/intel/issues/165
  [i915#1958]: https://gitlab.freedesktop.org/drm/intel/issues/1958
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2045]: https://gitlab.freedesktop.org/drm/intel/issues/2045
  [i915#2417]: https://gitlab.freedesktop.org/drm/intel/issues/2417


Participating hosts (39 -> 33)
------------------------------

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


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

  * Linux: CI_DRM_8959 -> Patchwork_18439

  CI-20190529: 20190529
  CI_DRM_8959: b570f98019dd32803271895707fb3ec98eb9636a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5777: c240b5c00d58860e376b012cc3c883c17ae63f37 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18439: d8b76f0b3574014ed467342e9b5aa029e449235e @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d8b76f0b3574 drm/i915: Unlock the shared hwsp_gtt object after pinning

== Logs ==

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

[-- Attachment #1.2: Type: text/html, Size: 3904 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