[PATCH 10/10] drm/xe: Kill xe_device_mem_access_{get*,put}

[Vivi, Rodrigo]

On Fri, 2024-04-26 at 18:30 -0700, Dixit, Ashutosh wrote:
> On Wed, 06 Mar 2024 16:15:54 -0800, Rodrigo Vivi wrote:
> > 
> 
> Hi Rodrigo/Matt,
> 
> > @@ -409,14 +410,14 @@ static int __xe_ggtt_insert_bo_at(struct
> > xe_ggtt *ggtt, struct xe_bo *bo,
> > 	if (err)
> > 		return err;
> > 
> > -	xe_device_mem_access_get(tile_to_xe(ggtt->tile));
> > +	xe_pm_runtime_get_noresume(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);
> > 	if (!err)
> > 		xe_ggtt_map_bo(ggtt, bo);
> > 	mutex_unlock(&ggtt->lock);
> > -	xe_device_mem_access_put(tile_to_xe(ggtt->tile));
> > +	xe_pm_runtime_put(tile_to_xe(ggtt->tile));
> > 
> > 	return err;
> >  }
> > @@ -434,7 +435,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)
> >  {
> > -	xe_device_mem_access_get(tile_to_xe(ggtt->tile));
> > +	xe_pm_runtime_get_noresume(tile_to_xe(ggtt->tile));
> > 	mutex_lock(&ggtt->lock);
> > 
> > 	xe_ggtt_clear(ggtt, node->start, node->size);
> > @@ -444,7 +445,7 @@ void xe_ggtt_remove_node(struct xe_ggtt *ggtt,
> > struct drm_mm_node *node)
> > 	xe_ggtt_invalidate(ggtt);
> > 
> > 	mutex_unlock(&ggtt->lock);
> > -	xe_device_mem_access_put(tile_to_xe(ggtt->tile));
> > +	xe_pm_runtime_put(tile_to_xe(ggtt->tile));
> 
> Do __xe_ggtt_insert_bo_at and xe_ggtt_insert_bo need a runtime_pm
> reference
> held?
> 
> In this series: https://patchwork.freedesktop.org/series/121084/
> 
> I am not holding a runtime_pm reference when these functions are
> called and
> it was fine with xe_device_mem_access_get/put (see
> xe_oa_alloc_oa_buffer/xe_oa_free_oa_buffer if needed). But after
> changing
> to xe_pm_runtime_get/put I now get this WARN:
> 
> [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.356437]  ? report_bug+0x1c7/0x1d0
> [11614.356442]  ? prb_read_valid+0x16/0x20
> [11614.356447]  ? handle_bug+0x3c/0x70
> [11614.356451]  ? exc_invalid_op+0x18/0x70
> [11614.356453]  ? asm_exc_invalid_op+0x1a/0x20
> [11614.356460]  ? 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]

in this case, apparently xe_oa_release is the right place to add
a xe_pm_runtime_{get,put} pair?!
or maybe some place even more to the outside

> [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
> 
> Also, the WARN above happens only for 'free' but not for 'alloc' (so
> not
> sure who gets the runtime_pm reference for 'alloc').
> 
> Holding the runtime_pm reference across alloc and free seems to be
> fine and
> makes this WARN disappear. So maybe I should just do that? Just
> trying to
> confirm.
> 
> Thanks.
> --
> Ashutosh