diff mbox series

drm/xe/xe_ggtt: No need to use xe_pm_runtime_get_noresume

Message ID 20240429162915.1831945-1-ashutosh.dixit@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/xe/xe_ggtt: No need to use xe_pm_runtime_get_noresume | expand

Commit Message

Dixit, Ashutosh April 29, 2024, 4:29 p.m. UTC 
Switching from xe_device_mem_access_get/put to xe_pm_runtime_get/put
results in the following WARNING in xe_oa:

[11614.356168] xe 0000:00:02.0: Missing outer runtime PM protection
[11614.356187] WARNING: CPU: 1 PID: 13075 at drivers/gpu/drm/xe/xe_pm.c:549 xe_pm_runtime_get_noresume+0x60/0x80 [xe]
...
[11614.356377] Call Trace:
[11614.356379]  <TASK>
[11614.356381]  ? __warn+0x7e/0x180
[11614.356387]  ? xe_pm_runtime_get_noresume+0x60/0x80 [xe]
[11614.356507]  xe_ggtt_remove_node+0x22/0x80 [xe]
[11614.356546]  xe_ttm_bo_destroy+0xea/0xf0 [xe]
[11614.356579]  xe_oa_stream_destroy+0xf7/0x120 [xe]
[11614.356627]  xe_oa_release+0x35/0xc0 [xe]
[11614.356673]  __fput+0xa1/0x2d0
[11614.356679]  __x64_sys_close+0x37/0x80
[11614.356697]  do_syscall_64+0x6d/0x140
[11614.356700]  entry_SYSCALL_64_after_hwframe+0x71/0x79
[11614.356702] RIP: 0033:0x7f2b37314f67

There seems to be no reason to use xe_pm_runtime_get_noresume in xe_ggtt
functions. Just use xe_pm_runtime_get.

Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/xe/xe_ggtt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Rodrigo Vivi April 29, 2024, 6:24 p.m. UTC | #1 
On Mon, Apr 29, 2024 at 09:29:15AM -0700, Ashutosh Dixit wrote:
> Switching from xe_device_mem_access_get/put to xe_pm_runtime_get/put
> results in the following WARNING in xe_oa:
> 
> [11614.356168] xe 0000:00:02.0: Missing outer runtime PM protection
> [11614.356187] WARNING: CPU: 1 PID: 13075 at drivers/gpu/drm/xe/xe_pm.c:549 xe_pm_runtime_get_noresume+0x60/0x80 [xe]
> ...
> [11614.356377] Call Trace:
> [11614.356379]  <TASK>
> [11614.356381]  ? __warn+0x7e/0x180
> [11614.356387]  ? xe_pm_runtime_get_noresume+0x60/0x80 [xe]
> [11614.356507]  xe_ggtt_remove_node+0x22/0x80 [xe]
> [11614.356546]  xe_ttm_bo_destroy+0xea/0xf0 [xe]
> [11614.356579]  xe_oa_stream_destroy+0xf7/0x120 [xe]
> [11614.356627]  xe_oa_release+0x35/0xc0 [xe]
> [11614.356673]  __fput+0xa1/0x2d0
> [11614.356679]  __x64_sys_close+0x37/0x80
> [11614.356697]  do_syscall_64+0x6d/0x140
> [11614.356700]  entry_SYSCALL_64_after_hwframe+0x71/0x79
> [11614.356702] RIP: 0033:0x7f2b37314f67
> 
> There seems to be no reason to use xe_pm_runtime_get_noresume in xe_ggtt
> functions. Just use xe_pm_runtime_get.
> 
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_ggtt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index 0d541f55b4fc..8548a2eb3b32 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -404,7 +404,7 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
>  	if (err)
>  		return err;
>  
> -	xe_pm_runtime_get_noresume(tile_to_xe(ggtt->tile));
> +	xe_pm_runtime_get(tile_to_xe(ggtt->tile));
>  	mutex_lock(&ggtt->lock);
>  	err = drm_mm_insert_node_in_range(&ggtt->mm, &bo->ggtt_node, bo->size,
>  					  alignment, 0, start, end, 0);
> @@ -433,7 +433,7 @@ int xe_ggtt_insert_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)
>  void xe_ggtt_remove_node(struct xe_ggtt *ggtt, struct drm_mm_node *node,
>  			 bool invalidate)
>  {
> -	xe_pm_runtime_get_noresume(tile_to_xe(ggtt->tile));
> +	xe_pm_runtime_get(tile_to_xe(ggtt->tile));

we cannot do this as this place gets called from locked places.
This is a deadlock risk.
We need to ensure to have an outer caller of the xe_pm_runtime_get that will
ensure to get the device waked first, then then we continue with the _noresume
variant here that only ensures that we have an extra reference.

These warnings are indeed poping up in multiple places, and this is a good
thing since we killed the mem_access... at least now we know and have a
backtrace of the places that are putting our device at risk of deadlock
and can use this information to now find the right outer place protections.

https://gitlab.freedesktop.org/drm/xe/kernel/issues/1705

>  
>  	mutex_lock(&ggtt->lock);
>  	xe_ggtt_clear(ggtt, node->start, node->size);
> -- 
> 2.41.0
>
Dixit, Ashutosh April 29, 2024, 9:19 p.m. UTC | #2 
On Mon, 29 Apr 2024 11:24:27 -0700, Rodrigo Vivi wrote:
>
> On Mon, Apr 29, 2024 at 09:29:15AM -0700, Ashutosh Dixit wrote:
> > Switching from xe_device_mem_access_get/put to xe_pm_runtime_get/put
> > results in the following WARNING in xe_oa:
> >
> > [11614.356168] xe 0000:00:02.0: Missing outer runtime PM protection
> > [11614.356187] WARNING: CPU: 1 PID: 13075 at drivers/gpu/drm/xe/xe_pm.c:549 xe_pm_runtime_get_noresume+0x60/0x80 [xe]
> > ...
> > [11614.356377] Call Trace:
> > [11614.356379]  <TASK>
> > [11614.356381]  ? __warn+0x7e/0x180
> > [11614.356387]  ? xe_pm_runtime_get_noresume+0x60/0x80 [xe]
> > [11614.356507]  xe_ggtt_remove_node+0x22/0x80 [xe]
> > [11614.356546]  xe_ttm_bo_destroy+0xea/0xf0 [xe]
> > [11614.356579]  xe_oa_stream_destroy+0xf7/0x120 [xe]
> > [11614.356627]  xe_oa_release+0x35/0xc0 [xe]
> > [11614.356673]  __fput+0xa1/0x2d0
> > [11614.356679]  __x64_sys_close+0x37/0x80
> > [11614.356697]  do_syscall_64+0x6d/0x140
> > [11614.356700]  entry_SYSCALL_64_after_hwframe+0x71/0x79
> > [11614.356702] RIP: 0033:0x7f2b37314f67
> >
> > There seems to be no reason to use xe_pm_runtime_get_noresume in xe_ggtt
> > functions. Just use xe_pm_runtime_get.
> >
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_ggtt.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> > index 0d541f55b4fc..8548a2eb3b32 100644
> > --- a/drivers/gpu/drm/xe/xe_ggtt.c
> > +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> > @@ -404,7 +404,7 @@ static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
> >	if (err)
> >		return err;
> >
> > -	xe_pm_runtime_get_noresume(tile_to_xe(ggtt->tile));
> > +	xe_pm_runtime_get(tile_to_xe(ggtt->tile));
> >	mutex_lock(&ggtt->lock);
> >	err = drm_mm_insert_node_in_range(&ggtt->mm, &bo->ggtt_node, bo->size,
> >					  alignment, 0, start, end, 0);
> > @@ -433,7 +433,7 @@ int xe_ggtt_insert_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)
> >  void xe_ggtt_remove_node(struct xe_ggtt *ggtt, struct drm_mm_node *node,
> >			 bool invalidate)
> >  {
> > -	xe_pm_runtime_get_noresume(tile_to_xe(ggtt->tile));
> > +	xe_pm_runtime_get(tile_to_xe(ggtt->tile));
>
> we cannot do this as this place gets called from locked places.
> This is a deadlock risk.
> We need to ensure to have an outer caller of the xe_pm_runtime_get that will
> ensure to get the device waked first, then then we continue with the _noresume
> variant here that only ensures that we have an extra reference.
>
> These warnings are indeed poping up in multiple places, and this is a good
> thing since we killed the mem_access... at least now we know and have a
> backtrace of the places that are putting our device at risk of deadlock
> and can use this information to now find the right outer place protections.
>
> https://gitlab.freedesktop.org/drm/xe/kernel/issues/1705

OK Rodrigo, thanks for the explanation. I wasn't sure, so I thought I'll
send the patch. Anyway, I'll add an outer call for
xe_pm_runtime_get. Thanks.

>
> >
> >	mutex_lock(&ggtt->lock);
> >	xe_ggtt_clear(ggtt, node->start, node->size);
> > --
> > 2.41.0
> >
diff mbox series

Patch

diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
index 0d541f55b4fc..8548a2eb3b32 100644
--- a/drivers/gpu/drm/xe/xe_ggtt.c
+++ b/drivers/gpu/drm/xe/xe_ggtt.c
@@ -404,7 +404,7 @@  static int __xe_ggtt_insert_bo_at(struct xe_ggtt *ggtt, struct xe_bo *bo,
 	if (err)
 		return err;
 
-	xe_pm_runtime_get_noresume(tile_to_xe(ggtt->tile));
+	xe_pm_runtime_get(tile_to_xe(ggtt->tile));
 	mutex_lock(&ggtt->lock);
 	err = drm_mm_insert_node_in_range(&ggtt->mm, &bo->ggtt_node, bo->size,
 					  alignment, 0, start, end, 0);
@@ -433,7 +433,7 @@  int xe_ggtt_insert_bo(struct xe_ggtt *ggtt, struct xe_bo *bo)
 void xe_ggtt_remove_node(struct xe_ggtt *ggtt, struct drm_mm_node *node,
 			 bool invalidate)
 {
-	xe_pm_runtime_get_noresume(tile_to_xe(ggtt->tile));
+	xe_pm_runtime_get(tile_to_xe(ggtt->tile));
 
 	mutex_lock(&ggtt->lock);
 	xe_ggtt_clear(ggtt, node->start, node->size);