Peculiarity in vmwgfx_mob.c

[Zack Rusin]

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