* [Intel-gfx] [PATCH 1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg
@ 2020-11-20 9:56 Tvrtko Ursulin
2020-11-20 9:56 ` [Intel-gfx] [PATCH 2/2] drm/i915/guc: Use correct lock for CT event handler Tvrtko Ursulin
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2020-11-20 9:56 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Guc->mmio_msg is set under the guc->irq_lock in guc_get_mmio_msg so it
should be consumed under the same lock from guc_handle_mmio_msg.
I am not sure if the overall flow here makes complete sense but at least
the correct lock is now used.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
drivers/gpu/drm/i915/gt/uc/intel_uc.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 4e6070e95fe9..220626c3ad81 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -175,19 +175,15 @@ static void guc_get_mmio_msg(struct intel_guc *guc)
static void guc_handle_mmio_msg(struct intel_guc *guc)
{
- struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
-
/* we need communication to be enabled to reply to GuC */
GEM_BUG_ON(!guc_communication_enabled(guc));
- if (!guc->mmio_msg)
- return;
-
- spin_lock_irq(&i915->irq_lock);
- intel_guc_to_host_process_recv_msg(guc, &guc->mmio_msg, 1);
- spin_unlock_irq(&i915->irq_lock);
-
- guc->mmio_msg = 0;
+ spin_lock_irq(&guc->irq_lock);
+ if (guc->mmio_msg) {
+ intel_guc_to_host_process_recv_msg(guc, &guc->mmio_msg, 1);
+ guc->mmio_msg = 0;
+ }
+ spin_unlock_irq(&guc->irq_lock);
}
static void guc_reset_interrupts(struct intel_guc *guc)
--
2.25.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Intel-gfx] [PATCH 2/2] drm/i915/guc: Use correct lock for CT event handler
2020-11-20 9:56 [Intel-gfx] [PATCH 1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg Tvrtko Ursulin
@ 2020-11-20 9:56 ` Tvrtko Ursulin
2020-11-20 14:32 ` Chris Wilson
2020-11-20 14:11 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg Patchwork
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2020-11-20 9:56 UTC (permalink / raw)
To: Intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
CT event handler is called under the gt->irq_lock from the interrupt
handling paths so make it the same from the init path. I don't think this
mismatch caused any functional issue but we need to wean the code of the
global i915->irq_lock.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
drivers/gpu/drm/i915/gt/uc/intel_uc.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 220626c3ad81..6a0452815c41 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -203,7 +203,8 @@ static void guc_disable_interrupts(struct intel_guc *guc)
static int guc_enable_communication(struct intel_guc *guc)
{
- struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
+ struct intel_gt *gt = guc_to_gt(guc);
+ struct drm_i915_private *i915 = gt->i915;
int ret;
GEM_BUG_ON(guc_communication_enabled(guc));
@@ -223,9 +224,9 @@ static int guc_enable_communication(struct intel_guc *guc)
guc_enable_interrupts(guc);
/* check for CT messages received before we enabled interrupts */
- spin_lock_irq(&i915->irq_lock);
+ spin_lock_irq(>->irq_lock);
intel_guc_ct_event_handler(&guc->ct);
- spin_unlock_irq(&i915->irq_lock);
+ spin_unlock_irq(>->irq_lock);
drm_dbg(&i915->drm, "GuC communication enabled\n");
--
2.25.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg
2020-11-20 9:56 [Intel-gfx] [PATCH 1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg Tvrtko Ursulin
2020-11-20 9:56 ` [Intel-gfx] [PATCH 2/2] drm/i915/guc: Use correct lock for CT event handler Tvrtko Ursulin
@ 2020-11-20 14:11 ` Patchwork
2020-11-20 14:26 ` [Intel-gfx] [PATCH 1/2] " Chris Wilson
2020-11-20 16:13 ` [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] " Patchwork
3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2020-11-20 14:11 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 4749 bytes --]
== Series Details ==
Series: series starting with [1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg
URL : https://patchwork.freedesktop.org/series/84100/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_9370 -> Patchwork_18950
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/index.html
New tests
---------
New tests have been introduced between CI_DRM_9370 and Patchwork_18950:
### New CI tests (1) ###
* boot:
- Statuses : 41 pass(s)
- Exec time: [0.0] s
Known issues
------------
Here are the changes found in Patchwork_18950 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@core_hotunplug@unbind-rebind:
- fi-tgl-u2: [PASS][1] -> [DMESG-WARN][2] ([i915#1982])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/fi-tgl-u2/igt@core_hotunplug@unbind-rebind.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/fi-tgl-u2/igt@core_hotunplug@unbind-rebind.html
* igt@i915_module_load@reload:
- fi-icl-u2: [PASS][3] -> [DMESG-WARN][4] ([i915#1982]) +2 similar issues
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/fi-icl-u2/igt@i915_module_load@reload.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/fi-icl-u2/igt@i915_module_load@reload.html
- fi-icl-y: [PASS][5] -> [DMESG-WARN][6] ([i915#1982])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/fi-icl-y/igt@i915_module_load@reload.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/fi-icl-y/igt@i915_module_load@reload.html
* igt@i915_selftest@live@execlists:
- fi-cml-s: [PASS][7] -> [INCOMPLETE][8] ([i915#1037])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/fi-cml-s/igt@i915_selftest@live@execlists.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/fi-cml-s/igt@i915_selftest@live@execlists.html
#### Possible fixes ####
* igt@i915_pm_rpm@basic-pci-d3-state:
- fi-bsw-kefka: [DMESG-WARN][9] ([i915#1982]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html
* igt@kms_chamelium@dp-crc-fast:
- fi-kbl-7500u: [DMESG-WARN][11] ([i915#1982] / [i915#262]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-apl-guc: [DMESG-WARN][13] ([i915#1635] / [i915#1982]) -> [PASS][14]
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/fi-apl-guc/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/fi-apl-guc/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
* igt@kms_cursor_legacy@basic-flip-before-cursor-atomic:
- fi-icl-u2: [DMESG-WARN][15] ([i915#1982]) -> [PASS][16] +1 similar issue
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-before-cursor-atomic.html
[i915#1037]: https://gitlab.freedesktop.org/drm/intel/issues/1037
[i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
[i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
[i915#262]: https://gitlab.freedesktop.org/drm/intel/issues/262
Participating hosts (45 -> 41)
------------------------------
Additional (1): fi-tgl-y
Missing (5): fi-ilk-m540 fi-hsw-4200u fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus
Build changes
-------------
* Linux: CI_DRM_9370 -> Patchwork_18950
CI-20190529: 20190529
CI_DRM_9370: e74e64a27fb256d20dc574e0eb741ca59630747d @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5863: 849de1780d33c6749e0a26dc3c642eb9b3d6cd42 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_18950: cd580a4110745957039f7da8f13ac93093f9d681 @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
cd580a411074 drm/i915/guc: Use correct lock for CT event handler
19ea2e77b317 drm/i915/guc: Use correct lock for accessing guc->mmio_msg
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/index.html
[-- Attachment #1.2: Type: text/html, Size: 5990 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg
2020-11-20 9:56 [Intel-gfx] [PATCH 1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg Tvrtko Ursulin
2020-11-20 9:56 ` [Intel-gfx] [PATCH 2/2] drm/i915/guc: Use correct lock for CT event handler Tvrtko Ursulin
2020-11-20 14:11 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg Patchwork
@ 2020-11-20 14:26 ` Chris Wilson
2020-11-20 14:48 ` Tvrtko Ursulin
2020-11-20 16:13 ` [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] " Patchwork
3 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2020-11-20 14:26 UTC (permalink / raw)
To: Intel-gfx, Tvrtko Ursulin
Quoting Tvrtko Ursulin (2020-11-20 09:56:35)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Guc->mmio_msg is set under the guc->irq_lock in guc_get_mmio_msg so it
> should be consumed under the same lock from guc_handle_mmio_msg.
>
> I am not sure if the overall flow here makes complete sense but at least
> the correct lock is now used.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 4e6070e95fe9..220626c3ad81 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -175,19 +175,15 @@ static void guc_get_mmio_msg(struct intel_guc *guc)
>
> static void guc_handle_mmio_msg(struct intel_guc *guc)
> {
> - struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> -
> /* we need communication to be enabled to reply to GuC */
> GEM_BUG_ON(!guc_communication_enabled(guc));
>
> - if (!guc->mmio_msg)
> - return;
> -
> - spin_lock_irq(&i915->irq_lock);
> - intel_guc_to_host_process_recv_msg(guc, &guc->mmio_msg, 1);
> - spin_unlock_irq(&i915->irq_lock);
> -
> - guc->mmio_msg = 0;
> + spin_lock_irq(&guc->irq_lock);
> + if (guc->mmio_msg) {
> + intel_guc_to_host_process_recv_msg(guc, &guc->mmio_msg, 1);
> + guc->mmio_msg = 0;
> + }
> + spin_unlock_irq(&guc->irq_lock);
Based on just looking at mmio_msg, the locking should be guc->irq_lock, and
guc->mmio_msg = 0 should be pulled under the lock.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm/i915/guc: Use correct lock for CT event handler
2020-11-20 9:56 ` [Intel-gfx] [PATCH 2/2] drm/i915/guc: Use correct lock for CT event handler Tvrtko Ursulin
@ 2020-11-20 14:32 ` Chris Wilson
2020-11-20 14:43 ` Tvrtko Ursulin
0 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2020-11-20 14:32 UTC (permalink / raw)
To: Intel-gfx, Tvrtko Ursulin
Quoting Tvrtko Ursulin (2020-11-20 09:56:36)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> CT event handler is called under the gt->irq_lock from the interrupt
> handling paths so make it the same from the init path. I don't think this
> mismatch caused any functional issue but we need to wean the code of the
> global i915->irq_lock.
ct_read definitely wants to be serialised. Is guc->irq_lock the right
choice?
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index 220626c3ad81..6a0452815c41 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -203,7 +203,8 @@ static void guc_disable_interrupts(struct intel_guc *guc)
>
> static int guc_enable_communication(struct intel_guc *guc)
> {
> - struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> + struct intel_gt *gt = guc_to_gt(guc);
> + struct drm_i915_private *i915 = gt->i915;
> int ret;
>
> GEM_BUG_ON(guc_communication_enabled(guc));
> @@ -223,9 +224,9 @@ static int guc_enable_communication(struct intel_guc *guc)
> guc_enable_interrupts(guc);
>
> /* check for CT messages received before we enabled interrupts */
> - spin_lock_irq(&i915->irq_lock);
> + spin_lock_irq(>->irq_lock);
> intel_guc_ct_event_handler(&guc->ct);
> - spin_unlock_irq(&i915->irq_lock);
> + spin_unlock_irq(>->irq_lock);
You used guc->irq_lock in the previous patch. I suggest
intel_guc_ct_event_handler() should specify what lock it requires.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm/i915/guc: Use correct lock for CT event handler
2020-11-20 14:32 ` Chris Wilson
@ 2020-11-20 14:43 ` Tvrtko Ursulin
2020-11-20 16:45 ` Daniele Ceraolo Spurio
0 siblings, 1 reply; 9+ messages in thread
From: Tvrtko Ursulin @ 2020-11-20 14:43 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx
On 20/11/2020 14:32, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-11-20 09:56:36)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> CT event handler is called under the gt->irq_lock from the interrupt
>> handling paths so make it the same from the init path. I don't think this
>> mismatch caused any functional issue but we need to wean the code of the
>> global i915->irq_lock.
>
> ct_read definitely wants to be serialised. Is guc->irq_lock the right
> choice?
Not under my understanding and also confirmed by Daniele off line.
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index 220626c3ad81..6a0452815c41 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -203,7 +203,8 @@ static void guc_disable_interrupts(struct intel_guc *guc)
>>
>> static int guc_enable_communication(struct intel_guc *guc)
>> {
>> - struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
>> + struct intel_gt *gt = guc_to_gt(guc);
>> + struct drm_i915_private *i915 = gt->i915;
>> int ret;
>>
>> GEM_BUG_ON(guc_communication_enabled(guc));
>> @@ -223,9 +224,9 @@ static int guc_enable_communication(struct intel_guc *guc)
>> guc_enable_interrupts(guc);
>>
>> /* check for CT messages received before we enabled interrupts */
>> - spin_lock_irq(&i915->irq_lock);
>> + spin_lock_irq(>->irq_lock);
>> intel_guc_ct_event_handler(&guc->ct);
>> - spin_unlock_irq(&i915->irq_lock);
>> + spin_unlock_irq(>->irq_lock);
>
> You used guc->irq_lock in the previous patch. I suggest
> intel_guc_ct_event_handler() should specify what lock it requires.
There are indeed too many locks and too little asserts to help the reader.
But the other end of the state ct_read needs is updated from the GuC
firmware itself, which then send the interrupt, which we process in:
guc_irq_handler
-> intel_guc_to_host_event_handler
-> intel_guc_ct_event_handler
And this side runs under the gt->irq_lock.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg
2020-11-20 14:26 ` [Intel-gfx] [PATCH 1/2] " Chris Wilson
@ 2020-11-20 14:48 ` Tvrtko Ursulin
0 siblings, 0 replies; 9+ messages in thread
From: Tvrtko Ursulin @ 2020-11-20 14:48 UTC (permalink / raw)
To: Chris Wilson, Intel-gfx
On 20/11/2020 14:26, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2020-11-20 09:56:35)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Guc->mmio_msg is set under the guc->irq_lock in guc_get_mmio_msg so it
>> should be consumed under the same lock from guc_handle_mmio_msg.
>>
>> I am not sure if the overall flow here makes complete sense but at least
>> the correct lock is now used.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 16 ++++++----------
>> 1 file changed, 6 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index 4e6070e95fe9..220626c3ad81 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -175,19 +175,15 @@ static void guc_get_mmio_msg(struct intel_guc *guc)
>>
>> static void guc_handle_mmio_msg(struct intel_guc *guc)
>> {
>> - struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
>> -
>> /* we need communication to be enabled to reply to GuC */
>> GEM_BUG_ON(!guc_communication_enabled(guc));
>>
>> - if (!guc->mmio_msg)
>> - return;
>> -
>> - spin_lock_irq(&i915->irq_lock);
>> - intel_guc_to_host_process_recv_msg(guc, &guc->mmio_msg, 1);
>> - spin_unlock_irq(&i915->irq_lock);
>> -
>> - guc->mmio_msg = 0;
>> + spin_lock_irq(&guc->irq_lock);
>> + if (guc->mmio_msg) {
>> + intel_guc_to_host_process_recv_msg(guc, &guc->mmio_msg, 1);
>> + guc->mmio_msg = 0;
>> + }
>> + spin_unlock_irq(&guc->irq_lock);
>
> Based on just looking at mmio_msg, the locking should be guc->irq_lock, and
> guc->mmio_msg = 0 should be pulled under the lock.
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Thanks, the thing which made me say that I am not sure it completely makes sense is that the mmio_msg appears to only be used from guc_enable_communication and
guc_disable_communication, which I would assume should be mutually exclusive by itself already. So I was not sure what value is there in the locking around mmio_msg access.
And even in guc_enable_communication we have a sequence of:
guc_get_mmio_msg(guc);
guc_handle_mmio_msg(guc);
Which expands to:
static void guc_get_mmio_msg(struct intel_guc *guc)
{
u32 val;
spin_lock_irq(&guc->irq_lock);
val = intel_uncore_read(guc_to_gt(guc)->uncore, SOFT_SCRATCH(15));
guc->mmio_msg |= val & guc->msg_enabled_mask;
/*
* clear all events, including the ones we're not currently servicing,
* to make sure we don't try to process a stale message if we enable
* handling of more events later.
*/
guc_clear_mmio_msg(guc);
spin_unlock_irq(&guc->irq_lock);
}
static void guc_handle_mmio_msg(struct intel_guc *guc)
{
/* we need communication to be enabled to reply to GuC */
GEM_BUG_ON(!guc_communication_enabled(guc));
spin_lock_irq(&guc->irq_lock);
if (guc->mmio_msg) {
intel_guc_to_host_process_recv_msg(guc, &guc->mmio_msg, 1);
guc->mmio_msg = 0;
}
spin_unlock_irq(&guc->irq_lock);
}
So it seems a bit pointless. Nevertheless I only wanted to remove usage of i915->irq_lock.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg
2020-11-20 9:56 [Intel-gfx] [PATCH 1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg Tvrtko Ursulin
` (2 preceding siblings ...)
2020-11-20 14:26 ` [Intel-gfx] [PATCH 1/2] " Chris Wilson
@ 2020-11-20 16:13 ` Patchwork
3 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2020-11-20 16:13 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 20950 bytes --]
== Series Details ==
Series: series starting with [1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg
URL : https://patchwork.freedesktop.org/series/84100/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_9370_full -> Patchwork_18950_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in Patchwork_18950_full:
### IGT changes ###
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* {igt@gem_userptr_blits@vma-merge}:
- shard-hsw: NOTRUN -> [FAIL][1]
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-hsw5/igt@gem_userptr_blits@vma-merge.html
New tests
---------
New tests have been introduced between CI_DRM_9370_full and Patchwork_18950_full:
### New CI tests (1) ###
* boot:
- Statuses : 200 pass(s)
- Exec time: [0.0] s
Known issues
------------
Here are the changes found in Patchwork_18950_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_pm_rpm@i2c:
- shard-glk: [PASS][2] -> [DMESG-WARN][3] ([i915#1982]) +2 similar issues
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-glk7/igt@i915_pm_rpm@i2c.html
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-glk9/igt@i915_pm_rpm@i2c.html
* igt@kms_cursor_crc@pipe-c-cursor-64x21-random:
- shard-skl: [PASS][4] -> [FAIL][5] ([i915#54]) +1 similar issue
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl5/igt@kms_cursor_crc@pipe-c-cursor-64x21-random.html
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl1/igt@kms_cursor_crc@pipe-c-cursor-64x21-random.html
* igt@kms_cursor_edge_walk@pipe-c-256x256-left-edge:
- shard-apl: [PASS][6] -> [DMESG-WARN][7] ([i915#1635] / [i915#1982]) +1 similar issue
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-apl2/igt@kms_cursor_edge_walk@pipe-c-256x256-left-edge.html
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-apl7/igt@kms_cursor_edge_walk@pipe-c-256x256-left-edge.html
* igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
- shard-hsw: [PASS][8] -> [FAIL][9] ([i915#96])
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-hsw2/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-hsw6/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html
* igt@kms_flip@flip-vs-blocking-wf-vblank@a-edp1:
- shard-skl: [PASS][10] -> [DMESG-WARN][11] ([i915#1982]) +9 similar issues
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl3/igt@kms_flip@flip-vs-blocking-wf-vblank@a-edp1.html
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl2/igt@kms_flip@flip-vs-blocking-wf-vblank@a-edp1.html
* igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1:
- shard-skl: [PASS][12] -> [FAIL][13] ([i915#79])
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl5/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl1/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html
- shard-tglb: [PASS][14] -> [FAIL][15] ([i915#2598])
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-tglb5/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-tglb1/igt@kms_flip@flip-vs-expired-vblank-interruptible@a-edp1.html
* igt@kms_flip@flip-vs-expired-vblank-interruptible@c-hdmi-a1:
- shard-glk: [PASS][16] -> [FAIL][17] ([i915#79]) +1 similar issue
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-glk9/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-hdmi-a1.html
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-glk7/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-hdmi-a1.html
* igt@kms_flip@plain-flip-fb-recreate-interruptible@b-edp1:
- shard-skl: [PASS][18] -> [FAIL][19] ([i915#2122]) +1 similar issue
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl5/igt@kms_flip@plain-flip-fb-recreate-interruptible@b-edp1.html
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl1/igt@kms_flip@plain-flip-fb-recreate-interruptible@b-edp1.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-wc:
- shard-kbl: [PASS][20] -> [DMESG-WARN][21] ([i915#1982]) +2 similar issues
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-kbl2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-wc.html
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-kbl4/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-pri-indfb-draw-mmap-wc.html
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-msflip-blt:
- shard-tglb: [PASS][22] -> [DMESG-WARN][23] ([i915#1982]) +2 similar issues
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-tglb2/igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-msflip-blt.html
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-tglb8/igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-msflip-blt.html
* igt@kms_psr2_su@frontbuffer:
- shard-iclb: [PASS][24] -> [SKIP][25] ([fdo#109642] / [fdo#111068])
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-iclb2/igt@kms_psr2_su@frontbuffer.html
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-iclb7/igt@kms_psr2_su@frontbuffer.html
* igt@kms_psr@psr2_primary_page_flip:
- shard-iclb: [PASS][26] -> [SKIP][27] ([fdo#109441]) +1 similar issue
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-iclb2/igt@kms_psr@psr2_primary_page_flip.html
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-iclb7/igt@kms_psr@psr2_primary_page_flip.html
* igt@perf_pmu@module-unload:
- shard-iclb: [PASS][28] -> [DMESG-WARN][29] ([i915#1982] / [i915#262])
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-iclb4/igt@perf_pmu@module-unload.html
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-iclb1/igt@perf_pmu@module-unload.html
#### Possible fixes ####
* igt@gem_exec_reloc@basic-many-active@rcs0:
- shard-apl: [FAIL][30] ([i915#1635] / [i915#2389]) -> [PASS][31]
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-apl2/igt@gem_exec_reloc@basic-many-active@rcs0.html
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-apl2/igt@gem_exec_reloc@basic-many-active@rcs0.html
* igt@gem_exec_whisper@basic-fds-forked-all:
- shard-glk: [DMESG-WARN][32] ([i915#118] / [i915#95]) -> [PASS][33]
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-glk5/igt@gem_exec_whisper@basic-fds-forked-all.html
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-glk8/igt@gem_exec_whisper@basic-fds-forked-all.html
* igt@gem_huc_copy@huc-copy:
- shard-tglb: [SKIP][34] ([i915#2190]) -> [PASS][35]
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-tglb6/igt@gem_huc_copy@huc-copy.html
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-tglb3/igt@gem_huc_copy@huc-copy.html
* igt@gen9_exec_parse@allowed-all:
- shard-skl: [DMESG-WARN][36] ([i915#1436] / [i915#716]) -> [PASS][37]
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl7/igt@gen9_exec_parse@allowed-all.html
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl4/igt@gen9_exec_parse@allowed-all.html
* igt@i915_pm_rpm@fences-dpms:
- shard-glk: [DMESG-WARN][38] ([i915#1982]) -> [PASS][39] +2 similar issues
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-glk8/igt@i915_pm_rpm@fences-dpms.html
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-glk6/igt@i915_pm_rpm@fences-dpms.html
* igt@kms_cursor_crc@pipe-a-cursor-128x128-onscreen:
- shard-skl: [FAIL][40] ([i915#54]) -> [PASS][41] +4 similar issues
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl8/igt@kms_cursor_crc@pipe-a-cursor-128x128-onscreen.html
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl3/igt@kms_cursor_crc@pipe-a-cursor-128x128-onscreen.html
* igt@kms_cursor_legacy@flip-vs-cursor-atomic:
- shard-skl: [FAIL][42] ([i915#2346]) -> [PASS][43]
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl10/igt@kms_cursor_legacy@flip-vs-cursor-atomic.html
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl2/igt@kms_cursor_legacy@flip-vs-cursor-atomic.html
* igt@kms_draw_crc@draw-method-xrgb2101010-mmap-wc-xtiled:
- shard-apl: [DMESG-WARN][44] ([i915#1635] / [i915#1982]) -> [PASS][45]
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-apl1/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-wc-xtiled.html
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-apl3/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-wc-xtiled.html
* igt@kms_flip@2x-flip-vs-suspend@ab-vga1-hdmi-a1:
- shard-hsw: [DMESG-WARN][46] ([i915#1982]) -> [PASS][47]
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-hsw1/igt@kms_flip@2x-flip-vs-suspend@ab-vga1-hdmi-a1.html
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-hsw7/igt@kms_flip@2x-flip-vs-suspend@ab-vga1-hdmi-a1.html
* igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1:
- shard-skl: [FAIL][48] ([i915#79]) -> [PASS][49]
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl5/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1.html
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl1/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1.html
* igt@kms_flip@flip-vs-expired-vblank@a-dp1:
- shard-apl: [FAIL][50] ([i915#1635] / [i915#79]) -> [PASS][51]
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-apl7/igt@kms_flip@flip-vs-expired-vblank@a-dp1.html
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-apl2/igt@kms_flip@flip-vs-expired-vblank@a-dp1.html
* igt@kms_flip@flip-vs-suspend-interruptible@c-edp1:
- shard-skl: [INCOMPLETE][52] ([i915#198]) -> [PASS][53]
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl10/igt@kms_flip@flip-vs-suspend-interruptible@c-edp1.html
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl6/igt@kms_flip@flip-vs-suspend-interruptible@c-edp1.html
* igt@kms_flip@plain-flip-fb-recreate-interruptible@c-edp1:
- shard-skl: [FAIL][54] ([i915#2122]) -> [PASS][55]
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl5/igt@kms_flip@plain-flip-fb-recreate-interruptible@c-edp1.html
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl1/igt@kms_flip@plain-flip-fb-recreate-interruptible@c-edp1.html
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt:
- shard-kbl: [DMESG-WARN][56] ([i915#1982]) -> [PASS][57] +1 similar issue
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-kbl6/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt.html
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-kbl2/igt@kms_frontbuffer_tracking@fbc-1p-primscrn-shrfb-msflip-blt.html
* igt@kms_frontbuffer_tracking@psr-rgb565-draw-blt:
- shard-tglb: [DMESG-WARN][58] ([i915#1982]) -> [PASS][59]
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-tglb8/igt@kms_frontbuffer_tracking@psr-rgb565-draw-blt.html
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-tglb7/igt@kms_frontbuffer_tracking@psr-rgb565-draw-blt.html
* igt@kms_getfb@getfb-addfb-different-handles:
- shard-skl: [DMESG-WARN][60] ([i915#1982]) -> [PASS][61] +3 similar issues
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl9/igt@kms_getfb@getfb-addfb-different-handles.html
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl8/igt@kms_getfb@getfb-addfb-different-handles.html
* igt@kms_hdr@bpc-switch-dpms:
- shard-skl: [FAIL][62] ([i915#1188]) -> [PASS][63] +1 similar issue
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl10/igt@kms_hdr@bpc-switch-dpms.html
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl5/igt@kms_hdr@bpc-switch-dpms.html
* igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
- shard-skl: [FAIL][64] ([fdo#108145] / [i915#265]) -> [PASS][65]
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl7/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl4/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
* igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
- shard-skl: [DMESG-FAIL][66] ([fdo#108145] / [i915#1982]) -> [PASS][67]
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl1/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
* igt@kms_plane_lowres@pipe-c-tiling-yf:
- shard-kbl: [DMESG-WARN][68] ([i915#165] / [i915#78]) -> [PASS][69]
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-kbl2/igt@kms_plane_lowres@pipe-c-tiling-yf.html
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-kbl6/igt@kms_plane_lowres@pipe-c-tiling-yf.html
* igt@kms_psr@psr2_sprite_plane_move:
- shard-iclb: [SKIP][70] ([fdo#109441]) -> [PASS][71] +2 similar issues
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-iclb8/igt@kms_psr@psr2_sprite_plane_move.html
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html
* igt@kms_setmode@basic:
- shard-hsw: [FAIL][72] ([i915#31]) -> [PASS][73]
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-hsw2/igt@kms_setmode@basic.html
[73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-hsw6/igt@kms_setmode@basic.html
* igt@perf_pmu@module-unload:
- shard-hsw: [DMESG-WARN][74] ([i915#1982] / [i915#262]) -> [PASS][75]
[74]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-hsw6/igt@perf_pmu@module-unload.html
[75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-hsw7/igt@perf_pmu@module-unload.html
* igt@sysfs_timeslice_duration@timeout@bcs0:
- shard-skl: [FAIL][76] ([i915#1732]) -> [PASS][77] +1 similar issue
[76]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl2/igt@sysfs_timeslice_duration@timeout@bcs0.html
[77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl6/igt@sysfs_timeslice_duration@timeout@bcs0.html
#### Warnings ####
* igt@i915_pm_dc@dc3co-vpb-simulation:
- shard-iclb: [SKIP][78] ([i915#588]) -> [SKIP][79] ([i915#658])
[78]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-iclb2/igt@i915_pm_dc@dc3co-vpb-simulation.html
[79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-iclb6/igt@i915_pm_dc@dc3co-vpb-simulation.html
* igt@i915_pm_dc@dc6-psr:
- shard-skl: [INCOMPLETE][80] ([i915#198]) -> [FAIL][81] ([i915#454])
[80]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl4/igt@i915_pm_dc@dc6-psr.html
[81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl5/igt@i915_pm_dc@dc6-psr.html
* igt@i915_pm_rc6_residency@rc6-idle:
- shard-iclb: [WARN][82] -> [FAIL][83] ([i915#2680])
[82]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-iclb2/igt@i915_pm_rc6_residency@rc6-idle.html
[83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-iclb6/igt@i915_pm_rc6_residency@rc6-idle.html
* igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend:
- shard-apl: [DMESG-WARN][84] ([i915#1635] / [i915#2635]) -> [INCOMPLETE][85] ([i915#1635] / [i915#2635])
[84]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-apl6/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html
[85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-apl6/igt@kms_vblank@pipe-b-ts-continuation-dpms-suspend.html
* igt@runner@aborted:
- shard-skl: ([FAIL][86], [FAIL][87]) ([i915#1436] / [i915#2295] / [i915#483]) -> ([FAIL][88], [FAIL][89], [FAIL][90]) ([i915#1436] / [i915#2029] / [i915#2295] / [i915#483])
[86]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl7/igt@runner@aborted.html
[87]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_9370/shard-skl1/igt@runner@aborted.html
[88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl9/igt@runner@aborted.html
[89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl3/igt@runner@aborted.html
[90]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/shard-skl3/igt@runner@aborted.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
[fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
[i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
[i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
[i915#1436]: https://gitlab.freedesktop.org/drm/intel/issues/1436
[i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
[i915#165]: https://gitlab.freedesktop.org/drm/intel/issues/165
[i915#1732]: https://gitlab.freedesktop.org/drm/intel/issues/1732
[i915#198]: https://gitlab.freedesktop.org/drm/intel/issues/198
[i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
[i915#2029]: https://gitlab.freedesktop.org/drm/intel/issues/2029
[i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
[i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190
[i915#2295]: https://gitlab.freedesktop.org/drm/intel/issues/2295
[i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
[i915#2389]: https://gitlab.freedesktop.org/drm/intel/issues/2389
[i915#2597]: https://gitlab.freedesktop.org/drm/intel/issues/2597
[i915#2598]: https://gitlab.freedesktop.org/drm/intel/issues/2598
[i915#262]: https://gitlab.freedesktop.org/drm/intel/issues/262
[i915#2635]: https://gitlab.freedesktop.org/drm/intel/issues/2635
[i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
[i915#2680]: https://gitlab.freedesktop.org/drm/intel/issues/2680
[i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
[i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
[i915#483]: https://gitlab.freedesktop.org/drm/intel/issues/483
[i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
[i915#588]: https://gitlab.freedesktop.org/drm/intel/issues/588
[i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
[i915#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
[i915#78]: https://gitlab.freedesktop.org/drm/intel/issues/78
[i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
[i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
[i915#96]: https://gitlab.freedesktop.org/drm/intel/issues/96
Participating hosts (10 -> 10)
------------------------------
No changes in participating hosts
Build changes
-------------
* Linux: CI_DRM_9370 -> Patchwork_18950
CI-20190529: 20190529
CI_DRM_9370: e74e64a27fb256d20dc574e0eb741ca59630747d @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5863: 849de1780d33c6749e0a26dc3c642eb9b3d6cd42 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_18950: cd580a4110745957039f7da8f13ac93093f9d681 @ git://anongit.freedesktop.org/gfx-ci/linux
piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18950/index.html
[-- Attachment #1.2: Type: text/html, Size: 25055 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-gfx] [PATCH 2/2] drm/i915/guc: Use correct lock for CT event handler
2020-11-20 14:43 ` Tvrtko Ursulin
@ 2020-11-20 16:45 ` Daniele Ceraolo Spurio
0 siblings, 0 replies; 9+ messages in thread
From: Daniele Ceraolo Spurio @ 2020-11-20 16:45 UTC (permalink / raw)
To: Tvrtko Ursulin, Chris Wilson, Intel-gfx
On 11/20/2020 6:43 AM, Tvrtko Ursulin wrote:
>
> On 20/11/2020 14:32, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2020-11-20 09:56:36)
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> CT event handler is called under the gt->irq_lock from the interrupt
>>> handling paths so make it the same from the init path. I don't think
>>> this
>>> mismatch caused any functional issue but we need to wean the code of
>>> the
>>> global i915->irq_lock.
>>
>> ct_read definitely wants to be serialised. Is guc->irq_lock the right
>> choice?
>
> Not under my understanding and also confirmed by Daniele off line.
>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> ---
>>> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> index 220626c3ad81..6a0452815c41 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> @@ -203,7 +203,8 @@ static void guc_disable_interrupts(struct
>>> intel_guc *guc)
>>> static int guc_enable_communication(struct intel_guc *guc)
>>> {
>>> - struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
>>> + struct intel_gt *gt = guc_to_gt(guc);
>>> + struct drm_i915_private *i915 = gt->i915;
>>> int ret;
>>> GEM_BUG_ON(guc_communication_enabled(guc));
>>> @@ -223,9 +224,9 @@ static int guc_enable_communication(struct
>>> intel_guc *guc)
>>> guc_enable_interrupts(guc);
>>> /* check for CT messages received before we enabled
>>> interrupts */
>>> - spin_lock_irq(&i915->irq_lock);
>>> + spin_lock_irq(>->irq_lock);
>>> intel_guc_ct_event_handler(&guc->ct);
>>> - spin_unlock_irq(&i915->irq_lock);
>>> + spin_unlock_irq(>->irq_lock);
>>
>> You used guc->irq_lock in the previous patch. I suggest
>> intel_guc_ct_event_handler() should specify what lock it requires.
>
> There are indeed too many locks and too little asserts to help the
> reader.
>
> But the other end of the state ct_read needs is updated from the GuC
> firmware itself, which then send the interrupt, which we process in:
>
> guc_irq_handler
> -> intel_guc_to_host_event_handler
> -> intel_guc_ct_event_handler
>
> And this side runs under the gt->irq_lock.
>
guc->irq_lock is not very aptly named, as it is used to protect access
to the guc interrupt state variables (msg_enabled_mask, mmio_msg) and
has nothing to do with protecting the interrupt handler. For that, as
Tvrtko said, the GuC code can use the same lock the rest of the GT uses,
i.e. gt->irq_lock. Maybe we can rename guc->irq_lock to
guc->msg_state_lock for clarity?
Anyway, this is:
Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Daniele
> Regards,
>
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-11-20 16:45 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 9:56 [Intel-gfx] [PATCH 1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg Tvrtko Ursulin
2020-11-20 9:56 ` [Intel-gfx] [PATCH 2/2] drm/i915/guc: Use correct lock for CT event handler Tvrtko Ursulin
2020-11-20 14:32 ` Chris Wilson
2020-11-20 14:43 ` Tvrtko Ursulin
2020-11-20 16:45 ` Daniele Ceraolo Spurio
2020-11-20 14:11 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915/guc: Use correct lock for accessing guc->mmio_msg Patchwork
2020-11-20 14:26 ` [Intel-gfx] [PATCH 1/2] " Chris Wilson
2020-11-20 14:48 ` Tvrtko Ursulin
2020-11-20 16:13 ` [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.