Re: [RFC 20/20] drm/xe: Mega Kill of mem_access

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Jan 09, 2024 at 06:27:13PM +0000, Matthew Auld wrote:
> On 09/01/2024 17:39, Rodrigo Vivi wrote:
> > On Tue, Jan 09, 2024 at 11:41:35AM +0000, Matthew Auld wrote:
> > > On 28/12/2023 02:12, Rodrigo Vivi wrote:
> > > > All of these remaining cases should already be protected
> > > > by the outer bound calls of runtime_pm
> > > > 
> > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > > ---
> > > >    drivers/gpu/drm/xe/display/xe_fb_pin.c |  7 +--
> > > >    drivers/gpu/drm/xe/tests/xe_bo.c       |  8 ----
> > > >    drivers/gpu/drm/xe/tests/xe_mocs.c     |  4 --
> > > >    drivers/gpu/drm/xe/xe_bo.c             |  5 ---
> > > >    drivers/gpu/drm/xe/xe_device.c         | 59 --------------------------
> > > >    drivers/gpu/drm/xe/xe_device.h         |  7 ---
> > > >    drivers/gpu/drm/xe/xe_device_types.h   |  9 ----
> > > >    drivers/gpu/drm/xe/xe_ggtt.c           |  6 ---
> > > >    drivers/gpu/drm/xe/xe_gsc.c            |  3 --
> > > >    drivers/gpu/drm/xe/xe_gt.c             | 17 --------
> > > >    drivers/gpu/drm/xe/xe_huc_debugfs.c    |  2 -
> > > >    drivers/gpu/drm/xe/xe_pat.c            | 10 -----
> > > >    drivers/gpu/drm/xe/xe_pm.c             | 27 ------------
> > > >    drivers/gpu/drm/xe/xe_query.c          |  4 --
> > > >    drivers/gpu/drm/xe/xe_tile.c           | 10 ++---
> > > >    drivers/gpu/drm/xe/xe_vm.c             |  7 ---
> > > >    16 files changed, 5 insertions(+), 180 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > > index 722c84a566073..077294ec50ece 100644
> > > > --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > > +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> > > > @@ -190,10 +190,9 @@ static int __xe_pin_fb_vma_ggtt(struct intel_framebuffer *fb,
> > > >    	/* TODO: Consider sharing framebuffer mapping?
> > > >    	 * embed i915_vma inside intel_framebuffer
> > > >    	 */
> > > > -	xe_device_mem_access_get(tile_to_xe(ggtt->tile));
> > > >    	ret = mutex_lock_interruptible(&ggtt->lock);
> > > >    	if (ret)
> > > > -		goto out;
> > > > +		return ret;
> > > >    	align = XE_PAGE_SIZE;
> > > >    	if (xe_bo_is_vram(bo) && ggtt->flags & XE_GGTT_FLAGS_64K)
> > > > @@ -241,8 +240,6 @@ static int __xe_pin_fb_vma_ggtt(struct intel_framebuffer *fb,
> > > >    	xe_ggtt_invalidate(ggtt);
> > > >    out_unlock:
> > > >    	mutex_unlock(&ggtt->lock);
> > > > -out:
> > > > -	xe_device_mem_access_put(tile_to_xe(ggtt->tile));
> > > >    	return ret;
> > > >    }
> > > > @@ -381,4 +378,4 @@ struct i915_address_space *intel_dpt_create(struct intel_framebuffer *fb)
> > > >    void intel_dpt_destroy(struct i915_address_space *vm)
> > > >    {
> > > >    	return;
> > > > -}
> > > > \ No newline at end of file
> > > > +}
> > > > diff --git a/drivers/gpu/drm/xe/tests/xe_bo.c b/drivers/gpu/drm/xe/tests/xe_bo.c
> > > > index 412b2e7ce40cb..97b10e597f0ad 100644
> > > > --- a/drivers/gpu/drm/xe/tests/xe_bo.c
> > > > +++ b/drivers/gpu/drm/xe/tests/xe_bo.c
> > > > @@ -164,8 +164,6 @@ static int ccs_test_run_device(struct xe_device *xe)
> > > >    		return 0;
> > > >    	}
> > > > -	xe_device_mem_access_get(xe);
> > > > -
> > > >    	for_each_tile(tile, xe, id) {
> > > >    		/* For igfx run only for primary tile */
> > > >    		if (!IS_DGFX(xe) && id > 0)
> > > > @@ -173,8 +171,6 @@ static int ccs_test_run_device(struct xe_device *xe)
> > > >    		ccs_test_run_tile(xe, tile, test);
> > > >    	}
> > > > -	xe_device_mem_access_put(xe);
> > > > -
> > > >    	return 0;
> > > >    }
> > > > @@ -336,13 +332,9 @@ static int evict_test_run_device(struct xe_device *xe)
> > > >    		return 0;
> > > >    	}
> > > > -	xe_device_mem_access_get(xe);
> > > > -
> > > >    	for_each_tile(tile, xe, id)
> > > >    		evict_test_run_tile(xe, tile, test);
> > > > -	xe_device_mem_access_put(xe);
> > > > -
> > > >    	return 0;
> > > >    }
> > > > diff --git a/drivers/gpu/drm/xe/tests/xe_mocs.c b/drivers/gpu/drm/xe/tests/xe_mocs.c
> > > > index 7dd34f94e8094..a12e7e2bb5861 100644
> > > > --- a/drivers/gpu/drm/xe/tests/xe_mocs.c
> > > > +++ b/drivers/gpu/drm/xe/tests/xe_mocs.c
> > > > @@ -45,7 +45,6 @@ static void read_l3cc_table(struct xe_gt *gt,
> > > >    	struct kunit *test = xe_cur_kunit();
> > > > -	xe_device_mem_access_get(gt_to_xe(gt));
> > > >    	ret = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> > > >    	KUNIT_ASSERT_EQ_MSG(test, ret, 0, "Forcewake Failed.\n");
> > > >    	mocs_dbg(&gt_to_xe(gt)->drm, "L3CC entries:%d\n", info->n_entries);
> > > > @@ -65,7 +64,6 @@ static void read_l3cc_table(struct xe_gt *gt,
> > > >    				   XELP_LNCFCMOCS(i).addr);
> > > >    	}
> > > >    	xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
> > > > -	xe_device_mem_access_put(gt_to_xe(gt));
> > > >    }
> > > >    static void read_mocs_table(struct xe_gt *gt,
> > > > @@ -80,7 +78,6 @@ static void read_mocs_table(struct xe_gt *gt,
> > > >    	struct kunit *test = xe_cur_kunit();
> > > > -	xe_device_mem_access_get(gt_to_xe(gt));
> > > >    	ret = xe_force_wake_get(gt_to_fw(gt), XE_FW_GT);
> > > >    	KUNIT_ASSERT_EQ_MSG(test, ret, 0, "Forcewake Failed.\n");
> > > >    	mocs_dbg(&gt_to_xe(gt)->drm, "Global MOCS entries:%d\n", info->n_entries);
> > > > @@ -100,7 +97,6 @@ static void read_mocs_table(struct xe_gt *gt,
> > > >    				   XELP_GLOBAL_MOCS(i).addr);
> > > >    	}
> > > >    	xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
> > > > -	xe_device_mem_access_put(gt_to_xe(gt));
> > > >    }
> > > >    static int mocs_kernel_test_run_device(struct xe_device *xe)
> > > > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > > > index 8e4a3b1f6b938..056c65c2675d8 100644
> > > > --- a/drivers/gpu/drm/xe/xe_bo.c
> > > > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > > > @@ -715,7 +715,6 @@ static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
> > > >    	xe_assert(xe, migrate);
> > > >    	trace_xe_bo_move(bo);
> > > > -	xe_device_mem_access_get(xe);
> > > >    	if (xe_bo_is_pinned(bo) && !xe_bo_is_user(bo)) {
> > > >    		/*
> > > > @@ -739,7 +738,6 @@ static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
> > > >    				if (XE_WARN_ON(new_mem->start == XE_BO_INVALID_OFFSET)) {
> > > >    					ret = -EINVAL;
> > > > -					xe_device_mem_access_put(xe);
> > > >    					goto out;
> > > >    				}
> > > > @@ -757,7 +755,6 @@ static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
> > > >    						new_mem, handle_system_ccs);
> > > >    		if (IS_ERR(fence)) {
> > > >    			ret = PTR_ERR(fence);
> > > > -			xe_device_mem_access_put(xe);
> > > >    			goto out;
> > > >    		}
> > > >    		if (!move_lacks_source) {
> > > > @@ -782,8 +779,6 @@ static int xe_bo_move(struct ttm_buffer_object *ttm_bo, bool evict,
> > > >    		dma_fence_put(fence);
> > > >    	}
> > > > -	xe_device_mem_access_put(xe);
> > > > -
> > > >    out:
> > > >    	return ret;
> > > > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > > > index c1c19264a58b4..cb08a4369bb9e 100644
> > > > --- a/drivers/gpu/drm/xe/xe_device.c
> > > > +++ b/drivers/gpu/drm/xe/xe_device.c
> > > > @@ -44,12 +44,6 @@
> > > >    #include "xe_wait_user_fence.h"
> > > >    #include "xe_hwmon.h"
> > > > -#ifdef CONFIG_LOCKDEP
> > > > -struct lockdep_map xe_device_mem_access_lockdep_map = {
> > > > -	.name = "xe_device_mem_access_lockdep_map"
> > > > -};
> > > > -#endif
> > > 
> > > Did you mean to drop this? IMO we should for sure keep the lockdep
> > > annotations. Otherwise it is going to be really hard to validate the locking
> > > design and have reasonable confidence that we don't have deadlocks lurking,
> > > or as new users come along sprinkling rpm get in the wrong place.
> > 
> > Well, the whole goal of this series is to actually avoid sprinkling RPM calls at all.
> 
> I mean new users are bound to appear, and they might add such calls in the
> wrong place. Lockdep would hopefully catch such things for us.

we should actually catch that during reviews and push back ;)
But I do see your point.

> 
> > We should only protect the outer bounds. I'm afraid that if we put this to the outer
> > bounds we would start getting false positives on this, no?!
> 
> What kind of false positives? With this series the sync rpm get should be
> the outermost thing for the most part, and so the locking dependences should
> be minimal. If we drop the annotations we get no help from lockdep to tell
> us if the rpm resume and suspend callbacks are grabbing locks that are
> already held when calling the sync rpm get.

yeap, you are right. I honestly didn't think so deeply there and was just
afraid of some inversions.