On 6/13/21 6:08 AM, Martin Krastev wrote:
Hey, Zack
Looking at vmw_otable_batch_takedown() in vmwgfx-next circa 467343468d53 I've noticed something puzzling. Here's the current state of the fn:
d80efd5cb3dec (Thomas Hellstrom 2015-08-10 10:39:35 -0700 338) static void vmw_otable_batch_takedown(struct vmw_private *dev_priv,
d80efd5cb3dec (Thomas Hellstrom 2015-08-10 10:39:35 -0700 339) struct vmw_otable_batch *batch)
3530bdc35e99c (Thomas Hellstrom 2012-11-21 10:49:52 +0100 340) {
3530bdc35e99c (Thomas Hellstrom 2012-11-21 10:49:52 +0100 341) SVGAOTableType i;
d80efd5cb3dec (Thomas Hellstrom 2015-08-10 10:39:35 -0700 342) struct ttm_buffer_object *bo = batch->otable_bo;
3530bdc35e99c (Thomas Hellstrom 2012-11-21 10:49:52 +0100 343) int ret;
3530bdc35e99c (Thomas Hellstrom 2012-11-21 10:49:52 +0100 344)
d80efd5cb3dec (Thomas Hellstrom 2015-08-10 10:39:35 -0700 345) for (i = 0; i < batch->num_otables; ++i)
d80efd5cb3dec (Thomas Hellstrom 2015-08-10 10:39:35 -0700 346) if (batch->otables[i].enabled)
d80efd5cb3dec (Thomas Hellstrom 2015-08-10 10:39:35 -0700 347) vmw_takedown_otable_base(dev_priv, i,
d80efd5cb3dec (Thomas Hellstrom 2015-08-10 10:39:35 -0700 348) &batch->otables[i]);
3530bdc35e99c (Thomas Hellstrom 2012-11-21 10:49:52 +0100 349)
dfd5e50ea43ca (Christian König 2016-04-06 11:12:03 +0200 350) ret = ttm_bo_reserve(bo, false, true, NULL);
3530bdc35e99c (Thomas Hellstrom 2012-11-21 10:49:52 +0100 351) BUG_ON(ret != 0);
3530bdc35e99c (Thomas Hellstrom 2012-11-21 10:49:52 +0100 352)
e9431ea5076a9 (Thomas Hellstrom 2018-06-19 15:33:53 +0200 353) vmw_bo_fence_single(bo, NULL);
2ef4fb92363c4 (Zack Rusin 2021-03-22 13:04:11 -0400 354) ttm_bo_unpin(bo);
3530bdc35e99c (Thomas Hellstrom 2012-11-21 10:49:52 +0100 355) ttm_bo_unreserve(bo);
3530bdc35e99c (Thomas Hellstrom 2012-11-21 10:49:52 +0100 356)
d1a73c641afd2 (Zack Rusin 2021-01-14 18:38:16 -0500 357) ttm_bo_unpin(batch->otable_bo);
6034d9d48e62a (Thomas Zimmermann 2019-01-25 12:02:09 +0100 358) ttm_bo_put(batch->otable_bo);
6034d9d48e62a (Thomas Zimmermann 2019-01-25 12:02:09 +0100 359) batch->otable_bo = NULL;
3530bdc35e99c (Thomas Hellstrom 2012-11-21 10:49:52 +0100 360) }
Line 357 does a repeat unpinning of the batch->otable_bo, which we already unpinned on line 354.
But according to 2ef4fb92363c4 line 357 should not look like that!
@@ -341,9 +351,9 @@ static void vmw_otable_batch_takedown(struct vmw_private *dev_priv,
BUG_ON(ret != 0);
vmw_bo_fence_single(bo, NULL);
+ ttm_bo_unpin(bo);
ttm_bo_unreserve(bo);
- ttm_bo_unpin(batch->otable_bo);
ttm_bo_put(batch->otable_bo);
batch->otable_bo = NULL;
}
Can you confirm you're seeing what I'm seeing?
Great catch Martin. Yes, this looks like a bad merge that I missed. It looks like it was introduced in:
68a32ba14177 ("Merge tag 'drm-next-2021-04-28' of git://anongit.freedesktop.org/drm/drm")
So, what most likely has happened was that the change:
2ef4fb92363c ("drm/vmwgfx: Make sure bo's are unpinned before putting them back")
probably conflicted with something in either drm-misc-next, Dave's tree or Linus' tree and it was manually resolved by the givens tree's maintainers during the merge and accidentally reintroduced the code that patch was fixing.
I don't know of an automatic way of monitoring for those types of issues, maybe Daniel or Dave know of something. I'll just have to pay a little more attention to what goes in Linus' tree in the future.
z
