Re: [Intel-gfx] [PATCH 14/28] drm/i915: Take object lock in i915_ggtt_pin if ww is not set

On 21-10-2021 19:39, Matthew Auld wrote:

On Thu, 21 Oct 2021 at 11:37, Maarten Lankhorst maarten.lankhorst@linux.intel.com wrote:

...

i915_vma_wait_for_bind needs the vma lock held, fix the caller.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

drivers/gpu/drm/i915/i915_vma.c | 40 +++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index bacc8d68e495..2877dcd62acb 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -1348,23 +1348,15 @@ static void flush_idle_contexts(struct intel_gt *gt) intel_gt_wait_for_idle(gt, MAX_SCHEDULE_TIMEOUT); }

-int i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,

•             u32 align, unsigned int flags)
  

+static int __i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,

•                      u32 align, unsigned int flags)
  

{ struct i915_address_space *vm = vma->vm; int err;

•   GEM_BUG_ON(!i915_vma_is_ggtt(vma));
  

-#ifdef CONFIG_LOCKDEP

•   WARN_ON(!ww && dma_resv_held(vma->obj->base.resv));
  

-#endif

•   do {
  

•           if (ww)
  

•                   err = i915_vma_pin_ww(vma, ww, 0, align, flags | PIN_GLOBAL);
  

•           else
  

•                   err = i915_vma_pin(vma, 0, align, flags | PIN_GLOBAL);
  

•           err = i915_vma_pin_ww(vma, ww, 0, align, flags | PIN_GLOBAL);
  

•           if (err != -ENOSPC) {
                    if (!err) {
                            err = i915_vma_wait_for_bind(vma);
  

@@ -1383,6 +1375,30 @@ int i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, } while (1); }

+int i915_ggtt_pin(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,

•             u32 align, unsigned int flags)
  

+{

•   struct i915_gem_ww_ctx _ww;
  

•   int err;
  

•   GEM_BUG_ON(!i915_vma_is_ggtt(vma));
  

•   if (ww)
  

•           return __i915_ggtt_pin(vma, ww, align, flags);
  

+#ifdef CONFIG_LOCKDEP

•   WARN_ON(dma_resv_held(vma->obj->base.resv));
  

Hmm, so this can't trigger, say if shrinker grabs the lock, or some other concurrent user?

No, it checks internally in lockdep that the current task doesn't hold the lock. Others can lock just fine.

dma_resv_is_locked() would check if anyone locked it. :)

~Maarten